search

openshift / oc-mirror

Lifecycle manager for internet-disconnected OpenShift environments
Apache License 2.0
88 stars 80 forks source link

CLID-101: Fix graph image mirroring after TLS verification enabling #840

Closed sherine-k closed 5 months ago

sherine-k commented 5 months ago

Description

Fixes # CLID-101

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration:

kind: ImageSetConfiguration
apiVersion: mirror.openshift.io/v2alpha1
mirror:
  platform:
    channels:
    - name: stable-4.13
    graph: true
  operators:
  - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14
    packages:
    - name: aws-load-balancer-operator
    - name: external-dns-operator
  additionalImages:
  - name: registry.redhat.io/ubi8/ubi:latest

Expected Outcome

Please describe the outcome expected from the tests

openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk: > * The error that was encountered was because in the specific case of the graph image, src and dst are both localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) to the cache registry (here tls-verify=false). > * The graph image is built and pushed to the cache during the collection phase (graph.go) > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk. >* Fix graph image mirroring during MirrorToMirror. > * Set up the cache registry for mirrorToMirror, because the graph image is going to be built and pushed there > * Set TLSVerify to false for the source image, when mirroring the graph image >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk: > * The error that was encountered was because in the specific case of the graph image, src and dst are both localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) to the cache registry (here tls-verify=false). > * The graph image is built and pushed to the cache during the collection phase (graph.go) > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk. >* Fix graph image mirroring during MirrorToMirror. > * Set up the cache registry for mirrorToMirror, because the graph image is going to be built and pushed there > * Set TLSVerify to false for the source image, when mirroring the graph image >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk: > * The error that was encountered was because in the specific case of the graph image, src and dst are both localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) to the cache registry (here tls-verify=false). > * The graph image is built and pushed to the cache during the collection phase (graph.go) > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk. >* Fix graph image mirroring during MirrorToMirror. > * Set up the cache registry for mirrorToMirror, because the graph image is going to be built and pushed there > * Set TLSVerify to false for the source image, when mirroring the graph image >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >Out of scope: While fixing this issue, I found out that generation of UpdateService.yaml file is broken for MirrorToMirror workflow. The root cause is that the imageSetConfig is getting modified by the collector (adds minVersion and maxVersion) which results in a miss while searching for the release filter file under working-dir. > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk and MirrorToMirror: > * The error that was encountered was because in the specific case of the graph image, src of the image is localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) . > * The graph image is already built and pushed to the destination registry during the collection phase (graph.go) > * destination registry is the cache for the case of mirrorToDisk > * destination registry is the remote mirror for the case of mirrorToMirror > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk and MirrorToMirror. >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >Out of scope: While fixing this issue, I found out that generation of UpdateService.yaml file is broken for MirrorToMirror workflow. The root cause is that the imageSetConfig is getting modified by the collector (adds minVersion and maxVersion) which results in a miss while searching for the release filter file under working-dir. > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk and MirrorToMirror: > * The error that was encountered was because in the specific case of the graph image, src of the image is localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) . > * The graph image is already built and pushed to the destination registry during the collection phase (graph.go) > * destination registry is the cache for the case of mirrorToDisk > * destination registry is the remote mirror for the case of mirrorToMirror > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk and MirrorToMirror. >* Fix the generation of UpdateService.yaml file for MirrorToMirror workflow. > * The root cause was that the imageSetConfig (o.Opts.Config) was getting modified by the collector (more exactly cincinnati's `GetReleaseReferenceImages` which was adding minVersion and maxVersion for releases). This made the executor fail while `GetRelease` to prepare for UpdateService.yaml generation. The modified filter did not match the one saved previously by the collector > * The fix consisted of using a copy of the Config for `GetReleaseReferenceImages`, instead of using the original one. >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 5 months ago

@sherine-k: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
zhouying7780 commented 5 months ago

done pre-merge test for mirror2disk+disk2mirror and mirror2mirror, the graph-data-tag-digest pod running well, no issue

zhouying7780 commented 5 months ago

/label qe-approved

openshift-ci-robot commented 5 months ago

@sherine-k: This pull request references CLID-101 which is a valid jira issue.

In response to [this](https://github.com/openshift/oc-mirror/pull/840): ># Description > >* Fix graph image mirroring during MirrorToDisk and MirrorToMirror: > * The error that was encountered was because in the specific case of the graph image, src of the image is localhost:55000/graph:latest. Therefore we were attempting to mirror from the cache registry (tls-verify not forced to false) . > * The graph image is already built and pushed to the destination registry during the collection phase (graph.go) > * destination registry is the cache for the case of mirrorToDisk > * destination registry is the remote mirror for the case of mirrorToMirror > * The fix is to exclude the graph image from the batch mirroring for the case of MirrorToDisk and MirrorToMirror. >* Fix the generation of UpdateService.yaml file for MirrorToMirror workflow. > * The root cause was that the imageSetConfig (o.Opts.Config) was getting modified by the collector (more exactly cincinnati's `GetReleaseReferenceImages` which was adding minVersion and maxVersion for releases). This made the executor fail while `GetRelease` to prepare for UpdateService.yaml generation. The modified filter did not match the one saved previously by the collector > * The fix consisted of using a copy of the Config for `GetReleaseReferenceImages`, instead of using the original one. >* Fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images. >Example of what is displayed in logs in case of errors during mirroring: >```yaml >2024/04/25 14:54:05 [INFO] : === Results === >2024/04/25 14:54:05 [INFO] : Some release images failed to mirror: mirrored 185 / 185 (185 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some operator images failed to mirror: mirrored 11 / 11 (11 failures) ❌ - please check the logs >2024/04/25 14:54:05 [INFO] : Some additional images failed to mirror: mirrored 1 / 1 (1 failures) ❌ - please check the logs >``` > >Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101) > >## Type of change > >Please delete options that are not relevant. > >- [x] Bug fix (non-breaking change which fixes an issue) >- [ ] New feature (non-breaking change which adds functionality) >- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) >- [ ] This change requires a documentation update > ># How Has This Been Tested? >Successfully perform MirrorToDisk, DiskToMirror and MirrorToMirror with the following imageSetConfiguration: >```yaml >kind: ImageSetConfiguration >apiVersion: mirror.openshift.io/v2alpha1 >mirror: > platform: > channels: > - name: stable-4.13 > graph: true > operators: > - catalog: registry.redhat.io/redhat/redhat-operator-index:v4.14 > packages: > - name: aws-load-balancer-operator > - name: external-dns-operator > additionalImages: > - name: registry.redhat.io/ubi8/ubi:latest >``` > >## Expected Outcome >Please describe the outcome expected from the tests Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 5 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aguidirh, sherine-k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/oc-mirror/blob/main/OWNERS)~~ [aguidirh,sherine-k] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
aguidirh commented 5 months ago

/lgtm

sherine-k commented 5 months ago

/reopen

openshift-ci[bot] commented 5 months ago

@sherine-k: Failed to re-open PR: state cannot be changed. The pull request cannot be reopened.

In response to [this](https://github.com/openshift/oc-mirror/pull/840#issuecomment-2085062512): >/reopen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.