Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite RHEL 7 image definitions using the new framework #3239

Merged
merged 16 commits into from
Jan 25, 2023

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Jan 24, 2023

Followup to #3120 and #3213.

This is a rewrite of the distro/rhel7/ package to use the new image definition framework. I've described and provided context for all changes in individual commit messages.

The changeset is much smaller because of the limited number (2) of image types in RHEL 7.

One notable change is the addition of the force_autorelabel option in the global ImageConfig. The RHEL 7 images need to enable the force_autorelabel option for the SELinux stage in osbuild. This option should almost never be used but it was added specifically for RHEL 7. With the rewrite to the new definitions and the sharing of pipeline code between all distros, we need to add support to all stages of the pipeline generation to be able to enable it.

EDIT: We don't test RHEL 7 in CI. I did a build test of all three test manifests without issue to confirm the build root isn't missing any required packages.

@achilleas-k achilleas-k changed the title Rewrite RHEL 8 and CS8 image definitions using the new framework Rewrite RHEL 7 image definitions using the new framework Jan 24, 2023
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD e4ae37c with the main merge-base 8624ff6). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3654368535/artifacts/browse

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see the work converging. I added a few comments, but noting serious 😉

Also found a typo in 4344de0 commit message. Specifically The RHEL 7 vpc subformat in qemu does support force_size so we need to be able to disable it. is missing NOT in ..qemu does <HERE> support... 😎

internal/distro/rhel7/distro.go Show resolved Hide resolved
internal/distro/rhel7/images.go Show resolved Hide resolved
internal/manifest/os.go Outdated Show resolved Hide resolved
internal/manifest/vpc.go Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member Author

Also found a typo in 4344de0 commit message. Specifically The RHEL 7 vpc subformat in qemu does support force_size so we need to be able to disable it. is missing NOT in ..qemu does <HERE> support... sunglasses

Good catch, thanks.

@achilleas-k
Copy link
Member Author

  • Removed Type: "vpd" from osbuild.VPCOptions{}
  • Dropped ForceAutorelabel: common.ToPtr(true) from irrelevant commit. This is later set to ForceAutorelabel: p.SELinuxForceRelabel.
  • Fixed typo in commit message.

The qcow2 image type for RHEL 7 doesn't have packageset chains defined.
This means that the blueprint packages are never merged into the os
pipeline.

This is unnecessary right now because of the upcoming rewrite, but it
will minimise the differences that will show up in the manifest.

The qcow2-customize manifest has an added block of options for the
grub2.legacy stage because now the dracut-config-rescue package is being
installed in the image.
Update the implementation of the distro.Distro interface to match the
one in RHEL 8, 9, and Fedora.  The main change is that the runner is a
runner.Runner and not a string.

The runner name is now rhel79 (changed from rhel7).  This is
functionally equivalent based on osbuild's runner version fallback
logic.
No platform-python for RHEL 7.
Add environment, compression, and image fields and define the imageFunc
function type.
Copied osCustomizations() and liveImage() functions from RHEL 8 and remove
unneeded customizations and options.
Moved the Azure image type definition to the top of the file for
consistency with the other image type files.
Separated the default image config struct from the base image type
definition to make it easier to read.
Moved the qcow2 image type definition to the top of the file for
consistency with the other image type files.
Separated the default image config struct from the base image type
definition to make it easier to read.
- Replace Manifest() and PackageSets() imageType methods with (adapted)
  copies from RHEL 8.
- Replace pipeline functions with liveImage image function.
- Specify xz compression for Azure RHUI.
- Add similar package name overrides as we did in RHEL 8.  For RHEL 7,
  we need to modify the capitalisation of python3-pyyaml.
Add the partition tool as an option on the Raw pipeline.  Set it to the
old value (sfdisk) by default.

Expose the option up through the liveImage image kind so that the
distribution can set it if needed.
For RHEL 7, set it to sgdisk.
Older OS versions (RHEL 7) with older versions of grub2 don't support
BLS entries.  Setting NoBLS to true configures the bootloader with
traditional menu entries through the grub2.legacy osbuild stage.  This
requires specifying extra information for the OS to the pipeline:
version, product, and nick.
The RHEL 7 vpc subformat in qemu does not support force_size so we need
to be able to disable it.  The parameter in all parts is defined as a
pointer because the default value is 'true'.  Not specifying it will
keep the option in the osbuild stage as 'nil', falling back to 'true' in
osbuild.
The RHEL 7 images need to enable the force_autorelabel option for the
SELinux stage in osbuild.  This option should almost never be used but
it was added specifically for RHEL 7.  With the rewrite to the new
definitions and the sharing of pipeline code between all distros, we
need to add support to all stages of the pipeline generation to be able
to enable it.
YUMConfig is supported in ImageConfig for RHEL 7.  We now copy the
options over to OSCustomizations and create the stage when necessary.
Changes:
- Removed unnecessary packages from build roots.  Build packages are
  added on-demand.
- Some stages changed order in the OS pipeline for azure-rhui.  This
  will have no functional effect.
- RHSM Facts stage added to all RHEL 7 manifests.  This wont be used
  since RHEL 7 is not built in the cloud service.  The RHSM fact is
  added by the test case generator to all image types to test that the
  option is working properly.
- Azure archive pipeline renamed to xz like in all the other rewritten
  image types that use the xz pipeline.
- Azure image filename before archiving has changed from 'disk.vhd' to
  'image.vhd'.  Again, this will have no functional effect because the
  name of the final artifact is unchanged (disk.vhd.xz).
@achilleas-k
Copy link
Member Author

And a quick rebase on main.

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 3af99aa with the main merge-base 2e3dd16). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3659107510/artifacts/browse

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@thozza thozza enabled auto-merge (rebase) January 25, 2023 16:12
@thozza thozza merged commit fe78607 into osbuild:main Jan 25, 2023
@achilleas-k achilleas-k deleted the rewrite/rhel7 branch January 25, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants