MG-182: add since and sinceTime fields in the MG CR and CRD#323
MG-182: add since and sinceTime fields in the MG CR and CRD#323praveencodes wants to merge 4 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@praveencodes: This pull request references MG-182 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
WalkthroughAdds optional time-based log collection filters to GatherSpec ( Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller\n(getJobFromInstance)
participant APIServer as API Server\n(ClusterVersion)
participant Template as Template\n(getJobTemplate)
participant Container as Gather Container\n(getGatherContainer)
Controller->>APIServer: GET ClusterVersion "version"
APIServer-->>Controller: CreationTimestamp or error
Controller->>Template: getJobTemplate(..., clusterCreationTime)
Template->>Template: Build GatherTimeFilter from Since/SinceTime
Template->>Container: getGatherContainer(timeFilter, clusterCreationTime)
Container->>Container: Clamp Since to cluster age\nClamp SinceTime not before cluster creation
Container-->>Controller: Return Job with envs set (gatherEnvSince/gatherEnvSinceTime)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: praveencodes 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 |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@praveencodes: This pull request references MG-182 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/mustgather/template_test.go`:
- Around line 549-567: The test panics because tt.audit can be nil and the
MustGatherTimeout field is not wired; update the MustGather creation (mg :=
MustGather{...}) so GatherSpec.Audit is set safely (e.g., assign Audit: tt.audit
or use a conditional to copy *tt.audit only when tt.audit != nil) to avoid
dereferencing nil, and add Spec.MustGatherTimeout = tt.wantTimeout (or set from
tt.wantTimeout) so the test's expected timeout is populated; locate and change
the MustGather construction in template_test.go (mg, Spec, GatherSpec,
UploadTarget) to apply these fixes.
In `@examples/mustgather_with_since.yaml`:
- Around line 5-10: The example places the audit field at the top-level under
spec where it won't be applied; move the audit boolean into the gatherSpec
object so it matches the API (i.e., under gatherSpec alongside since). Update
the YAML so gatherSpec contains both since: 10m and audit: false (preserve
spacing/indentation and keep serviceAccountName at the same spec level).
In `@examples/mustgather_with_sincetime.yaml`:
- Around line 5-10: The example places the audit field at the top level under
spec instead of inside gatherSpec; move the audit: false entry into the
gatherSpec mapping (so spec.gatherSpec.audit: false) to match the API contract
and ensure it is applied by the controller (leave other keys like sinceTime and
serviceAccountName unchanged).
| spec: | ||
| # Only collect logs from the last 10 minutes (reduces output size significantly) | ||
| gatherSpec: | ||
| since: 10m | ||
| audit: false | ||
| serviceAccountName: must-gather-admin |
There was a problem hiding this comment.
Move audit under gatherSpec to match the API.
At the current level, audit won’t be applied (and may be pruned), so the example is misleading.
✅ Proposed fix
spec:
# Only collect logs from the last 10 minutes (reduces output size significantly)
gatherSpec:
since: 10m
- audit: false
+ audit: false
serviceAccountName: must-gather-admin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/mustgather_with_since.yaml` around lines 5 - 10, The example places
the audit field at the top-level under spec where it won't be applied; move the
audit boolean into the gatherSpec object so it matches the API (i.e., under
gatherSpec alongside since). Update the YAML so gatherSpec contains both since:
10m and audit: false (preserve spacing/indentation and keep serviceAccountName
at the same spec level).
| spec: | ||
| gatherSpec: | ||
| sinceTime: "2026-02-02T00:00:00Z" | ||
| audit: false | ||
| serviceAccountName: must-gather-admin | ||
| uploadTarget: |
There was a problem hiding this comment.
Move audit under gatherSpec to match the API.
This example currently places audit at the wrong level, so it won’t be applied.
✅ Proposed fix
spec:
gatherSpec:
sinceTime: "2026-02-02T00:00:00Z"
- audit: false
+ audit: false
serviceAccountName: must-gather-admin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec: | |
| gatherSpec: | |
| sinceTime: "2026-02-02T00:00:00Z" | |
| audit: false | |
| serviceAccountName: must-gather-admin | |
| uploadTarget: | |
| spec: | |
| gatherSpec: | |
| sinceTime: "2026-02-02T00:00:00Z" | |
| audit: false | |
| serviceAccountName: must-gather-admin | |
| uploadTarget: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/mustgather_with_sincetime.yaml` around lines 5 - 10, The example
places the audit field at the top level under spec instead of inside gatherSpec;
move the audit: false entry into the gatherSpec mapping (so
spec.gatherSpec.audit: false) to match the API contract and ensure it is applied
by the controller (leave other keys like sinceTime and serviceAccountName
unchanged).
|
@praveencodes: This pull request references MG-182 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/mustgather/template_test.go (1)
278-303: Consider using explicit expected values instead of name-based branching.The assertions branch on
tt.namestring comparison, which is fragile—if a test name changes, the wrong assertion branch could execute silently. Consider adding explicit expected fields (e.g.,wantSinceEnv,wantSinceTimeEnv,expectSinceOmitted bool) to the test struct for clearer intent and safer refactoring.♻️ Example refactor
tests := []struct { name string // ... existing fields ... timeFilter *GatherTimeFilter clusterCreation *time.Time + wantSinceEnv string // expected MUST_GATHER_SINCE value, empty means not set + wantSinceTimeEnv string // expected MUST_GATHER_SINCE_TIME value, empty means not set + expectSinceOmitted bool // true if MUST_GATHER_SINCE should be absent }{Then assertions become:
if tt.expectSinceOmitted { if _, ok := envMap[gatherEnvSince]; ok { t.Fatalf("did not expect %s env var", gatherEnvSince) } } else if tt.wantSinceEnv != "" { if envMap[gatherEnvSince] != tt.wantSinceEnv { t.Fatalf("expected %s=%s, got %s", gatherEnvSince, tt.wantSinceEnv, envMap[gatherEnvSince]) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/mustgather/template_test.go` around lines 278 - 303, The test branches on tt.name which is fragile; modify the test case struct (used in template_test.go) to include explicit expectation fields like wantSinceEnv string, wantSinceTimeEnv string, and expectSinceOmitted bool, then replace the name-based conditional logic around envValues(container)/envMap assertions (currently referencing tt.timeFilter, gatherEnvSince, gatherEnvSinceTime) to check these new fields: if expectSinceOmitted ensure gatherEnvSince is absent, else if wantSinceEnv != "" assert envMap[gatherEnvSince] == wantSinceEnv; similarly assert wantSinceTimeEnv against envMap[gatherEnvSinceTime] when non-empty. Update each test case entry to populate these new expectation fields instead of relying on tt.name.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
controllers/mustgather/template_test.godeploy/crds/operator.openshift.io_mustgathers.yaml
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/mustgather/template_test.go`:
- Around line 278-303: The test branches on tt.name which is fragile; modify the
test case struct (used in template_test.go) to include explicit expectation
fields like wantSinceEnv string, wantSinceTimeEnv string, and expectSinceOmitted
bool, then replace the name-based conditional logic around
envValues(container)/envMap assertions (currently referencing tt.timeFilter,
gatherEnvSince, gatherEnvSinceTime) to check these new fields: if
expectSinceOmitted ensure gatherEnvSince is absent, else if wantSinceEnv != ""
assert envMap[gatherEnvSince] == wantSinceEnv; similarly assert wantSinceTimeEnv
against envMap[gatherEnvSinceTime] when non-empty. Update each test case entry
to populate these new expectation fields instead of relying on tt.name.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #323 +/- ##
==========================================
+ Coverage 58.56% 63.13% +4.57%
==========================================
Files 8 8
Lines 806 849 +43
==========================================
+ Hits 472 536 +64
+ Misses 322 306 -16
+ Partials 12 7 -5
🚀 New features to boost your workflow:
|
|
@praveencodes: This pull request references MG-182 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/mustgather_types.go`:
- Around line 40-44: Update the Go doc comment above the GatherSpec field in
mustgather_types.go: remove the inaccurate sentence "This field is ignored if
ImageStreamRef is not specified" and replace it with a precise description
noting that while command/args from GatherSpec are only applied when
ImageStreamRef is provided, time-based filters (since and sinceTime) are
applicable even without ImageStreamRef; reference the GatherSpec field,
ImageStreamRef, and the since/sinceTime filters in the comment so the generated
CRD correctly documents the behavior.
In `@deploy/crds/operator.openshift.io_mustgathers.yaml`:
- Around line 44-46: The gatherSpec description incorrectly states the whole
field is ignored without ImageStreamRef; update the doc text for gatherSpec to
clarify that only command and args are ignored when ImageStreamRef is not
specified, while time-based filters (since and sinceTime) remain honored;
reference the gatherSpec, ImageStreamRef, command, args, since and sinceTime
symbols so the maintainer changes the descriptive block accordingly to reflect
current validation.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
api/v1alpha1/mustgather_types.godeploy/02_must-gather-operator.ClusterRole.yamldeploy/crds/operator.openshift.io_mustgathers.yamlexamples/mustgather_with_since.yamlexamples/mustgather_with_sincetime.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/mustgather_with_sincetime.yaml
| // GatherSpec allows overriding the command and/or arguments for the custom must-gather image | ||
| // and configures time-based collection filters. | ||
| // This field is ignored if ImageStreamRef is not specified. | ||
| // +kubebuilder:validation:Optional | ||
| GatherSpec *GatherSpec `json:"gatherSpec,omitempty"` |
There was a problem hiding this comment.
Root cause of the misleading description — update the Go doc comment.
Same issue as in the CRD: line 42 states "This field is ignored if ImageStreamRef is not specified," which is no longer accurate since since/sinceTime are usable without imageStreamRef. The CRD is generated from these markers, so the fix belongs here.
Proposed fix
- // GatherSpec allows overriding the command and/or arguments for the custom must-gather image
- // and configures time-based collection filters.
- // This field is ignored if ImageStreamRef is not specified.
+ // GatherSpec allows overriding the command and/or arguments for the custom must-gather image
+ // and configures time-based collection filters.
+ // The command and args fields are only honored when ImageStreamRef is specified.
+ // Time-based filters (since, sinceTime) apply regardless of ImageStreamRef.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/mustgather_types.go` around lines 40 - 44, Update the Go doc
comment above the GatherSpec field in mustgather_types.go: remove the inaccurate
sentence "This field is ignored if ImageStreamRef is not specified" and replace
it with a precise description noting that while command/args from GatherSpec are
only applied when ImageStreamRef is provided, time-based filters (since and
sinceTime) are applicable even without ImageStreamRef; reference the GatherSpec
field, ImageStreamRef, and the since/sinceTime filters in the comment so the
generated CRD correctly documents the behavior.
| GatherSpec allows overriding the command and/or arguments for the custom must-gather image | ||
| and configures time-based collection filters. | ||
| This field is ignored if ImageStreamRef is not specified. |
There was a problem hiding this comment.
gatherSpec description is now misleading — since/sinceTime should work without imageStreamRef.
Line 46 says "This field is ignored if ImageStreamRef is not specified," but that's no longer true. The since and sinceTime sub-fields are not gated behind imageStreamRef (only command/args are, per the validation on lines 234–236). A user reading this description may think time filtering requires a custom image.
Consider updating to something like:
Proposed fix
description: |-
GatherSpec allows overriding the command and/or arguments for the custom must-gather image
and configures time-based collection filters.
- This field is ignored if ImageStreamRef is not specified.
+ The command and args fields are only honored when ImageStreamRef is specified.
+ Time-based filters (since, sinceTime) apply regardless of ImageStreamRef.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GatherSpec allows overriding the command and/or arguments for the custom must-gather image | |
| and configures time-based collection filters. | |
| This field is ignored if ImageStreamRef is not specified. | |
| GatherSpec allows overriding the command and/or arguments for the custom must-gather image | |
| and configures time-based collection filters. | |
| The command and args fields are only honored when ImageStreamRef is specified. | |
| Time-based filters (since, sinceTime) apply regardless of ImageStreamRef. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/crds/operator.openshift.io_mustgathers.yaml` around lines 44 - 46, The
gatherSpec description incorrectly states the whole field is ignored without
ImageStreamRef; update the doc text for gatherSpec to clarify that only command
and args are ignored when ImageStreamRef is not specified, while time-based
filters (since and sinceTime) remain honored; reference the gatherSpec,
ImageStreamRef, command, args, since and sinceTime symbols so the maintainer
changes the descriptive block accordingly to reflect current validation.
|
@praveencodes: This pull request references MG-182 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deploy/crds/operator.openshift.io_mustgathers.yaml (1)
44-46:⚠️ Potential issue | 🟡 MinorClarify
gatherSpecdocs: onlycommand/argsare gated byimageStreamRef.Line 46 still implies all of
gatherSpecis ignored withoutimageStreamRef, which is not true forsince/sinceTime.Proposed doc fix
description: |- GatherSpec allows overriding the command and/or arguments for the custom must-gather image and configures time-based collection filters. - This field is ignored if ImageStreamRef is not specified. + The command and args fields are only honored when ImageStreamRef is specified. + Time-based filters (since, sinceTime) apply regardless of ImageStreamRef.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/crds/operator.openshift.io_mustgathers.yaml` around lines 44 - 46, The gatherSpec documentation incorrectly states the entire field is ignored if ImageStreamRef is not specified; update the text for gatherSpec to clarify that only the custom image overrides (command and args) are ignored when ImageStreamRef is not provided, while time-based filters (since and sinceTime) still apply; reference the gatherSpec object and the ImageStreamRef field and explicitly mention the command, args, since, and sinceTime properties in the revised sentence so readers know which parts are gated by ImageStreamRef and which are independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@deploy/crds/operator.openshift.io_mustgathers.yaml`:
- Around line 44-46: The gatherSpec documentation incorrectly states the entire
field is ignored if ImageStreamRef is not specified; update the text for
gatherSpec to clarify that only the custom image overrides (command and args)
are ignored when ImageStreamRef is not provided, while time-based filters (since
and sinceTime) still apply; reference the gatherSpec object and the
ImageStreamRef field and explicitly mention the command, args, since, and
sinceTime properties in the revised sentence so readers know which parts are
gated by ImageStreamRef and which are independent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7bde281-a126-45bf-9f75-e93aa3220f64
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (1)
deploy/crds/operator.openshift.io_mustgathers.yaml
|
@praveencodes: 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. |
4 participants
MG-182:
sinceandsinceTimeare introduced in thegatherSpecinside MustGather spec to reduce the time taken to collect the Mustgather logs thus reducing the size.Note: The
--sinceand--sinceTimeflags are already available in the CLIoc adm mustgather. We are introducing these fields in the must gather operator now.Summary by CodeRabbit
New Features
Validation
Documentation
Tests
Chores