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 5 commits intoopenshift:mainfrom
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)
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
Include the last 12 characters of the cluster ID and a UTC timestamp in the auto-generated must-gather destination directory name. This helps support engineers distinguish must-gather archives from different clusters and collection times, especially when customers upload multiple archives to the same support case. New format: must-gather.local.<rand>.<cluster-id-suffix>.<timestamp> Example: must-gather.local.5119224030749742202.76708af6b91c.20260413T141030Z If the cluster ID cannot be retrieved (e.g. cluster is unreachable), the directory name falls back to: must-gather.local.<rand>.<timestamp> See: https://issues.redhat.com/browse/RFE-5434
asadawar
force-pushed
the
rfe-5434-must-gather-dir-naming
branch
from
67a47be to
dda7aec
Compare
The example text now mentions both the format with cluster ID suffix and the fallback format without it, so users know what to expect when the cluster ID is unavailable.
| 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.
Renamed Clock to clock (unexported) since only tests need to set it.
Removed the default from the constructor and added a getClock() helper
that returns RealClock{} when the field is nil.
|
@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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asadawar 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 |
|
Fixed the import ordering. Ready for another look. |
|
Let me wait for CI, I will tag this when it seems ok. |
|
@asadawar: The following test 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. |
3 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