Gate SPC deletion + delete-operation on removal of all remaining ApplyDesires by redhat-chai-bot · Pull Request #6661 · Azure/ARO-HCP

Chai-bot (redhat-chai-bot) · 2026-08-24T17:47:23Z

Summary

Blocks removal of a cluster's ServiceProviderCluster (SPC) document and holds the cluster delete operation non-terminal while any kube-applier ApplyDesire still exists for the cluster. This ensures controllers finish deleting their ApplyDesires before the SPC/cluster is torn down.

Per review, the gate is intentionally controller-agnostic — any remaining ApplyDesire blocks deletion, regardless of which controller authored it. The per-controller breakdown is surfaced purely for diagnostics.

Changes

backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go — extraDeleteGateShouldDeleteServiceProviderCluster now blocks SPC deletion while any ApplyDesire remains, logging a per-controller breakdown.
backend/pkg/controllers/cluster/operations/operation_cluster_delete.go — the remaining-ApplyDesire computation is folded into buildDeletionTimeoutMessage alongside the other deletion-timeout calculations; SynchronizeOperation holds the operation non-terminal (after the deadline check) while any remain, and the operation message reports how many ApplyDesires are left per controller.
internal/database/cosmosstorage/kubeappliercosmosstorage/apply_desire_summary.go — new shared SummarizeApplyDesiresByController helper (enumerate ApplyDesires, group by Tags[TagControllerName] with untagged bucketed as unknown, sorted by controller name). Reused by both gates so the association logic lives in one place.
docs/cosmos-data-flow.md — updated the OperationClusterDelete and ClusterChildResourcesCleanupController sections to document the new reads.
Unit tests for both gates and the shared helper.

Behavior notes

Treated as "gone" (deletion proceeds) when there is no ServiceProviderCluster, a nil ManagementClusterResourceID, or no kube-applier client for the management cluster.
No struct/constructor changes.

Validation

make -C backend build → exit 0
go test for cluster/deletion, cluster/operations, and kubeappliercosmosstorage → all pass
lint/fmt/licenses → clean

…s removal Add a delete-time gate that blocks tearing a cluster down while the ClusterResourcesController still owns ApplyDesires, implemented standalone on main (the clusterresources controller from PR Azure#6070 is not yet present). - internal/api/kubeapplierapi: define ClusterResourcesControllerName, the TagControllerName value the ClusterResourcesController stamps on its ApplyDesires. It mirrors the constant PR Azure#6070 introduces in its clusterresources package and is placed here so the delete gates can reference the value on main; Azure#6070 should consume this constant rather than redefine it. - clusterChildResourcesCleanupController: block ServiceProviderCluster deletion (the document carrying ManagementClusterResourceID) until no ApplyDesires tagged with kubeapplierapi.ClusterResourcesControllerName remain. - operationClusterDelete: hold the delete operation non-terminal until those tagged ApplyDesires are gone (placed after the deadline check so the timeout-failure path still fires). Both gates use the existing kubeApplierDBClients DB-client mechanism with no struct/constructor changes, treat a nil client / nil ManagementClusterResourceID / missing ServiceProviderCluster as "gone", and add table-driven unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Adds deletion-time gating to ensure a cluster delete doesn’t complete (SPC removal + delete operation terminalization) until kube-applier ApplyDesire documents authored by ClusterResourcesController have been removed.

Changes:

Introduces a shared tag value (ClusterResourcesControllerName) under kubeapplierapi for identifying controller-owned desires.
Adds a new precondition to block ServiceProviderCluster deletion while controller-tagged ApplyDesires remain.
Adds a new precondition to hold the cluster delete operation non-terminal while controller-tagged ApplyDesires remain, plus unit tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/api/kubeapplierapi/tags.go	Adds a new controller tag constant used for deletion gating.
backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go	Blocks SPC deletion until controller-tagged ApplyDesires are gone.
backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller_test.go	Adds unit tests for the SPC deletion gate + helper.
backend/pkg/controllers/cluster/operations/operation_cluster_delete.go	Holds delete operation non-terminal until controller-tagged ApplyDesires are gone.
backend/pkg/controllers/cluster/operations/operation_cluster_delete_test.go	Adds unit tests for the operation-level gate behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Chai-bot (redhat-chai-bot) · 2026-08-24T18:04:04Z

+	// cluster-deletion gates can reference the value on main before that package
+	// exists. When #6070 lands it should consume this constant rather than
+	// redefine its own, so the tag value stays single-sourced.
+	ClusterResourcesControllerName = "ClusterResourcesController"


Good catch on the convention. I'm intentionally keeping the value as "ClusterResourcesController" here because it must byte-for-byte match the tag that #6070's ClusterResourcesController stamps on its ApplyDesire documents — if this value diverges, the delete gates would match nothing. It's defined in kubeapplierapi specifically so #6070 can consume it rather than redefine its own copy, keeping the value single-sourced.

If we'd prefer to drop the Controller suffix for consistency with the other TagControllerName values, that rename should be done in lockstep with #6070 so the writer and the gates change together. Happy to coordinate that on #6070 if that's preferred.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-24T18:10:35Z

+	remaining := 0
+	for _, desire := range applyDesireIterator.Items(ctx) {
+		if desire.Tags == nil {
+			continue
+		}
+		if desire.Tags[kubeapplierapi.TagControllerName] == kubeapplierapi.ClusterResourcesControllerName {
+			remaining++
+		}
+	}
+	if err := applyDesireIterator.GetError(); err != nil {
+		return false, utils.TrackError(fmt.Errorf("error iterating ApplyDesires for precondition check: %w", err))
+	}
+
+	return remaining == 0, nil


Addressed in dd56f9b — the helper now returns on the first matching tagged desire instead of counting all of them, so it no longer scans every page. iterator.GetError() is checked both on the early exit and after full exhaustion; "gone" is only returned once the iterator is exhausted with no match.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-24T18:10:35Z

+	remaining := 0
+	for _, desire := range applyDesireIterator.Items(ctx) {
+		if desire.Tags == nil {
+			continue
+		}
+		if desire.Tags[kubeapplierapi.TagControllerName] == kubeapplierapi.ClusterResourcesControllerName {
+			remaining++
+		}
+	}
+	if err := applyDesireIterator.GetError(); err != nil {
+		return false, utils.TrackError(fmt.Errorf("error iterating ApplyDesires for precondition check: %w", err))
+	}
+
+	return remaining == 0, nil


Addressed in dd56f9b — same short-circuit here: the gate now returns "not gone" on the first ClusterResourcesController-tagged desire rather than paging through the whole result set.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-24T18:10:35Z

+		{
+			name:               "tagged ClusterResourcesController ApplyDesire present -> operation held non-terminal",
+			spc:                newSPC(managementClusterResourceID),
+			kubeApplierDesires: []any{taggedDesire("cluster-resource-desire")},
+			// No CS mock: the gate returns before reconcile, so ClusterService must not be called.
+			wantStatus: coreapi.ProvisioningStateAccepted,
+		},
+		{
+			name:               "no tagged ApplyDesires -> operation proceeds to reconcile",
+			spc:                newSPC(managementClusterResourceID),
+			kubeApplierDesires: nil,
+			setupCSMock: func(ctrl *gomock.Controller, fixture *operationtesting.ClusterTestFixture) ocm.ClusterServiceClientSpec {
+				mockCSClient := ocm.NewMockClusterServiceClientSpec(ctrl)
+				clusterStatus, _ := arohcpv1alpha1.NewClusterStatus().
+					State(arohcpv1alpha1.ClusterStateUninstalling).
+					Build()
+				mockCSClient.EXPECT().
+					GetClusterStatus(gomock.Any(), fixture.ClusterInternalID).
+					Return(clusterStatus, nil)
+				return mockCSClient
+			},
+			wantStatus: coreapi.ProvisioningStateDeleting,
+		},
+	}


Addressed in dd56f9b — added table cases covering the early-return branches: missing/NotFound ServiceProviderCluster, SPC present with a nil ManagementClusterResourceID, and an unregistered management cluster (nil kube-applier client). Each asserts the operation is treated as "gone" and proceeds.

AI-generated. Review for accuracy.

…branches Copilot review feedback on the ClusterResourcesController ApplyDesires delete gate: - clusterResourceApplyDesiresGone (both the child-resources cleanup controller and the delete operation): stop counting all tagged ApplyDesires and instead return "not gone" on the first match, so a single remaining desire short-circuits the listing. The iterator error is still surfaced both on the early exit and after full exhaustion, and "gone" is returned only once the iterator is exhausted with no match. The nil-client / nil ManagementClusterResourceID / missing ServiceProviderCluster short-circuits and log messages are unchanged. - operation_cluster_delete_test: add table cases exercising the early-return "gone -> proceeds" branches (missing ServiceProviderCluster, SPC with a nil ManagementClusterResourceID, and an unregistered management cluster whose kube-applier client is nil). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:27:26Z

+	// Hold the delete operation non-terminal until the ClusterResourcesController has
+	// removed all the ApplyDesires it owns. Placed after the deadline check above so
+	// the timeout-failure path still fires if this cleanup stalls.
+	applyDesiresGone, err := c.clusterResourceApplyDesiresGone(ctx, cluster)
+	if err != nil {
+		return utils.TrackError(fmt.Errorf("failed to check ClusterResourcesController ApplyDesire precondition: %w", err))
+	}
+	if !applyDesiresGone {
+		logger.Info("waiting for ClusterResourcesController to delete its ApplyDesires before completing delete operation")
+		return nil
+	}


The OperationClusterDelete section of docs/cosmos-data-flow.md documents these reads as of the current commit — the ServiceProviderCluster read for Status.ManagementClusterResourceID, then listing ApplyDesires grouped by Tags[ControllerName], with the operation held non-terminal while any remain. No further doc change is needed.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:27:26Z

+	// Check that the ClusterResourcesController has removed all the ApplyDesires it
+	// owns. That controller is responsible for deleting its own desires during
+	// cluster deletion; we only remove the ServiceProviderCluster document (which
+	// carries the ManagementClusterResourceID needed to reach them) once they are gone.
+	applyDesiresGone, err := c.clusterResourceApplyDesiresGone(
+		ctx,
+		spc,
+		serviceProviderClusterResourceID.SubscriptionID,
+		serviceProviderClusterResourceID.ResourceGroupName,
+		clusterName,
+	)
+	if err != nil {
+		return false, utils.TrackError(fmt.Errorf("failed to check ClusterResourcesController ApplyDesire precondition: %w", err))
+	}
+	if !applyDesiresGone {
+		logger.Info("waiting for ClusterResourcesController to delete its ApplyDesires before removing ServiceProviderCluster",
+			"serviceProviderClusterResourceID", spc.ResourceID.String())
+		return false, nil
+	}


The ClusterChildResourcesCleanupController section of docs/cosmos-data-flow.md reflects this extra precondition and the added reads — SPC read for Status.ManagementClusterResourceID, ApplyDesire list grouped by Tags[ControllerName], with SPC deletion blocked while any remain. Accurate for the current commit.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

David Eads (deads2k) · 2026-08-25T11:01:18Z

+			continue
+		}
+		if desire.Tags[kubeapplierapi.TagControllerName] == kubeapplierapi.ClusterResourcesControllerName {
+			// A tagged ApplyDesire still exists; the gate is not satisfied. Surface


no, count how many we have for each controller and log a message saying we're waiting on X many for each controller to be cleaned up.

Done in 14ef6f4a. The SPC-deletion gate now counts remaining ApplyDesires grouped by controller (Tags[TagControllerName], untagged bucketed as unknown) and logs a breakdown like "N ApplyDesire(s) still exist: 2 for controller X, 1 for controller unknown" while it blocks.

AI-generated. Review for accuracy.

David Eads (deads2k) · 2026-08-25T11:03:01Z

+	// Hold the delete operation non-terminal until the ClusterResourcesController has
+	// removed all the ApplyDesires it owns. Placed after the deadline check above so
+	// the timeout-failure path still fires if this cleanup stalls.
+	applyDesiresGone, err := c.clusterResourceApplyDesiresGone(ctx, cluster)


no, do it like hostedClusterDeletionStatus so we can have a message for the operation about how many applydesires are remaining for each controller

Done in 14ef6f4a. Added applyDesiresDeletionStatus modeled on hostedClusterDeletionStatus (returns *operationbase.OperationState) and wired it into buildDeletionTimeoutMessage (source applyDesires), so the delete operation now carries a per-controller message about how many ApplyDesires remain, and holds non-terminal while any exist.

AI-generated. Review for accuracy.

David Eads (deads2k) · 2026-08-25T11:03:52Z

+	// cluster-deletion gates can reference the value on main before that package
+	// exists. When #6070 lands it should consume this constant rather than
+	// redefine its own, so the tag value stays single-sourced.
+	ClusterResourcesControllerName = "ClusterResourcesController"


No. The controller name doesn't matter in this PR. Any ApplyDesire should stop deletion

Done in 14ef6f4a. Reverted the tags.go change entirely — the gate no longer filters by controller name. Any remaining ApplyDesire now blocks deletion.

AI-generated. Review for accuracy.

… counts deads2k review of the ApplyDesire delete gate: - Revert the ClusterResourcesControllerName constant added to internal/api/kubeapplierapi/tags.go. The gate no longer filters by authoring controller, so the value is not needed on main (PR Azure#6070 continues to define it in its own clusterresources package). - Both gates now block on ANY remaining ApplyDesire for the cluster, not just those authored by a particular controller. Each enumerates ApplyDesires via kubeApplierDBClients.For(ctx, mcID).ApplyDesiresForCluster(...).List(...) and builds a count grouped by Tags[kubeapplierapi.TagControllerName] (ApplyDesires with no such tag bucket under "unknown"), emitting a human-readable per-controller breakdown while any remain. - clusterChildResourcesCleanupController: extraDeleteGateShouldDeleteServiceProviderCluster logs the per-controller breakdown and blocks ServiceProviderCluster deletion while any ApplyDesire remains. - operationClusterDelete: applyDesiresDeletionStatus mirrors hostedClusterDeletionStatus, so the delete operation carries a per-controller ApplyDesire status (surfaced via the delete-timeout message) and is held non-terminal while any remain, after the deadline check. - Keep the "gone" short-circuits: nil kube-applier client, nil ManagementClusterResourceID, or missing ServiceProviderCluster are all treated as no remaining ApplyDesires. - Update both packages' tests for the new any-ApplyDesire behavior and the per-controller breakdown, and refresh the OperationClusterDelete and ClusterChildResourcesCleanupController sections of docs/cosmos-data-flow.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:17:50Z

+	// Hold the delete operation non-terminal until every ApplyDesire for the cluster
+	// has been removed, surfacing a per-controller breakdown on the operation status.
+	// Placed after the deadline check above so the timeout-failure path still fires if
+	// this cleanup stalls.
+	applyDesiresState, err := c.applyDesiresDeletionStatus(ctx, cluster)
+	if err != nil {
+		return utils.TrackError(fmt.Errorf("failed to check remaining ApplyDesires: %w", err))
+	}
+	if applyDesiresState.ProvisioningState != coreapi.ProvisioningStateSucceeded {
+		logger.Info("waiting for ApplyDesires to be deleted before completing delete operation", "remaining", applyDesiresState.Message)
+		return nil
+	}


Updated the PR title and description to match: the gate intentionally blocks while any ApplyDesire remains for the cluster (not only ClusterResourcesController-authored), per review feedback. The per-controller breakdown is for diagnostics only.

AI-generated. Review for accuracy.

Aligned in 75a66f4. The gate intentionally blocks on any remaining ApplyDesire — there is no controller-specific filtering and no ClusterResourcesControllerName constant. I've updated the PR title to match the implemented behavior. The per-Tags[ControllerName] breakdown is diagnostic only (surfaced in the delete-timeout message), not a gating filter.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:17:50Z

+	parts := make([]string, 0, len(countsByController))
+	for controllerName, count := range countsByController {
+		parts = append(parts, fmt.Sprintf("%d for controller %s", count, controllerName))
+	}
+	slices.Sort(parts)
+	return total, strings.Join(parts, ", "), nil


Done in 75a66f4 — the breakdown is now sorted by controller name (in the shared helper) before formatting, so output ordering is stable across reconciles.

AI-generated. Review for accuracy.

Fixed in 75a66f4 — the breakdown now sorts by controller name (map keys) instead of the formatted "%d for controller %s" strings. This lives in the shared helper kubeappliercosmosstorage.SummarizeApplyDesiresByController and is covered by TestSummarizeApplyDesiresByController.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

David Eads (deads2k) · 2026-08-25T13:46:54Z

+	parts := make([]string, 0, len(countsByController))
+	for controllerName, count := range countsByController {
+		parts = append(parts, fmt.Sprintf("%d for controller %s", count, controllerName))
+	}
+	slices.Sort(parts)
+	return total, strings.Join(parts, ", "), nil


Done in 75a66f4. The per-controller breakdown is now sorted by controller name (the map key) before formatting — done once in the shared helper — so ordering is stable across reconciles regardless of counts.

AI-generated. Review for accuracy.

Done in 75a66f4 — same shared helper (SummarizeApplyDesiresByController) now sorts by controller name, so this path is stable too.

AI-generated. Review for accuracy.

Fixed in 75a66f4 — same sort-by-controller-name fix, now centralized in the shared helper SummarizeApplyDesiresByController used by both this controller and the delete operation (the byte-for-byte duplicate here was removed).

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

Done in 75a66f4. The breakdown now sorts by controller name (sorting the map keys, then formatting) instead of the "%d for controller %s" string, so ordering is stable across reconciles regardless of counts. The logic is centralized in the shared helper kubeappliercosmosstorage.SummarizeApplyDesiresByController, and TestSummarizeApplyDesiresByController ("breakdown sorted by controller name, not count") asserts the ordering.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

David Eads (deads2k)

fixes needed.

David Eads (deads2k) · 2026-08-25T13:46:54Z

+	parts := make([]string, 0, len(countsByController))
+	for controllerName, count := range countsByController {
+		parts = append(parts, fmt.Sprintf("%d for controller %s", count, controllerName))
+	}
+	slices.Sort(parts)
+	return total, strings.Join(parts, ", "), nil


David Eads (deads2k) · 2026-08-25T13:49:10Z

+	// has been removed, surfacing a per-controller breakdown on the operation status.
+	// Placed after the deadline check above so the timeout-failure path still fires if
+	// this cleanup stalls.
+	applyDesiresState, err := c.applyDesiresDeletionStatus(ctx, cluster)


no! put the computation in buildDeletionTimeoutMessage with the rest of our calculations. And re-use the logic for associating applydesires with controllers.

Done in 75a66f4. Removed the standalone applyDesiresDeletionStatus; the remaining-ApplyDesires computation is folded into buildDeletionTimeoutMessage alongside the other deletion-timeout calculations. The "enumerate ApplyDesires + associate with controllers" logic is now a single shared helper — SummarizeApplyDesiresByController in kubeappliercosmosstorage — reused by both buildDeletionTimeoutMessage/the operation gate and the child-cleanup gate (no more duplicated copies).

AI-generated. Review for accuracy.

Done in 75a66f4. The per-controller computation now lives with the other deletion-status calculations in buildDeletionTimeoutMessage (via the shared countRemainingApplyDesires data helper, emitted as a .WithSource("applyDesires") state). The SynchronizeOperation gate reuses that same countRemainingApplyDesires rather than computing its own, and the controller-association logic is now the single shared helper kubeappliercosmosstorage.SummarizeApplyDesiresByController. The standalone applyDesiresDeletionStatus is gone.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

…calc into buildDeletionTimeoutMessage deads2k review follow-ups: - Sort the per-controller ApplyDesire breakdown by controller NAME before formatting (previously the formatted "%d for controller %s" strings were sorted, so ordering depended on counts and drifted across reconciles). - Factor the "enumerate ApplyDesires + group by Tags[kubeapplierapi.TagControllerName] (untagged -> unknown), sorted by name" logic into a single shared helper, kubeappliercosmosstorage.SummarizeApplyDesiresByController, and reuse it from BOTH the ClusterChildResourcesCleanupController SPC-deletion gate and the delete operation. Removes the two duplicated per-package copies. Placed in kubeappliercosmosstorage, which both controllers already import and which owns the ApplyDesire CRUD (no import cycle). - operationClusterDelete: remove the standalone applyDesiresDeletionStatus and fold the "remaining ApplyDesires per controller" computation into buildDeletionTimeoutMessage alongside the other deletion-timeout calculations. A small countRemainingApplyDesires data helper (SPC -> kube-applier client -> shared summarizer) is reused by both buildDeletionTimeoutMessage and the SynchronizeOperation gate, which still holds the operation non-terminal while any ApplyDesire remains (after the deadline check). Behavior unchanged: gate on ANY remaining ApplyDesire (no controller-name filter); nil client / nil ManagementClusterResourceID / missing ServiceProviderCluster are treated as gone/proceed. tags.go remains untouched vs origin/main. Tests updated for the stable name-sorted breakdown, and a direct unit test covers the shared summarizer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

openshift-ci · 2026-08-25T14:14:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign geoberle for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

backend/pkg/controllers/cluster/operations/operation_cluster_delete.go:193

The PR description states these new delete gates are scoped to ApplyDesires authored by ClusterResourcesController (via Tags[TagControllerName] == ClusterResourcesControllerName), but the implementation here gates on any remaining ApplyDesire for the cluster and tags are only used for grouping in the breakdown. Please align either the implementation (filter by the specific controller tag) or the PR description/docs so reviewers and operators have the same understanding of what blocks deletion.

	// Hold the delete operation non-terminal until every ApplyDesire for the cluster
	// has been removed. Placed after the deadline check above so the timeout-failure
	// path still fires if this cleanup stalls; that path (buildDeletionTimeoutMessage)
	// surfaces the same per-controller breakdown on the operation.
	remainingApplyDesires, applyDesireBreakdown, err := c.countRemainingApplyDesires(ctx, cluster)
	if err != nil {
		return utils.TrackError(fmt.Errorf("failed to check remaining ApplyDesires: %w", err))
	}
	if remainingApplyDesires > 0 {
		logger.Info("waiting for ApplyDesires to be deleted before completing delete operation", "remaining", applyDesireBreakdown)
		return nil
	}

backend/pkg/controllers/cluster/operations/operation_cluster_delete.go:199

Returning early when ApplyDesires remain prevents reconcileOperationAndResourceStatus from running, which can leave the delete Operation status stale (e.g., staying in Accepted) for long periods even while Cluster Service reports Uninstalling. If the goal is only to prevent the operation from becoming terminal, consider allowing status reconciliation to proceed and only gating terminal completion (or explicitly document that this controller intentionally stops polling Cluster Service until ApplyDesires are gone).

	// Hold the delete operation non-terminal until every ApplyDesire for the cluster
	// has been removed. Placed after the deadline check above so the timeout-failure
	// path still fires if this cleanup stalls; that path (buildDeletionTimeoutMessage)
	// surfaces the same per-controller breakdown on the operation.
	remainingApplyDesires, applyDesireBreakdown, err := c.countRemainingApplyDesires(ctx, cluster)
	if err != nil {
		return utils.TrackError(fmt.Errorf("failed to check remaining ApplyDesires: %w", err))
	}
	if remainingApplyDesires > 0 {
		logger.Info("waiting for ApplyDesires to be deleted before completing delete operation", "remaining", applyDesireBreakdown)
		return nil
	}

	if !c.shouldReconcileOperationAndResourceStatus(cluster) {
		return nil
	}
	err = c.reconcileOperationAndResourceStatus(ctx, operation, cluster)
	if err != nil {

Chai-bot (redhat-chai-bot) · 2026-08-25T14:35:40Z

+func SummarizeApplyDesiresByController(ctx context.Context, applyDesireCRUD cosmosstorageutils.ResourceCRUD[kubeapplierapi.ApplyDesire, *kubeapplierapi.ApplyDesire]) (int, string, error) {
+	applyDesireIterator, err := applyDesireCRUD.List(ctx, &cosmosstorageutils.DBClientListResourceDocsOptions{})
+	if err != nil {
+		return 0, "", fmt.Errorf("failed to list ApplyDesire documents: %w", err)
+	}
+
+	countsByController := map[string]int{}
+	total := 0
+	for _, desire := range applyDesireIterator.Items(ctx) {
+		controllerName := desire.Tags[kubeapplierapi.TagControllerName]
+		if controllerName == "" {
+			controllerName = UnknownApplyDesireController
+		}
+		countsByController[controllerName]++
+		total++
+	}


The full enumeration is intentional. deads2k asked for a per-controller count breakdown in the wait message ("count how many we have for each controller and log a message saying we're waiting on X many for each controller"), which requires visiting every ApplyDesire — a boolean short-circuit can't produce per-controller counts. The gate blocks while total > 0, so this only pages the full set during the (bounded) tail of deletion while desires are still being cleaned up. If the scan cost becomes a concern for very large clusters, a follow-up could cap/paginate the breakdown, but that's out of scope for this PR.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:38:32Z

+	if remainingApplyDesires > 0 {
+		logger.Info("waiting for ApplyDesires to be deleted before completing delete operation", "remaining", applyDesireBreakdown)
+		return nil
+	}


Done in c54c328. The delete-operation wait log now logs the numeric count under remaining (an int) and moves the human-readable breakdown to a separate breakdown field — so remaining is queryable/alertable as an integer:

logger.Info("waiting for ApplyDesires to be deleted before completing delete operation", "remaining", remainingApplyDesires, "breakdown", applyDesireBreakdown)

The cleanup controller's SPC-deletion gate log builds the count + breakdown into the message via fmt.Sprintf (its only structured field is serviceProviderClusterResourceID), so it doesn't have the "remaining"=string pattern and was left as-is.

AI-generated. Review for accuracy.

AI-generated. Review for accuracy.

Chai-bot (redhat-chai-bot) · 2026-08-25T14:35:40Z

/retest

ci/prow/e2e-parallel failed in the image-build step, before any test ran: the aro-hcp-frontend build failed (DockerBuildFailed) because go mod download hit a transient proxy.golang.org HTTP/2 INTERNAL_ERROR (on go.opentelemetry.io/otel/metric and github.com/beorn7/perks). That's a network flake in the build, unrelated to this change — ci/prow/test-unit is green on 75a66f4. Re-running.

AI-generated. Review for accuracy.

…er separate key The SynchronizeOperation delete gate's structured log set the "remaining" field to the human-readable breakdown string instead of the numeric count. Log the numeric remainingApplyDesires count under "remaining" and move the breakdown string to a separate "breakdown" field (Copilot, operation_cluster_delete.go). The ClusterChildResourcesCleanupController SPC-deletion gate log already embeds the count and breakdown in the message via fmt.Sprintf (no structured "remaining" field), so it needs no change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Chai Bot <ship-help-github@redhat.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go:357

remainingApplyDesires() already returns an error to a caller that wraps it with utils.TrackError. Wrapping this error with utils.TrackError here can lead to double-tracked / noisy errors. Prefer returning a plain wrapped error and let the caller track once.

This issue also appears on line 359 of the same file.

	applyDesireCRUD, err := kubeApplierDBClient.ApplyDesiresForCluster(subscriptionID, resourceGroupName, clusterName)
	if err != nil {
		return 0, "", utils.TrackError(fmt.Errorf("failed to get kube-applier CRUD for ApplyDesire precondition: %w", err))
	}

backend/pkg/controllers/cluster/operations/operation_cluster_delete_test.go:417

This test uses time.Now() even though the controller is configured with a fake clock (fixedTime). Using wall-clock time makes the test less deterministic and can become flaky/confusing if any logic compares timestamps to c.clock.Now(). Prefer using fixedTime so the entire test is time-stable.

	clusterPassingReconcileGate := func() *coreapi.HCPOpenShiftCluster {
		now := time.Now()
		cluster := fixture.NewCluster(nil)
		cluster.ServiceProviderProperties.DeletionTimestamp = &metav1.Time{Time: now}
		cluster.ServiceProviderProperties.ClusterServiceDeletionTimestamp = &metav1.Time{Time: now}
		return cluster

backend/pkg/controllers/cluster/deletion/cluster_child_resources_cleanup_controller.go:362

Same double-tracking concern as above: returning utils.TrackError(err) here means the caller will TrackError again. Return a plain wrapped error instead so tracking happens once at the top-level.

	total, breakdown, err := kubeappliercosmosstorage.SummarizeApplyDesiresByController(ctx, applyDesireCRUD)
	if err != nil {
		return 0, "", utils.TrackError(err)
	}

openshift-ci · 2026-08-25T15:39:13Z

Chai-bot (@redhat-chai-bot): The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-parallel	`c54c328`	link	true	`/test e2e-parallel`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copilot AI lite review requested due to automatic review settings August 24, 2026 17:47

openshift-ci Bot requested review from David Eads (deads2k) and Miguel Soriano (miguelsorianod) August 24, 2026 17:47

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 24, 2026 17:48 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 24, 2026 18:08

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 24, 2026 18:09 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

David Eads (deads2k) reviewed Aug 25, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 25, 2026 13:39

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 25, 2026 13:39 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

David Eads (deads2k) requested changes Aug 25, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 25, 2026 14:14

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 25, 2026 14:14 View session

Chai-bot (redhat-chai-bot) changed the title ~~Gate SPC deletion + delete-operation on ClusterResourcesController ApplyDesires removal~~ Gate ServiceProviderCluster deletion + delete operation on remaining ApplyDesires Aug 25, 2026

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Chai-bot (redhat-chai-bot) changed the title ~~Gate ServiceProviderCluster deletion + delete operation on remaining ApplyDesires~~ Gate SPC deletion + delete-operation on removal of all remaining ApplyDesires Aug 25, 2026

Copilot AI review requested due to automatic review settings August 25, 2026 14:37

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 25, 2026 14:38 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Uh oh!

Conversation

Chai-bot (redhat-chai-bot) commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Behavior notes

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

David Eads (deads2k) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Chai-bot (redhat-chai-bot) commented Aug 24, 2026 •

edited

Loading