Improve must-gather image handling and annotation checks#2071
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWhen Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/cli/admin/mustgather/mustgather.go (2)
273-276: Make fallback explicit and avoid double newline in log.Current
o.log("%v\n", err)adds two newlines and doesn’t state that a fallback is used.Apply this diff:
- if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { - o.log("%v\n", err) - image = "registry.redhat.io/openshift4/ose-must-gather:latest" - } + if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { + fallback := "registry.redhat.io/openshift4/ose-must-gather:latest" + o.log("WARNING: unable to resolve imagestream openshift/must-gather:latest: %v; falling back to %s", err, fallback) + image = fallback + }
309-314: Dedup across equivalent pullspecs, not just exact string match.If a CSV annotates the default image via a different but equivalent reference (e.g., tag vs digest),
delete(pluginImages, o.Images[0])won’t dedupe it.Consider normalizing with
imagereference.Parseand comparingExact()(or repository+ID) before appending, e.g., parseo.Images[0]once and skip plugin entries that resolve to the same canonical ref.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/mustgather/mustgather.go(3 hunks)
🔇 Additional comments (1)
pkg/cli/admin/mustgather/mustgather.go (1)
315-317: Confirm intended UX for duplicate “Using must-gather plug-in image” lines.This now prints once during Complete() to stdout and again into must-gather.logs in Run(). If that’s unintentional, drop the Complete() log.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/cli/admin/mustgather/mustgather.go (3)
269-278: Avoid double newline and log explicit fallback
o.log("%v\n", err)prints two newlines becauseo.logappends one. Also add an explicit fallback warning for clarity.Apply this diff:
- if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { - o.log("%v\n", err) - image = "registry.redhat.io/openshift4/ose-must-gather:latest" - } + if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { + o.log("WARNING: %v", err) + fallback := "registry.redhat.io/openshift4/ose-must-gather:latest" + o.log("WARNING: falling back to default must-gather image %q", fallback) + image = fallback + }Please confirm the intended behavior is to always include the default image when
--all-imagesis set (then add discovered plug-ins), which matches the PR summary.
294-301: Include namespace in per-CSV warningCSV names can collide across namespaces. Log the fully-qualified
ns/namefor disambiguation.Apply this diff:
- for _, u := range uList.Items { - ann := u.GetAnnotations() - if v, ok := ann[mgAnnotation]; ok { - pluginImages[v] = struct{}{} - } else { - o.log("WARNING: CSV operator %s doesn't have the must-gather-image annotation.", u.GetName()) - } - } + for _, u := range uList.Items { + ann := u.GetAnnotations() + if v, ok := ann[mgAnnotation]; ok { + pluginImages[v] = struct{}{} + } else { + name := u.GetName() + if ns := u.GetNamespace(); ns != "" { + name = ns + "/" + name + } + o.log("WARNING: CSV operator %s doesn't have the must-gather-image annotation.", name) + } + }
315-318: Deterministic ordering for appended plug-in imagesAppending from a map yields nondeterministic order, which affects logs and pod creation order. Sort for stable output.
Apply this diff:
- delete(pluginImages, o.Images[0]) - for i := range pluginImages { - o.Images = append(o.Images, i) - } + delete(pluginImages, o.Images[0]) + if len(pluginImages) > 0 { + extras := make([]string, 0, len(pluginImages)) + for img := range pluginImages { + extras = append(extras, img) + } + sort.Strings(extras) + o.Images = append(o.Images, extras...) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/mustgather/mustgather.go(2 hunks)
🔇 Additional comments (4)
pkg/cli/admin/mustgather/mustgather.go (4)
261-268: LGTM: explicit imagestream resolutionNo issues spotted in resolving user-provided imagestreams.
284-293: CSV listing via DynamicClient: goodSwitch to
DynamicClientlooks correct; error handling message is clear.
303-307: LGTM: ClusterOperators path and error messageError message is specific and consistent with the CSV case.
325-345: Remove now-unused annotatedCSVs() helper — safe to delete.
No call sites found; delete the function in pkg/cli/admin/mustgather/mustgather.go (lines ~325–345) or rewire it if it was intended to be used.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: midu16 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cli/admin/mustgather/mustgather.go (1)
284-296: Fix compile error: OperatorClient is not a field; use DynamicClient to list CSVs
MustGatherOptionshas noOperatorClient. Use the already-initializedDynamicClientto list CSVs and keep per-CSV warnings.Apply:
- // Annotated CSVs - csvs, err := o.OperatorClient.OperatorsV1alpha1().ClusterServiceVersions("").List(context.TODO(), metav1.ListOptions{}) - if err != nil { - return fmt.Errorf("failed to list CSVs: %v", err) - } - for _, csv := range csvs.Items { - ann := csv.GetAnnotations() - if v, ok := ann[mgAnnotation]; ok { - pluginImages[v] = struct{}{} - } else { - o.log("WARNING: CSV operator %s doesn't have the must-gather-image annotation.", csv.GetName()) - } - } + // Annotated CSVs (via dynamic client) + csvGVR := schema.GroupVersionResource{ + Group: "operators.coreos.com", + Version: "v1alpha1", + Resource: "clusterserviceversions", + } + uList, err := o.DynamicClient.Resource(csvGVR).Namespace(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return fmt.Errorf("failed to list CSVs: %w", err) + } + for _, u := range uList.Items { + ann := u.GetAnnotations() + if v, ok := ann[mgAnnotation]; ok { + pluginImages[v] = struct{}{} + } else { + o.log("WARNING: CSV operator %s doesn't have the must-gather-image annotation.", u.GetName()) + } + }
🧹 Nitpick comments (3)
pkg/cli/admin/mustgather/mustgather.go (3)
269-278: Emit explicit fallback WARNING and avoid double newline in log
- o.log already appends a newline; passing "%v\n" yields double newlines.
- Per PR goals, the fallback should clearly log a WARNING that a hardcoded image is used.
Apply:
- if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { - o.log("%v\n", err) - image = "registry.redhat.io/openshift4/ose-must-gather:latest" - } + if image, err = o.resolveImageStreamTag("openshift", "must-gather", "latest"); err != nil { + o.log("WARNING: %v", err) + image = "registry.redhat.io/openshift4/ose-must-gather:latest" + o.log("WARNING: falling back to default must-gather image %s", image) + }
309-314: Make plug-in image ordering deterministic; consider stronger dedup
- Map iteration order is random; sort for reproducible CLI output and pod ordering.
- Optional: normalize refs (tag vs digest) before dedup so the default isn’t re-added under a different form.
Apply:
- for i := range pluginImages { - o.Images = append(o.Images, i) - } + imgs := make([]string, 0, len(pluginImages)) + for img := range pluginImages { + imgs = append(imgs, img) + } + sort.Strings(imgs) + o.Images = append(o.Images, imgs...)Optional follow-up (separate change): normalize references via imagereference.Parse and compare by digest (when present) to enhance dedup. Verify current CSV annotations’ formats across operators before changing behavior.
315-316: LGTM: summary log of selected imagesThe consolidated log line is helpful. Keep it; the extra blank line after isn’t needed but harmless.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/mustgather/mustgather.go(3 hunks)
🔇 Additional comments (2)
pkg/cli/admin/mustgather/mustgather.go (2)
261-268: LGTM: explicit image-stream resolution pathResolving user-specified imagestream tags up-front is correct and keeps error handling tight.
299-302: Wrap underlying error with %w for better contextUse error wrapping to preserve cause.
[ suggest_nitpick_refactor ]
- if err != nil { - return fmt.Errorf("failed to list ClusterOperators: %v", err) - } + if err != nil { + return fmt.Errorf("failed to list ClusterOperators: %w", err) + }
|
@ingvagabund wondering if you can assist on this review, intention here its to improve the logging in the scenario where a given operator doesnt have the must-gather-image annotation within its CSV. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
Hi @midu16 , can you please check the CI job failures. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cli/admin/mustgather/mustgather.go`:
- Around line 290-301: The undefined `csvs` bug is caused by moving CSV
iteration out of annotatedCSVs; restore per-CSV warning logic by changing
annotatedCSVs to accept the logger (e.g., add a parameter for o.log or a func
like logf) and move the loop that inspects csvs.Items and emits warnings using
mgAnnotation into annotatedCSVs; update annotatedCSVs' signature (e.g.,
annotatedCSVs(ctx context.Context, logf func(string, ...interface{}))
(map[string]struct{}, error)), implement the per-CSV warning there when an
annotation is missing, and then call it from mustgather.go as pluginImages, err
= o.annotatedCSVs(ctx, o.log) and remove the orphaned for-loop in mustgather.go;
also update any other callers to match the new signature.
|
@midu16: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
@shivprakashmuley can you review now ? |
|
hi @ardaguclu i wonder if you can help me with this PR ? |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Dropped a few comments. Can you please test this on real OCP cluster and paste the log here to see how many operators lack the annotation, so that we can see what customers see in real cluster.
4 participants
Pull Request: Improve must-gather image handling and annotation checks
Summary
This PR enhances the
MustGatherOptions.completeImages()method by:must-gather-imageannotation.Changes
Moved CSV annotation scanning inline to allow per-CSV warning messages.
Added warning log in the format:
Enhanced image resolution logic to:
Maintained support for
--all(akaAllImages) by collecting images from both:Ensured duplicate images (especially the default one) are not added to the final list.
Testing
Related
operators.openshift.io/must-gather-imageoc adm must-gather --all-images.Notes
Future enhancement may consider moving CSV image collection into a separate method again, but returning both:
pluginImages[]stringof CSVs missing the annotation for external logging.