feat: observe managed resource group and gate cluster deletion on it - #6648
feat: observe managed resource group and gate cluster deletion on it#6648Chai-bot (redhat-chai-bot) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an observe-only backend controller that checks whether a cluster’s managed resource group (MRG) exists in Azure and mirrors that state onto ServiceProviderCluster.Status.AzureResources.ManagedResourceGroup. It also adds a deletion gate so the ServiceProviderCluster Cosmos document is not deleted while the MRG is still reflected as present (pending or confirmed).
Changes:
- Introduces
ObserveManagedResourceGroup(cluster-watching) to reflect MRG existence intoServiceProviderCluster.Status.AzureResources.ManagedResourceGroup. - Gates
ServiceProviderClusterdocument deletion on the reflected MRG state inClusterChildResourcesCleanupController. - Wires the controller into backend startup and adds unit tests + data-flow documentation updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/coreapi/types_serviceprovider_cluster.go | Adds // Written by: annotation for the ManagedResourceGroup status field. |
| docs/cosmos-data-flow.md | Documents the new controller and the new deletion gate behavior. |
| backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_mrg_gate_test.go | Adds unit tests verifying the new deletion gate behavior. |
| backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go | Blocks ServiceProviderCluster deletion while MRG is reflected as pending/present. |
| backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go | Adds the observe-only controller that queries Azure RG existence and updates SPC status. |
| backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go | Adds unit tests for the syncer’s state transitions (non-deleting vs deleting). |
| backend/pkg/app/backend.go | Constructs and starts the new controller under leader election. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Deletion fast-path: nothing left to reflect once both references are cleared. | ||
| if isDeleting && existingReference.AzureResource == nil && existingReference.PendingAzureResource == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Good catch. Fixed in 5019103: removed the deletion fast-path so we always query Azure during deletion. If the MRG still exists we now set AzureResource (so the gate blocks) even when the reference was previously empty — e.g. a cluster deleted shortly after creation. Both references are cleared only once the RG is confirmed gone.
AI-generated. Review for accuracy.
| { | ||
| name: "deleting and resource group gone clears both", | ||
| deleting: true, | ||
| initialReference: coreapi.AzureReference{AzureResource: mrgID}, | ||
| getResponse: armresources.ResourceGroupsClientGetResponse{}, |
There was a problem hiding this comment.
Added in 5019103: a regression test "deleting and resource group present with empty reference sets actual" — deletion in progress, initial ManagedResourceGroup reference empty, RG still exists in Azure → asserts AzureResource is set and PendingAzureResource is nil (so the gate would block).
AI-generated. Review for accuracy.
| **Trigger:** Cluster informer, 5-minute resync | ||
| **Behavior:** Observe-only — never creates or deletes the managed resource group. | ||
| - Not deleting: records the managed resource group as `PendingAzureResource` while it is missing; sets `AzureResource` (and clears `PendingAzureResource`) once it exists. | ||
| - Deleting: clears both references once the managed resource group is gone; leaves reflected state untouched while it still exists. |
There was a problem hiding this comment.
Chai-bot (@redhat-chai-bot) if the MRG still exists but not managed by this aro-hcp cluster (compare resources IDs ignoring casing) then unblock deletion by setting the resourceIDs as nil
There was a problem hiding this comment.
Done in 43d6eb1. SyncOnce now captures the Get response and treats the MRG as "owned & present" only when resp.ManagedBy != nil && strings.EqualFold(*resp.ManagedBy, cluster.ID.String()) (case-insensitive). A resource group that exists but isn't managed by this cluster is treated the same as missing:
- Deleting → clear both references (unblock deletion) instead of holding the gate on a foreign RG.
- Not deleting → set
PendingAzureResource(we only promote toAzureResourceonce it exists and is owned by this cluster).
Added tests for both "exists but foreign ManagedBy" paths.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
Heads-up Manyanda Chitimbo (@machi1990): the ownership handling was restructured in 510b664 per David Eads (@deads2k)'s switch-based review, and it now differs from this original request — flagging so you two can align:
- Reconcile (not deleting): MRG exists but owned by another cluster → return an error (does not set
AzureResource). Previously this setPendingAzureResource/ treated it as not-ours. - Deletion: the deletion switch has no ownership case — a foreign-but-present MRG → do nothing, so the references stay set and the gate keeps blocking deletion. The earlier "foreign MRG → clear the resource IDs to unblock deletion" behavior you requested is not in the current revision.
Could you and David Eads (@deads2k) confirm the desired deletion behavior for a foreign MRG (clear refs to unblock vs. hold/error)? Happy to implement whichever you settle on.
AI-generated. Review for accuracy.
AI-generated. Review for accuracy.
Address PR Azure#6648 review: the ObserveManagedResourceGroup controller could let the ServiceProviderCluster be deleted while the managed resource group still existed in Azure. Two paths caused the race: - the deletion fast-path returned early (without querying Azure) whenever the cluster was deleting and both MRG references were already nil, and - the deletion branch returned without recording AzureResource when the resource group still existed. If a cluster was deleted shortly after creation (before the reference was ever populated), the deletion gate in the cluster child resources cleanup controller never saw a set reference and allowed removal. SyncOnce now always queries Azure and reflects observed existence: an existing MRG is recorded as AzureResource regardless of deletion state (so the gate blocks), and once the MRG is gone during deletion both references are cleared (opening the gate). The DeepEqual no-op guard, IsPreconditionFailedError handling, and empty-managed-resource-group-name guard are unchanged. Add a regression test covering deletion in progress with an empty initial reference while the resource group still exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| |---|--------|--------| | ||
| | Read | `HCPOpenShiftCluster` | <ul><li>`CustomerProperties.Platform.ManagedResourceGroup` (SyncOnce: skipped when empty)</li><li>`ServiceProviderProperties.DeletionTimestamp` (branches deletion vs non-deletion)</li><li>`ID` (subscription / resource group / name)</li></ul> | | ||
| | Read | `Subscription` | <ul><li>`Properties.TenantId` (to build the FPA ResourceGroups client)</li></ul> | | ||
| | Read | `ServiceProviderCluster` | <ul><li>`Status.AzureResources.ManagedResourceGroup` (compared before write; deletion fast-path when both references already nil)</li></ul> | |
There was a problem hiding this comment.
Fixed in 43d6eb1. The data-flow doc no longer mentions a "deletion fast-path when both references are nil" — it now states the controller always queries Azure during deletion.
AI-generated. Review for accuracy.
| - Not deleting: records the managed resource group as `PendingAzureResource` while it is missing; sets `AzureResource` (and clears `PendingAzureResource`) once it exists. | ||
| - Deleting: clears both references once the managed resource group is gone; leaves reflected state untouched while it still exists. |
There was a problem hiding this comment.
Updated in 43d6eb1. The doc now describes the actual SyncOnce behavior: during deletion the controller reflects observed existence (sets AzureResource when the RG exists and is owned by this cluster, even from an empty/pending state) and clears both references when the RG is missing or not owned by this cluster.
AI-generated. Review for accuracy.
Address PR Azure#6648 maintainer review: the ObserveManagedResourceGroup controller treated any existing resource group with the cluster's managed resource group name as the cluster's own, without verifying ownership. It could therefore claim (or block deletion on) a resource group that happens to share the name but is managed by something else. SyncOnce now captures the Get response and considers the managed resource group "owned and present" only when it exists AND its ManagedBy equals the cluster's resource ID (case-insensitive via strings.EqualFold). A resource group that exists but is not owned by this cluster is treated the same as missing: - Not deleting: owned & present -> AzureResource set, Pending cleared; otherwise (missing or not owned) -> PendingAzureResource set, AzureResource cleared. - Deleting: owned & present -> AzureResource set so the deletion gate blocks; otherwise (missing or not owned) -> both references cleared, opening the gate. The empty-managed-resource-group-name guard, DeepCopy + equality.Semantic no-op guard, IsPreconditionFailedError handling, and the non-not-found Get error return are unchanged. Add tests for "present but owned by a different cluster" in both the non-deletion (expects pending) and deletion (expects both cleared) paths, set ManagedBy on the present-response helper, and update docs/cosmos-data-flow.md to describe the ownership rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go:152
- This controller unconditionally calls Azure ResourceGroupsClient.Get on every resync (5m) for every cluster, even when the managed resource group is already confirmed and the cluster is not deleting. At scale this can create unnecessary Azure ARM traffic and increase the chance of throttling; the AzureReference type includes EarliestRecheckTime specifically to support backing off repeated Azure checks (internal/api/coreapi/types_serviceprovider_cluster.go:436-443). Consider honoring and setting EarliestRecheckTime (with substantial jitter) in the non-deleting, stable "AzureResource set" state, while still always querying during deletion.
// Always query Azure to observe the managed resource group, including during
// deletion. We must detect a still-existing resource group so the
// ServiceProviderCluster deletion gate keeps blocking until the MRG is
// actually gone, even when the reference was never populated before deletion
// started (for example when a cluster is deleted shortly after creation).
getResponse, getErr := rgClient.Get(ctx, managedResourceGroupID.Name, nil)
resourceGroupMissing := azureclient.IsResourceGroupNotFoundErr(getErr)
if getErr != nil && !resourceGroupMissing {
return utils.TrackError(fmt.Errorf("failed to get managed resource group %q: %w", managedResourceGroupName, getErr))
backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go:276
- This log statement dereferences spc.ResourceID; if the stored document is ever missing ResourceID (or is partially deserialized), this could panic. Since the method already has serviceProviderClusterResourceID, logging that value avoids the extra dependency and is guaranteed non-nil here.
logger.Info("waiting for the managed resource group to be deleted before removing the ServiceProviderCluster document",
"serviceProviderClusterResourceID", spc.ResourceID.String())
return false, nil
…lected and the cluster is not deleting Address PR Azure#6648 maintainer optimization: in SyncOnce, when the cluster is not being deleted and the ServiceProviderCluster already reflects the managed resource group as AzureResource (matching the derived MRG resource ID, case-insensitive), short-circuit before building the FPA ResourceGroups client or calling Get. There is nothing new to observe in that steady state. The deletion path is intentionally not short-circuited: it must always re-query Azure to detect when the resource group is finally gone. When the cluster is not deleting and the resource group is not yet reflected, the controller still queries Azure and applies the ManagedBy ownership check. The DeepCopy + equality.Semantic.DeepEqual no-op guard, IsPreconditionFailedError handling, and non-not-found Get error return are unchanged. Add a test asserting that, when AzureResource is already reflected and the cluster is not deleting, neither the FPA client builder nor ResourceGroupsClient Get is called (EXPECT().Times(0)) and the reference is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| } | ||
|
|
||
| managedResourceGroupName := cluster.CustomerProperties.Platform.ManagedResourceGroup | ||
| if managedResourceGroupName == "" { |
There was a problem hiding this comment.
The MRG should always be there; we are better off returning an error here
There was a problem hiding this comment.
Done in 6d6747a. SyncOnce now returns an error when the managed resource group name is empty instead of silently returning:
return utils.TrackError(fmt.Errorf("managed resource group name is empty for cluster %q", cluster.ID.String()))so the syncer retries rather than treating a missing name as a no-op.
AI-generated. Review for accuracy.
… resource group is immutable Address PR Azure#6648 maintainer feedback: the managed resource group is immutable, so the non-deletion short-circuit does not need to compare the reflected AzureResource against the derived resource ID. A non-nil AzureResource is sufficient to know there is nothing new to observe. Simplify the early return in SyncOnce to: if !isDeleting && existingReference.AzureResource != nil { return nil } removing only the strings.EqualFold(existingReference.AzureResource.String(), managedResourceGroupID.String()) comparison. The `strings` import is retained (still used by the ManagedBy ownership check). All other behavior is unchanged: the deletion path still always re-queries Azure, the ManagedBy ownership check still gates pending/actual, and the DeepEqual no-op guard and IsPreconditionFailedError handling are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // missing, so we neither claim it nor block this cluster's deletion on it. | ||
| ownedAndPresent := getErr == nil && | ||
| getResponse.ManagedBy != nil && | ||
| strings.EqualFold(*getResponse.ManagedBy, cluster.ID.String()) |
There was a problem hiding this comment.
There is utils function that checks for resource IDs equality use that...
There was a problem hiding this comment.
Done in 6d6747a. Switched the ownership check to controllerutil.ResourceIDsEqual (internal/controllerutils). Since ManagedBy is a *string, I parse it first and treat a nil/unparseable value as not-owned:
var managedByID *azcorearm.ResourceID
var parseErr error
if getResponse.ManagedBy != nil {
managedByID, parseErr = azcorearm.ParseResourceID(*getResponse.ManagedBy)
}
ownedAndPresent := getErr == nil && getResponse.ManagedBy != nil && parseErr == nil &&
controllerutil.ResourceIDsEqual(managedByID, cluster.ID)Removed the now-unused strings import. One heads-up: ResourceIDsEqual compares canonically via ParseResourceID (which is case-sensitive on the subscription/RG/name segments), whereas my earlier version used a case-insensitive EqualFold. In practice ARM sets ManagedBy to the cluster's own resource ID so this should match, but let me know if you'd prefer an explicit case-insensitive compare.
AI-generated. Review for accuracy.
|
|
||
| **File:** [managed_resource_group_controller.go](../backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go) | ||
| **Trigger:** Cluster informer, 5-minute resync | ||
| **Behavior:** Observe-only — never creates or deletes the managed resource group. It always queries Azure (there is no early-return fast-path during deletion). The managed resource group is treated as "owned and present" only when it exists AND its `ManagedBy` equals this cluster's resource ID (case-insensitive); a resource group that exists but is owned by something else is treated the same as missing. |
There was a problem hiding this comment.
Updated in 6d6747a. The data-flow doc now documents the non-deletion short-circuit: when the cluster is not deleting and ManagedResourceGroup.AzureResource is already set, the controller returns early without querying Azure (the MRG is immutable).
AI-generated. Review for accuracy.
… use ResourceIDsEqual for ManagedBy ownership; document non-deletion short-circuit Round-4 PR Azure#6648 maintainer review: - A cluster should always have a managed resource group name, so SyncOnce now returns a wrapped error (utils.TrackError) instead of silently returning nil when CustomerProperties.Platform.ManagedResourceGroup is empty, letting the syncer retry. - The ManagedBy ownership check now uses the repo helper controllerutil.ResourceIDsEqual (internal/controllerutils) instead of strings.EqualFold. ManagedBy is a *string, so it is first parsed with azcorearm.ParseResourceID; a nil ManagedBy or a parse error is treated as not owned. The now-unused strings import is removed. - docs/cosmos-data-flow.md documents the non-deletion short-circuit (when the cluster is not deleting and AzureResource is already set, the controller returns early without querying Azure because the managed resource group is immutable) and the ManagedBy/ResourceIDsEqual ownership rule. Tests set ManagedBy to the cluster ID for owned/present cases and to a valid but different resource ID for foreign cases, add an unparseable-ManagedBy case (treated as not owned: pending when not deleting, both cleared when deleting), and assert the empty-name path now returns an error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ring deletion Round-5 PR Azure#6648 review: during deletion, if rgClient.Get returned a non-404 error while the ServiceProviderCluster's ManagedResourceGroup reference was empty (AzureResource == nil && PendingAzureResource == nil), SyncOnce returned the error without writing anything. The cluster child-resources cleanup gate then saw a nil/nil reference and could delete the ServiceProviderCluster even though the managed resource group might still exist (transient Azure-unavailability race). SyncOnce now fails closed: on a non-404 Get error during deletion with an empty reference, it first records PendingAzureResource = managedResourceGroupID (via the existing Replace path, treating a precondition failure as already-handled) and then returns the wrapped error so the syncer retries. This keeps the deletion gate closed until Azure can be queried successfully. Non-deletion behavior on non-404 errors is unchanged (still just returns the error), and a deletion error when a reference already holds the gate closed makes no extra write. Add tests: (a) deleting + empty reference + transient Get error -> pending set and error returned; (b) deleting + transient Get error + existing AzureResource -> no change and error returned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/hold Revision d5a9f48 was retested 3 times: holding |
ARM resource IDs are case-insensitive for their provider namespaces and resource types, but ResourceIDsEqual compared canonical string forms with case-sensitive equality. Azure returns the managed resource group's ManagedBy as ".../Microsoft.RedHatOpenshift/..." while our internal types use ".../Microsoft.RedHatOpenShift/...", so the observe ManagedResourceGroup controller's ownedByAnotherCluster check treated the cluster's own MRG as owned by another cluster and hot-looped. Compare with strings.EqualFold so IDs that differ only by casing compare equal. The fix lives in the shared helper so every caller benefits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed Root cause: Fix (in the shared helper): Tests: added a
AI-generated. Review for accuracy. |
|
/lgtm |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go:103
- During deletion, NeedsWork returns false when both ManagedResourceGroup references are nil. That means SyncOnce won’t run on a deleting cluster that never had Pending/AzureResource populated (e.g., cluster deleted soon after creation), and the delete gate can then fail-open and remove the ServiceProviderCluster document even while the MRG still exists in Azure. NeedsWork/deleteManagedResourceGroup should ensure a deleting cluster with an empty reference still performs at least one Azure check and persists a blocking marker (pending/actual) on errors or existence until the RG is confirmed gone.
func (c *managedResourceGroupSyncer) NeedsWork(cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster) bool {
managedResourceGroup := serviceProviderCluster.Status.AzureResources.ManagedResourceGroup
if cluster.ServiceProviderProperties.DeletionTimestamp != nil {
return managedResourceGroup.PendingAzureResource != nil || managedResourceGroup.AzureResource != nil
}
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go:393
- This test encodes the current (problematic) behavior that a deleting cluster with an empty ManagedResourceGroup reference has “no work”. That expectation allows the observe controller to skip deletion-time observation entirely when the reference was never populated, which is exactly the race that can make the delete gate fail-open. The test should be updated to expect work in this case, and ideally add an assertion that deletion with an empty reference persists a blocking marker when Azure state can’t yet be confirmed.
{
name: "deleting and empty reference has no work",
deleting: true,
reference: coreapi.AzureReference{},
expect: false,
| // The comparison is case-insensitive: ARM resource IDs are case-insensitive for | ||
| // their provider namespaces and resource types (for example Azure may return | ||
| // "Microsoft.RedHatOpenshift" where our internal types use | ||
| // "Microsoft.RedHatOpenShift"), so two IDs that differ only by casing represent | ||
| // the same resource and must compare equal. |
| return utils.TrackError(err) | ||
| case getErr != nil: | ||
| return utils.TrackError(getErr) | ||
| default: |
There was a problem hiding this comment.
if exist and not managed by this cluster (similar to https://github.com/redhat-chai-bot/Azure_ARO-HCP/blob/6dae18cb7bddae4b2f55c2cf0b82e98cba475db5/backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go#L197), we shouldn't block cluster deletion
Chai-bot (@redhat-chai-bot)
There was a problem hiding this comment.
Done in 4277637 (fresh commit on top of 6dae18c, no squash/force).
The deletion path now has the ownership case, using the same ownedByAnotherCluster helper as reconcile (parses ManagedBy, compares via controllerutil.ResourceIDsEqual, which is now case-insensitive). deleteManagedResourceGroup captures the Get response and switches:
switch {
case isNotFound(err):
// RG gone -> clear both refs -> unblock
case getErr != nil:
return utils.TrackError(err) // gate stays closed
case ownedByAnotherCluster(getResponse.ManagedBy, cluster.ID):
// exists but not ours -> clear both refs -> unblock (NEW)
default: // exists and owned by this cluster
// do nothing, gate holds (TODO: begin deletion)
}So a foreign / pre-existing MRG that Cluster Service won't delete no longer keeps the gate closed forever — the references are cleared and cluster deletion proceeds, exactly as you asked. Owned-by-us + present still holds the gate until CS deletes the RG (then we see the 404 and clear).
Ref-clearing goes through the existing persistIfChanged path wrapped in utils.TrackError, extracted into a small clearManagedResourceGroupReferences helper so the not-found and ownership cases don't duplicate the clear-and-persist block. Reconcile is untouched; the observe-only contract is preserved (never Create/BeginDelete). Added a deletion regression test (deleting + MRG exists + foreign ManagedBy ⇒ both refs cleared, gate opens) and kept the owned-by-this-cluster case (refs unchanged, gate holds). Local build/vet/test/lint all green.
AI-generated. Review for accuracy.
AI-generated. Review for accuracy.
|
/hold Until #6648 (comment) is addressed |
… another cluster During cluster deletion the observe controller waited for the managed resource group to disappear before clearing the ServiceProviderCluster references that gate cluster deletion. If the resource group exists but is owned by another cluster (a foreign / pre-existing resource group that Cluster Service will not delete on our behalf) that wait never ends, so the references are never cleared and cluster deletion is stuck forever. Mirror the reconcile path's ownedByAnotherCluster check in the deletion switch: a present-but-foreign resource group is not ours to wait on, so clear both references and open the deletion gate. A resource group owned by this cluster still holds the gate closed until it is actually gone. The controller remains observe-only and never creates or deletes the resource group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go:105
NeedsWorkreturns false during deletion when bothPendingAzureResourceandAzureResourceare nil. In that state,SyncOncenever enters the deletion path, so the controller can’t populate a blocking marker and theServiceProviderClusterdelete gate can fail-open even if the managed resource group still exists in Azure (e.g., deletion starts before any prior reflection).
func (c *managedResourceGroupSyncer) NeedsWork(cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster) bool {
managedResourceGroup := serviceProviderCluster.Status.AzureResources.ManagedResourceGroup
if cluster.ServiceProviderProperties.DeletionTimestamp != nil {
return managedResourceGroup.PendingAzureResource != nil || managedResourceGroup.AzureResource != nil
}
return managedResourceGroup.AzureResource == nil
}
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go:231
deleteManagedResourceGroupassumes a managed resource group reference is already set (viaNeedsWork). If deletion starts before any reflection, this function can’t fail-closed by persisting a pending marker, and ifNeedsWorkis updated to run during deletion it will also hit a nil-reference path. Consider deriving the MRG ID from the cluster when references are empty and persistingPendingAzureResourcebefore any fallible Azure/client construction so the delete gate stays closed.
// A reference is guaranteed set here (see NeedsWork). Prefer the confirmed
// AzureResource, falling back to the PendingAzureResource marker.
currentReference := existingServiceProviderCluster.Status.AzureResources.ManagedResourceGroup
managedResourceGroupID := currentReference.AzureResource
if managedResourceGroupID == nil {
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller_test.go:273
- Unit tests don’t cover the case where deletion starts with an empty
ManagedResourceGroupreference but the managed resource group still exists in Azure. That scenario is the critical race the delete gate is meant to protect against; adding a regression test will prevent fail-open behavior from being reintroduced.
{
name: "deleting and resource group gone clears both",
deleting: true,
initialReference: coreapi.AzureReference{AzureResource: mrgID},
getResponse: armresources.ResourceGroupsClientGetResponse{},
getErr: resourceGroupNotFoundError(),
expectAzure: nil,
expectPending: nil,
},
docs/cosmos-data-flow.md:1171
- This table row says
ManagedByis inspected only in the non-deletion path, but the controller also inspects it during deletion to decide whether to clear references for a foreign resource group.
| Read | Azure (ResourceGroupsClient) | <ul><li>`Get` on the managed resource group -> exists / ResourceGroupNotFound; `ManagedBy` (ownership check) is inspected only in the non-deletion path when the resource group exists</li></ul> |
| case getErr != nil: | ||
| return utils.TrackError(getErr) |
| - **not found** → does nothing, leaving the pending marker in place (Cluster Service owns creation; this controller is observe-only). | ||
| - **other error** → returns the error so the sync retries. | ||
| - **exists** → if the resource group is owned by another cluster (its `ManagedBy` is set and does not equal this cluster's ID via the `ResourceIDsEqual` helper) it returns an error and does **not** set `AzureResource`; otherwise (owned by this cluster, or `ManagedBy` absent) it clears `PendingAzureResource` and records the resource group as `AzureResource`. | ||
| - Deleting: derives the resource group ID from the reference still on the document (guaranteed set by `NeedsWork`), queries Azure and switches on the result: once the resource group is gone it clears both references so the deletion gate opens; on any other error it returns the error so the gate stays closed until the state is known; while the resource group still exists it does nothing (it does not set `AzureResource`, write a pending marker, or perform the ownership check). |
Manyanda Chitimbo (machi1990)
left a comment
Manyanda Chitimbo (machi1990)
left a comment
There was a problem hiding this comment.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, machi1990, redhat-chai-bot 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 |
|
/hold cancel |
|
Chai-bot (@redhat-chai-bot): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest e2e-parallel failed on
Retesting. AI-generated. Review for accuracy. AI-generated. Review for accuracy. |
Summary
Adds an observe-only backend controller,
ObserveManagedResourceGroup, that reflects the existence of a cluster's managed resource group (MRG) in Azure ontoServiceProviderCluster.Status.AzureResources.ManagedResourceGroup, and gatesServiceProviderClusterdeletion until that MRG is gone.This begins populating the
AzureResourcessection ofServiceProviderClusterStatuswith resources that Cluster Service creates, so we can inspect them during deletion and watch them disappear as cleanup proceeds.Cluster Service owns the MRG lifecycle — this controller never creates or deletes a resource group (
CreateOrUpdate/BeginDeleteare never called). It only mirrors observed state. The MRG resource ID is derived the same way as in the reference PR #6282 (coreapi.ToResourceGroupResourceID(subscriptionID, CustomerProperties.Platform.ManagedResourceGroup)), and existence is checked with the FPA credential viaFirstPartyApplicationClientBuilder.ResourceGroupsClient(...).Get.Behavior
SyncOncegates onNeedsWorkand then splits into reconcile vs. delete based onServiceProviderProperties.DeletionTimestamp.NeedsWork(cluster, spc)Reconcile (not deleting)
PendingAzureResourceisn't set, set it and persist it before theGet, so the delete gate is populated even for a cluster that deletes immediately.switchon theGet:return TrackError(err)!controllerutil.ResourceIDsEqual) → return an error (references untouched); otherwise clearPendingAzureResource, setAzureResource, persist.Deletion (only reached when a reference is set)
switchon theGet:return TrackError(err)Writes only happen on an actual change (
persistIfChanged), andPreconditionFailedonReplaceis treated as a no-op retry.Deletion gate
In
clusterChildResourcesCleanupController.extraDeleteGateShouldDeleteServiceProviderCluster, after theServiceProviderClusteris fetched: ifStatus.AzureResources.ManagedResourceGroup.AzureResource != nilorPendingAzureResource != nil, the gate logs and returns(false, nil)— theServiceProviderClusterdocument is not deleted while the MRG is still reflected as present.Changes
backend/pkg/controllers/cluster/azureresources/managed_resource_group_controller.go(cluster-watchingClusterSyncer, 5m resync).backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go.backend/pkg/app/backend.go(constructed alongside the other FPA cluster controllers; started under leader election).// Written by:annotation on theManagedResourceGroupfield anddocs/cosmos-data-flow.md.NeedsWork, the reconcile switch (not-found, exists/owned, exists/owned-by-other → error, empty name), and the deletion switch (clears both when gone).Testing
go build ./...— passgo vet ./...— passgo test ./backend/pkg/controllers/cluster/azureresources/...— passgolangci-lint(v2.5.0, repo-pinned) — 0 issuesNotes / open questions for reviewers
EarliestRecheckTimecooldown field onAzureReference. Happy to add cooldown/jitter if preferred.AI-generated. Review for accuracy.