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
CFE-783: A variety of changes needed for correct operation with multi architecture catalogs #611
CFE-783: A variety of changes needed for correct operation with multi architecture catalogs #611
Conversation
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jchunkins: This pull request references CFE-783 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Hi John,
Thank you for an overall great job explaining and commenting on the code to make this improvement easy.
I have one question overall, and moving on to some testing. @zhouying7780 will also be helping us out
8059d85
to
d22ba27
Compare
/retest |
Functional Changes pkg/cli/mirror/fbc_operators.go - remove findFBCConfig & replace with extractDeclarativeConfigFromImage This is necessary because the layer processing did not handle whiteout files. Switched to go-containerregistry which handles this correctly. - modify getManifest so that it is fat manifest aware, and returns the first manifest in the list, or for single images, returns that manifest - Removed functions that are no longer necessary: GetCatalogConfigPath, getConfigPathFromConfigLayer, UntarLayers pkg/cli/mirror/create.go - make createOlmArtifactsForOCI manifest list aware and discovers the first image in the list, or for single images, uses that image directly - calls replacement function extractDeclarativeConfigFromImage - stores full path to the artifact in a map so this path does not need to be recalculated later pkg/cli/mirror/operator.go - make use of operatorCatalogToFullArtifactPath map to obtain the full path to the olm_artifacts directory and its contents and remove getOperatorCatalogRef() since it is no longer necessary - make writeLayout() use the image config file to discover the platform when adding images, otherwise fall back to linux/amd64 pkg/cli/mirror/options.go - add operatorCatalogToFullArtifactPath map[string]string pkg/image/image.go - replace getManifest with getFirstDigestFromPath which now gets the first digest for manifest lists, or the image digest for single image - change logging behavior in ParseReference() when path does not exist and invalidate the ID in that case. Non-functional Changes pkg/cli/mirror/mirror.go - removed redundant AsRespoitory() call since RepositoryName() already does this - use constant for olm_artifacts directory pkg/cli/options.go - use constant for workspace all files - add documentation and change formatting for long function signatures - if any file used deprecated io/ioutil, replace with io or os package as necessary - update unit tests as necessary (the bulk of the files are related to new test cases)
- The original PR had concerns about using latest tag, so did not bring this change over, therefore this test case had to be reverted too.
This commit brings over the remaining changes needed for correct manifest list handling. internal/testutils/testutils.go - update and create new functions for creating single/multiarch images - created *WithURL function variants to allow pre-existing registry in tests because the defaults server won't allow external calls while debugging pkg/cli/mirror/catalog_images.go - in rebuildCatalogs, make sure that the catalog source docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/cli/mirror/create.go - add missing call to strip protocol from ctlg.OCIFBCPath when constructing a layoutPath pkg/cli/mirror/mirror.go - add missing initialization of operatorCatalogToFullArtifactPath pkg/image/image.go - in ParseReference, make sure that the docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/image/builder/image_builder.go - make image builder handle single arch and multi arch images pkg/image/builder/image_builder_test.go - update test cases for image builder update go modules and vendoring
- make function return error when number of manifests > 1 - do basic checks to ensure digest is valid before returning - return image digest instead of config hash - adjust documentation
When copying the OCI layout, make sure we only copy files/folders that are part of the OCI layout specification. The bug resulted because the copy destination was inside the source directory, which lead to an infinite circular copy operation when using the github.com/otiai10/copy package. This fix avoids this because it copies only the OCI layout files/folders and ignores others.
- based on review, make inner function processImageIndex into a method of ImageBuilder - add/update documentation
fee0a75
to
269e67c
Compare
@sherine-k I performed another rebase from main to make sure everything is in sync, and then did a line by line comparison of the files in Please let me know if there was something else that does not seem correct. |
/lgtm |
/payload 4.14 nightly informing |
@sherine-k: trigger 55 job(s) of type informing for the nightly release of OCP 4.14
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f6cb7370-0474-11ee-999e-3debd10eddbe-0 |
/payload 4.14 nightly blocking In general blocking is probably the better signal for pre-merge testing. (expensive, but worth if it we're worried this might cause regressions) Hoping to return to this in one of the next batches. |
@dgoodwin: trigger 7 job(s) of type blocking for the nightly release of OCP 4.14
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c4047590-05ff-11ee-970a-891597cb4abb-0 |
Hi @dgoodwin |
/label jira/valid-bug |
@jchunkins: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
… architecture catalogs (openshift#611) * A variety of changes needed for correct operation Functional Changes pkg/cli/mirror/fbc_operators.go - remove findFBCConfig & replace with extractDeclarativeConfigFromImage This is necessary because the layer processing did not handle whiteout files. Switched to go-containerregistry which handles this correctly. - modify getManifest so that it is fat manifest aware, and returns the first manifest in the list, or for single images, returns that manifest - Removed functions that are no longer necessary: GetCatalogConfigPath, getConfigPathFromConfigLayer, UntarLayers pkg/cli/mirror/create.go - make createOlmArtifactsForOCI manifest list aware and discovers the first image in the list, or for single images, uses that image directly - calls replacement function extractDeclarativeConfigFromImage - stores full path to the artifact in a map so this path does not need to be recalculated later pkg/cli/mirror/operator.go - make use of operatorCatalogToFullArtifactPath map to obtain the full path to the olm_artifacts directory and its contents and remove getOperatorCatalogRef() since it is no longer necessary - make writeLayout() use the image config file to discover the platform when adding images, otherwise fall back to linux/amd64 pkg/cli/mirror/options.go - add operatorCatalogToFullArtifactPath map[string]string pkg/image/image.go - replace getManifest with getFirstDigestFromPath which now gets the first digest for manifest lists, or the image digest for single image - change logging behavior in ParseReference() when path does not exist and invalidate the ID in that case. Non-functional Changes pkg/cli/mirror/mirror.go - removed redundant AsRespoitory() call since RepositoryName() already does this - use constant for olm_artifacts directory pkg/cli/options.go - use constant for workspace all files - add documentation and change formatting for long function signatures - if any file used deprecated io/ioutil, replace with io or os package as necessary - update unit tests as necessary (the bulk of the files are related to new test cases) * reverted test case result - The original PR had concerns about using latest tag, so did not bring this change over, therefore this test case had to be reverted too. * bring over remaining changes This commit brings over the remaining changes needed for correct manifest list handling. internal/testutils/testutils.go - update and create new functions for creating single/multiarch images - created *WithURL function variants to allow pre-existing registry in tests because the defaults server won't allow external calls while debugging pkg/cli/mirror/catalog_images.go - in rebuildCatalogs, make sure that the catalog source docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/cli/mirror/create.go - add missing call to strip protocol from ctlg.OCIFBCPath when constructing a layoutPath pkg/cli/mirror/mirror.go - add missing initialization of operatorCatalogToFullArtifactPath pkg/image/image.go - in ParseReference, make sure that the docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/image/builder/image_builder.go - make image builder handle single arch and multi arch images pkg/image/builder/image_builder_test.go - update test cases for image builder update go modules and vendoring * fix test case error message * Change getFirstDigestFromPath based on feedback - make function return error when number of manifests > 1 - do basic checks to ensure digest is valid before returning - return image digest instead of config hash - adjust documentation * fix test case * fix uninitialized error * fix test case after rebase * Fix OCPBUGS-13198 When copying the OCI layout, make sure we only copy files/folders that are part of the OCI layout specification. The bug resulted because the copy destination was inside the source directory, which lead to an infinite circular copy operation when using the github.com/otiai10/copy package. This fix avoids this because it copies only the OCI layout files/folders and ignores others. * refactor inner function to standalone per feedback * refactor processImageIndex to be a method - based on review, make inner function processImageIndex into a method of ImageBuilder - add/update documentation * remove unused getManifest func and test case
… operation with multi… (#661) * CFE-783: A variety of changes needed for correct operation with multi architecture catalogs (#611) * A variety of changes needed for correct operation Functional Changes pkg/cli/mirror/fbc_operators.go - remove findFBCConfig & replace with extractDeclarativeConfigFromImage This is necessary because the layer processing did not handle whiteout files. Switched to go-containerregistry which handles this correctly. - modify getManifest so that it is fat manifest aware, and returns the first manifest in the list, or for single images, returns that manifest - Removed functions that are no longer necessary: GetCatalogConfigPath, getConfigPathFromConfigLayer, UntarLayers pkg/cli/mirror/create.go - make createOlmArtifactsForOCI manifest list aware and discovers the first image in the list, or for single images, uses that image directly - calls replacement function extractDeclarativeConfigFromImage - stores full path to the artifact in a map so this path does not need to be recalculated later pkg/cli/mirror/operator.go - make use of operatorCatalogToFullArtifactPath map to obtain the full path to the olm_artifacts directory and its contents and remove getOperatorCatalogRef() since it is no longer necessary - make writeLayout() use the image config file to discover the platform when adding images, otherwise fall back to linux/amd64 pkg/cli/mirror/options.go - add operatorCatalogToFullArtifactPath map[string]string pkg/image/image.go - replace getManifest with getFirstDigestFromPath which now gets the first digest for manifest lists, or the image digest for single image - change logging behavior in ParseReference() when path does not exist and invalidate the ID in that case. Non-functional Changes pkg/cli/mirror/mirror.go - removed redundant AsRespoitory() call since RepositoryName() already does this - use constant for olm_artifacts directory pkg/cli/options.go - use constant for workspace all files - add documentation and change formatting for long function signatures - if any file used deprecated io/ioutil, replace with io or os package as necessary - update unit tests as necessary (the bulk of the files are related to new test cases) * reverted test case result - The original PR had concerns about using latest tag, so did not bring this change over, therefore this test case had to be reverted too. * bring over remaining changes This commit brings over the remaining changes needed for correct manifest list handling. internal/testutils/testutils.go - update and create new functions for creating single/multiarch images - created *WithURL function variants to allow pre-existing registry in tests because the defaults server won't allow external calls while debugging pkg/cli/mirror/catalog_images.go - in rebuildCatalogs, make sure that the catalog source docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/cli/mirror/create.go - add missing call to strip protocol from ctlg.OCIFBCPath when constructing a layoutPath pkg/cli/mirror/mirror.go - add missing initialization of operatorCatalogToFullArtifactPath pkg/image/image.go - in ParseReference, make sure that the docker reference uses lower case values for "name" and "namespace" which is required to make valid docker references. This covers the case where a OCI filepath uses uppercase letters in its path. pkg/image/builder/image_builder.go - make image builder handle single arch and multi arch images pkg/image/builder/image_builder_test.go - update test cases for image builder update go modules and vendoring * fix test case error message * Change getFirstDigestFromPath based on feedback - make function return error when number of manifests > 1 - do basic checks to ensure digest is valid before returning - return image digest instead of config hash - adjust documentation * fix test case * fix uninitialized error * fix test case after rebase * Fix OCPBUGS-13198 When copying the OCI layout, make sure we only copy files/folders that are part of the OCI layout specification. The bug resulted because the copy destination was inside the source directory, which lead to an infinite circular copy operation when using the github.com/otiai10/copy package. This fix avoids this because it copies only the OCI layout files/folders and ignores others. * refactor inner function to standalone per feedback * refactor processImageIndex to be a method - based on review, make inner function processImageIndex into a method of ImageBuilder - add/update documentation * remove unused getManifest func and test case * OCPBUGS-13762: make addRelatedImageToMapping multithreaded (#638) * fix for OCPBUGS-13762 - make execution of addRelatedImageToMapping multi threaded - make addRelatedImageToMapping take a sync.Map to support concurrency - convert sync.Map back to image.TypedImageMapping after processing - fix bug so that we actually use the mirror value if we found one * add missing checkin for test case * run make tidy --------- Co-authored-by: John Hunkins <jhunkins@us.ibm.com>
Description
This change brings support for processing multi and single architecture catalog images within oc mirror.
TL;DR: The first item in the manifest list has its FBC catalog extracted, which is then processed as needed and then “reinserted” (i.e. added as a new layer) into each image referenced in the manifest list. In other words, the images are modified with the same updated FBC content. The binaries within each architecture catalog are unmodified.
To understand the changes in this PR, we need to discuss OCI layouts, which is a on disk representation of image references. The OCI layout specification shares similar concepts with the docker image v2s2 specification in that they both define manifests (i.e. a JSON document) that describe a single image or a list of images (i.e. a fat manifest). The primary difference between the two standards is the
mediaType
which indicates the standard being used (you can see the compatibility matrix for details). Essentially, OCI attempts to keep backwards compatibility with docker, and adds its own enhancements (e.g. annotations). These manifests can be pushed/pulled to/from a remote docker registry in either format, and can be converted from one type to another if desired. In the diagram above, you can see that manifest lists and image manifests are the entities that can be moved into and out of a OCI layout on a file system.An OCI layout consists of an index.json file that acts as the entry point for discovering what images are stored within the layout on the disk. This index.json file can reference any number of single or multi architecture images, however oc mirror only references a single image at a time in a given OCI layout. The image stored in the OCI layout can be either a single architecture image (i.e. an image manifest) or a multi architecture image (i.e. a manifest list). A manifest list image is a logical grouping of an architecture specific image manifests stored in an array. This arrangement allows a single digest reference to represent all of the architecture specific images, and tools such as skopeo or podman can automatically pull an image that matches the platform that the tool is being executed on. An individual architecture specific image is referred to as an image manifest. A image manifest has references to a config JSON file that describes detailed layer history, the platform the image runs on and runtime information necessary to start an image. A image manifest, also has references to one or more layers, which are just tar files, which could be compressed or uncompressed. As you can see in the above diagram, all of these internal references are by digest, which is a hash computed by taking a sha256sum of the manifest or tar file. Note that the digest of the config file itself is referred to as the "image id" whereas an image digest is hash of the image manifest itself.
An example of a OCI layout on disk looks like this:
The index.json will have references to manifests and tar files that reside in the
blobs/sha256
directory. Technically the index.json file is its own form of manifest list, but it is important to note that the index.json file itself has no representation in a docker registry. Tools such as skopeo will generate a OCI layout using what I call "manifest list indirection" where the manifest list resides in the blobs directory and the index.json references this manifest list by its digest. However some tools will set the media type of the index.json to indicate that its a manifest list and not use this "indirection" (i.e. the index.json directly refers to the image manifests and includes the platform information directly). This PR handles both of these scenarios, and ensures that the appropriate images are processed.OCI layouts are stored in the oc mirror workspace under
<oc-mirror-workspace>/src/catalogs/<repoPath>/layout
. A diagram of this workspace looks like this:At a high level, this PR allows oc mirror traverse the OCI layout to pick an image for processing depending on what's stored in the layout:
Once an image is chosen, the declarative config (i.e. the JSON/YAML files that represent the OLM catalog) is extracted from the image for processing. The existing code which performed this extraction was done incorrectly, and this PR addresses this issue. When an OCI image is referenced in the ImageSetConfiguration, and the
--use-oci-feature
is set, this declarative config is placed into<current working directory>/olm_artifacts/<repo>/<config folder>
which acts as a temporary location to store the extracted data. After the catalog is processed, the content is written to<oc-mirror-workspace>/src/catalogs/<repoPath>/index
. This catalog content is then added to ALL image manifests that are stored within the OCI layout by adding new layers that represent the updated catalog content. This is handled within the ImageBuilder, which has been modified to handle image updates for both single and multi arch images. Other changes within the PR relate to properly handling file paths and docker reference parsing.Also, to be clear, additional images defined in the ImageSetConfiguration, and related images found within the catalog itself are already capable of mirroring single and multi platform images already because the "oc image mirror" codebase knows how to do this already. This PR is primarily focused on making catalog image handling multi architecture "aware".
User Stories
Acceptance Criteria
Functional Changes
pkg/cli/mirror/fbc_operators.go
pkg/cli/mirror/create.go
pkg/cli/mirror/options.go
operatorCatalogToFullArtifactPath map[string]string
which stores temporary paths to declarative config directorypkg/cli/mirror/operator.go
operatorCatalogToFullArtifactPath
operatorCatalogToFullArtifactPath
map to obtain the full path to the olm_artifacts directory and its contents and remove getOperatorCatalogRef() since it is no longer necessarypkg/image/image.go
pkg/cli/mirror/catalog_images.go
pkg/image/builder/image_builder.go
Non-functional Changes
pkg/cli/mirror/mirror.go
pkg/cli/options.go
all files
Test Changes
internal/testutils/testutils.go
pkg/image/builder/image_builder_test.go
Fixes # (CFE-783)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: