Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package deletion
import (
"context"
"fmt"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -271,6 +272,27 @@ func (c *clusterChildResourcesCleanupController) extraDeleteGateShouldDeleteServ
return false, nil
}

// Wait until every ApplyDesire for this cluster has been removed. Each authoring
// controller is responsible for deleting the ApplyDesires it owns during cluster
// deletion; we only remove the ServiceProviderCluster document (which carries the
// ManagementClusterResourceID needed to reach them) once they are all gone. Report
// the per-controller breakdown so it is clear which controller is lagging.
remaining, applyDesireBreakdown, err := c.remainingApplyDesires(
ctx,
spc,
serviceProviderClusterResourceID.SubscriptionID,
serviceProviderClusterResourceID.ResourceGroupName,
clusterName,
)
if err != nil {
return false, utils.TrackError(fmt.Errorf("failed to check remaining ApplyDesires: %w", err))
}
if remaining > 0 {
logger.Info(fmt.Sprintf("waiting for %d ApplyDesire(s) to be deleted before removing ServiceProviderCluster: %s", remaining, applyDesireBreakdown),
"serviceProviderClusterResourceID", spc.ResourceID.String())
return false, nil
}

// Check if there are any cluster-scoped kube-applier *Desire documents remaining.
if spc.Status.ManagementClusterResourceID != nil {
kaClient := c.kubeApplierDBClients.For(ctx, spc.Status.ManagementClusterResourceID)
Expand Down Expand Up @@ -307,6 +329,78 @@ func (c *clusterChildResourcesCleanupController) extraDeleteGateShouldDeleteServ
return true, nil
}

// remainingApplyDesires reports how many ApplyDesires still exist for the cluster
// owning the given ServiceProviderCluster, along with a human-readable breakdown
// grouped by the authoring controller. Each authoring controller is responsible for
// deleting the ApplyDesires it owns during cluster deletion; this controller only
// verifies they are gone before removing the ServiceProviderCluster document (which
// carries the ManagementClusterResourceID needed to reach them). A nil management
// cluster reference or unavailable kube-applier client is treated as "no remaining
// ApplyDesires", consistent with the best-effort behavior elsewhere in this file.
func (c *clusterChildResourcesCleanupController) remainingApplyDesires(ctx context.Context, spc *coreapi.ServiceProviderCluster, subscriptionID, resourceGroupName, clusterName string) (int, string, error) {
logger := utils.LoggerFromContext(ctx)

if spc == nil || spc.Status.ManagementClusterResourceID == nil {
return 0, "", nil
}

managementClusterID := spc.Status.ManagementClusterResourceID
kubeApplierDBClient := c.kubeApplierDBClients.For(ctx, managementClusterID)
if kubeApplierDBClient == nil {
logger.Info("no kube-applier client for management cluster; treating ApplyDesires as gone",
"managementClusterResourceID", managementClusterID.String())
return 0, "", nil
}

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))
}

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

// unknownApplyDesireController is the bucket used for ApplyDesires that carry no
// kubeapplierapi.TagControllerName tag.
const unknownApplyDesireController = "unknown"

// applyDesireControllerCounts iterates every ApplyDesire reachable through the given
// CRUD and returns the total count plus a stable, human-readable breakdown grouped
// by the authoring controller recorded in Tags[kubeapplierapi.TagControllerName].
// ApplyDesires with no controller tag are bucketed under "unknown", e.g.
// "2 for controller SomeController, 1 for controller unknown".
func applyDesireControllerCounts(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++
}
if err := applyDesireIterator.GetError(); err != nil {
return 0, "", fmt.Errorf("error iterating ApplyDesire documents: %w", err)
}

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix this

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


AI-generated. Review for accuracy.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// ensureClusterScopedKubeApplierResourcesDeleted ensures that the cluster-scoped *Desire documents are deleted
// from the database. *Desire documents on non-cluster scoped resources are deleted by their corresponding deletion controllers.
func (c *clusterChildResourcesCleanupController) ensureClusterScopedKubeApplierResourcesDeleted(ctx context.Context, clusterResourceID *azcorearm.ResourceID) error {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -729,3 +729,202 @@ func TestIsUnderSkippedSubtree(t *testing.T) {
})
}
}

func TestClusterChildResourcesCleanupController_remainingApplyDesires(t *testing.T) {
managementClusterResourceID := metadataapi.Must(azcorearm.ParseResourceID(
"/providers/microsoft.redhatopenshift/stamps/1/managementclusters/default"))
unregisteredManagementClusterResourceID := metadataapi.Must(azcorearm.ParseResourceID(
"/providers/microsoft.redhatopenshift/stamps/1/managementclusters/unregistered"))

newSPC := func(mc *azcorearm.ResourceID) *coreapi.ServiceProviderCluster {
spcResourceID := metadataapi.Must(azcorearm.ParseResourceID(
"/subscriptions/" + testSubscriptionID +
"/resourceGroups/" + testResourceGroupName +
"/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + testClusterName +
"/serviceProviderClusters/default"))
return &coreapi.ServiceProviderCluster{
CosmosMetadata: coreapi.CosmosMetadata{
ResourceID: spcResourceID,
PartitionKey: strings.ToLower(spcResourceID.SubscriptionID),
},
Status: coreapi.ServiceProviderClusterStatus{
ManagementClusterResourceID: mc,
},
}
}
newApplyDesire := func(name string, tags map[string]string) *kubeapplierapi.ApplyDesire {
resourceID := metadataapi.Must(azcorearm.ParseResourceID(
kubeapplierapi.ToClusterScopedApplyDesireResourceIDString(
testSubscriptionID, testResourceGroupName, testClusterName, name)))
return &kubeapplierapi.ApplyDesire{
CosmosMetadata: coreapi.CosmosMetadata{
ResourceID: resourceID,
PartitionKey: strings.ToLower(managementClusterResourceID.String()),
},
Spec: kubeapplierapi.ApplyDesireSpec{
ManagementCluster: managementClusterResourceID,
},
Tags: tags,
}
}
taggedDesire := func(name, controllerName string) *kubeapplierapi.ApplyDesire {
return newApplyDesire(name, map[string]string{kubeapplierapi.TagControllerName: controllerName})
}
untaggedDesire := func(name string) *kubeapplierapi.ApplyDesire {
return newApplyDesire(name, nil)
}

testCases := []struct {
name string
spc *coreapi.ServiceProviderCluster
kubeApplierDesires []any
wantTotal int
wantBreakdown string
}{
{
name: "tagged ApplyDesire present -> counted by controller",
spc: newSPC(managementClusterResourceID),
kubeApplierDesires: []any{taggedDesire("desire-a", "test-controller")},
wantTotal: 1,
wantBreakdown: "1 for controller test-controller",
},
{
name: "untagged ApplyDesire present -> counted as unknown",
spc: newSPC(managementClusterResourceID),
kubeApplierDesires: []any{untaggedDesire("desire-a")},
wantTotal: 1,
wantBreakdown: "1 for controller unknown",
},
{
name: "tagged and untagged ApplyDesires -> per-controller breakdown",
spc: newSPC(managementClusterResourceID),
kubeApplierDesires: []any{
taggedDesire("desire-a", "test-controller"),
untaggedDesire("desire-b"),
},
wantTotal: 2,
wantBreakdown: "1 for controller test-controller, 1 for controller unknown",
},
{
name: "no ApplyDesires -> none remaining",
spc: newSPC(managementClusterResourceID),
wantTotal: 0,
wantBreakdown: "",
},
{
name: "nil management cluster resource ID -> none remaining",
spc: newSPC(nil),
wantTotal: 0,
wantBreakdown: "",
},
{
name: "unregistered management cluster (nil client) -> none remaining",
spc: newSPC(unregisteredManagementClusterResourceID),
kubeApplierDesires: []any{taggedDesire("desire-a", "test-controller")},
wantTotal: 0,
wantBreakdown: "",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := utils.ContextWithLogger(context.Background(), testr.New(t))

mockKubeApplierDBClients := kubeappliercosmosstoragetesting.NewMockKubeApplierDBClients()
mockKubeApplierClient, err := kubeappliercosmosstoragetesting.NewMockKubeApplierDBClientWithResources(ctx, tc.kubeApplierDesires)
require.NoError(t, err)
mockKubeApplierDBClients.Register(managementClusterResourceID, mockKubeApplierClient)

syncer := &clusterChildResourcesCleanupController{
kubeApplierDBClients: mockKubeApplierDBClients,
}

total, breakdown, err := syncer.remainingApplyDesires(ctx, tc.spc, testSubscriptionID, testResourceGroupName, testClusterName)
require.NoError(t, err)
assert.Equal(t, tc.wantTotal, total)
assert.Equal(t, tc.wantBreakdown, breakdown)
})
}
}

func TestClusterChildResourcesCleanupController_extraDeleteGate_ApplyDesires(t *testing.T) {
managementClusterResourceID := metadataapi.Must(azcorearm.ParseResourceID(
"/providers/microsoft.redhatopenshift/stamps/1/managementclusters/default"))

spcResourceID := metadataapi.Must(azcorearm.ParseResourceID(
"/subscriptions/" + testSubscriptionID +
"/resourceGroups/" + testResourceGroupName +
"/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + testClusterName +
"/serviceProviderClusters/default"))

newSPC := func() *coreapi.ServiceProviderCluster {
return &coreapi.ServiceProviderCluster{
CosmosMetadata: coreapi.CosmosMetadata{
ResourceID: spcResourceID,
PartitionKey: strings.ToLower(spcResourceID.SubscriptionID),
},
Status: coreapi.ServiceProviderClusterStatus{
ManagementClusterResourceID: managementClusterResourceID,
},
}
}
newApplyDesire := func(name string, tags map[string]string) *kubeapplierapi.ApplyDesire {
resourceID := metadataapi.Must(azcorearm.ParseResourceID(
kubeapplierapi.ToClusterScopedApplyDesireResourceIDString(
testSubscriptionID, testResourceGroupName, testClusterName, name)))
return &kubeapplierapi.ApplyDesire{
CosmosMetadata: coreapi.CosmosMetadata{
ResourceID: resourceID,
PartitionKey: strings.ToLower(managementClusterResourceID.String()),
},
Spec: kubeapplierapi.ApplyDesireSpec{
ManagementCluster: managementClusterResourceID,
},
Tags: tags,
}
}

testCases := []struct {
name string
kubeApplierDesires []any
wantShouldDelete bool
}{
{
name: "tagged ApplyDesire present -> SPC deletion blocked",
kubeApplierDesires: []any{newApplyDesire("desire-a", map[string]string{kubeapplierapi.TagControllerName: "test-controller"})},
wantShouldDelete: false,
},
{
name: "untagged ApplyDesire present -> SPC deletion blocked",
kubeApplierDesires: []any{newApplyDesire("desire-a", nil)},
wantShouldDelete: false,
},
{
name: "no kube-applier desires -> SPC deletion allowed",
wantShouldDelete: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := utils.ContextWithLogger(context.Background(), testr.New(t))

mockResourcesDBClient, err := corecosmosstoragetesting.NewMockResourcesDBClientWithResources(ctx, []any{newSPC()})
require.NoError(t, err)

mockKubeApplierDBClients := kubeappliercosmosstoragetesting.NewMockKubeApplierDBClients()
mockKubeApplierClient, err := kubeappliercosmosstoragetesting.NewMockKubeApplierDBClientWithResources(ctx, tc.kubeApplierDesires)
require.NoError(t, err)
mockKubeApplierDBClients.Register(managementClusterResourceID, mockKubeApplierClient)

syncer := &clusterChildResourcesCleanupController{
resourcesDBClient: mockResourcesDBClient,
kubeApplierDBClients: mockKubeApplierDBClients,
}

shouldDelete, err := syncer.extraDeleteGateShouldDeleteServiceProviderCluster(ctx, spcResourceID)
require.NoError(t, err)
assert.Equal(t, tc.wantShouldDelete, shouldDelete)
})
}
}
Loading