Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Jun 23, 2021

We can't use:

$ oc adm release mirror ...

to split the release image out into LOCAL_RELEASE_IMAGES_REPOSITORY, because oc does not currently understand mirroring. So there's no way to tell it "the referenced images you're looking for are actually in ${LOCAL_REGISTRY}/${TEMP_REPOSITORY}", and it fails with:

error: unable to connect to quay.io/openshift-release-dev/ocp-v4.0-art-dev: Get "https://quay.io/v2/"...
error: an error occurred during planning

Instead, send the images from disk directly to the target repositoy for referenced images, and then use oc image mirror again to copy the release image over to the release-image repository without worrying about referenced images. We still need the release images in that separate repository to avoid Cincinnati consuming excessive memory trying to load referenced-image layers as if they were release images (comment, rhbz#1850781).

I've also softened some REMOVABLE_MEDIA_PATH wording. The important point is that REMOVABLE_MEDIA_PATH points to where the removable media is mounted for both commands, not that you use the same path string. For example, maybe you mounted to /mnt/a on your external machine, and then mounted that media to /mnt/b on your internal machine. You'd want to use /mnt/b for REMOVABLE_MEDIA_PATH when mirroring from disk, not /mnt/a.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 23, 2021
@netlify
Copy link

netlify bot commented Jun 23, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: d49d9d6

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/61004f33af888e0007bffb50

😎 Browse the preview: https://deploy-preview-33841--osdocs.netlify.app/openshift-enterprise/latest/updating/installing-update-service

@LalatenduMohanty
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2021
@wking
Copy link
Member Author

wking commented Jun 23, 2021

@jiajliu, can you take a look? Because this is tweaking the original implementation that responded to your comment.

@jiajliu
Copy link

jiajliu commented Jun 24, 2021

I don't remember we ever hit the issue when using oc adm release mirror. For the known OOM issue, i remember oc adm release mirror can work well as long as we make --to and --to-release-image into different repos. I think we need a reproduce first to identify if anything missing here. @shellyyang1989 Could you follow up the steps in our doc and try to reproduce the issue in this pr?

@shellyyang1989
Copy link

Checking on:
oc adm release mirror -a ${LOCAL_SECRET_JSON} --from=${LOCAL_REGISTRY}/${TEMP_REPOSITORY}:${OCP_RELEASE}-${ARCHITECTURE} --to=${LOCAL_REGISTRY}/${LOCAL_REPOSITORY} --to-release-image=${LOCAL_REGISTRY}/${LOCAL_RELEASE_IMAGES_REPOSITORY}:${OCP_RELEASE}-${ARCHITECTURE}

It works and --from specifies the referenced image.

Anyway, oc image mirror works as well. The service can be created successfully w/o OOM error.

LGTM.

@shellyyang1989
Copy link

Checking on:
oc adm release mirror -a ${LOCAL_SECRET_JSON} --from=${LOCAL_REGISTRY}/${TEMP_REPOSITORY}:${OCP_RELEASE}-${ARCHITECTURE} --to=${LOCAL_REGISTRY}/${LOCAL_REPOSITORY} --to-release-image=${LOCAL_REGISTRY}/${LOCAL_RELEASE_IMAGES_REPOSITORY}:${OCP_RELEASE}-${ARCHITECTURE}

It works and --from specifies the referenced image.

Correcting it. I can reproduce the issue. oc adm release mirror fails on a mirror host which does not have internet access.

@wking wking force-pushed the update-service-release-image-mirror-fix branch from 7ecf7c9 to 5fa060b Compare July 7, 2021 22:59
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@shellyyang1989
Copy link

LGTM.

@jiajliu
Copy link

jiajliu commented Jul 9, 2021

NeedsTestCase

Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor formatting comment, but LGTM!

@bergerhoffer bergerhoffer added the peer-review-done Signifies that the peer review team has reviewed this PR label Jul 27, 2021
@bergerhoffer bergerhoffer added this to the Next Release milestone Jul 27, 2021
@bergerhoffer
Copy link
Contributor

@shellyyang1989 @jiajliu @soltysh Can anyone confirm whether this update should be applied to all supported OCP version (4.6+)?

…the release image

We can't use:

  $ oc adm release mirror ...

to split the release image out into LOCAL_RELEASE_IMAGES_REPOSITORY,
because oc does not currently understand mirroring.  So there's no way
to tell it "the referenced images you're looking for are actually in
${LOCAL_REGISTRY}/${TEMP_REPOSITORY}", and it fails with:

  error: unable to connect to quay.io/openshift-release-dev/ocp-v4.0-art-dev: Get "https://quay.io/v2/"...
  error: an error occurred during planning

Instead, send the images from disk directly to the target repositoy
for referenced images, and then use 'oc image mirror' again to copy
the release image over to the release-image repository without
worrying about referenced images.  We still need the release images in
that separate repository to avoid Cincinnati consuming excessive
memory trying to load referenced-image layers as if they were release
images [1,2].

I've also softened some REMOVABLE_MEDIA_PATH wording.  The important
point is that REMOVABLE_MEDIA_PATH points to where the removable media
is mounted for both commands, not that you use the same path string.
For example, maybe you mounted to /mnt/a on your external machine, and
then mounted that media to /mnt/b on your internal machine.  You'd
want to use /mnt/b for REMOVABLE_MEDIA_PATH when mirroring from disk,
not /mnt/a.

[1]: openshift#29630 (comment)
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=1850781
@wking wking force-pushed the update-service-release-image-mirror-fix branch from 5fa060b to d49d9d6 Compare July 27, 2021 18:23
@jiajliu
Copy link

jiajliu commented Jul 28, 2021

@bergerhoffer I think it's good to apply this update to OCP v4.6/v4.7/v4.8 since osus is supported on v4.6+.

@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.9

@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.8

@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.7

@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #34971

Details

In response to this:

/cherrypick enterprise-4.9

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.

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #34972

Details

In response to this:

/cherrypick enterprise-4.8

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.

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #34973

Details

In response to this:

/cherrypick enterprise-4.7

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.

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #34974

Details

In response to this:

/cherrypick enterprise-4.6

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.

@wking wking deleted the update-service-release-image-mirror-fix branch July 29, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 branch/enterprise-4.9 peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants