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

Fix layer-related issues for oci_rebase_image #50

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Aug 11, 2023

Specifically:

  • Make sure we're using platforms.OnlyStrict instead of platforms.Only when constructing the platform matcher, because with Only, arm/v8 will also match the other arm/* platforms, and, more immediately relevant, amd64 will match the first item in the index with either amd64 or 386. Which is...not what we want, obviously.
  • When generating the build files in GenerateBuildFilesCmd, don't add oci_blobs and traverse children for blobs which we've already added an oci_blob and traversed for. If there's an identical layer in more than one platform's image, and we've got shallow set to false (so that we're pulling layers as well as configs/indexes/manifests), the generated BUILD.bazel will end up invalid if we don't do this, because there'll be duplicate oci_blob rules (and duplicate rules for children, but in practice I didn't run into that because the only duplicates I hit in testing were layers).
  • Add the OCI base image name/digest annotations to the layers we're copying from the original image to the rebased image. CopyContent is leveraging those annotations to determine when we can avoid actually pushing a layer to the registry in favor of using Mount, and we're already doing that for the base image layers in AppendLayers.

Note that there are still some interesting/annoying issues or quirks, and not just with oci_rebase_image:

  • With either oci_image or oci_rebase_image followed by an oci_push, if you're pushing to a different registry than the image(s) containing the pre-existing layers in the new image, you'll get a confusing error (failed to push child content to registry: failed to create reader from ingestor: not found). I think this is because the Mount fails, because you can't cross-repo mount a blob from a different registry host, and oci_push doesn't know where to find the layers from the pre-existing image(s) locally. I'm still trying to unravel that one, but am guessing it's happening because the layout passed to ocitool push doesn't contain the actual layers from the pre-existing image(s) and I'm not yet sure how to solve that.
  • With oci_rebase_image, the oci_pull for the original image must have shallow = False, or RebaseImage's call to AppendLayers can't locate the diff ID for the layers from the original image, for the fairly obvious in retrospect reason that...we're not pulling the layers unless we pass shallow = False to oci_pull. Oops.

Specifically:
* Make sure we're using `platforms.OnlyStrict` instead of `platforms.Only` when constructing the platform matcher, because with `Only`, `arm/v8` will also match the other `arm/*` platforms, and, more immediately relevant, `amd64` will match the first item in the index with _either_ `amd64` or `386`. Which is...not what we want, obviously.
* When generating the build files in `GenerateBuildFilesCmd`, don't add `oci_blob`s and traverse children for blobs which we've already added an `oci_blob` and traversed for. If there's an identical layer in more than one platform's image, and we've got `shallow` set to false (so that we're pulling layers as well as configs/indexes/manifests), the generated `BUILD.bazel` will end up invalid if we don't do this, because there'll be duplicate `oci_blob` rules (and duplicate rules for children, but in practice I didn't run into that because the only duplicates I hit in testing were layers).
* Add the OCI base image name/digest annotations to the layers we're copying from the original image to the rebased image. `CopyContent` is leveraging those annotations to determine when we can avoid actually pushing a layer to the registry in favor of using `Mount`, and we're already doing that for the base image layers in `AppendLayers`.

Note that there are still some interesting/annoying issues or quirks, and not just with `oci_rebase_image`:
* With either `oci_image` or `oci_rebase_image` followed by an `oci_push`, if you're pushing to a different registry than the image(s) containing the pre-existing layers in the new image, you'll get a confusing error (`failed to push child content to registry: failed to create reader from ingestor: not found`). I think this is because the `Mount` fails, because you can't cross-repo mount a blob from a different registry host, and `oci_push` doesn't know where to find the layers from the pre-existing image(s) locally. I'm still trying to unravel that one, but am guessing it's happening because the layout passed to `ocitool push` doesn't contain the actual layers from the pre-existing image(s) and I'm not yet sure how to solve that.
* With `oci_rebase_image`, the `oci_pull` for the original image _must_ have `shallow = False`, or `RebaseImage`'s call to `AppendLayers` can't locate the diff ID for the layers from the original image, for the fairly obvious in retrospect reason that...we're not pulling the layers unless we pass `shallow = False` to `oci_pull`. Oops.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer
Copy link
Contributor Author

abayer commented Aug 11, 2023

cc @jprobinson - given the messiness so far, I'm not sure if it doesn't just make sense to remove oci_rebase_image and give up on this. Needs more thought.

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.

1 participant