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

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

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

sherine-k
Copy link
Contributor

@sherine-k sherine-k commented Apr 25, 2024

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:
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

Type of change

Please delete options that are not relevant.

  • 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:

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 openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 25, 2024

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

In response to this:

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:
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

Type of change

Please delete options that are not relevant.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 25, 2024

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

In response to this:

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:
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

Type of change

Please delete options that are not relevant.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 25, 2024

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

In response to this:

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:
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

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.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 25, 2024

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

In response to this:

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:
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

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.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@sherine-k sherine-k changed the title WIP: CLID-101: Fix graph image mirroring after TLS verification enabling CLID-101: Fix graph image mirroring after TLS verification enabling Apr 25, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 26, 2024

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

In response to this:

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:
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

Type of change

Please delete options that are not relevant.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Fix generation of upgradeService.yaml for MirrorToMirror use case
We also fix the way we count errors during the batch phase, by creating error counters for release, operator and additional images.
Copy link

openshift-ci bot commented Apr 29, 2024

@sherine-k: 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.

@zhouying7780
Copy link

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

@zhouying7780
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 30, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 30, 2024

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

In response to this:

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:
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

Type of change

Please delete options that are not relevant.

  • 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:

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

@aguidirh aguidirh left a comment

Choose a reason for hiding this comment

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

Thanks @sherine-k.

Amazing work here fixing the TLS and making the log to show the errors when it happened.

Copy link

openshift-ci bot commented Apr 30, 2024

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aguidirh
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit b06c617 into openshift:main Apr 30, 2024
5 checks passed
@sherine-k
Copy link
Contributor Author

/reopen

Copy link

openshift-ci bot commented Apr 30, 2024

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

In response to this:

/reopen

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants