Add annotation-based resource adoption policy#3798
Open
dislbenn wants to merge 7 commits intostolostron:mainfrom
Open
Add annotation-based resource adoption policy#3798dislbenn wants to merge 7 commits intostolostron:mainfrom
dislbenn wants to merge 7 commits intostolostron:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds policy-driven resource adoption and ownership checks to template apply/delete paths, expands annotation matching to include a resource-adoption-policy key, and adjusts hub status handling to mark pruning-complete as Progressing when appropriate. Changes
Sequence DiagramsequenceDiagram
participant R as Reconciler
participant M as MultiClusterHub
participant E as ensureResourceOwnership
participant Res as ExistingResource
R->>M: Read adoption annotation
M-->>R: Annotation (value or missing)
R->>R: getAdoptionPolicy(annotation)
R->>Res: Fetch existing resource
Res-->>R: Return resource (labels)
R->>E: ensureResourceOwnership(existing, desired, policy)
alt Policy = Strict
E-->>R: false (reject if installer labels missing/partial)
R->>R: Skip update/delete
else Policy = Adopt
E->>R: true (inject installer labels into desired)
R->>R: Proceed with update/delete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dislbenn 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 |
Introduce a configurable resource adoption policy controlled via the installer.open-cluster-management.io/resource-adoption-policy annotation on the MultiClusterHub resource. This allows operators to control how MCH handles existing resources that lack installer labels: - "Strict" (default): Only manage resources with installer.name and installer.namespace labels - "Adopt": Automatically adopt unlabeled resources by adding installer labels during reconciliation Key features: - Validates adoption policy and logs warnings for invalid values - Prevents adoption of resources with partial/corrupted installer labels - Adds labels to template (desired state) rather than existing resource - Comprehensive unit test coverage for all scenarios This enables seamless migration from manually created resources and provides flexibility for different deployment scenarios while maintaining safe defaults. Signed-off-by: dislbenn <dbennett@redhat.com>
dislbenn
force-pushed
the
feature/resource-adoption-policy
branch
from
90ffa31 to
4fe7f7a
Compare
Contributor
Author
|
/test sonar-pre-submit |
Reintroduce condition-clearing logic that was accidentally removed in PR stolostron#3688 when ensureRemovalsGone() and ensureAppsubsGone() were deleted. When all components are successful but a pruning condition exists (OldComponentRemovedReason or OldComponentNotRemovedReason), the hub is stuck in Pending status even though all resources were successfully deleted. This fix detects this state and sets AllOldComponentsRemovedReason, allowing the next reconcile to clear the condition and transition to Running. The AllOldComponentsRemovedReason constant (line 52) has been orphaned since March 2026 but is now reactivated. Fixes issue where MCH remains Pending with 'Not all components successfully pruned' message after disabling components like cluster-backup. Signed-off-by: dislbenn <dbennett@redhat.com>
Fix AnnotationsMatch() to trigger reconciles when resource-adoption-policy annotation is added or changed. Previously, only a whitelist of specific annotations would trigger reconciles, and resource-adoption-policy was not included. This change uses a combined approach: 1. Explicitly tracks important annotations including resource-adoption-policy 2. Detects when ANY annotation is added or removed (enables manual triggering) 3. Ignores value changes to non-important annotations (reduces noise) Now when users add/change the resource-adoption-policy annotation on MCH, the controller will immediately start a new reconcile loop. Also allows forcing reconciles by adding any temporary annotation. Signed-off-by: dislbenn <dbennett@redhat.com>
![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
🧹 Nitpick comments (2)
controllers/templates.go (2)
246-247: Consider defining constants for installer label keys.The label keys
"installer.name"and"installer.namespace"are hardcoded here and also inpkg/utils/utils.go:AddInstallerLabel. If these label names ever need to change, multiple locations must be updated.♻️ Suggested refactor to use constants
Add constants in
pkg/utils/labels.goor similar:const ( InstallerNameLabel = "installer.name" InstallerNamespaceLabel = "installer.namespace" )Then update both locations to use these constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/templates.go` around lines 246 - 247, Define shared constants for the installer label keys (e.g., InstallerNameLabel = "installer.name" and InstallerNamespaceLabel = "installer.namespace") in a central utils/labels.go and replace the hardcoded strings in controllers.templates.go (where existingLabels is checked) and in pkg/utils/utils.go:AddInstallerLabel to reference those constants; update any other occurrences to use these constants so changes to the label names are made in one place.
87-94: Ownership check failures are logged but may appear as silent success to callers.When
ensureResourceOwnershipreturnsfalse, bothapplyTemplateanddeleteTemplatereturnctrl.Result{}, nilwithout error. Per the calling code incontrollers/components.go, the loop continues without any indication that the resource was skipped.While the log messages provide observability, consider whether callers should be notified that resources were skipped (e.g., via a custom result type or metric) to help operators understand why certain resources aren't being managed.
Also applies to: 214-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/templates.go` around lines 87 - 94, When ensureResourceOwnership(existing, template, m) returns false, currently applyTemplate and deleteTemplate return ctrl.Result{}, nil which hides skips from callers; change both functions to return a distinguishable error (e.g., a sentinel ErrResourceNotOwned) instead of nil so callers (such as the reconciliation loop in controllers/components.go) can detect skips and act (metrics, logging, or special handling). Update the code paths at the two sites where ensureResourceOwnership is checked (the block around ensureResourceOwnership(...) returning false at lines shown and the similar block at 214-222) to return ErrResourceNotOwned, declare that sentinel error near the top of the file, and adjust callers to treat ErrResourceNotOwned as a non-fatal, observable condition.
🤖 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/utils/annotations.go`:
- Around line 177-186: The current key-set comparison in AnnotationsMatch causes
false negatives when system-managed annotations change; update AnnotationsMatch
to first filter out known system annotation keys/prefixes (e.g. keys starting
with "kubectl.kubernetes.io/", "kubernetes.io/", and known exact keys like
"controller-revision-hash" or "deployment.kubernetes.io/revision") from both old
and new annotation maps, then compare lengths and key sets of the filtered maps;
keep the existing behavior of allowing manual triggers by only excluding
system-managed annotations (not user annotations). Reference: function
AnnotationsMatch and its use in GenerationChangedPredicate.Update to locate and
modify the comparison logic.
---
Nitpick comments:
In `@controllers/templates.go`:
- Around line 246-247: Define shared constants for the installer label keys
(e.g., InstallerNameLabel = "installer.name" and InstallerNamespaceLabel =
"installer.namespace") in a central utils/labels.go and replace the hardcoded
strings in controllers.templates.go (where existingLabels is checked) and in
pkg/utils/utils.go:AddInstallerLabel to reference those constants; update any
other occurrences to use these constants so changes to the label names are made
in one place.
- Around line 87-94: When ensureResourceOwnership(existing, template, m) returns
false, currently applyTemplate and deleteTemplate return ctrl.Result{}, nil
which hides skips from callers; change both functions to return a
distinguishable error (e.g., a sentinel ErrResourceNotOwned) instead of nil so
callers (such as the reconciliation loop in controllers/components.go) can
detect skips and act (metrics, logging, or special handling). Update the code
paths at the two sites where ensureResourceOwnership is checked (the block
around ensureResourceOwnership(...) returning false at lines shown and the
similar block at 214-222) to return ErrResourceNotOwned, declare that sentinel
error near the top of the file, and adjust callers to treat ErrResourceNotOwned
as a non-fatal, observable condition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0f2c6d9-e6da-4d55-a33c-c5f9edfae024
📒 Files selected for processing (4)
controllers/status.gocontrollers/templates.gocontrollers/templates_test.gopkg/utils/annotations.go
pkg/utils/annotations.go
Outdated
Comment on lines
+177
to
+186
| // Also check if any annotation was added or removed (allows manual triggering) | ||
| // Don't check values, just keys - value changes to non-important annotations are ignored | ||
| if len(old) != len(new) { | ||
| return false | ||
| } | ||
| for k := range old { | ||
| if _, exists := new[k]; !exists { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Key-set comparison may trigger unnecessary reconciles from system annotations.
The key-set check causes AnnotationsMatch to return false when any annotation key is added or removed, including system-managed annotations like kubectl.kubernetes.io/last-applied-configuration. This could trigger reconciles even when no meaningful annotation has changed.
Per the context snippet in pkg/predicate/predicate.go, this function is used in GenerationChangedPredicate.Update() to decide whether to trigger reconciliation. While the comment indicates this enables "manual triggering," it may also increase reconcile frequency from unrelated annotation changes.
Consider whether this trade-off is acceptable, or if you want to filter out known system annotation prefixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/annotations.go` around lines 177 - 186, The current key-set
comparison in AnnotationsMatch causes false negatives when system-managed
annotations change; update AnnotationsMatch to first filter out known system
annotation keys/prefixes (e.g. keys starting with "kubectl.kubernetes.io/",
"kubernetes.io/", and known exact keys like "controller-revision-hash" or
"deployment.kubernetes.io/revision") from both old and new annotation maps, then
compare lengths and key sets of the filtered maps; keep the existing behavior of
allowing manual triggers by only excluding system-managed annotations (not user
annotations). Reference: function AnnotationsMatch and its use in
GenerationChangedPredicate.Update to locate and modify the comparison logic.
The key-set comparison was incorrectly treating deprecated annotation keys as different from their current equivalents, causing test failures when old maps used deprecated keys (e.g. "mch-pause") and new maps used current keys (e.g. "installer.open-cluster-management.io/pause"). Now we filter out already-checked annotation keys (including deprecated ones) before comparing the remaining key sets. This allows the semantic checks to handle deprecated fallbacks while still detecting additions/removals of unchecked annotations for manual triggering. Fixes test: Test_AnnotationMatch/Annotations_should_match Signed-off-by: dislbenn <dbennett@redhat.com>
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.
Description
Introduce a configurable resource adoption policy controlled via the
installer.open-cluster-management.io/resource-adoption-policyannotation on theMultiClusterHubresource.This allows operators to control how MCH handles existing resources that lack installer labels:
installer.nameandinstaller.namespacelabelsThis enables seamless migration from manually created resources and provides flexibility for different deployment scenarios while maintaining safe defaults.
Related Issue
If applicable, please reference the issue(s) that this pull request addresses.
Changes Made
Screenshots (if applicable)
Add screenshots or GIFs that demonstrate the changes visually, if relevant.
Checklist
Additional Notes
Add any additional notes, context, or information that might be helpful for reviewers.
Reviewers
Tag the appropriate reviewers who should review this pull request. To add reviewers, please add the following line:
/cc @reviewer1 @reviewer2/cc @cameronmwall @ngraham20
Definition of Done
Summary by CodeRabbit
New Features
Bug Fixes
Tests