Rewrite RHEL 8 and CS8 image definitions using the new framework #3213
Conversation
achilleas-k
force-pushed
the
rewrite/rhel8
branch
from
152c8e7 to
462cf89
Compare
achilleas-k
changed the title
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
achilleas-k
force-pushed
the
rewrite/rhel8
branch
4 times, most recently
from
2532e30 to
ff81b93
Compare
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
achilleas-k
force-pushed
the
rewrite/rhel8
branch
from
ff81b93 to
0b25500
Compare
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
|
Installer and edge commit tests are failing. |
There was a problem hiding this comment.
Nice work 👏
I added a few comments, but functionally changes look good to me.
FWIW, I have a few design-related comments:
- I didn't check it thoroughly, but functions in
internal/distro/*/images.golook identical to me and I would expect these to be deduplicated and moved to a common location / package, instead of being copied to distros. Maybe that's the plan, but it looked like something that could have been part of this PR, thus I'm mentioning it. (I'm not saying it should be made part of this PR at this point) - I'm not really in favor of making
imageTypesfunctions shared among various distros due to (all are my opinions):- It is a step away from ever allowing images to be defined not as a Go code, which is something that I think we'll have to consider in the long term. Having to recompile a binary to define an image is IMHO very inconvenient and user-unfriendly for anyone wanting to experiment and define their own image (like Fedora Spins do).
- I think this will become a mess full of conditionals mixing all distros and variants as we would be adding support for new distros (RHEL-10 and later) as things will inevitably differ.
- IMHO defining image types as functions makes sense for distros that are almost the same and CentOS Stream / RHEL are good (and probably the only) example of where it makes sense, but only within one major version.
- IMHO figuring our what an image definitions consist of should be reasonably easy without having to generate a manifest. We are making it actually harder and these cross-distro functions would make it even harder.
I think that's where we're headed, yes. I initially copied them from RHEL 9 expecting to need to make changes, but none were needed. I'd do this in a follow-up and see if all distros (even Fedora) can share the same image functions. I seem to recall I needed to make some changes from Fedora to RHEL 9, so maybe that will require a bit of work.
I re-read my original comment on the PR and I think I was unclear about the image definition functions. My thinking wasn't to share the, e.g., func (a *architecture) addImageTypes(platform platform.Platform, imageTypes ...imageTypeFunc)where type imageTypeFunc func(d distribution) imageTypeWe would still have different private functions for |
Sounds like a solid plan. Thanks!
Oh, this makes absolute sense. So +1 based on your explanation from my side 👍 |
achilleas-k
force-pushed
the
rewrite/rhel8
branch
from
0b25500 to
7310d80
Compare
|
Updates in recent force-push:
|
Update the implementation of the distro.Distro interface to match the one in RHEL 9 and Fedora. The main change is that the runner is a runner.Runner and not a string. The distroMap is replaced by a switch that initialises the distribution struct strings based on the minor version number. The default minor version, created with rhel8.New(), creates a copy of RHEL 8.6 and renames it to "rhel8".
|
I need to rebase on main because #3130 changed some functions used in distros and, while it doesn't cause conflicts, it creates errors on rebase. |
Regenerate manifests for changes from recent repository snapshot update. Not all were generated after the most recent change.
Inline the distribution structs in the common constructor. Make the same changes that were made to RHEL 8: - Remove major version argument: it's always 9 - Make the default constructor New() create the default minor version and rename it to plain RHEL 9. The distroregistry now contains both rhel-9 and rhel-90, which point to the same configuration but with different names. The default rhel-9 should be updated to be an alias to rhel-91, the current GA.
The host distro object was identical to the regular distro objects for a while now. The constructors in the registry have been aliases to the base constructors for a long time. - Deleted all HostDistro constructors from the distributions. - Changed the supported distro list to only contain base constructor functions. - The host distro in the distro registry is a copy of the base distro that matches the host and does not call a separate constructor.
Add platform attribute to imageType. Create platform configurations for each image type, copied from RHEL 9. Currently this has no effect on the image definition / manifest.
Start splitting image type definitions into separate files by logical groups (mostly by footprint and cloud platform) for easier navigation, like we did for rhel9. Split AMI and Edge image types; the rest will follow in separate commits. Image specific package sets are defined in the file for the image type grouping instead of the package_sets file. A notable difference with the way it was done in rhel9 is that every image type is defined in a function rather than a global where possible and a function when distro version specific configuration is needed. This is done for consistency and the change will likely be done in the other distributions as well. Also, instead of passing only required values to the image type constructor (for example, osVersion and a RHEL boolean), we pass the whole distribution object and each constructor can read whatever information it needs.
Continuing image type splitting. Constructor for qcow2 type requires the distribution object to determine whether to add RHSM to the image config (RHEL only).
The changes in these manifest are the same as for the edge raw image. The installer definition isn't changed, only the raw image.
Using the same pipeline functions as Fedora and RHEL 9 and copied the image function from RHEL 9. The most notable change is the replacement of the deprecated bootiso.mono stage with the more granular stages.
Using the same pipeline functions as Fedora and RHEL 9 and copied the image function from RHEL 9. The most notable change is the replacment of the deprecated bootiso.mono stage with the more granular stages, just like with the image installer.
Override the kernel-rt test case for CS8 with kernel-rt-core. The kernel-rt package resolves to kernel-rt-core (no kernel-rt metapackage exists). More details can be found at osbuild#3211
The python3-toml package is called python3-pytoml in RHEL 8, so the name must be replaced before depsolving. The package is defined in manifest/os.go which does not have access to the distribution name or version. This solution is a temporary workaround. The future solution should depend on distributions resolving package names based on required features.
Rename new implementations in their place.
Make RHEL 9 without a minor version point to RHEL 9.1, the current GA version.
Release repositories (in repositories/) for RHEL 9 are the CDN repos without a minor release, which should always track GA. Test repositories (in test/data/ and test-case-generators/) point to RHEL 9.1, the current GA.
These are identical to the rhel-91 counterparts.
Image types no longer report their chains. Instead, pipelines report their packages and chains and blueprint packages are added to the workload. The distro.ImageType interface retains the PackageSetsChains() methods for RHEL 7 until that is rewritten as well. The osbuild-dnf-json-test doesn't use the PackageSetsChains() method anymore. Instead, since it only test the centos-8 qcow2 image, it hardcodes the expected package set names.
Make RHEL 8 without a minor version point to RHEL 8.7, the current GA version.
- repositories/: add google-compute-engine and google-cloud-sdk repos to package repositories. - test/data/repositories/: add rt, rhui, and rhui-azure to test repositories. - test-case-generators/: update unversioned rhel-8 repos to point to RHEL 8.7 snapshots.
The format-request-map is updated to remove the override for the customized qcow for rhel-8. The rhel-8 manifests are now identical to the rhel-87 counterparts.
Every image type defines a list of build pipeline names and a list of payload pipeline names. These should match the names of the pipelines that will exist in the manifest when it's generated. They should match exactly, otherwise issues can occur when reading the metadata from an osbuild result. The cloud API needs to know the names of the pipelines and specifically the name of the build pipeline and the payload pipeline in order to differentiated between build and payload packages in the metadata. This new test generates every manifest, parses it into a minimal struct, and compares the pipeline names with the ones reported statically on the image type definition.
Some pipeline lists weren't updated when the image types were rewritten. Discovered now from the new test and fixed.
achilleas-k
force-pushed
the
rewrite/rhel8
branch
from
7310d80 to
4104905
Compare
|
New force push is for rebase on main and fixes from that rebase. Also, added a commit at the top of the PR that regenerates all RHEL 9 manifests because the sources were changed in a previous, merged PR, but the manifests weren't all updated. |
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
|
Thanks for the reviews and comments. I'll note all the ideas for followups. |
Followup to #3120.
This is a rewrite of the
distro/rhel8/package to use the new image definition framework that was so far used only for Fedora.I've described and provided context for all changes in individual commit messages.
Some things differ from how I did them for RHEL 9 because I had the opportunity to reevaluate certain decisions. For example, all image types are now defined in functions. I think we could standardise this and make every
imageTypebe generated by a function that takes adistributionargument for making conditional checks on distribution name and version number. We could use even for image types that don't need these checks for consistency.The most notable new thing is the
resolvePackageNames()function inrhel8/imagetype.go(see commit with subjectdistro/rhel8: resolve package names for RHEL 8). This is an issue we saw coming but the proper solution requires a lot more work. The issue is that, outside of the preset package sets for each distro and image, we also select specific packages outside of the distribution implementation, in the manifest generation code inmanifest/. If a package has a different name on different distributions, this wont work, and we ran into it withpython3-tomlbeing selected when containers are embedded in ostree commits, because the package is namedpython3-pytomlon RHEL 8. I have a rough plan for how to solve this and it involves having stages report their own package requirements but as "features" rather than package names. Each distribution would then be responsible for resolving each feature into a known package name. Since we only ran into this with a single package so far, we can get away with a single function in RHEL 8, but I wrote it in a way that can be extended for now in case we run into more examples before any further redesigns.One somewhat unrelated change in this PR is the removal of the HostDistro in the distro registry. These have been deprecated for a while and only created extra noise because of the requirement that we have distro constructors with extra arguments that were ignored.