OCPBUGS-105226: secrets and configmap revisions count gathering - #1316
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a clusterconfig gatherer for counting revisioned ConfigMaps and Secrets by base name, registers it, tests its filtering and aggregation behavior, and documents the resulting archive record with sample JSON. ChangesRevisioned object counts
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GatherRevisionedObjectCounts
participant KubernetesAPI
participant ArchiveRecord
GatherRevisionedObjectCounts->>KubernetesAPI: List ConfigMaps and Secrets
KubernetesAPI-->>GatherRevisionedObjectCounts: Return revision-owned objects
GatherRevisionedObjectCounts->>ArchiveRecord: Store JSON revision counts
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
opokornyy
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-15210 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 task 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. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go`:
- Around line 62-110: gatherRevisionedObjectCounts currently logs
ConfigMaps/Secrets List failures at klog.V(2) and still returns a nil error
slice, which hides partial data collection failures. Update the function to
collect list errors from coreClient.ConfigMaps(namespace).List and
coreClient.Secrets(namespace).List, preserve any successful counts, and return
those errors in the second result so callers can detect incomplete output. Keep
the existing record assembly in gatherRevisionedObjectCounts and adjust the
return path to include the accumulated errors alongside the
config/revisioned_objects record.
🪄 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: Enterprise
Run ID: 047a4af8-2f28-4e3c-a5e4-f9a73b01fe19
📒 Files selected for processing (6)
docs/gathered-data.mddocs/insights-archive-sample/config/revisioned_objects.jsonpkg/gatherers/clusterconfig/clusterconfig_gatherer.gopkg/gatherers/clusterconfig/const.gopkg/gatherers/clusterconfig/gather_revisioned_objects.gopkg/gatherers/clusterconfig/gather_revisioned_objects_test.go
| func gatherRevisionedObjectCounts(ctx context.Context, coreClient corev1client.CoreV1Interface) ([]record.Record, []error) { | ||
| namespaceCounts := make(map[string]*NamespaceRevisionCounts) | ||
|
|
||
| for _, namespace := range revisionedObjectNamespaces { | ||
| nsCounts := &NamespaceRevisionCounts{ | ||
| ConfigMaps: make(map[string]int), | ||
| Secrets: make(map[string]int), | ||
| } | ||
|
|
||
| // Gather ConfigMap counts | ||
| configMaps, err := coreClient.ConfigMaps(namespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.V(2).Infof("Unable to read ConfigMaps in namespace %s: %v", namespace, err) | ||
| } else { | ||
| for i := range configMaps.Items { | ||
| cm := &configMaps.Items[i] | ||
| if hasRevisionStatusOwner(cm.OwnerReferences) { | ||
| baseName := extractBaseName(cm.Name) | ||
| nsCounts.ConfigMaps[baseName]++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Gather Secret counts | ||
| secrets, err := coreClient.Secrets(namespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.V(2).Infof("Unable to read Secrets in namespace %s: %v", namespace, err) | ||
| } else { | ||
| for i := range secrets.Items { | ||
| secret := &secrets.Items[i] | ||
| if hasRevisionStatusOwner(secret.OwnerReferences) { | ||
| baseName := extractBaseName(secret.Name) | ||
| nsCounts.Secrets[baseName]++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Only add namespace to output if it has any revisioned objects | ||
| if len(nsCounts.ConfigMaps) > 0 || len(nsCounts.Secrets) > 0 { | ||
| namespaceCounts[namespace] = nsCounts | ||
| } | ||
| } | ||
|
|
||
| // Return single record with all counts | ||
| return []record.Record{{ | ||
| Name: "config/revisioned_objects", | ||
| Item: record.JSONMarshaller{Object: namespaceCounts}, | ||
| }}, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
List errors are silently swallowed instead of being returned.
When ConfigMaps(namespace).List(...) or Secrets(namespace).List(...) fails (RBAC issue, API server hiccup, namespace missing, etc.), the error is only logged at klog.V(2) (verbose, off by default) and the function proceeds as if the namespace had zero revisioned objects. The function's second return value []error is always nil (Line 109), so callers/archive tooling never learn that data is incomplete or missing for that namespace — this silently produces misleading "0 revisions" output instead of surfacing a gathering failure.
Consider collecting these errors and returning them alongside the record, consistent with how other gatherers propagate partial failures.
🐛 Proposed fix to propagate list errors
func gatherRevisionedObjectCounts(ctx context.Context, coreClient corev1client.CoreV1Interface) ([]record.Record, []error) {
namespaceCounts := make(map[string]*NamespaceRevisionCounts)
+ var errs []error
for _, namespace := range revisionedObjectNamespaces {
nsCounts := &NamespaceRevisionCounts{
ConfigMaps: make(map[string]int),
Secrets: make(map[string]int),
}
// Gather ConfigMap counts
configMaps, err := coreClient.ConfigMaps(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
klog.V(2).Infof("Unable to read ConfigMaps in namespace %s: %v", namespace, err)
+ errs = append(errs, err)
} else {
for i := range configMaps.Items {
cm := &configMaps.Items[i]
if hasRevisionStatusOwner(cm.OwnerReferences) {
baseName := extractBaseName(cm.Name)
nsCounts.ConfigMaps[baseName]++
}
}
}
// Gather Secret counts
secrets, err := coreClient.Secrets(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
klog.V(2).Infof("Unable to read Secrets in namespace %s: %v", namespace, err)
+ errs = append(errs, err)
} else {
for i := range secrets.Items {
secret := &secrets.Items[i]
if hasRevisionStatusOwner(secret.OwnerReferences) {
baseName := extractBaseName(secret.Name)
nsCounts.Secrets[baseName]++
}
}
}
// Only add namespace to output if it has any revisioned objects
if len(nsCounts.ConfigMaps) > 0 || len(nsCounts.Secrets) > 0 {
namespaceCounts[namespace] = nsCounts
}
}
// Return single record with all counts
return []record.Record{{
Name: "config/revisioned_objects",
Item: record.JSONMarshaller{Object: namespaceCounts},
- }}, nil
+ }}, errs
}📝 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.
| func gatherRevisionedObjectCounts(ctx context.Context, coreClient corev1client.CoreV1Interface) ([]record.Record, []error) { | |
| namespaceCounts := make(map[string]*NamespaceRevisionCounts) | |
| for _, namespace := range revisionedObjectNamespaces { | |
| nsCounts := &NamespaceRevisionCounts{ | |
| ConfigMaps: make(map[string]int), | |
| Secrets: make(map[string]int), | |
| } | |
| // Gather ConfigMap counts | |
| configMaps, err := coreClient.ConfigMaps(namespace).List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.V(2).Infof("Unable to read ConfigMaps in namespace %s: %v", namespace, err) | |
| } else { | |
| for i := range configMaps.Items { | |
| cm := &configMaps.Items[i] | |
| if hasRevisionStatusOwner(cm.OwnerReferences) { | |
| baseName := extractBaseName(cm.Name) | |
| nsCounts.ConfigMaps[baseName]++ | |
| } | |
| } | |
| } | |
| // Gather Secret counts | |
| secrets, err := coreClient.Secrets(namespace).List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.V(2).Infof("Unable to read Secrets in namespace %s: %v", namespace, err) | |
| } else { | |
| for i := range secrets.Items { | |
| secret := &secrets.Items[i] | |
| if hasRevisionStatusOwner(secret.OwnerReferences) { | |
| baseName := extractBaseName(secret.Name) | |
| nsCounts.Secrets[baseName]++ | |
| } | |
| } | |
| } | |
| // Only add namespace to output if it has any revisioned objects | |
| if len(nsCounts.ConfigMaps) > 0 || len(nsCounts.Secrets) > 0 { | |
| namespaceCounts[namespace] = nsCounts | |
| } | |
| } | |
| // Return single record with all counts | |
| return []record.Record{{ | |
| Name: "config/revisioned_objects", | |
| Item: record.JSONMarshaller{Object: namespaceCounts}, | |
| }}, nil | |
| } | |
| func gatherRevisionedObjectCounts(ctx context.Context, coreClient corev1client.CoreV1Interface) ([]record.Record, []error) { | |
| namespaceCounts := make(map[string]*NamespaceRevisionCounts) | |
| var errs []error | |
| for _, namespace := range revisionedObjectNamespaces { | |
| nsCounts := &NamespaceRevisionCounts{ | |
| ConfigMaps: make(map[string]int), | |
| Secrets: make(map[string]int), | |
| } | |
| // Gather ConfigMap counts | |
| configMaps, err := coreClient.ConfigMaps(namespace).List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.V(2).Infof("Unable to read ConfigMaps in namespace %s: %v", namespace, err) | |
| errs = append(errs, err) | |
| } else { | |
| for i := range configMaps.Items { | |
| cm := &configMaps.Items[i] | |
| if hasRevisionStatusOwner(cm.OwnerReferences) { | |
| baseName := extractBaseName(cm.Name) | |
| nsCounts.ConfigMaps[baseName]++ | |
| } | |
| } | |
| } | |
| // Gather Secret counts | |
| secrets, err := coreClient.Secrets(namespace).List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.V(2).Infof("Unable to read Secrets in namespace %s: %v", namespace, err) | |
| errs = append(errs, err) | |
| } else { | |
| for i := range secrets.Items { | |
| secret := &secrets.Items[i] | |
| if hasRevisionStatusOwner(secret.OwnerReferences) { | |
| baseName := extractBaseName(secret.Name) | |
| nsCounts.Secrets[baseName]++ | |
| } | |
| } | |
| } | |
| // Only add namespace to output if it has any revisioned objects | |
| if len(nsCounts.ConfigMaps) > 0 || len(nsCounts.Secrets) > 0 { | |
| namespaceCounts[namespace] = nsCounts | |
| } | |
| } | |
| // Return single record with all counts | |
| return []record.Record{{ | |
| Name: "config/revisioned_objects", | |
| Item: record.JSONMarshaller{Object: namespaceCounts}, | |
| }}, errs | |
| } |
🤖 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 `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go` around lines 62 -
110, gatherRevisionedObjectCounts currently logs ConfigMaps/Secrets List
failures at klog.V(2) and still returns a nil error slice, which hides partial
data collection failures. Update the function to collect list errors from
coreClient.ConfigMaps(namespace).List and coreClient.Secrets(namespace).List,
preserve any successful counts, and return those errors in the second result so
callers can detect incomplete output. Keep the existing record assembly in
gatherRevisionedObjectCounts and adjust the return path to include the
accumulated errors alongside the config/revisioned_objects record.
|
/retest |
1 similar comment
|
/retest |
Add new gatherer to count ConfigMap and Secret revisions in openshift-kube-apiserver namespace. Groups objects by base name and reports version counts to identify objects with excessive historical revisions (>20 or >50) for cleanup recommendations. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
91352a7 to
948ecd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/gatherers/clusterconfig/gather_revisioned_objects.go (1)
62-110: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winList errors are silently swallowed instead of being returned.
coreClient.ConfigMaps(namespace).ListandcoreClient.Secrets(namespace).Listfailures are only logged atklog.V(2)(Lines 74, 88) and the function always returnsnilfor errors (Line 109), so callers never learn that data is incomplete for a namespace — this was already flagged in a previous review and remains unresolved.🤖 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 `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go` around lines 62 - 110, Update gatherRevisionedObjectCounts to collect and return errors from the ConfigMaps and Secrets List calls instead of only logging them. Preserve processing for other namespaces and the existing record output, but return the accumulated errors in the function’s []error result so callers can detect incomplete data.
🤖 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 `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go`:
- Around line 39-46: Update the emitted record name in the gatherer’s
record-construction logic to match the documented and sampled archive location
`config/revisioned_objects.json`; change the value currently identifying
`config/versioned_object_revision_counts` while preserving the existing data
collection behavior and `clusterconfig/revisioned_objects` configuration
identity.
---
Duplicate comments:
In `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go`:
- Around line 62-110: Update gatherRevisionedObjectCounts to collect and return
errors from the ConfigMaps and Secrets List calls instead of only logging them.
Preserve processing for other namespaces and the existing record output, but
return the accumulated errors in the function’s []error result so callers can
detect incomplete data.
🪄 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: Enterprise
Run ID: 87723d86-8f0a-498b-a4f5-937fb8762cbc
📒 Files selected for processing (6)
docs/gathered-data.mddocs/insights-archive-sample/config/revisioned_objects.jsonpkg/gatherers/clusterconfig/clusterconfig_gatherer.gopkg/gatherers/clusterconfig/const.gopkg/gatherers/clusterconfig/gather_revisioned_objects.gopkg/gatherers/clusterconfig/gather_revisioned_objects_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/insights-archive-sample/config/revisioned_objects.json
- pkg/gatherers/clusterconfig/gather_revisioned_objects_test.go
- pkg/gatherers/clusterconfig/const.go
- docs/gathered-data.md
| // ### Sample data | ||
| // - docs/insights-archive-sample/config/revisioned_objects.json | ||
| // | ||
| // ### Location in archive | ||
| // - `config/revisioned_objects.json` | ||
| // | ||
| // ### Config ID | ||
| // `clusterconfig/revisioned_objects` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record name mismatched with documented archive location and sample data.
The doc comment states the archive location is config/revisioned_objects.json (Lines 40-43), matching the PR's sample archive file docs/insights-archive-sample/config/revisioned_objects.json, but the actual emitted record uses Name: "config/versioned_object_revision_counts" (Line 107). This mismatch means the gathered data will land at a different archive path than what's documented and sampled, breaking the cross-file contract between this gatherer, the docs, and the sample archive.
🐛 Proposed fix
return []record.Record{{
- Name: "config/versioned_object_revision_counts",
+ Name: "config/revisioned_objects",
Item: record.JSONMarshaller{Object: namespaceCounts},
}}, nilAlso applies to: 106-109
🤖 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 `@pkg/gatherers/clusterconfig/gather_revisioned_objects.go` around lines 39 -
46, Update the emitted record name in the gatherer’s record-construction logic
to match the documented and sampled archive location
`config/revisioned_objects.json`; change the value currently identifying
`config/versioned_object_revision_counts` while preserving the existing data
collection behavior and `clusterconfig/revisioned_objects` configuration
identity.
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
Rename the sample JSON file and update code comments to reflect the correct archive path after the gatherer rename. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/insights-operator-e2e-tests Failures related to missing opentelemetry operator |
|
@opokornyy: Overrode contexts on behalf of opokornyy: ci/prow/insights-operator-e2e-tests 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 kubernetes-sigs/prow repository. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncaak, opokornyy 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 |
|
/retest |
|
/verified later @opokornyy |
openshift-ci-robot
added
verified-later
verified
|
@opokornyy: This PR has been marked to be verified later by 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. |
|
@opokornyy: all tests passed! 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. |
|
/retitle OCPBUGS-105226: secrets and configmap revisions count gathering |
openshift-ci
Bot
changed the title
|
@opokornyy: Jira Issue OCPBUGS-105226: All pull requests linked via external trackers have merged: This pull request has the 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. |
|
/cherry-pick release-4.22 |
|
@opokornyy: new pull request created: #1339 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 kubernetes-sigs/prow repository. |
4 participants
Add new gatherer to count ConfigMap and Secret revisions in openshift-kube-apiserver namespace. Groups objects by base name and reports version counts to identify objects with excessive historical revisions (>20 or >50) for cleanup recommendations.
Categories
Sample Archive
docs/insights-archive-sample/config/revisioned_objects.jsonDocumentation
docs/gathered-data.mdUnit Tests
pkg/gatherers/clusterconfig/gather_revisioned_objects_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
https://redhat.atlassian.net/browse/CCXDEV-15210
Summary by CodeRabbit
New Features
Documentation