Update Cloud API with new manifest generation process #3482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schutzbot
reviewed
Jun 6, 2023
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
achilleas-k
force-pushed
the
cloudapi/job-order
branch
2 times, most recently
from
d0a35c6
to
9234ae7
Compare
schutzbot
reviewed
Jun 6, 2023
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
achilleas-k
force-pushed
the
cloudapi/job-order
branch
from
9234ae7
to
14ee89f
Compare
The function hasn't been in use since we updated the image types to the new definitions.
Same as with package specs and container specs, the commit specs are added to the manifest serialization after being resolved.
Pipelines that don't require packages didn't need to implement the serializeStart() method, but now we need to set the resolved ostree commit spec when a pipelines requires it.
The FetchChecksum on ostree.ImageOptions was the resolved commit ID of the parent ref to be pulled (for ostree commits and containers) or the commit ID of the content ref (for ostree installers and raw images). With the new process of manifest creation and serialisation, using the image options to transport resolved content references is bad and confusing. Image options should only reflect user and image type options before any references are resolved. With this change, the ostree.ImageOptions should only reflect the ostree-related options specified by the user. Commit IDs will only be available after the manifest is initialised when the commit sources are resolved (before serialisation).
Add an ostree commit source (instead of a resolved commit spec) to pipelines that support ostree commits. Source specs are used when initialising a manifest for package selection. The resolved commit spec is added after manifest initialisation through the serialization function for stage creation. Pipelines that require or support an ostree commit (either as payload or a parent) must return the source specs using getOSTreeCommitSources() after initialisation and the commit specs using getOSTreeCommits() during serialization.
The checksum won't be resolved before initialising the manifest (where checkOptions() is called), so we can't rely on it for validating options. Instead, we can just make sure that the URL was provided, when required by the image type. Also, if a parent ref is specified in the options, a URL *must* be specified regardless of image type, so verify this combination here and return the same error message that is returned from ostree.Resolve(). The parameter combination check will soon be removed from the ostree.Resolve() function.
When initialising a manifest, use the ostree commit source spec only. Then, when serialising, use the resolved ostree commit. This is the same process we use for the other content types, packages and containers. Two new functions are defined in the distros to handle resolving the ostree options: one for creating the parent commit source spec and image ref for image types that have an optional parent commit (ostree commits and containers) and one for creating the payload commit source spec for image types that require an ostree payload (ostree raw images and installers).
In tests (and dev tools) that apply to all image types, set just the ostree URL instead of all the options. The default ref is handled by the image functions when needed, so it doesn't need to be set from the caller.
achilleas-k
force-pushed
the
cloudapi/job-order
branch
from
14ee89f
to
db6d72a
Compare
schutzbot
reviewed
Jun 7, 2023
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
achilleas-k
commented
Jun 7, 2023
When a test manifest requires a commit to be resolved for content, fake the commit ID resolution deterministically by hashing the URL + ref. Store the resolved commit spec with the manifest metadata alongside the other content (packages and containers). Also add the secrets field if RHSM is true, which is now supposed to be done by the resolver.
Read the ostree options in the compose request into a copy of the struct that's used to write them. Read the ostree commits from the content part of the manifest files and use them in the serialisation function.
For ostree image types in the test distro, create an ostree source spec and return it with the rest of the content.
OSTree commits are now resolved after manifest initialisation just like packages and containers. The test commit hash handling is moved to the resolver function.
Use the commit sources provided by the content on the initialised manifest instead of the image options to resolve commits. The ostree sources on the manifest are a map keyed by the name of the pipeline that will use the resolved commit spec, but unlike with the package sets, the worker's commit resolve job works on a slice, so we reread the content map to get the pipeline name. This requires that there be only one pipeline that requires a resolved ostree commit, which is currently true of all our image types. Setting the default ostree ref on the image options before calling Manifest() isn't needed anymore.
Use the container sources provided by the content on the initialised manifest instead of the blueprint to resolve containers. The container sources on the manifest are a map keyed by the name of the pipeline that will use the resolved containers, but the worker's container resolve job works on a slice, so we reread the content map to get the pipeline name (instead of taking the first payload pipeline from the image type). This requires that there be only one pipeline that embeds containers, which currently true of all our image types.
Initialise the manifest only once in the enqueue functions (ImageType.Manifest()) and pass it to the manifest generation function with the workers and the dependency IDs. The function is renamed from generateManifest() to serializeManifest() to reflect its new functionality more accurately. The arguments to the function have also been trimmed since we no longer need the image type, blueprint, and image options. The new functionality of the function is to collect all the resolved content from the workers and serialize the manifest object.
Define a public pipeline implementation that allows initialising with content, serialising with resolved content, but produces no stages. This is useful for testing.
Use the new ContentTest pipeline to define a manifest instead of assigning content to the manifest directly.
The new test_distro's manifest produces a slightly different empty manifest when serialized even without content. Cloud API and Koji tests have been adapted to match. Weldr tests have been updated in several ways: - The test_distro content resolver is used to resolve manifest content before serializing. - The test scenarios in TestCompose have been named for easier troubleshooting (easier to identify a failing test by name). - Manifests that work with the secondary ostree repo (the "other") use the appropriate URL and ref and create a secondary "other" serialized manifest. The weldr API's test flag for resolving ostree commits does not produce the same, fixed hash every time but instead computes a sha256 from the URL + ref, like we do in the test manifests.
- Remove redundant types in struct literals (linter warning) - Fixed indentation in json string literals
Do not expose the content of the manifest statically and instead rely on the public methods to retrieve source specifications dynamically. Since the methods require iterating through the pipelines to collect source specifications, we should avoid calling the function multiple times when we can reuse the returned values.
Add a Distro enum to the Manifest struct for selecting package selection. Packages are sometimes renamed between distribution versions and since we do the package selection in the Manifest, we need a way to select distro-version-specific package names inside the manifest initialiser. This may change in the future.
There's nothing parent-y about what we're resolving. It's the checksum of a ref in a given repository.
The ostree options for a compose request should be specified using the ImageOptions struct, not the source spec. The source spec should be specifying the source parameters for a single ostree commit internally.
Test all combinations of ostree options and verify the ostree commit source spec returned by the manifest.
The Resolve() function is now only responsible for resolving a SourceSpec to a CommitSpec. It only resolves a checksum if a URL is set and sets the option for the RHSM secrets. The Parent has been removed from the SourceSpec. The SourceSpec is a simple reference to a single ostree ref and has no connection with the ostree options.
Change the OSTreeResolveSpec to match the ostree SourceSpec by removing the Parent field. Change OSTreeResolveResultSpec to match the CommitSpec by adding the Secrets field. The RHSM field is kept for backwards compatibility with older workers.
Use ostree.ImageOptions for the request parameters instead of a SourceSpec on the imageRequest. When preparing the image request, add the ostree values from the API's compose request to the ostree options on the image options of the image request. It's not necessary to create a source spec and it's also not necessary to add the default ref when it's not specified in the request for an ostree-based image type. Both of these will be handled by the Manifest generation based on the ostree options (imageOptions.OSTree). The image functions will take care of setting any missing parameters or returning errors if any required parameters are missing.
Properly resolve any ostree commit checksums. Don't make the user specify the checksum through the parent.
Add a checksum as a hash of URL + Ref. Use the parent ref instead of the image ref when it's set. This makes the test distro always behave like ostree commit and container types (image types that can use an ostree parent) and not raw image or installers (that use ostree commits as a payload). Modify the weldr API test with the expected error message.
Don't specify ref ID for parent, instead only specify a ref and URL and let the manifest generator resolve the ID using the sha256 hash.
Edge and IoT manifests are modified from the new option handling. The "parent" commit ID isn't specified in the options anymore, but it is (fake) resolved by the manifest generator. Of particular note is the iot-raw-image manifest that now properly uses the commit ID in the copy stage for the firmware. The resolved ostree commits are now stored in the content part of the manifest metadata alongside package specs and containers.
achilleas-k
force-pushed
the
cloudapi/job-order
branch
from
db6d72a
to
4521c34
Compare
schutzbot
reviewed
Jun 7, 2023
There was a problem hiding this comment.
Manifest-diff job [0] as manifests.diff.
ondrejbudai
reviewed
Jun 13, 2023
| // Create an ostree SourceSpec to define an ostree parent commit using the user | ||
| // options and the default ref for the image type. Additionally returns the | ||
| // ref to be used for the new commit to be created. | ||
| func makeOSTreeParentCommit(options *ostree.ImageOptions, defaultRef string) (*ostree.SourceSpec, string) { |
There was a problem hiding this comment.
I wonder if this function should live in ostree, so the other packages can just freely use it. The current state creates quite a bit of code duplication.
There was a problem hiding this comment.
Yeah, I considered it but didn't do it but since you brought it up too, I'll move it and reuse it.
| } | ||
| return id, HTTPErrorWithInternal(ErrorEnqueueingJob, fmt.Errorf("manifest returned %d pipelines with ostree commits (at most 1 is supported): %s", len(commitSources), strings.Join(pipelines, ", "))) | ||
| } | ||
| for _, sources := range commitSources { |
There was a problem hiding this comment.
Is a loop really needed? len(commitSources) is always <= 1`.
There was a problem hiding this comment.
Yeah, let's drop the loop.
| } | ||
| return id, HTTPErrorWithInternal(ErrorEnqueueingJob, fmt.Errorf("manifest returned %d pipelines with ostree commits (at most 1 is supported): %s", len(commitSources), strings.Join(pipelines, ", "))) | ||
| } | ||
| for _, sources := range commitSources { |
| var ostreeCommitPipeline string | ||
| for name := range manifest.Content.OSTreeCommits { | ||
| ostreeCommitPipeline = name | ||
| break |
croissanne
approved these changes
Jun 13, 2023
3 tasks
|
Let's do the fixes in a follow-up, they are not functional changes, just stylistic things. |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up to #3444 and completes the redesign of the image definition interface.
There's two major changes here:
OSTree options
Until now, the ostree option handling was a bit weird. We would take what the user provided (ref, url, parent ref), resolve any commit ID we might need, then put it back on the image options. Also, when an image required an ostree ref to be specified, we read the default one off the image type and put it in the options if it wasn't already set. I've changed this now so that the ostree options always hold only what the user provided. Using the default ref when an image type needs it is a responsibility of the image type function (each corresponding
imageFuncimplementation). The options will never be used for resolved information. So the process now is:Manifest()function with ostree options exactly as the user specified them (just like we do with the blueprint for example)checkOptions()returns an error if an ostree URL was required and not specified (for ostree installers and raw images which require an ostree commit payload to be pulled)checkOptions()also returns an error if a parent ref was provided without a URLManifest()function passes the ostree options along with the image options without modificationimageFuncconstructs the ostree commit source specification from the ostree optionsmanifest.Serialize()takes a resolved ostree commit spec to serialise into the manifest as content (like we do with package specs and resolved container specs)This unifies the handling of all three content types and makes it easier to separate user options from manifest values.
Note that the ostree
SourceSpecdoesn't define aParentanymore. The ostree options and structs have been changed and reused (and misused) for a very long time. TheSourceSpecsued to be the image options before we had ostreeImageOptionsand theParentused to be used for a checksum as well as a parent ref. TheSourceSpecnow is only used to resolve commit checksums so that aCommitSpeccan be created to serialize a manifest. The options are only ever set on the ostreeImageOptions. If a commit checksum must be resolved to be a parent for a new commit, a wholeSourceSpecshould be defined to represent the parent. The same applies to payload commits for installers and raw images.Cloud API manifest generation
The manifest initialisation now only happens once in the Cloud API. The manifest is initialised in the composer handler and the content getters (e.g.,
GetPackageSetChains()) are used to retrieve content source specifications and set up the resolve jobs on the worker queue. The initialised manifest is passed through to theserializeManifest()function (which was previously calledgenerateManifest()) along with the dependency job IDs. Once each resolve job is finished, the manifest is serialised with the content specs.Other changes
Package selection
The
Content, which held the source specs for all the content for aManifestwas removed. Content sources aren't stored statically on theManifestanymore and can only be dynamically retrieved using theGet...()functions. Since package selection happens in the manifest package and can't be changes externally, aDistroenum is defined and added to theManifestso that package selection can add packages with older names when necessary (python3-pytomlfor EL8 andpython3-PyYAMLfor EL7).I'm still not completely settled on this solution, but this change has the smallest footprint of all the alternatives I considered so it makes it easier to revert if we decide to go another way.
This pull request includes: