CCXDEV-15829: subscription gathering#1235
CCXDEV-15829: subscription gathering#1235opokornyy wants to merge 2 commits intoopenshift:masterfrom
Conversation
This commit implements a new gatherer that collects data from all Subscription CRs present in the cluster. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-15829 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. |
WalkthroughIntroduces operator subscription gathering capability to the cluster configuration gatherer. Adds a sample Subscription manifest, implements the GatherSubscription method to collect subscription resources across namespaces via the dynamic client, registers the gatherer function, and includes comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
![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/gatherers/clusterconfig/gather_subscription.go`:
- Around line 64-71: The record path for collected Subscriptions uses only
rc.GetName() which can collide across namespaces; update the record creation in
the loop that iterates subscriptionList.Items so the Name uses both namespace
and name (e.g., fmt.Sprintf("config/subscriptions/%s/%s", rc.GetNamespace(),
rc.GetName())) when constructing the record.Record for
record.ResourceMarshaller{Resource: &subscriptionList.Items[i]}. Also add a
unit/integration test that creates two Subscription objects with the same name
in different namespaces and confirms both records are produced with distinct
paths to prevent overwrite/collision.
| for i, rc := range subscriptionList.Items { | ||
| // Drop status field that is not needed | ||
| delete(subscriptionList.Items[i].Object, "status") | ||
|
|
||
| records = append(records, record.Record{ | ||
| Name: fmt.Sprintf("config/subscriptions/%s", rc.GetName()), | ||
| Item: record.ResourceMarshaller{Resource: &subscriptionList.Items[i]}, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check naming conventions for config/subscriptions and other namespaced resources
echo "=== Looking for config/subscriptions references ==="
rg -n --type=go -C2 'config/subscriptions'
echo -e "\n=== Checking for other namespaced resource patterns ==="
rg -n --type=go 'config/[a-z]+/.*GetNamespace'
echo -e "\n=== Looking for test files related to gather_subscription ==="
fd -e "test" | xargs grep -l "gather_subscription\|config/subscriptions" 2>/dev/null
echo -e "\n=== Checking if other resources include namespace in path ==="
rg -n --type=go 'GetNamespace.*fmt.Sprintf\|fmt.Sprintf.*GetNamespace' pkg/gatherers/clusterconfigRepository: openshift/insights-operator
Length of output: 1839
🏁 Script executed:
#!/bin/bash
# Examine the gather_subscription.go file to understand the context better
echo "=== Full context of gather_subscription.go around lines 64-71 ==="
cat -n pkg/gatherers/clusterconfig/gather_subscription.go | sed -n '55,80p'
echo -e "\n=== Check the resource type definition ==="
rg -n "type.*Subscription" --type=go | head -20Repository: openshift/insights-operator
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Check if Subscriptions are namespace-scoped or cluster-scoped
echo "=== Looking for Subscription resource metadata/schema ==="
rg -n "Subscription" --type=go pkg/gatherers/clusterconfig/ -A5 -B5 | head -60
echo -e "\n=== Check for any test files that might validate the path format ==="
find . -name "*subscription*test*" -o -name "*test*subscription*" 2>/dev/null | head -10Repository: openshift/insights-operator
Length of output: 5007
🏁 Script executed:
#!/bin/bash
# Check how the subscriptions are fetched - cluster or namespace scoped
echo "=== Full gatherSubscriptions function ==="
cat -n pkg/gatherers/clusterconfig/gather_subscription.go | sed -n '46,75p'
echo -e "\n=== Check dynamic client usage for list parameters ==="
rg -n "\.List\(" pkg/gatherers/clusterconfig/gather_subscription.go -A3 -B3Repository: openshift/insights-operator
Length of output: 1390
🏁 Script executed:
#!/bin/bash
# Compare with other namespace-scoped resources to see naming patterns
echo "=== Look for other gatherers that handle namespaced resources ==="
ls -la pkg/gatherers/clusterconfig/gather_*.go | head -20
echo -e "\n=== Check for patterns in other gatherers with namespace ==="
rg -n "GetNamespace()" pkg/gatherers/clusterconfig/ --type=go | head -20
echo -e "\n=== Examine one example: check gather_pod or similar ==="
rg -n "config/.*GetNamespace\|config/.*fmt.Sprintf.*namespace" pkg/gatherers/clusterconfig/ --type=goRepository: openshift/insights-operator
Length of output: 4789
🏁 Script executed:
#!/bin/bash
# Check what the test expects for the record path
echo "=== Check test expectations for path format ==="
cat -n pkg/gatherers/clusterconfig/gather_subscription_test.go | sed -n '80,150p'
echo -e "\n=== Look for assertions on record names ==="
rg -n "config/subscriptions\|Record{" pkg/gatherers/clusterconfig/gather_subscription_test.go -A3 -B1Repository: openshift/insights-operator
Length of output: 2888
🏁 Script executed:
#!/bin/bash
# Check the test assertions more carefully
echo "=== Full test assertions section ==="
cat -n pkg/gatherers/clusterconfig/gather_subscription_test.go | sed -n '147,200p'
echo -e "\n=== Check if test validates record names ==="
grep -n "expectedRecordName\|assert.*Name" pkg/gatherers/clusterconfig/gather_subscription_test.goRepository: openshift/insights-operator
Length of output: 2772
🏁 Script executed:
#!/bin/bash
# Check if the two subscriptions in the test have the same name or different names
echo "=== Check subscription names and namespaces in test YAML ==="
cat -n pkg/gatherers/clusterconfig/gather_subscription_test.go | sed -n '21,75p'Repository: openshift/insights-operator
Length of output: 2143
Include namespace in Subscription record path to prevent collisions.
The code collects Subscriptions from all namespaces but uses only the Subscription name in the archive path. If two namespaces contain the same Subscription name, records will collide and data will be lost. This pattern is inconsistent with how other namespace-scoped resources are handled throughout the codebase (e.g., config/pdbs/{namespace}/{name}, config/logging/{namespace}/{name}, config/machines/{namespace}/{name}).
Update line 69 to include namespace in the path and add test coverage for same-named Subscriptions across different namespaces.
Proposed fix
- Name: fmt.Sprintf("config/subscriptions/%s", rc.GetName()),
+ Name: fmt.Sprintf("config/subscriptions/%s/%s", rc.GetNamespace(), rc.GetName()),📝 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.
| for i, rc := range subscriptionList.Items { | |
| // Drop status field that is not needed | |
| delete(subscriptionList.Items[i].Object, "status") | |
| records = append(records, record.Record{ | |
| Name: fmt.Sprintf("config/subscriptions/%s", rc.GetName()), | |
| Item: record.ResourceMarshaller{Resource: &subscriptionList.Items[i]}, | |
| }) | |
| for i, rc := range subscriptionList.Items { | |
| // Drop status field that is not needed | |
| delete(subscriptionList.Items[i].Object, "status") | |
| records = append(records, record.Record{ | |
| Name: fmt.Sprintf("config/subscriptions/%s/%s", rc.GetNamespace(), rc.GetName()), | |
| Item: record.ResourceMarshaller{Resource: &subscriptionList.Items[i]}, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gatherers/clusterconfig/gather_subscription.go` around lines 64 - 71, The
record path for collected Subscriptions uses only rc.GetName() which can collide
across namespaces; update the record creation in the loop that iterates
subscriptionList.Items so the Name uses both namespace and name (e.g.,
fmt.Sprintf("config/subscriptions/%s/%s", rc.GetNamespace(), rc.GetName())) when
constructing the record.Record for record.ResourceMarshaller{Resource:
&subscriptionList.Items[i]}. Also add a unit/integration test that creates two
Subscription objects with the same name in different namespaces and confirms
both records are produced with distinct paths to prevent overwrite/collision.
|
@opokornyy: 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. |
2 participants
This PR implements a new gatherer that collects data from all Subscription CRs present in the cluster.
Categories
Sample Archive
docs/insights-archive-sample/config/subscriptions/community-kubevirt-hyperconverged.jsonDocumentation
NoneUnit Tests
pkg/gatherers/clusterconfig/gather_subscription_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
CCXDEV-15829