MG-271: Implement standard must-gather directory naming convention#364
MG-271: Implement standard must-gather directory naming convention#364neha037 wants to merge 10 commits into
Conversation
This change aligns the must-gather-operator with the oc adm must-gather directory naming convention for consistency across collection methods. Changes: - Add generateMustGatherDirectoryName() to create directory names following the pattern: must-gather.local.<cluster-id-suffix>.<timestamp>.<random> - Retrieve cluster ID from ClusterVersion resource (last 12 characters) - Fallback to timestamp+random format if cluster ID unavailable - Pass generated directory name to gather container via MUST_GATHER_DEST_DIR env var - Add comprehensive unit tests for directory name generation Implements: MG-271 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Code review verdict: Approved (9/10) - All acceptance criteria met - Comprehensive unit tests (426 lines) - RBAC properly configured - No critical issues found Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughThis PR adds must-gather destination directory generation, passes it into the gather container as ChangesMust-gather directory naming feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neha037 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #364 +/- ##
==========================================
+ Coverage 82.32% 82.81% +0.49%
==========================================
Files 8 9 +1
Lines 905 931 +26
==========================================
+ Hits 745 771 +26
Misses 148 148
Partials 12 12
🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/mustgather/template_test.go (1)
219-219: ⚡ Quick winAdd assertions for
MUST_GATHER_DEST_DIRin the updated tests.These tests pass a directory name but never verify
gatherEnvDestDiris emitted, so the new wiring can break without test failure.🔧 Suggested additions
- container := getGatherContainer(tt.mustGatherImage, tt.audit, tt.timeout, tt.storage, tt.caConfigMap, tt.timeFilter, tt.command, tt.args, "must-gather.local.test.20240101T120000Z.000001") + expectedDir := "must-gather.local.test.20240101T120000Z.000001" + container := getGatherContainer(tt.mustGatherImage, tt.audit, tt.timeout, tt.storage, tt.caConfigMap, tt.timeFilter, tt.command, tt.args, expectedDir) + envMap := envValues(container) + if envMap[gatherEnvDestDir] != expectedDir { + t.Fatalf("expected %s=%s, got %s", gatherEnvDestDir, expectedDir, envMap[gatherEnvDestDir]) + }- job := getJobTemplate("img", "operator-image", mg, "", "must-gather.local.test.20240101T120000Z.000001") + expectedDir := "must-gather.local.test.20240101T120000Z.000001" + job := getJobTemplate("img", "operator-image", mg, "", expectedDir) gather := findGatherContainerInJob(t, job) got := envValues(gather) + if got[gatherEnvDestDir] != expectedDir { + t.Fatalf("expected %s=%s, got %s", gatherEnvDestDir, expectedDir, got[gatherEnvDestDir]) + }Also applies to: 592-592, 678-678
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/mustgather/template_test.go` at line 219, The tests call getGatherContainer with a directory name parameter but do not verify that the MUST_GATHER_DEST_DIR environment variable (gatherEnvDestDir) is properly set in the returned container. Add assertions in the test cases at lines 219, 592, and 678 to verify that the container environment includes the correct gatherEnvDestDir value matching the directory name passed to getGatherContainer, ensuring the new wiring cannot break without test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/mustgather/directory_naming_test.go`:
- Line 107: The `configv1.Install(scheme)` function calls at lines 107, 195,
300, and 370 are discarding their error return values using blank identifier
assignment. Instead of ignoring these errors, check the error return value from
each `configv1.Install(scheme)` call and fail the test immediately with a clear
error message if installation fails, ensuring proper test setup validation
before continuing with test execution.
In `@controllers/mustgather/directory_naming.go`:
- Line 72: The log.V(1).Info call in directory_naming.go logs the full
directoryName which contains the cluster ID suffix and exposes
customer-identifying metadata. Remove the "directoryName", dirName parameters
from the log statement, keeping only the non-sensitive "hasClusterID" parameter
to prevent exposing cluster-identifying data in operator logs.
---
Nitpick comments:
In `@controllers/mustgather/template_test.go`:
- Line 219: The tests call getGatherContainer with a directory name parameter
but do not verify that the MUST_GATHER_DEST_DIR environment variable
(gatherEnvDestDir) is properly set in the returned container. Add assertions in
the test cases at lines 219, 592, and 678 to verify that the container
environment includes the correct gatherEnvDestDir value matching the directory
name passed to getGatherContainer, ensuring the new wiring cannot break without
test failures.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 16271d96-ec22-431e-9bb3-b23cc5aa0346
📒 Files selected for processing (6)
controllers/mustgather/directory_naming.gocontrollers/mustgather/directory_naming_test.gocontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/template.gocontrollers/mustgather/template_test.goreview-report-MG-271.json
neha037
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@neha037: This pull request references MG-271 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 "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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/mustgather/directory_naming.go (1)
54-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid logging raw cluster ID retrieval errors.
Line 57 logs the full
err, which can leak internal API endpoints/hostnames from client-go error text. Log a sanitized reason/classification instead of the raw error payload.🔧 Suggested change
- log.V(2).Info("Unable to retrieve cluster ID, continuing without it", "error", err) + var reason string + switch { + case errors.IsNotFound(err): + reason = "not_found" + case errors.IsForbidden(err): + reason = "forbidden" + default: + reason = "lookup_failed" + } + log.V(2).Info("Unable to retrieve cluster ID, continuing without it", "reason", reason)As per coding guidelines,
**/*.{go,py,sh,bash,java,js,ts,c,cpp}: "Flag logging that may expose ... internal hostnames, or customer data".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/mustgather/directory_naming.go` around lines 54 - 58, The error logging in the cluster ID retrieval error handling block is logging the raw err value directly, which can expose internal API endpoints or hostnames from client-go error messages. Replace the "error", err parameter in the log.V(2).Info call with a sanitized classification or reason string that describes the type of error without exposing internal details. For example, instead of passing the raw err, classify it as a generic reason like "cluster_id_unavailable" or similar that indicates the failure type without revealing sensitive infrastructure information.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controllers/mustgather/directory_naming.go`:
- Around line 54-58: The error logging in the cluster ID retrieval error
handling block is logging the raw err value directly, which can expose internal
API endpoints or hostnames from client-go error messages. Replace the "error",
err parameter in the log.V(2).Info call with a sanitized classification or
reason string that describes the type of error without exposing internal
details. For example, instead of passing the raw err, classify it as a generic
reason like "cluster_id_unavailable" or similar that indicates the failure type
without revealing sensitive infrastructure information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 83a413cb-4faa-4e3c-ac93-3e35b2c6f0d3
📒 Files selected for processing (1)
controllers/mustgather/directory_naming.go
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/mustgather/directory_naming.go`:
- Around line 42-43: The random number generation in the directory naming logic
uses rand.Int63() which can produce up to 19 digits, but the format specifier
%06d only sets a minimum width, not a maximum, resulting in inconsistent
directory name lengths. Replace the rand.Int63() call (around line 95) with
rand.Intn(1_000_000) to ensure the random component is limited to exactly 6
digits when formatted. This will maintain the correct directory naming format of
must-gather.local.<random>.<cluster-id-suffix>.<timestamp> with consistent
6-digit random values.
In `@controllers/mustgather/mustgather_controller.go`:
- Around line 201-202: The reqLogger.Error call is logging the full instance
object which may expose customer metadata. Replace the instance parameter in the
error logging statement with a stable identifier such as instance.Name,
instance.Namespace, or another unique identifier that does not expose sensitive
customer data. This ensures the log message provides necessary debugging context
while maintaining security and privacy standards.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4a8e9a23-b963-4d6f-a3dd-8fff2d950ae6
📒 Files selected for processing (3)
controllers/mustgather/directory_naming.gocontrollers/mustgather/directory_naming_test.gocontrollers/mustgather/mustgather_controller.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/mustgather/directory_naming.go`:
- Line 49: In the log.V(2).Info call on line 49 of directory_naming.go, the raw
error object is being logged as the "error" field, which may expose sensitive
internal cluster metadata and violates security guidelines. Remove the "error",
err parameters from the log statement to use only the sanitized message "Unable
to retrieve cluster ID, continuing without it" without including the raw error
payload.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4233e81-0e8b-477e-bf63-c325a6d53075
📒 Files selected for processing (3)
controllers/mustgather/directory_naming.gocontrollers/mustgather/directory_naming_test.gocontrollers/mustgather/mustgather_controller.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/mustgather/mustgather_controller.go
- controllers/mustgather/directory_naming_test.go
|
/retest |
|
/retest |
|
/retest |
| @@ -176,31 +177,31 @@ func (r *MustGatherReconciler) Reconcile(ctx context.Context, request reconcile. | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
the restructuring is done because
Old code (master) — built the job template before checking if the job already exists:
job := getJobFromInstance(...) // always runs — API calls, image lookup, etc.
job1 := client.Get(job.Name, ...) // then check if it exists
if not found → create
this branch — checks first, only builds if needed:
existingJob := client.Get(instance.Name, ...) // check first
if not found → job := getJobFromInstance(...) // only build when we need to create
getJobFromInstance now calls generateMustGatherDirectoryName, which makes an API call to fetch the ClusterVersion. On the old code path, that API call would happen on every reconcile — even when the job already exists and is running. With restructuring, the directory name generation (and the ClusterVersion lookup) only happens once, when actually creating the job.
The rename from job1 to existingJob is just a clarity
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/jira refresh |
|
@neha037: This pull request references MG-271 which is a valid jira issue. 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 summary |
✅ Action performedSummary regeneration triggered. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
@neha037: 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
Implements standard must-gather directory naming convention to align with
oc adm must-gather, ensuring consistency across all must-gather collection methods.Changes
Implementation
pkg/mustgatherutil/directory_naming.go- Directory name generation utilityGenerateMustGatherDirectoryName(): Generates directory names following the oc adm must-gather conventiongetClusterIDSuffix(): Retrieves last 12 characters of cluster ID from ClusterVersion resourcegenerateRandomSuffix(): Generates 6-digit random suffix for uniquenesscontrollers/mustgather/mustgather_controller.go- Callsmustgatherutil.GenerateMustGatherDirectoryName()before job creationcontrollers/mustgather/template.go- Passes directory name to gather container viaMUST_GATHER_DEST_DIRenvironment variableTests
pkg/mustgatherutil/directory_naming_test.gocontrollers/mustgather/mustgather_controller_test.go- Tests for directory name integration in job creationcontrollers/mustgather/template_test.go- Updated test calls to include directory name parametertest/e2e/must_gather_operator_test.go- E2E tests for directory naming behaviorDirectory Naming Format
With Cluster ID (default)
Example:
must-gather.local.ef0123456789.20260617T143025Z.042315Without Cluster ID (fallback)
Example:
must-gather.local.20260617T143025Z.042315Where:
clusterversion.config.openshift.io/versionYYYYMMDDTHHMMSSZRBAC
RBAC for ClusterVersion access is already present:
//+kubebuilder:rbac:groups=config.openshift.io,resources=clusterversions,verbs=get;list;watchTesting
Unit Tests
go test ./pkg/mustgatherutil/... ./controllers/mustgather/... -vTestGenerateMustGatherDirectoryName- Format validation with/without cluster ID (9 sub-tests)TestGenerateMustGatherDirectoryName_Uniqueness- Uniqueness across 100 runsTestGenerateMustGatherDirectoryName_UTCConversion- Timezone handling (3 sub-tests)TestGetClusterIDSuffix_WrongClusterVersionName- Fallback when ClusterVersion name doesn't matchE2E Tests
go test ./test/e2e/... -vVerification
Additional Notes
Implements
Jira: MG-271 - Change must-gather directory name in must-gather-operator
Acceptance Criteria
oc adm mustgather🤖 Generated with Claude Code