Draft: feat(analytics): migrate right-sizing from Policy-based (MCO) to always-MCOA for ACM 5.0#2567
Open
rajusem wants to merge 8 commits into
Open
Draft: feat(analytics): migrate right-sizing from Policy-based (MCO) to always-MCOA for ACM 5.0#2567rajusem wants to merge 8 commits into
rajusem wants to merge 8 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rajusem 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 |
Contributor
|
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:
📝 WalkthroughWalkthroughRight-sizing management is consolidated around MCOA. Delegation and creation paths are removed, cleanup uses labeled and named resource deletion, analytics deletion gains stabilization handling, RBAC is reduced, and tests validate ADC state and absence of legacy Policy resources. ChangesMCOA right-sizing consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCOController
participant MCORenderer
participant AnalyticsReconciler
participant AddOnDeploymentConfig
participant RightSizingCleanup
MCOController->>MCORenderer: render MCOA resources from CR state
MCORenderer->>AddOnDeploymentConfig: set right-sizing variables
AnalyticsReconciler->>AddOnDeploymentConfig: sync disabled state during deletion
AnalyticsReconciler->>RightSizingCleanup: clean legacy and labeled resources
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
rajusem
changed the title
openshift-ci
Bot
added
dco-signoff: yes
needs-rebase
and removed
dco-signoff: no
needs-rebase
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operators/multiclusterobservability/controllers/analytics/analytics_controller.go (1)
176-231: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winApply the defaults in memory instead of re-reading through the cache
r.Client.Getcan still return the pre-patch MCO here, so this reconcile may hand stale right-sizing flags toCleanupLegacyPolicyResourcesandsyncRightSizingStateToADC. The cache is a slippery gearbox right after a write.🤖 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 `@operators/multiclusterobservability/controllers/analytics/analytics_controller.go` around lines 176 - 231, Update ensureRightSizingDefaults to apply the default right-sizing values directly to the in-memory instance after determining missing fields, instead of re-reading it through r.Client.Get after the patch. Preserve existing explicitly configured values, set only absent namespaceRightSizingRecommendation and virtualizationRightSizingRecommendation enabled fields to true, and ensure downstream callers receive the updated instance even if the cache is stale.
🧹 Nitpick comments (1)
operators/multiclusterobservability/controllers/analytics/analytics_controller_test.go (1)
196-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Great coverage of the stabilization-window and ADC-sync branches, and the mid-window test's explicit anti-flake comment is a nice touch (9.999s of buffer — no flapping on this watch 🔍).
One optional gap: no test drives
Reconcilethrough a failingCleanupLegacyPolicyResources/syncRightSizingStateToADCin the normal (non-deletion) path to confirmmigrationDonestaysfalseon failure and the error propagates. As per path instructions: "Verify integration test scenarios cover error cases." Not blocking — the current suite already covers the success and stabilization paths solidly.Also applies to: 422-617
🤖 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 `@operators/multiclusterobservability/controllers/analytics/analytics_controller_test.go` around lines 196 - 342, Add a normal, non-deletion Reconcile test covering failures from CleanupLegacyPolicyResources or syncRightSizingStateToADC. Assert the returned error is propagated and the reconciler’s migrationDone remains false, using the existing AnalyticsReconciler test setup and failure-injection patterns.Source: Path instructions
🤖 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
`@operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go`:
- Around line 67-130: Update the per-object delete error handling in
CleanupRSResourcesByLabel for Policy, PlacementBinding, Placement, and ConfigMap
to wrap non-NotFound errors with fmt.Errorf, including each resource’s namespace
and name and preserving the original error with %w. Keep the existing NotFound
suppression and errors.Join aggregation unchanged, matching the context pattern
used by CleanupLegacyPolicyResourcesByName.
---
Outside diff comments:
In
`@operators/multiclusterobservability/controllers/analytics/analytics_controller.go`:
- Around line 176-231: Update ensureRightSizingDefaults to apply the default
right-sizing values directly to the in-memory instance after determining missing
fields, instead of re-reading it through r.Client.Get after the patch. Preserve
existing explicitly configured values, set only absent
namespaceRightSizingRecommendation and virtualizationRightSizingRecommendation
enabled fields to true, and ensure downstream callers receive the updated
instance even if the cache is stale.
---
Nitpick comments:
In
`@operators/multiclusterobservability/controllers/analytics/analytics_controller_test.go`:
- Around line 196-342: Add a normal, non-deletion Reconcile test covering
failures from CleanupLegacyPolicyResources or syncRightSizingStateToADC. Assert
the returned error is propagated and the reconciler’s migrationDone remains
false, using the existing AnalyticsReconciler test setup and failure-injection
patterns.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8261fce9-de80-45e9-bd6f-0bedc4609fae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
go.modoperators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yamloperators/multiclusterobservability/config/rbac/mco_role.yamloperators/multiclusterobservability/controllers/analytics/analytics_controller.gooperators/multiclusterobservability/controllers/analytics/analytics_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/types.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule_test.gooperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/controllers/multiclusterobservability/predicate_func_test.gooperators/multiclusterobservability/main.gooperators/multiclusterobservability/pkg/rendering/renderer.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.gooperators/multiclusterobservability/pkg/util/rightsizing.gooperators/multiclusterobservability/pkg/util/rightsizing_test.gotests/pkg/tests/observability_right_sizing_test.go
💤 Files with no reviewable changes (41)
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap_test.go
- operators/multiclusterobservability/pkg/util/rightsizing_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule.go
- operators/multiclusterobservability/main.go
- go.mod
- operators/multiclusterobservability/pkg/util/rightsizing.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy.go
Contributor
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)
operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go (1)
153-157: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap per-object delete errors with name and namespace context.
We successfully upgraded the error telemetry in
CleanupRSResourcesByLabelbased on previous feedback, but it looks like its sibling function,CleanupLegacyPolicyResourcesByLabel, is still transmitting unencrypted telemetry (bare errors). Let's wrap these errors with the appropriate context so we aren't left hunting ghosts during a stuck-deletion incident. As per coding guidelines: "Wrap errors withfmt.Errorf("...: %w", err)."
operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go#L153-L157: Wrap the error for the legacy Policy deletion usingfmt.Errorf.operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go#L169-L173: Wrap the error for the legacy PlacementBinding deletion usingfmt.Errorf.operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go#L185-L189: Wrap the error for the legacy Placement deletion usingfmt.Errorf.We're debugging together! 🔍🚨✅
🔧 Proposed fixes
For Policy deletion (lines 153-157):
if err := c.Delete(ctx, &policyList.Items[i]); err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, err) + errs = append(errs, fmt.Errorf("rs - failed to delete legacy Policy %s/%s: %w", policyList.Items[i].Namespace, policyList.Items[i].Name, err)) } else if err == nil {For PlacementBinding deletion (lines 169-173):
if err := c.Delete(ctx, &pbList.Items[i]); err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, err) + errs = append(errs, fmt.Errorf("rs - failed to delete legacy PlacementBinding %s/%s: %w", pbList.Items[i].Namespace, pbList.Items[i].Name, err)) } else if err == nil {For Placement deletion (lines 185-189):
if err := c.Delete(ctx, &placementList.Items[i]); err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, err) + errs = append(errs, fmt.Errorf("rs - failed to delete legacy Placement %s/%s: %w", placementList.Items[i].Namespace, placementList.Items[i].Name, err)) } else if err == nil {🤖 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 `@operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go` around lines 153 - 157, Update CleanupLegacyPolicyResourcesByLabel in operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go at lines 153-157, 169-173, and 185-189 to wrap each non-NotFound deletion error with fmt.Errorf using the deleted object's resource type, name, namespace, and %w, then append the wrapped error to errs.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
`@operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go`:
- Around line 153-157: Update CleanupLegacyPolicyResourcesByLabel in
operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go
at lines 153-157, 169-173, and 185-189 to wrap each non-NotFound deletion error
with fmt.Errorf using the deleted object's resource type, name, namespace, and
%w, then append the wrapped error to errs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f33e3507-3010-48bd-9771-42e7baa9cd0b
📒 Files selected for processing (4)
operators/multiclusterobservability/controllers/analytics/analytics_controller.gooperators/multiclusterobservability/controllers/analytics/analytics_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.gooperators/multiclusterobservability/pkg/util/rightsizing.go
💤 Files with no reviewable changes (1)
- operators/multiclusterobservability/pkg/util/rightsizing.go
🚧 Files skipped from review as they are similar to previous changes (1)
- operators/multiclusterobservability/controllers/analytics/analytics_controller.go
rajusem
force-pushed
the
OBSINTA-967
branch
2 times, most recently
from
d2c9a2f to
049e394
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yaml (1)
490-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign policy RBAC with placement RBAC for strict least privilege.
Great job trimming down these permissions! Tightening the RBAC valves is exactly what we want when retiring a subsystem. 🚨
I noticed a minor inconsistency: we successfully dropped
getforplacements(leaving justlistanddelete), but we kept it here forpoliciesandplacementbindings. Since our cleanup sweeps inrs-utility/component.goonly rely onlistanddeletewithout callingget, we can safely strip it here too, giving us a perfectly sealed security bulkhead. We're debugging together! ✅As per path instructions, check for security best practices (RBAC, SecurityContext, NetworkPolicies).
🔒️ Proposed RBAC adjustment
- apiGroups: - policy.open-cluster-management.io resources: - policies - placementbindings verbs: - - get - list - delete🤖 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 `@operators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yaml` around lines 490 - 498, Remove the get verb from the policy.open-cluster-management.io RBAC rule covering policies and placementbindings, leaving only list and delete to match the cleanup behavior in rs-utility/component.go and the corresponding placement permissions.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@operators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yaml`:
- Around line 490-498: Remove the get verb from the
policy.open-cluster-management.io RBAC rule covering policies and
placementbindings, leaving only list and delete to match the cleanup behavior in
rs-utility/component.go and the corresponding placement permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69eaf8c2-f92d-462f-9f21-c8a64eacc1e2
📒 Files selected for processing (7)
operators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yamloperators/multiclusterobservability/config/rbac/mco_role.yamloperators/multiclusterobservability/controllers/analytics/analytics_controller.gooperators/multiclusterobservability/controllers/analytics/analytics_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.gooperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/pkg/util/rightsizing.go
💤 Files with no reviewable changes (1)
- operators/multiclusterobservability/pkg/util/rightsizing.go
🚧 Files skipped from review as they are similar to previous changes (5)
- operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go
- operators/multiclusterobservability/controllers/analytics/analytics_controller.go
- operators/multiclusterobservability/config/rbac/mco_role.yaml
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go
- operators/multiclusterobservability/controllers/analytics/analytics_controller_test.go
In ACM 5.0 GA, right-sizing uses ManifestWork-based deployment exclusively. Removes the annotation-based delegation check and all Policy-based code. Key changes: - analytics_controller: always runs in MCOA mode; one-time legacy Policy cleanup on first reconcile (idempotent on pod restart); syncs RS state to ADC so MCOA knows what to deploy - Deletion path: stabilization window (10s) prevents premature finalizer removal; Phase 2 re-syncs "disabled" to ADC before cleanup to guard against stale main-controller ADC overwrites - Remove rs-namespace/ and rs-virtualization/ packages (Policy-based, now owned by MCOA); remove policy/placement/configmap builders from rs-utility; retain cleanup functions only (label-based and name-based for pre-label 2.16 unlabeled resources) - RBAC: downscope analytics controller — remove write permissions for Policy/PlacementBinding/Placement (list and delete only) - renderer_mcoa.go: rightSizingDelegated always true; remove IsRightSizingDelegated from pkg/util (annotation check gone) - Add E2E test for RS migration phases Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
…ow path - Promote stabilizationWindow from local const inside Reconcile to package-level analyticsStabilizationWindow so tests can reference it without hardcoding magic literals. - Add TestAnalyticsReconciler_DeletionBlocksMidWindow: exercises the mid-stabilization-window requeue path (lines 92-95) that was previously untested. Sets cleanupAt to 1ms ago so elapsed<<window; asserts RequeueAfter > 0 and < window, and that the analytics finalizer is not removed prematurely. - Enhance TestAnalyticsReconciler_DeletionCleansUp: pre-create ADC with enabled values; assert Phase 1 flips all three ADC vars to disabled and result.RequeueAfter == analyticsStabilizationWindow; assert Phase 2 removes only the analytics finalizer. - Fix misleading Phase 1 error log: "continuing with cleanup" replaced with "starting stabilization window" since cleanup has not begun at that point. - Add leader-election comment on AnalyticsReconciler struct explaining that migrationDone and cleanupAt are in-memory state requiring singleton execution. Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
…gthen cleanup tests RS-009: Remove MCOARendererOptions.RightSizingDelegated — the field was always set to true in ACM 5.0 GA. Inline the condition in renderer.go as MCOAEnabled(cr) || RightSizingEnabled(cr). Removes dead code across renderer_mcoa.go, renderer.go, and the main controller. TEST-001: Replace require.True(t, err != nil) with the stricter require.True(t, apierrors.IsNotFound(err)) in migration test assertions — a non-nil error that is not NotFound would previously be a silent false-pass. F9: Pre-create labeled Placement and ConfigMap in DeletionCleansUp test so Phase 2 cleanup is verified to actually delete resources, not just remove the finalizer. RS-002: Add component_test.go in rsutility with two tests that pin the key behavioral difference between the two cleanup functions: - CleanupLegacyPolicyResourcesByLabel: deletes Policy/PB/Placement, preserves ConfigMaps - CleanupRSResourcesByLabel: deletes all labeled resources including ConfigMaps Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
… sites The _ error discard on GetComponentConfig was unexplained. The function only errors on unknown component types, which can't happen when callers pass compile-time constants (ComponentTypeNamespace/Virtualization). Added a one-line comment at each call site in CleanupRightSizingResources and CleanupLegacyPolicyResources. Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
Update E2E tests to match the always-MCOA right-sizing migration: - g0/g1: Remove Policy resource expectations (MCO no longer creates Policies), add ADC verification for delegation state and RS feature state - g2: Complete rewrite — replace annotation-based delegation test with always-MCOA behavior: ADC always has rightSizingDelegated=true, RS variables reflect CR spec, no Policy resources at any point - Add Consistently checks to verify no Policy resources are created Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
- Remove trailing blank lines in analytics_controller.go, rightsizing.go - Align constant declarations in component.go - Wrap per-object delete errors with fmt.Errorf in CleanupRSResourcesByLabel - Apply defaults in-memory in ensureRightSizingDefaults instead of re-reading - Add TestReconcile_MigrationSetsFlag for normal-path migration coverage Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/pkg/tests/observability_right_sizing_test.go (1)
749-767: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore-to-defaults
AfterAlldoesn't wait for ADC to converge, unlike every other teardown in this file.Every other
AfterAll/state-change assertion in this file pollsgetADCCustomVarwithEventuallyafter flipping the MCO CR (e.g. Lines 277-287, 500-510). ThisAfterAllonly waits for the CRUpdateAPI call to succeed, not for the ADC to actually reflectenabledagain. If any test elsewhere depends on right-sizing being enabled by default right after this suite finishes, it can race against reconciliation and flake.As per path instructions, "Ensure tests are deterministic and not flaky."
♻️ Proposed fix: verify ADC state before returning
_, err = dynClient.Resource(mcoGVR).Update(context.TODO(), mco, metav1.UpdateOptions{}) return err }, 2*time.Minute, 10*time.Second).Should(Succeed()) + + Eventually(func() error { + nsVal, err := getADCCustomVar(dynClient, adcName, adcNamespace, nsRSKey) + if err != nil { + return err + } + if nsVal != "enabled" { + return fmt.Errorf("expected ADC %s=%q, got %q", nsRSKey, "enabled", nsVal) + } + virtVal, err := getADCCustomVar(dynClient, adcName, adcNamespace, virtRSKey) + if err != nil { + return err + } + if virtVal != "enabled" { + return fmt.Errorf("expected ADC %s=%q, got %q", virtRSKey, "enabled", virtVal) + } + return nil + }, 3*time.Minute, 10*time.Second).Should(Succeed()) })🤖 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 `@tests/pkg/tests/observability_right_sizing_test.go` around lines 749 - 767, Update the restore-to-defaults AfterAll around the MCO CR update to wait for ADC reconciliation before completing. After successfully updating both right-sizing recommendation fields in the Eventually callback, poll getADCCustomVar until namespaceRightSizingRecommendation and virtualizationRightSizingRecommendation both report enabled, preserving the existing timeout and retry behavior so the suite does not finish before defaults are effective.Source: Path instructions
🤖 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 `@tests/pkg/tests/observability_right_sizing_test.go`:
- Around line 643-658: The nested context containing the BeforeAll setup must be
part of an Ordered Ginkgo container. Update the relevant context hierarchy
around “when namespace RS is disabled but virtualization RS remains enabled” so
this setup is under an Ordered container, either by making the context Ordered
or moving BeforeAll to the existing parent Ordered block.
---
Nitpick comments:
In `@tests/pkg/tests/observability_right_sizing_test.go`:
- Around line 749-767: Update the restore-to-defaults AfterAll around the MCO CR
update to wait for ADC reconciliation before completing. After successfully
updating both right-sizing recommendation fields in the Eventually callback,
poll getADCCustomVar until namespaceRightSizingRecommendation and
virtualizationRightSizingRecommendation both report enabled, preserving the
existing timeout and retry behavior so the suite does not finish before defaults
are effective.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: da835ce4-3384-4ab9-b9d8-c451bdcaf049
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
go.modoperators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yamloperators/multiclusterobservability/config/rbac/mco_role.yamloperators/multiclusterobservability/controllers/analytics/analytics_controller.gooperators/multiclusterobservability/controllers/analytics/analytics_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/types.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy_test.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule.gooperators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule_test.gooperators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.gooperators/multiclusterobservability/controllers/multiclusterobservability/predicate_func_test.gooperators/multiclusterobservability/main.gooperators/multiclusterobservability/pkg/rendering/renderer.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa.gooperators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.gooperators/multiclusterobservability/pkg/util/rightsizing.gooperators/multiclusterobservability/pkg/util/rightsizing_test.gotests/pkg/tests/observability_right_sizing_test.go
💤 Files with no reviewable changes (43)
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placementbinding_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/policy_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placementbinding_test.go
- operators/multiclusterobservability/bundle/manifests/multicluster-observability-operator.clusterserviceversion.yaml
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placementbinding_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/prometheusrule_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/helper_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/policy.go
- operators/multiclusterobservability/main.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/helper.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/configmap_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/configmap.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule_test.go
- operators/multiclusterobservability/config/rbac/mco_role.yaml
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-virtualization/prometheusrule.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/placement_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/prometheusrule.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/policy_test.go
- operators/multiclusterobservability/pkg/util/rightsizing.go
- operators/multiclusterobservability/pkg/util/rightsizing_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-namespace/helper_test.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (11)
- operators/multiclusterobservability/controllers/multiclusterobservability/predicate_func_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/types.go
- operators/multiclusterobservability/pkg/rendering/renderer.go
- operators/multiclusterobservability/pkg/rendering/renderer_mcoa_test.go
- operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rightsizing_controller.go
- operators/multiclusterobservability/pkg/rendering/renderer_mcoa.go
- operators/multiclusterobservability/controllers/analytics/analytics_controller.go
- operators/multiclusterobservability/controllers/analytics/analytics_controller_test.go
- operators/multiclusterobservability/controllers/analytics/rightsizing/rs-utility/component.go
- TestGetComponentConfig: namespace/virt/nil/unknown-type cases - TestCleanupLegacyPolicyResourcesByName: 2.16 upgrade path (6 resources deleted, NotFound ignored) - TestEnsureRightSizingDefaultsPreservesExistingValue: partial defaults (one present, one absent) Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
- Fix go fmt import ordering in renderer_mcoa.go - Add Ordered to nested Context blocks in E2E tests (required by Ginkgo for BeforeAll) Signed-off-by: Raj Zalavadia <rzalavad@redhat.com>
|
@rajusem: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(ManifestWork-based) for ACM 5.0 GA
rs-utility policy/placement/placementbinding code)
and dual-mode switching logic
Changes
namespace and virtualization right-sizing
Summary by CodeRabbit
Summary