Improve must-gather image handling and annotation checks #2071
Conversation
- Updated completeImages() to log a warning when a ClusterServiceVersion (CSV) is missing the "operators.openshift.io/must-gather-image" annotation. - Inlined CSV processing logic to allow detailed per-CSV warning logging. - Improved error handling and logging for image stream tag resolution. - Ensured deduplication of default must-gather image from plug-in list. - Maintained compatibility with --all-images and fallback logic.
WalkthroughInline discovery for --all-images now lists CSVs and ClusterOperators to collect must-gather images from the mgAnnotation, logs warnings for missing annotations, removes the default image to avoid duplicates, updates listing error messages, and still resolves a default image when none are provided. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This reverts commit 3845d4b.
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) + }
|
@midu16: The following tests 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. |
1 participant
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.