Acknowledge all ASO resource types before creating #5571

nojnhuh · 2025-04-17T19:59:04Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

This PR fixes a potential issue where if CAPZ creates an ASO resource defined by an ASOAPI resource, then crashes or otherwise fails to record that resource in the PATCH to the CAPZ resource status at the end of the reconciliation loop, a quickly following delete of the CAPZ resource (or removing that ASO resource from spec) will not know to also delete that ASO resource if it's not recorded in status.

This change makes CAPZ record each ASO resource group/version/kind in a sigs.k8s.io/cluster-api-provider-azure-owned-aso-kinds annotation and send those changes to the API server before continuing to actually create resources of that type. This way, no matter when a CAPZ ASO API resource is deleted, CAPZ can rely on any ASO resource type that needs to be deleted to be in that annotation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

I initially split this into a couple commits in case that's easier to review: one for the functional change and one to refactor things a bit.

TODOs:

squashed commits
includes documentation
adds unit tests
cherry-pick candidate

Release note:

ASOAPI: Fixed a possible bug that could leave ASO resources dangling when they should be deleted.

nojnhuh · 2025-04-17T19:59:12Z

/hold for squash

codecov · 2025-04-17T20:06:17Z

Codecov Report

Attention: Patch coverage is 84.88372% with 26 lines in your changes missing coverage. Please review.

Project coverage is 52.97%. Comparing base (bbe28d1) to head (17a1280).

Files with missing lines	Patch %	Lines
controllers/resource_reconciler.go	83.95%	17 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5571      +/-   ##
==========================================
+ Coverage   52.84%   52.97%   +0.12%     
==========================================
  Files         278      278              
  Lines       29610    29674      +64     
==========================================
+ Hits        15647    15719      +72     
+ Misses      13146    13139       -7     
+ Partials      817      816       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nojnhuh · 2025-04-23T00:08:13Z

@Jont828 points out that status should generally not be informing any reconcile logic. I now remember seeing that written down somewhere too, but can't find it in the API conventions or kubebuilder book. If you have a link that would be great to add here for posterity. I'll see if I can rework this PR.

nawazkh · 2025-04-23T21:46:13Z

The status summarizes the current state of the object in the system, and is usually persisted with the object by an automated processes but may be generated on the fly. At some cost and perhaps some temporary degradation in behavior, the status could be reconstructed by observation if it were lost

The PUT and POST verbs on objects MUST ignore the "status" values, to avoid accidentally overwriting the status in read-modify-write scenarios.

Reference 1 for the above text: https://book-v1.book.kubebuilder.io/basics/status_subresource
Reference 2 that originates from Ref 1. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

Although, the above links are from older generations of kubebuilder, the underlying convention of not relying on Status of a resource to perform a reconcile logic still is widely used.

Acknowledge all ASO resource types before creating

nojnhuh · 2025-04-28T13:57:24Z

I've updated this to avoid using status from previous reconciliations as an input to anything. Main changes:

Orphaned resources are identified as resources not in spec that have an owner reference to this CAPZ resource.
The types of resources that might need to be checked are stored in an annotation to avoid having to list every resource type advertised by the API server.

/retitle Acknowledge all ASO resource types before creating

nawazkh · 2025-04-30T16:49:50Z

/retest

willie-yao · 2025-05-02T18:54:15Z

/retest

nawazkh · 2025-05-02T20:51:09Z

/retest

nawazkh · 2025-05-02T22:27:38Z

As of this comment, the apidiff job is expected to fail

+ WRAPPED_COMMAND_PID=18
+ wait 18
+ ./scripts/ci-apidiff.sh
*** Running go-apidiff ***
GOBIN=/home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin ./scripts/go_install.sh github.com/joelanford/go-apidiff go-apidiff v0.8.2
/home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/go-apidiff 49f9ded3ce052ca01a9721342cb685c842e1570d
 sigs.k8s.io/cluster-api-provider-azure/api/v1alpha1
  Incompatible changes:
  - (*AzureASOManagedCluster).GetResourceStatuses: removed
  - (*AzureASOManagedControlPlane).GetResourceStatuses: removed
  - (*AzureASOManagedMachinePool).GetResourceStatuses: removed
 sigs.k8s.io/cluster-api-provider-azure/controllers
  Incompatible changes:
  - (*ResourceReconciler).Reconcile: changed from func(context.Context) error to func(context.Context) (bool, error)
+ EXIT_VALUE=1
+ set +o xtrace

nawazkh · 2025-05-08T04:24:52Z

Still reviewing this PR, little too dense to skim through.

nawazkh

Hey @nojnhuh ! This is a cool PR, took some time to understand it. Sorry for the delay there.

I have some questions and suggestions listed below.

nawazkh · 2025-05-09T19:41:42Z

controllers/resource_reconciler.go

@@ -39,6 +42,8 @@ import (
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
 )

+const ownedKindsAnnotation = "sigs.k8s.io/cluster-api-provider-azure-owned-aso-kinds"


We should move this annotation to /azure/const.go unless there is a specific reason to keep it here ?
We also should add explanatory comment on the significance of using this annotation.

This annotation is strictly an implementation detail, so I'd prefer to keep here mostly to signal the intent that this shouldn't be used by anything else.

nawazkh · 2025-05-09T19:44:19Z

controllers/resource_reconciler.go

-	for _, spec := range r.owner.GetResourceStatuses() {
-		newStatus, err := r.deleteResource(ctx, spec.Resource)
+	ownedKindsValue := r.owner.GetAnnotations()[ownedKindsAnnotation]
+	ownedKinds, err := parseOwnedKinds(ownedKindsValue)


It be great if we can add a supporting unit test for parseOwnedKinds()

Since this annotation is an implementation detail, I'd prefer to let the higher-level tests exercise this indirectly.

nawazkh · 2025-05-09T19:49:10Z

controllers/resource_reconciler.go

+		return false, fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue)
+	}
+
+	ownedObjs, err := r.ownedObjs(ctx, ownedKinds)


Same suggestion as above, would love to see a unit test validating r.ownedObjs().

nawazkh · 2025-05-09T19:53:16Z

controllers/resource_reconciler.go

+
+// partitionResources splits the sets of resources in spec and the current set
+// of owned, existing resources into three groups:
+// - unobservedTypeResources are of a type not yet known to this owning CAPZ resource.
+// - observedTypeResources are of a type already known to this owning CAPZ resource.
+// - deletedResources exist but are not defined in spec.


Could we update deletedResources to toBeDeletedResources since the resources saved in this struct will be issued a delete later on ?

- observedTypeResources are of a type already known to this owning CAPZ resource.

I might be missing the significance of the word "Type" here ?
When you say observedTypeResources does "TypeResource" signify something other than a typed entity in Go ?

Should we update below resources instead ?

unobservedTypeResources to unrecordedResources

observedTypeResources to recordedResources

The "type" in the name is important because that's the granularity at which CAPZ is tracking whether or not a resource is safe to create. CAPZ doesn't track whether or not it's seen every individual resource in spec.resources, only the types of those resources. So "observedTypeResources" are "resources whose type has been observed."

nawazkh · 2025-05-09T20:50:31Z

controllers/resource_reconciler.go

+	}
+
+	for _, spec := range observedTypeResources {
+		spec.SetNamespace(r.owner.GetNamespace())


Could this update, spec.SetNamespace(), result in validation-update-error due to immutability of Namespace on an ASO resource ?

Changing the resource's namespace in a SSA patch would create a new resource in that new namespace. I'm not sure how a user would get into that scenario here in practice though. CAPZ always makes sure the ASO resource's namespace matches that of its owning CAPZ resource.

nawazkh · 2025-05-09T21:38:22Z

controllers/resource_reconciler.go

+		if ownedKinds.Has(typeMeta) {
+			observedTypeResources = append(observedTypeResources, spec)
+		} else {
+			unobservedTypeResources = append(unobservedTypeResources, spec)
+		}


If ownedKinds also gets updated with the TypeMeta of the deletedResources, will the user be able to recreate a deleted resource with a new name ?

Users can always create a resource of any ASO type. The annotation contains the types of the deleted resources only so CAPZ knows how to find resources which might be orphaned and construct delete requests for them.

nawazkh · 2025-05-09T21:52:08Z

controllers/resource_reconciler.go

+	unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)
+


Could we incorporate the below suggestion for better readability ?

Suggested change

unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)

unrecordedResources, recordedResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)

nawazkh · 2025-05-09T22:08:59Z

controllers/resource_reconciler.go

+
+		gvk := spec.GroupVersionKind()
+		log.V(4).Info("applying resource", "resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind)
+		err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager"), client.ForceOwnership)


For my understanding, how is the controller's GUID set upon creation ? Is client.ForceOwnership enforcing the controller's GUID ?

I'm not sure I'm following your question entirely. If by "controller" you mean "the resource reference in the ASO resource's metadata.ownerReferences with controller: true and "GUID" you mean metadata.uid, then this is how things work:

AzureASOManagedControlPlane is created, API server sets metadata.uid

CAPZ reconciles AzureASOManagedControlPlane, seeing it defines a ManagedCluster in its spec.resources

CAPZ sets the ManagedCluster's owner references to include the AzureASOManagedControlPlane, setting the reference's uid to the AzureASOManagedControlPlane's metadata.uid.

The ForceOwnership only deals with how conflicts in the SSA patch are handled. The docs for that option say that most controllers should use this.

nawazkh · 2025-05-09T22:26:40Z

controllers/resource_reconciler.go

 	r.owner.SetResourceStatuses(newResourceStatuses)

-	return nil
+	return len(unobservedTypeResources) > 0, nil


Are we only going to trigger a requeue if len(unobservedTypeResources) > 0 ? Should we be triggering a requeue nonetheless ?

We only explicitly trigger a requeue here because we're already watching ASO objects and requeueing their owners when those ASO objects change. If this is the first reconciliation though and no ASO objects were created, then we can't requeue the CAPZ object based on ASO object changes (since the objects don't exist).

Actually, CAPZ already requeues the CAPZ object in reaction to its annotations changing though, so we don't need to do any explicit requeueing here.

nawazkh · 2025-05-09T22:36:27Z

controllers/resource_reconciler.go

+	}
+}
+
+func parseOwnedKinds(value string) (sets.Set[metav1.TypeMeta], error) {


We should probably have a unit test for this function validating a scenario with an ownerKind with multiple / in it.

I've simplified this to use logic from k8s.io/apimachinery to parse the annotation, so maybe not worth testing this rigorously directly.

willie-yao · 2025-05-29T16:16:23Z

@nojnhuh Let me know if this is ready for review again!

nawazkh · 2025-05-30T19:01:08Z

Un-assigning myself from this PR for now since I won't be able to review it again. :)
/unassign

rename variables

tweak annotation format

don't explicitly requeue

nojnhuh · 2025-06-24T21:43:06Z

@willie-yao I think I've addressed all the feedback on this so far, so this is ready for another look when time permits.

k8s-ci-robot · 2025-06-24T21:43:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-06-24T21:48:04Z

@nojnhuh: 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
pull-cluster-api-provider-azure-apidiff	`17a1280`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

nojnhuh added 2 commits April 17, 2025 14:07

Acknowledge all ASO resources before creating

8e8f717

fixup! Acknowledge all ASO resources before creating

ef07155

github-project-automation bot added this to CAPZ Planning Apr 17, 2025

github-project-automation bot moved this to Todo in CAPZ Planning Apr 17, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 17, 2025

k8s-ci-robot requested review from jackfrancis and marosset April 17, 2025 19:59

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 17, 2025

nojnhuh moved this from Todo to Needs Review in CAPZ Planning Apr 17, 2025

nawazkh self-assigned this Apr 18, 2025

amend! Acknowledge all ASO resources before creating

725ec6d

Acknowledge all ASO resource types before creating

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2025

k8s-ci-robot changed the title ~~Acknowledge all ASO resources before creating~~ Acknowledge all ASO resource types before creating Apr 28, 2025

nawazkh reviewed May 9, 2025

View reviewed changes

k8s-ci-robot unassigned nawazkh May 30, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2025

nojnhuh added 4 commits June 24, 2025 11:58

Merge remote-tracking branch 'upstream/main' into aso-airtight

5717746

fixup! Acknowledge all ASO resources before creating

661caf1

rename variables

fixup! Acknowledge all ASO resources before creating

759025a

tweak annotation format

fixup! Acknowledge all ASO resources before creating

17a1280

don't explicitly requeue

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2025

		unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)

	unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)
	unrecordedResources, recordedResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs)

Acknowledge all ASO resource types before creating #5571

Are you sure you want to change the base?

Acknowledge all ASO resource types before creating #5571

Uh oh!

Conversation

nojnhuh commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojnhuh commented Apr 17, 2025

Uh oh!

codecov bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nojnhuh commented Apr 23, 2025

Uh oh!

nawazkh commented Apr 23, 2025

Uh oh!

nojnhuh commented Apr 28, 2025

Uh oh!

nawazkh commented Apr 30, 2025

Uh oh!

willie-yao commented May 2, 2025

Uh oh!

nawazkh commented May 2, 2025

Uh oh!

nawazkh commented May 2, 2025

Uh oh!

nawazkh commented May 8, 2025

Uh oh!

nawazkh 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!

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!

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!

willie-yao commented May 29, 2025

Uh oh!

nawazkh commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojnhuh commented Jun 24, 2025

Uh oh!

nojnhuh commented Apr 17, 2025 •

edited

Loading

codecov bot commented Apr 17, 2025 •

edited

Loading

nawazkh commented May 30, 2025 •

edited

Loading