OCPBUGS-49991: fixes bundle related images being skipped #1071
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@aguidirh: This pull request references Jira Issue OCPBUGS-49991, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aguidirh 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 |
openshift-ci
bot
added
the
approved
|
@aguidirh: This pull request references Jira Issue OCPBUGS-49991, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 openshift-eng/jira-lifecycle-plugin repository. |
| ris, err := handleRelatedImages(bundle, bundle.Package, copyImageSchemaMap) | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| continue |
There was a problem hiding this comment.
Because we cannot determine if a bundle is necessary or not for the operator, it's safer to just return an error here and skip the whole operator.
There was a problem hiding this comment.
Probably we need to handle this in another place, if we just return an error here we will skip the entire catalog, even if there were operators which successfully retrieved all the related images.
There was a problem hiding this comment.
It would require to change the batch as well and ask QE to test all the scenarios again.
My suggestion is to do this after 4.18 GA.
There was a problem hiding this comment.
I see. So we are skipping the operator and that's happening because of your changes in handleRelatedImages
There was a problem hiding this comment.
@r4f4 my changes should skip only the bundle, not the entire operator.
There was a problem hiding this comment.
In the example in the description you see openshift-nfd-operator and kuadrant-operator being skipped only because all the bundles inside them had missing images. Since an operator is basically made of a collection of bundles (versions), with no bundles, there is no operator to be mirrored.
If you had at least one bundle inside of them with all related images working, then you would see that operator being mirrored.
There was a problem hiding this comment.
Right we're skipping this specific operator version. Got it.
aguidirh
force-pushed
the
OCPBUGS-49991
branch
from
585ffd6 to
2c8a221
Compare
aguidirh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@aguidirh I tested the PR here and i see that operators are being skipped if some of the images are failing during the collection phase though i do not see any errors or anything being printed on the console which talks about the operators that are being skipped, also i saw in this PR that errors will be handled in a future PR, will you be raising that PR once this is merged or how that PR will be handled ? could you please help clarify ? thanks !! |
|
Hi @kasturinarra, This PR is necessary also for the exit code feature, which is going to be handled in a different PR from @r4f4. I'm not sure if the PR will be #1062 or a different one. |
|
As discussed with alex will retest the PR once alex has put warnings back for the operator bundles that will be skipped. |
aguidirh
force-pushed
the
OCPBUGS-49991
branch
from
2c8a221 to
957bbb7
Compare
|
Hi @kasturinarra and @r4f4, I just pushed the Warning messages back, with some additional information that were not being displayed before (bundle and operator name of the images that were skipped). Output example: Please let me know if you want me to change something else. |
@aguidirh how about changing message something like below , feel this more readable and helpful for the customers when they look at the warn messages, WDYT ? |
aguidirh
force-pushed
the
OCPBUGS-49991
branch
from
957bbb7 to
04e8efa
Compare
|
Based on your request in the last comment @kasturinarra, Here is the new output example: |
| ris := handleRelatedImages(bundle, bundle.Package, copyImageSchemaMap) | ||
| ris, err := handleRelatedImages(bundle, bundle.Package, copyImageSchemaMap) | ||
| if err != nil { | ||
| internalLog.Warn("%s", err.Error()) |
There was a problem hiding this comment.
What about
| internalLog.Warn("%s", err.Error()) | |
| internalLog.Warn("SKIPPING bundle %s of operator %s: %s", bundle.Name, bundle.Package, err.Error()) |
then change the error returned by handleRelatedImages to not mention the operator/bundle?
| var relatedImages []v2alpha1.RelatedImage | ||
|
|
||
| for _, ri := range bundle.RelatedImages { | ||
| if strings.Contains(ri.Image, "oci://") { | ||
| internalLog.Warn("%s 'oci' is not supported in operator catalogs : SKIPPING", ri.Image) | ||
| continue | ||
| msg := fmt.Sprintf("image: %s 'oci' is not supported in operator catalogs SKIPPING bundle: %s for Operator: %s", ri.Image, bundle.Name, operatorName) |
There was a problem hiding this comment.
| msg := fmt.Sprintf("image: %s 'oci' is not supported in operator catalogs SKIPPING bundle: %s for Operator: %s", ri.Image, bundle.Name, operatorName) | |
| msg := fmt.Sprintf("invalid image %s: 'oci' is not supported in operator catalogs", ri.Image) |
There was a problem hiding this comment.
The error message here should not "know" whether it's causing the bundle to be skipped or not.
| } | ||
|
|
||
| imgSpec, err := image.ParseRef(ri.Image) | ||
| if err != nil { | ||
| internalLog.Warn("error parsing image %s : %v", ri.Image, err) | ||
| msg := fmt.Sprintf("image: %s %s SKIPPING bundle: %s for operator: %s", ri.Image, err.Error(), bundle.Name, operatorName) |
There was a problem hiding this comment.
| msg := fmt.Sprintf("image: %s %s SKIPPING bundle: %s for operator: %s", ri.Image, err.Error(), bundle.Name, operatorName) | |
| msg := fmt.Sprintf("error parsing image %s: %s", ri.Image, err.Error()) |
|
@aguidirh: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
Description
This PR fixes the bug where a bundle related image could be skipped during the collection. The operator would not work as expected because of missing related images.
Github / Jira issue: OCPBUGS-49991
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
With the following ImageSetConfiguration:
Run
m2dExpected Outcome
Only the
argocd-operatorwill be mirrored. Theopenshift-nfd-operatorandkuadrant-operatorwill be skipped on the collector phase because of issues on its related images.NOTE: this PR does not add the errors on the collector phase to the error file. This behavior will be added in a future PR.