RFE-5434: Add cluster ID and timestamp to must-gather directory name#2252
RFE-5434: Add cluster ID and timestamp to must-gather directory name#2252asadawar wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@asadawar: This pull request references RFE-5434 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 feature request 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. |
|
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:
WalkthroughDefault must-gather output directory naming changed: when unset, DestDir is now generated by a new generateDestDir(ctx) method that includes a UTC timestamp and, when available, a cluster-id suffix (last 12 chars of cluster ID); if unavailable, the suffix is omitted. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as MustGatherOptions
participant Config as Config API (ClusterVersions)
participant Clock as PassiveClock
CLI->>Clock: Now() (UTC timestamp)
alt ConfigClient available
CLI->>Config: Get(ClusterVersion) (with 5s timeout)
Config-->>CLI: ClusterVersion (includes ClusterID)
CLI->>CLI: extract last 12 chars of ClusterID
else ConfigClient nil or unavailable
CLI-->>CLI: no cluster-id suffix
end
CLI->>CLI: format dest dir "must-gather.local.[<suffix>.]<timestamp>.<rand>"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @asadawar. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/mustgather/mustgather.go`:
- Around line 277-279: generateDestDir() currently calls
o.ConfigClient.ConfigV1().ClusterVersions().Get with context.TODO(), which can
block startup on slow/unreachable API servers; replace the context.TODO() with a
short timeout context (e.g., using context.WithTimeout) and defer cancel so the
lookup is bounded, then use that ctx when calling
ConfigV1().ClusterVersions().Get; target the call site referencing
o.ConfigClient, ConfigV1(), ClusterVersions().Get and ensure errors from context
deadline are handled the same as other lookup failures so the fallback path
still executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b20ea37c-8b72-4cbd-a950-65a1fbfb2bad
📒 Files selected for processing (2)
pkg/cli/admin/mustgather/mustgather.gopkg/cli/admin/mustgather/mustgather_test.go
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
1c1f293 to
7785cf8
Compare
|
Wouldn't it be nicer to use |
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
7785cf8 to
7396e5b
Compare
|
Good point, reordered to |
|
@asadawar: This pull request references RFE-5434 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 feature request 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. |
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
7396e5b to
67a47be
Compare
|
@asadawar: This pull request references RFE-5434 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 feature request 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. |
| // times. If the cluster ID cannot be retrieved (e.g. cluster is unreachable), it falls back | ||
| // to the timestamp and random ID only. | ||
| // See: https://issues.redhat.com/browse/RFE-5434 | ||
| func (o *MustGatherOptions) generateDestDir(ctx context.Context) string { |
There was a problem hiding this comment.
Honestly would be nicer to pass a clock.Clock interface so that the tests are deterministic. And then align the tests to check for exact names instead of just contains.
There was a problem hiding this comment.
Good suggestion, done. Added a clock.PassiveClock field to MustGatherOptions — that's a smaller interface from k8s.io/utils/clock that only requires Now() and Since(), which is all we need here (the full Clock interface includes timers that we don't use).
In production, it's initialized with clock.RealClock{} in NewMustGatherOptions(), so it calls the real time.Now(). In tests, I created a simple fakeClock struct that returns a fixed time (2026-04-14T12:00:00Z), which lets us assert exact prefixes like must-gather.local.76708af6b91c.20260414T120000Z. instead of regex patterns.
The random suffix from rand.Int63() is still non-deterministic, but that's fine — the important parts (cluster ID and timestamp) are now fully testable.
There was a problem hiding this comment.
@tchap just checking in on this. All your feedback has been addressed (reordering, clock.PassiveClock, klog.V(4) debug logging, fallback format in example text). Could you take a quick look when you get a chance? Also, would you be able to run /ok-to-test so CI can pick it up? Thanks!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/mustgather/mustgather.go`:
- Around line 76-77: Update the example text in the must-gather help/usage block
(the comment containing "# Gather information..." and the example line "oc adm
must-gather") to explicitly show that the `<cluster-id-suffix>` is optional —
state that the default output directory is
`must-gather.local.<cluster-id-suffix>.<timestamp>.<rand>` but when a cluster ID
is unavailable the suffix is omitted producing
`must-gather.local.<timestamp>.<rand>`, and adjust the example line or add a
second example to demonstrate the fallback form without `<cluster-id-suffix>`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8609b9a3-7893-43d3-83a6-26b82ef7ddbe
📒 Files selected for processing (2)
pkg/cli/admin/mustgather/mustgather.gopkg/cli/admin/mustgather/mustgather_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/admin/mustgather/mustgather_test.go
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
67a47be to
dda7aec
Compare
| IOStreams: streams, | ||
| Timeout: 10 * time.Minute, | ||
| VolumePercentage: defaultVolumePercentage, | ||
| Clock: clock.RealClock{}, |
There was a problem hiding this comment.
Better to export as few fields as necessary. I would rename this to clock and only set it in tests. And in generateDestDir() you can do
clock := o.clock
if clock == nil {
clock = clock.RealClock{}
}Or feel free to add a helper method on MustGatherOptions like getClock() and that one returns o.clock or RealClock{} when that field is unset.
There was a problem hiding this comment.
Good point, updated. Renamed to unexported clock field, removed the default from the constructor, and added a getClock() helper that returns RealClock{} when nil. Went with the helper approach since there's already a similar getNamespace() pattern in the same file. Tests set the field directly since they're in the same package.
|
@asadawar: This pull request references RFE-5434 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 feature request to target the "5.0.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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/admin/mustgather/mustgather.go (1)
272-285: Godoc comment is attached to the wrong function.The doc block on lines 272–276 describes
generateDestDirbut is placed immediately abovegetClock(), making it the godoc forgetClock. The stray// See: https://issues.redhat.com/browse/RFE-5434at line 284 is what ends up attached togenerateDestDir. Move the descriptive comment down togenerateDestDirand add a short godoc ongetClock.✏️ Proposed fix
-// generateDestDir builds the default destination directory name for must-gather output. -// The format includes a partial cluster ID (last 12 characters), a UTC timestamp, and a -// random ID to help distinguish must-gather archives from different clusters and collection -// times. If the cluster ID cannot be retrieved (e.g. cluster is unreachable), it falls back -// to the timestamp and random ID only. +// getClock returns the injected PassiveClock, defaulting to clock.RealClock{} when unset. func (o *MustGatherOptions) getClock() clock.PassiveClock { if o.clock != nil { return o.clock } return clock.RealClock{} } -// See: https://issues.redhat.com/browse/RFE-5434 +// generateDestDir builds the default destination directory name for must-gather output. +// The format includes a partial cluster ID (last 12 characters), a UTC timestamp, and a +// random ID to help distinguish must-gather archives from different clusters and collection +// times. If the cluster ID cannot be retrieved (e.g. cluster is unreachable), it falls back +// to the timestamp and random ID only. +// See: https://issues.redhat.com/browse/RFE-5434 func (o *MustGatherOptions) generateDestDir(ctx context.Context) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/mustgather/mustgather.go` around lines 272 - 285, The long doc comment that describes generateDestDir is currently placed above getClock, so move the descriptive block (the lines explaining the destination directory format and fallback behavior) down so it immediately precedes generateDestDir; then replace that moved comment's original location with a short godoc for getClock (e.g., "getClock returns the configured clock or the real clock") so getClock has its own concise documentation and generateDestDir retains the detailed explanation and the existing "See: ..." line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/mustgather/mustgather.go`:
- Around line 44-46: The import block in mustgather.go has unsorted imports
causing gofmt/verify to fail; reorder the two k8s.io/utils imports so
"k8s.io/utils/clock" comes before "k8s.io/utils/exec" (i.e., update the import
list containing "k8s.io/utils/exec", "k8s.io/utils/clock", and utilptr
"k8s.io/utils/ptr" to follow alphabetical order) and run gofmt/verify to ensure
the import ordering is correct.
---
Nitpick comments:
In `@pkg/cli/admin/mustgather/mustgather.go`:
- Around line 272-285: The long doc comment that describes generateDestDir is
currently placed above getClock, so move the descriptive block (the lines
explaining the destination directory format and fallback behavior) down so it
immediately precedes generateDestDir; then replace that moved comment's original
location with a short godoc for getClock (e.g., "getClock returns the configured
clock or the real clock") so getClock has its own concise documentation and
generateDestDir retains the detailed explanation and the existing "See: ..."
line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4790b8f8-9ca1-4b21-b81a-68f40bf1c04b
📒 Files selected for processing (2)
pkg/cli/admin/mustgather/mustgather.gopkg/cli/admin/mustgather/mustgather_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/mustgather/mustgather_test.go
|
/lgtm cancel Please fix the import ordering as mentioned by CodeRabbit. |
|
/lgtm |
|
Hello @atiratree, this PR has received /lgtm from @tchap and all CI tests are passing. Would you be able to take a look and /approve when you get a chance? Happy to address any feedback. Thanks! |
|
I think that @ardaguclu is more likely to review/approve. |
|
Hello @ardaguclu this PR has received /lgtm from @tchap and all CI tests are passing. Would you be able to take a look and /approve when you get a chance? Happy to address any feedback. Thanks! |
|
As stated in the Jira ticket, this needs to be reviewed and approved by the maintainers of must-gather (openshift/must-gather), since this change has an impact on it. RFE exists but as far as I can see it is not accepted yet. |
|
@shivprakashmuley Hi, Nick mentioned in the RFE ticket that he's passed this along to you for review. This PR adds cluster ID and timestamp to must-gather directory names to make it easier to identify must-gathers from different clusters in the same support case. It has /lgtm from tchap and all CI is green. Would appreciate your thoughts or approval when you get a chance. Thanks! |
|
@ardaguclu Hello, Nick has approved the RFE Jira ticket https://redhat.atlassian.net/browse/RFE-5434. The PR is ready with all CI checks passing and /lgtm from @tchap. Could you please review and provide your feedback/approval? Thanks. |
|
Hi @ardaguclu, thank you for the review. I've addressed all the feedback:
All tests passing. Please take a look when you get a chance, thanks! |
|
@asadawar thank you for spending time on this. Did you verify/test this on live cluster?. So that everything works properly? |
|
Hi @ardaguclu, yes I tested this on a live OCP 4.21 cluster. Here are the results: The last 12 characters of the cluster ID ( Thank you for the thorough review and the suggestions — I learned a lot from your feedback on this PR. Really appreciate you taking the time! |
|
/approve |
openshift-ci
Bot
added
the
approved
| ) | ||
|
|
||
| // fakeClock implements clock.PassiveClock for deterministic tests. | ||
| type fakeClock struct { |
There was a problem hiding this comment.
There is k8s.io/utils/clock/testing
There was a problem hiding this comment.
Good catch, thanks! Replaced with clocktesting.NewFakePassiveClock from k8s.io/utils/clock/testing.
|
Please squash this change into a single commit, thanks. |
When customers upload multiple must-gathers from different clusters to the same support case, the directory names are indistinguishable random numbers. This makes it difficult for support engineers to identify which must-gather belongs to which cluster. Add a partial cluster ID (last 12 characters) and UTC timestamp to the default must-gather directory name: Before: must-gather.local.5119224030749742202 After: must-gather.local.a1b2c3d4e5f6.20260413T143052Z.005765 The cluster ID is retrieved from ClusterVersion with a 5-second timeout. If unavailable (e.g. cluster unreachable), the directory name falls back to timestamp and random ID only. Signed-off-by: Abhijeet Sadawarte <asadawar@redhat.com>
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
b9fcd14 to
7d57753
Compare
|
Squashed into a single commit. Also replaced the custom fakeClock with |
|
Thanks. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, asadawar, tchap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
/retest |
|
/retest-required |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn-serial-1of2 |
|
@asadawar: 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
Summary
New format:
must-gather.local.<rand>.<cluster-id-suffix>.<timestamp>Example:
must-gather.local.5119224030749742202.76708af6b91c.20260413T141030ZFallback (cluster unreachable):
must-gather.local.<rand>.<timestamp>Details
The change is in
Complete()withinpkg/cli/admin/mustgather/mustgather.go. A newgenerateDestDir()method:ClusterVersion"version" via the existingConfigClientto retrieveSpec.ClusterID20060102T150405Z) — theZsuffix avoids timezone ambiguitymust-gather.local.<rand>.<timestamp>ifClusterVersionis unreachableNo new CLI flags are introduced — the consensus in the RFE discussion was to change the default naming rather than adding an opt-in option.
Test plan
generateDestDir()covering: full cluster ID, short cluster ID, empty cluster ID, nil ConfigClientoc adm must-gatherproduces directory with cluster ID suffix and timestamp--dest-dirflag: custom directory name is still respected (no change in behavior)must-gather.local.<rand>.<timestamp>See: https://issues.redhat.com/browse/RFE-5434
Summary by CodeRabbit
New Features
Tests