-
Notifications
You must be signed in to change notification settings - Fork 106
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
Finalise interface between composer and image definitions #3444
Finalise interface between composer and image definitions #3444
Conversation
395f41c
to
6281b22
Compare
Rebased for container fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
Needs a rebase and conflict fix now as well as some small fixes that I see from the test failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I like these changes and the overall direction. I added a few comments and questions inline...
6281b22
to
b0adbb0
Compare
Pushed changes that address all the comments. Going to work on the rebase and conflict resolution now so the two change sets are easier to review. |
b0adbb0
to
517e80c
Compare
Rebased and resolved conflicts. |
Added a fixup commit but not rebasing to avoid messing with reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yesssssssssss
ee8f41a
to
651dde3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yessssssssssssssssssssssssssssssssss 🐍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
651dde3
to
e301d9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
I think the test failure is missing container embeds in the cloud API. |
e301d9e
to
86d2d39
Compare
Fixed. I was missing the containers in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 👍
Inline the initializeManifest() function so we can start simplifying the PackageSets() and Manifest() (the two callers) separately.
The merging of payload repositories into the os pipeline had the unwanted side effect of using the payload repos for the first depsolve in the os chain when instead they should only be used for the second (the depsolve for the blueprint or workload packages). This wasn't an issue before because we didn't do the merging in the PackageSets() function, but now we rely on the Manifest() function for that functionality instead. Before the merging of the two functions, the PackageSets() function did not merge repositories and the repository-to-package-set mapping was maintained correctly, but the merging was needed in the Manifest() function so that rpm stage options were generated for all repositories. With this change, we are removing the merging so that the mapping is maintained, and will fix the rpm stage option generation in the pipeline generator.
In getPackageSetChain(), the workload repositories did not include the ExtraBaseRepos. In serialize(), when creating the rpm stage options (which collects repository GPG keys), only the base repos were used, which is why we previously had to merge repositories. Instead of merging repositories in the calling function in distro, we should keep them separated so that we can easily distinguish which repositories are only meant for the blueprint or workload when we need to.
There's no need to do anything if the options fail to validate, so do that first.
Use the new manifest generation procedure in the distro tests. Use assert instead of require in TestImageTypePipelineNames to continue running the rest of the subtests after a failure. Some initialisations (image options and blueprint customizations) had to be adjusted to work with the ImageType.Manifest() function. Some helper functions in distro_test_common are no longer necessary and have been removed. The TestPipelineRepositories and TestImageTypePipelineNames tests must be (partially) skipped for image types which specify a workload (currently only azure-eap7-rhui), because they ignore payload repositories.
The ImageType.PackageSets() function is going away and instead we will rely on the ImageType.Manifest() function to both prepare the manifest and return the package sets. The Manifest() function should never be called without an ostree ref for ostree type images.
Much like the GetPackageSetChains() manifest method, these two new methods collect the container and ostree source specifications from the pipelines that support them. Currently, only one pipeline per manifest contains references to containers or ostree commits, but we collect them in a map, keyed by the pipeline name, both for consistency with the package sets and for any potential future changes that may require differentiating which pipeline a content source belongs to.
When creating a Manifest object, collect container SourceSpecs instead of resolved Specs. This is the same way we handle packages: The blueprint option is converted to source specs and attached to the Manifest object during creation. Later, the SourceSpecs will be resolved to full container Specs and used during serialization.
Add a second argument, map[string][]container.Spec, during serialization, which serves the same purpose as the depsolved package sets.
Demonstrate the new workflow for resolving containers. 1. First call Manifest(). 2. Get container SourceSpecs from manifest struct. 3. Resolve them. 4. Serialize() with resolved container specs. The changes in the test manifests are just the information about the container sources (was a slice but is now a map) and the actual manifest object isn't affected. The TestDistro_Manifest test in distro_test_common is adapted accordingly as well.
Use the new workflow for generating the manifest before resolving containers. The resolver function is adjusted to handle a map of container slices, but with all current use cases, the map should only ever have one key for the payload (os) pipeline.
This has no effect since we don't use any containers in the test.
The arguments aren't used in the function anymore.
Silence linter warning.
Still not using the new process for generating the manifest exactly. This commit only replaces the call to PackageSets() with a call to Manifest() to get the Content.PackageSets. This is essentially the same as before, when we were initialising the manifest object twice. The Manifest() function does a tiny bit more work than PackageSets(), but it's minimal and we gain the benefit of only having a single code path and, although it's run twice, it's always run in the same way.
To add the container specs to Serialize(), we need to map them to the payload (OS) pipeline. We assume the first name in the image type's PayloadPipelines() list is the OS pipeline, which is true of all image types right now but might not be necessarily in the future. This is a temporary workaround. Eventually, the mapping will be set by the image type itself when we use the container source specs attached to the Manifest object.
Drop the PackageSets() function completely since it's no longer needed.
86d2d39
to
f321275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
We're back to the old bugs where things are getting a little messy from having to initialise the manifest twice. The cloud API was calling Feels a bit like it was a mistake to leave the cloud API operation re-ordering for a follow-up and have this inbetween state that has all the issues of the old method, but I still think we shouldn't make this PR any longer. |
Before, this was done in the PackageSets() function. The reason for this is that having an ostree ref affects package selection (for example, it adds rpm-ostree). At the package selection phase, it doesn't matter what the ostree ref is; it is just used to determine if a pipeline is for an ostree-based image type and it doesn't affect non-ostree-based image types because the image functions ignore it. This is only needed in the cloudapi now because other places have switched to using the new order of operations, where the manifest is generated after the ostree commit is resolved, so it's always added when needed.
95b3e91
to
ce10100
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest-diff
job [0] as manifests.diff
.
This PR is the biggest step towards moving the image definitions (the current
internal/distro/
package) into a separate repository. The main goal of this change was to simplify the manifest generation process and especially to avoid initialising the manifest twice (once inPackageSets()
and once inManifest()
). This lead to a lot of confusion (and ultimately bugs) because it was hard to understand the what exactly was happening in each separate initialisation.To this end, the following major changes were made:
PackageSets()
function of theImageType
interface is now gone.Manifest()
function returns amanifest.Manifest
struct instead of the serialized manifest ([]byte
).manifest.Manifest
struct holds all unresolved content specs, i.e., package sets, containers, and ostree commits.This changes the manifest generation process.
OLD process:
PackageSets()
to determine which package sets are needed for an image type.Manifest()
to generate and serialize manifest with resolved packages and containers.The container resolution was independent and could be done at any time before
Manifest()
. ThePackageSets()
function would generate a manifest object, get the packages from it, and discard it. It needed to "fake" certain things like ostree commit references and container specs.NEW process:
Manifest()
to determine which package sets are needed and collect container source specs (unresolved container specifications) and return amanifest.Manifest
.manifest
and resolve them separately.manifest.Serialize()
with the resolved content.Future changes
To keep this PR from getting even bigger, I decided to hold off some changes for future PRs.
Manifest()
function will create source specs to return with theManifest
object and the caller will resolve them and pass them in duringmanifest.Serialize()
.PackageSets()
call replaced byManifest()
. This will require a lot of work to get right and should be done separately. We're still initialising the manifest twice, but now at least the manifest generation is run the same way both times and should be easier to work with.Relevant links:
Image Type interface redesign discussion: #3386
Finalise interface between composer and image definitions: COMPOSER-1944
Split out Image Definitions: COMPOSER-1853
This pull request includes: