Acknowledge all ASO resource types before creating #5571
Conversation
k8s-ci-robot
added
release-note
|
/hold for squash |
k8s-ci-robot
added
size/L
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5571 +/- ##
==========================================
+ Coverage 52.87% 53.40% +0.53%
==========================================
Files 272 272
Lines 29473 29602 +129
==========================================
+ Hits 15583 15810 +227
+ Misses 13083 12978 -105
- Partials 807 814 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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. |
Reference 1 for the above text: https://book-v1.book.kubebuilder.io/basics/status_subresource 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
|
I've updated this to avoid using status from previous reconciliations as an input to anything. Main changes:
/retitle Acknowledge all ASO resource types before creating |
k8s-ci-robot
added
size/XL
k8s-ci-robot
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@nojnhuh: The following test failed, say
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. |
|
As of this comment, the apidiff job is expected to fail |
|
Still reviewing this PR, little too dense to skim through. |
| @@ -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" | |||
There was a problem hiding this comment.
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.
| for _, spec := range r.owner.GetResourceStatuses() { | ||
| newStatus, err := r.deleteResource(ctx, spec.Resource) | ||
| ownedKindsValue := r.owner.GetAnnotations()[ownedKindsAnnotation] | ||
| ownedKinds, err := parseOwnedKinds(ownedKindsValue) |
There was a problem hiding this comment.
It be great if we can add a supporting unit test for parseOwnedKinds()
| return false, fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue) | ||
| } | ||
|
|
||
| ownedObjs, err := r.ownedObjs(ctx, ownedKinds) |
There was a problem hiding this comment.
Same suggestion as above, would love to see a unit test validating r.ownedObjs().
|
|
||
| // 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. |
There was a problem hiding this comment.
Could we update deletedResources to toBeDeletedResources since the resources saved in this struct will be issued a delete later on ?
There was a problem hiding this comment.
- 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 ?
There was a problem hiding this comment.
Should we update below resources instead ?
unobservedTypeResourcestounrecordedResourcesobservedTypeResourcestorecordedResources
| } | ||
|
|
||
| for _, spec := range observedTypeResources { | ||
| spec.SetNamespace(r.owner.GetNamespace()) |
There was a problem hiding this comment.
Could this update, spec.SetNamespace(), result in validation-update-error due to immutability of Namespace on an ASO resource ?
| if ownedKinds.Has(typeMeta) { | ||
| observedTypeResources = append(observedTypeResources, spec) | ||
| } else { | ||
| unobservedTypeResources = append(unobservedTypeResources, spec) | ||
| } |
There was a problem hiding this comment.
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 ?
| unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) | ||
|
|
There was a problem hiding this comment.
Could we incorporate the below suggestion for better readability ?
| unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) | |
| unrecordedResources, recordedResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) | |
|
|
||
| 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) |
There was a problem hiding this comment.
For my understanding, how is the controller's GUID set upon creation ? Is client.ForceOwnership enforcing the controller's GUID ?
| r.owner.SetResourceStatuses(newResourceStatuses) | ||
|
|
||
| return nil | ||
| return len(unobservedTypeResources) > 0, nil |
There was a problem hiding this comment.
Are we only going to trigger a requeue if len(unobservedTypeResources) > 0 ? Should we be triggering a requeue nonetheless ?
| } | ||
| } | ||
|
|
||
| func parseOwnedKinds(value string) (sets.Set[metav1.TypeMeta], error) { |
There was a problem hiding this comment.
We should probably have a unit test for this function validating a scenario with an ownerKind with multiple / in it.
|
@nojnhuh Let me know if this is ready for review again! |
|
Un-assigning myself from this PR for now since I won't be able to review it again. :) |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
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 instatus.This change makes CAPZ record each ASO resource group/version/kind in a
sigs.k8s.io/cluster-api-provider-azure-owned-aso-kindsannotation 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:
Release note: