From 8e8f7175272ec8c37cb87333e671fdb9964a4e6e Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Sun, 3 Nov 2024 14:44:33 -0600 Subject: [PATCH 1/6] Acknowledge all ASO resources before creating --- .../azureasomanagedcluster_controller.go | 9 +- .../azureasomanagedcluster_controller_test.go | 14 +-- .../azureasomanagedcontrolplane_controller.go | 4 +- ...easomanagedcontrolplane_controller_test.go | 26 +++-- .../azureasomanagedmachinepool_controller.go | 4 +- ...reasomanagedmachinepool_controller_test.go | 12 +-- controllers/resource_reconciler.go | 100 ++++++++++++------ controllers/resource_reconciler_test.go | 45 ++++++-- 8 files changed, 142 insertions(+), 72 deletions(-) diff --git a/controllers/azureasomanagedcluster_controller.go b/controllers/azureasomanagedcluster_controller.go index a6ac8d650e7..79b44dc0412 100644 --- a/controllers/azureasomanagedcluster_controller.go +++ b/controllers/azureasomanagedcluster_controller.go @@ -56,8 +56,9 @@ type AzureASOManagedClusterReconciler struct { type resourceReconciler interface { // Reconcile reconciles resources defined by this object and updates this object's status to reflect the - // state of the specified resources. - Reconcile(context.Context) error + // state of the specified resources. It returns a boolean indicating if controllers need to immediately + // requeue the object and an error if one occurred. + Reconcile(context.Context) (bool, error) // Pause stops ASO from continuously reconciling the specified resources. Pause(context.Context) error @@ -240,13 +241,13 @@ func (r *AzureASOManagedClusterReconciler) reconcileNormal(ctx context.Context, return ctrl.Result{}, err } resourceReconciler := r.newResourceReconciler(asoManagedCluster, resources) - err = resourceReconciler.Reconcile(ctx) + needsRequeue, err := resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedCluster.Status.Resources { if !status.Ready { - return ctrl.Result{}, nil + return ctrl.Result{Requeue: needsRequeue}, nil } } diff --git a/controllers/azureasomanagedcluster_controller_test.go b/controllers/azureasomanagedcluster_controller_test.go index 03f34915720..22cc23dbac2 100644 --- a/controllers/azureasomanagedcluster_controller_test.go +++ b/controllers/azureasomanagedcluster_controller_test.go @@ -39,14 +39,14 @@ import ( type fakeResourceReconciler struct { owner client.Object - reconcileFunc func(context.Context, client.Object) error + reconcileFunc func(context.Context, client.Object) (bool, error) pauseFunc func(context.Context, client.Object) error deleteFunc func(context.Context, client.Object) error } -func (r *fakeResourceReconciler) Reconcile(ctx context.Context) error { +func (r *fakeResourceReconciler) Reconcile(ctx context.Context) (bool, error) { if r.reconcileFunc == nil { - return nil + return false, nil } return r.reconcileFunc(ctx, r.owner) } @@ -208,13 +208,13 @@ func TestAzureASOManagedClusterReconcile(t *testing.T) { newResourceReconciler: func(asoManagedCluster *infrav1alpha.AzureASOManagedCluster, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedCluster, - reconcileFunc: func(ctx context.Context, o client.Object) error { + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { asoManagedCluster.SetResourceStatuses([]infrav1alpha.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return nil + return false, nil }, } }, @@ -282,8 +282,8 @@ func TestAzureASOManagedClusterReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedCluster, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, diff --git a/controllers/azureasomanagedcontrolplane_controller.go b/controllers/azureasomanagedcontrolplane_controller.go index 5bd9f90dea0..abd684beac9 100644 --- a/controllers/azureasomanagedcontrolplane_controller.go +++ b/controllers/azureasomanagedcontrolplane_controller.go @@ -227,13 +227,13 @@ func (r *AzureASOManagedControlPlaneReconciler) reconcileNormal(ctx context.Cont } resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, resources) - err = resourceReconciler.Reconcile(ctx) + needsRequeue, err := resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedControlPlane.Status.Resources { if !status.Ready { - return ctrl.Result{}, nil + return ctrl.Result{Requeue: needsRequeue}, nil } } diff --git a/controllers/azureasomanagedcontrolplane_controller_test.go b/controllers/azureasomanagedcontrolplane_controller_test.go index 501227c51b4..0f30b30846e 100644 --- a/controllers/azureasomanagedcontrolplane_controller_test.go +++ b/controllers/azureasomanagedcontrolplane_controller_test.go @@ -203,13 +203,13 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { newResourceReconciler: func(asoManagedControlPlane *infrav1alpha.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedControlPlane, - reconcileFunc: func(ctx context.Context, o client.Object) error { + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { asoManagedControlPlane.SetResourceStatuses([]infrav1alpha.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return nil + return false, nil }, } }, @@ -317,8 +317,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, @@ -435,7 +435,10 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { Client: &FakeClient{ Client: c, patchFunc: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { - kubeconfigSecret := obj.(*corev1.Secret) + kubeconfigSecret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } g.Expect(kubeconfigSecret.Data[secret.KubeconfigDataName]).NotTo(BeEmpty()) kubeConfigPatched = true @@ -451,8 +454,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, @@ -571,7 +574,10 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { Client: &FakeClient{ Client: c, patchFunc: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { - kubeconfigSecret := obj.(*corev1.Secret) + kubeconfigSecret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } g.Expect(kubeconfigSecret.Data[secret.KubeconfigDataName]).NotTo(BeEmpty()) kubeConfigPatched = true @@ -587,8 +593,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, diff --git a/controllers/azureasomanagedmachinepool_controller.go b/controllers/azureasomanagedmachinepool_controller.go index 8a942a4d9a4..02e413b841e 100644 --- a/controllers/azureasomanagedmachinepool_controller.go +++ b/controllers/azureasomanagedmachinepool_controller.go @@ -232,13 +232,13 @@ func (r *AzureASOManagedMachinePoolReconciler) reconcileNormal(ctx context.Conte } resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, resources) - err = resourceReconciler.Reconcile(ctx) + needsRequeue, err := resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedMachinePool.Status.Resources { if !status.Ready { - return ctrl.Result{}, nil + return ctrl.Result{Requeue: needsRequeue}, nil } } diff --git a/controllers/azureasomanagedmachinepool_controller_test.go b/controllers/azureasomanagedmachinepool_controller_test.go index 18128ab036d..c916d6ebdee 100644 --- a/controllers/azureasomanagedmachinepool_controller_test.go +++ b/controllers/azureasomanagedmachinepool_controller_test.go @@ -266,13 +266,13 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { newResourceReconciler: func(asoManagedMachinePool *infrav1alpha.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedMachinePool, - reconcileFunc: func(ctx context.Context, o client.Object) error { + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { asoManagedMachinePool.SetResourceStatuses([]infrav1alpha.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return nil + return false, nil }, } }, @@ -374,8 +374,8 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, @@ -514,8 +514,8 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1alpha.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) error { - return nil + reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + return false, nil }, } }, diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index 36a29952e0c..f7e3151c644 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -58,36 +58,81 @@ type resourceStatusObject interface { } // Reconcile creates or updates the specified resources. -func (r *ResourceReconciler) Reconcile(ctx context.Context) error { +func (r *ResourceReconciler) Reconcile(ctx context.Context) (bool, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Reconcile") defer done() log.V(4).Info("reconciling resources") var newResourceStatuses []infrav1alpha.ResourceStatus - for _, spec := range r.resources { + // Newly-defined resources in the CAPZ spec are first recorded in the status without performing a patch + // that creates the resource. CAPZ only patches resources that have already been recorded in status to + // ensure no resources are orphaned. + needsRequeue := false + for _, resource := range r.resources { + hasStatus := false + for _, status := range r.owner.GetResourceStatuses() { + if statusRefersToResource(status, resource) { + hasStatus = true + break + } + } + if !hasStatus { + needsRequeue = true + gvk := resource.GroupVersionKind() + newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ + Resource: infrav1alpha.StatusResource{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: resource.GetName(), + }, + Ready: false, + }) + } + } + + for _, status := range r.owner.GetResourceStatuses() { + var spec *unstructured.Unstructured + for _, resource := range r.resources { + if statusRefersToResource(status, resource) { + spec = resource + break + } + } + if spec == nil { + updatedStatus, err := r.deleteResource(ctx, status.Resource) + if err != nil { + return false, err + } + if updatedStatus != nil { + newResourceStatuses = append(newResourceStatuses, *updatedStatus) + } + continue + } + gvk := spec.GroupVersionKind() spec.SetNamespace(r.owner.GetNamespace()) log := log.WithValues("resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { - return fmt.Errorf("failed to set owner reference: %w", err) + return false, fmt.Errorf("failed to set owner reference: %w", err) } if err := r.watcher.Watch(log, spec, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { - return fmt.Errorf("failed to watch resource: %w", err) + return false, fmt.Errorf("failed to watch resource: %w", err) } log.V(4).Info("applying resource") err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager"), client.ForceOwnership) if err != nil { - return fmt.Errorf("failed to apply resource: %w", err) + return false, fmt.Errorf("failed to apply resource: %w", err) } ready, err := readyStatus(ctx, spec) if err != nil { - return fmt.Errorf("failed to get ready status: %w", err) + return false, fmt.Errorf("failed to get ready status: %w", err) } newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ Resource: infrav1alpha.StatusResource{ @@ -100,31 +145,9 @@ func (r *ResourceReconciler) Reconcile(ctx context.Context) error { }) } - for _, oldStatus := range r.owner.GetResourceStatuses() { - needsDelete := true - for _, newStatus := range newResourceStatuses { - if oldStatus.Resource.Group == newStatus.Resource.Group && - oldStatus.Resource.Kind == newStatus.Resource.Kind && - oldStatus.Resource.Name == newStatus.Resource.Name { - needsDelete = false - break - } - } - - if needsDelete { - updatedStatus, err := r.deleteResource(ctx, oldStatus.Resource) - if err != nil { - return err - } - if updatedStatus != nil { - newResourceStatuses = append(newResourceStatuses, *updatedStatus) - } - } - } - r.owner.SetResourceStatuses(newResourceStatuses) - return nil + return needsRequeue, nil } // Pause pauses reconciliation of the specified resources. @@ -141,7 +164,17 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return err } - for _, spec := range r.resources { + for _, status := range r.owner.GetResourceStatuses() { + var spec *unstructured.Unstructured + for _, resource := range r.resources { + if statusRefersToResource(status, resource) { + spec = resource + break + } + } + if spec == nil { + continue + } gvk := spec.GroupVersionKind() spec.SetNamespace(r.owner.GetNamespace()) @@ -261,3 +294,10 @@ func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error // no ready condition is set return false, nil } + +func statusRefersToResource(status infrav1alpha.ResourceStatus, resource *unstructured.Unstructured) bool { + gvk := resource.GroupVersionKind() + return status.Resource.Group == gvk.Group && + status.Resource.Kind == gvk.Kind && + status.Resource.Name == resource.GetName() +} diff --git a/controllers/resource_reconciler_test.go b/controllers/resource_reconciler_test.go index 2b51f72aeaf..73735f6e1f2 100644 --- a/controllers/resource_reconciler_test.go +++ b/controllers/resource_reconciler_test.go @@ -87,8 +87,9 @@ func TestResourceReconcilerReconcile(t *testing.T) { resources: []*unstructured.Unstructured{}, owner: &infrav1alpha.AzureASOManagedCluster{}, } - - g.Expect(r.Reconcile(ctx)).To(Succeed()) + needsRequeue, err := r.Reconcile(ctx) + g.Expect(needsRequeue).To(BeFalse()) + g.Expect(err).NotTo(HaveOccurred()) }) t.Run("reconcile several resources", func(t *testing.T) { @@ -98,11 +99,16 @@ func TestResourceReconcilerReconcile(t *testing.T) { c := fakeClientBuilder(). Build() - asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{} + asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{ + Status: infrav1alpha.AzureASOManagedClusterStatus{ + Resources: []infrav1alpha.ResourceStatus{ + rgStatus("rg1"), + }, + }, + } unpatchedRGs := map[string]struct{}{ "rg1": {}, - "rg2": {}, } r := &ResourceReconciler{ Client: &FakeClient{ @@ -138,16 +144,18 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: w, } - g.Expect(r.Reconcile(ctx)).To(Succeed()) + needsRequeue, err := r.Reconcile(ctx) + g.Expect(needsRequeue).To(BeTrue()) + g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(HaveKey("ResourceGroup.resources.azure.com")) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched resourcesStatuses := asoManagedCluster.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(2)) - g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) - g.Expect(resourcesStatuses[0].Ready).To(BeTrue()) - g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) - g.Expect(resourcesStatuses[1].Ready).To(BeFalse()) + g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg2")) + g.Expect(resourcesStatuses[0].Ready).To(BeFalse()) + g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg1")) + g.Expect(resourcesStatuses[1].Ready).To(BeTrue()) }) t.Run("delete stale resources", func(t *testing.T) { @@ -219,7 +227,9 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: &FakeWatcher{}, } - g.Expect(r.Reconcile(ctx)).To(Succeed()) + needsRequeue, err := r.Reconcile(ctx) + g.Expect(needsRequeue).To(BeFalse()) + g.Expect(err).NotTo(HaveOccurred()) resourcesStatuses := owner.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(3)) @@ -263,7 +273,15 @@ func TestResourceReconcilerPause(t *testing.T) { c := fakeClientBuilder(). Build() - asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{} + asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{ + Status: infrav1alpha.AzureASOManagedClusterStatus{ + Resources: []infrav1alpha.ResourceStatus{ + rgStatus("rg1"), + rgStatus("rg2"), + rgStatus("deleted-from-spec"), + }, + }, + } var patchedRGs []string r := &ResourceReconciler{ @@ -286,6 +304,11 @@ func TestResourceReconcilerPause(t *testing.T) { Name: "rg2", }, }), + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "not-yet-created", + }, + }), }, owner: asoManagedCluster, } From ef071558666326f09f0321e8ea5a1da109948696 Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Sat, 9 Nov 2024 21:03:01 -0600 Subject: [PATCH 2/6] fixup! Acknowledge all ASO resources before creating --- .../azureasomanagedcluster_controller.go | 14 +- .../azureasomanagedcontrolplane_controller.go | 14 +- .../azureasomanagedmachinepool_controller.go | 8 +- controllers/resource_reconciler.go | 221 +++++++++--------- 4 files changed, 123 insertions(+), 134 deletions(-) diff --git a/controllers/azureasomanagedcluster_controller.go b/controllers/azureasomanagedcluster_controller.go index 79b44dc0412..a3b87a86aa8 100644 --- a/controllers/azureasomanagedcluster_controller.go +++ b/controllers/azureasomanagedcluster_controller.go @@ -295,19 +295,15 @@ func (r *AzureASOManagedClusterReconciler) reconcileDelete(ctx context.Context, defer done() log.V(4).Info("reconciling delete") - resources, err := mutators.ToUnstructured(ctx, asoManagedCluster.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedCluster, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedCluster, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } - if len(asoManagedCluster.Status.Resources) > 0 { - return ctrl.Result{}, nil + + if len(asoManagedCluster.Status.Resources) == 0 { + controllerutil.RemoveFinalizer(asoManagedCluster, clusterv1.ClusterFinalizer) } - controllerutil.RemoveFinalizer(asoManagedCluster, clusterv1.ClusterFinalizer) return ctrl.Result{}, nil } diff --git a/controllers/azureasomanagedcontrolplane_controller.go b/controllers/azureasomanagedcontrolplane_controller.go index abd684beac9..0cd5ddcf6e6 100644 --- a/controllers/azureasomanagedcontrolplane_controller.go +++ b/controllers/azureasomanagedcontrolplane_controller.go @@ -379,20 +379,16 @@ func (r *AzureASOManagedControlPlaneReconciler) reconcileDelete(ctx context.Cont defer done() log.V(4).Info("reconciling delete") - resources, err := mutators.ToUnstructured(ctx, asoManagedControlPlane.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } - if len(asoManagedControlPlane.Status.Resources) > 0 { - return ctrl.Result{}, nil + + if len(asoManagedControlPlane.Status.Resources) == 0 { + controllerutil.RemoveFinalizer(asoManagedControlPlane, infrav1alpha.AzureASOManagedControlPlaneFinalizer) } - controllerutil.RemoveFinalizer(asoManagedControlPlane, infrav1alpha.AzureASOManagedControlPlaneFinalizer) return ctrl.Result{}, nil } diff --git a/controllers/azureasomanagedmachinepool_controller.go b/controllers/azureasomanagedmachinepool_controller.go index 02e413b841e..a51606bab94 100644 --- a/controllers/azureasomanagedmachinepool_controller.go +++ b/controllers/azureasomanagedmachinepool_controller.go @@ -333,12 +333,8 @@ func (r *AzureASOManagedMachinePoolReconciler) reconcileDelete(ctx context.Conte // If the entire cluster is being deleted, this ASO ManagedClustersAgentPool will be deleted with the rest // of the ManagedCluster. if cluster.DeletionTimestamp.IsZero() { - resources, err := mutators.ToUnstructured(ctx, asoManagedMachinePool.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index f7e3151c644..5bbc15ee9ae 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -62,92 +62,20 @@ func (r *ResourceReconciler) Reconcile(ctx context.Context) (bool, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Reconcile") defer done() log.V(4).Info("reconciling resources") + return r.reconcile(ctx) +} - var newResourceStatuses []infrav1alpha.ResourceStatus - - // Newly-defined resources in the CAPZ spec are first recorded in the status without performing a patch - // that creates the resource. CAPZ only patches resources that have already been recorded in status to - // ensure no resources are orphaned. - needsRequeue := false - for _, resource := range r.resources { - hasStatus := false - for _, status := range r.owner.GetResourceStatuses() { - if statusRefersToResource(status, resource) { - hasStatus = true - break - } - } - if !hasStatus { - needsRequeue = true - gvk := resource.GroupVersionKind() - newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ - Resource: infrav1alpha.StatusResource{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - Name: resource.GetName(), - }, - Ready: false, - }) - } - } - - for _, status := range r.owner.GetResourceStatuses() { - var spec *unstructured.Unstructured - for _, resource := range r.resources { - if statusRefersToResource(status, resource) { - spec = resource - break - } - } - if spec == nil { - updatedStatus, err := r.deleteResource(ctx, status.Resource) - if err != nil { - return false, err - } - if updatedStatus != nil { - newResourceStatuses = append(newResourceStatuses, *updatedStatus) - } - continue - } - - gvk := spec.GroupVersionKind() - spec.SetNamespace(r.owner.GetNamespace()) - - log := log.WithValues("resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) - - if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { - return false, fmt.Errorf("failed to set owner reference: %w", err) - } - - if err := r.watcher.Watch(log, spec, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { - return false, fmt.Errorf("failed to watch resource: %w", err) - } - - log.V(4).Info("applying resource") - err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager"), client.ForceOwnership) - if err != nil { - return false, fmt.Errorf("failed to apply resource: %w", err) - } - - ready, err := readyStatus(ctx, spec) - if err != nil { - return false, fmt.Errorf("failed to get ready status: %w", err) - } - newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ - Resource: infrav1alpha.StatusResource{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - Name: spec.GetName(), - }, - Ready: ready, - }) - } - - r.owner.SetResourceStatuses(newResourceStatuses) +// Delete deletes the specified resources. +func (r *ResourceReconciler) Delete(ctx context.Context) error { + ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Delete") + defer done() + log.V(4).Info("deleting resources") - return needsRequeue, nil + // Delete is a special case of a normal reconciliation which is equivalent to all resources from spec + // being deleted. + r.resources = nil + _, err := r.reconcile(ctx) + return err } // Pause pauses reconciliation of the specified resources. @@ -164,23 +92,11 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return err } - for _, status := range r.owner.GetResourceStatuses() { - var spec *unstructured.Unstructured - for _, resource := range r.resources { - if statusRefersToResource(status, resource) { - spec = resource - break - } - } - if spec == nil { - continue - } - gvk := spec.GroupVersionKind() - spec.SetNamespace(r.owner.GetNamespace()) - - log := log.WithValues("resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) + _, observedResources, _ := partitionResources(r.resources, r.owner.GetResourceStatuses()) - log.V(4).Info("pausing resource") + for _, spec := range observedResources { + gvk := spec.GroupVersionKind() + log.V(4).Info("pausing resource", "resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager")) if err != nil { return fmt.Errorf("failed to patch resource: %w", err) @@ -190,27 +106,65 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return nil } -// Delete deletes the specified resources. -func (r *ResourceReconciler) Delete(ctx context.Context) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Delete") +func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { + ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.reconcile") defer done() - log.V(4).Info("deleting resources") var newResourceStatuses []infrav1alpha.ResourceStatus - for _, spec := range r.owner.GetResourceStatuses() { - newStatus, err := r.deleteResource(ctx, spec.Resource) + unobservedResources, observedResources, deletedResources := partitionResources(r.resources, r.owner.GetResourceStatuses()) + + // Newly-defined resources in the CAPZ spec are first recorded in the status without performing a patch + // that would create the resource. CAPZ only patches resources that have already been recorded in status + // to ensure no resources are orphaned. + for _, spec := range unobservedResources { + newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ + Resource: statusResource(spec), + Ready: false, + }) + } + + for _, spec := range observedResources { + spec.SetNamespace(r.owner.GetNamespace()) + + if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { + return false, fmt.Errorf("failed to set owner reference: %w", err) + } + + if err := r.watcher.Watch(log, spec, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { + return false, fmt.Errorf("failed to watch resource: %w", err) + } + + 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) + if err != nil { + return false, fmt.Errorf("failed to apply resource: %w", err) + } + + ready, err := readyStatus(ctx, spec) + if err != nil { + return false, fmt.Errorf("failed to get ready status: %w", err) + } + newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ + Resource: statusResource(spec), + Ready: ready, + }) + } + + for _, status := range deletedResources { + updatedStatus, err := r.deleteResource(ctx, status.Resource) if err != nil { - return err + return false, err } - if newStatus != nil { - newResourceStatuses = append(newResourceStatuses, *newStatus) + if updatedStatus != nil { + newResourceStatuses = append(newResourceStatuses, *updatedStatus) } } r.owner.SetResourceStatuses(newResourceStatuses) - return nil + return len(unobservedResources) > 0, nil } func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav1alpha.StatusResource) (*infrav1alpha.ResourceStatus, error) { @@ -295,8 +249,55 @@ func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error return false, nil } +// partitionResources splits sets of resources defined in spec and status into three groups: +// - unobservedResources exist in spec but not status. +// - observedResources exist in both spec and status. +// - deletedResources exist in status but not spec. +func partitionResources( + specs []*unstructured.Unstructured, + statuses []infrav1alpha.ResourceStatus, +) ( + unobservedResources []*unstructured.Unstructured, + observedResources []*unstructured.Unstructured, + deletedResources []infrav1alpha.ResourceStatus, +) { +specs: + for _, spec := range specs { + for _, status := range statuses { + if statusRefersToResource(status, spec) { + observedResources = append(observedResources, spec) + continue specs + } + } + unobservedResources = append(unobservedResources, spec) + } + +statuses: + for _, status := range statuses { + for _, resource := range specs { + if statusRefersToResource(status, resource) { + continue statuses + } + } + deletedResources = append(deletedResources, status) + } + return +} + +func statusResource(resource *unstructured.Unstructured) infrav1alpha.StatusResource { + gvk := resource.GroupVersionKind() + return infrav1alpha.StatusResource{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: resource.GetName(), + } +} + func statusRefersToResource(status infrav1alpha.ResourceStatus, resource *unstructured.Unstructured) bool { gvk := resource.GroupVersionKind() + // Version is not a stable property of a particular resource. The API version of an ASO resource may + // change in the CAPZ spec from v1 to v2 but that still represents the same underlying resource. return status.Resource.Group == gvk.Group && status.Resource.Kind == gvk.Kind && status.Resource.Name == resource.GetName() From 725ec6de6d9e640c11c432dd9601337650bede7d Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Thu, 24 Apr 2025 20:03:04 -0500 Subject: [PATCH 3/6] amend! Acknowledge all ASO resources before creating Acknowledge all ASO resource types before creating --- api/v1alpha1/azureasomanagedcluster_types.go | 5 - .../azureasomanagedcontrolplane_types.go | 5 - .../azureasomanagedmachinepool_types.go | 5 - controllers/resource_reconciler.go | 207 +++++++++----- controllers/resource_reconciler_test.go | 261 ++++++++++++++---- 5 files changed, 353 insertions(+), 130 deletions(-) diff --git a/api/v1alpha1/azureasomanagedcluster_types.go b/api/v1alpha1/azureasomanagedcluster_types.go index 026f4237a85..5f0e3ee68ed 100644 --- a/api/v1alpha1/azureasomanagedcluster_types.go +++ b/api/v1alpha1/azureasomanagedcluster_types.go @@ -78,11 +78,6 @@ type AzureASOManagedCluster struct { Status AzureASOManagedClusterStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedCluster) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedCluster) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/api/v1alpha1/azureasomanagedcontrolplane_types.go b/api/v1alpha1/azureasomanagedcontrolplane_types.go index 7ea30afa1e9..dabb30763b9 100644 --- a/api/v1alpha1/azureasomanagedcontrolplane_types.go +++ b/api/v1alpha1/azureasomanagedcontrolplane_types.go @@ -66,11 +66,6 @@ type AzureASOManagedControlPlane struct { Status AzureASOManagedControlPlaneStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedControlPlane) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedControlPlane) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/api/v1alpha1/azureasomanagedmachinepool_types.go b/api/v1alpha1/azureasomanagedmachinepool_types.go index f082e12da07..d3c9fb825bb 100644 --- a/api/v1alpha1/azureasomanagedmachinepool_types.go +++ b/api/v1alpha1/azureasomanagedmachinepool_types.go @@ -61,11 +61,6 @@ type AzureASOManagedMachinePool struct { Status AzureASOManagedMachinePoolStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedMachinePool) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedMachinePool) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index 5bbc15ee9ae..3424174e77a 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -20,13 +20,16 @@ import ( "context" "errors" "fmt" + "slices" + "strings" "github.com/Azure/azure-service-operator/v2/pkg/genruntime/conditions" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -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" + // ResourceReconciler reconciles a set of arbitrary ASO resources. type ResourceReconciler struct { client.Client @@ -53,7 +58,6 @@ type watcher interface { type resourceStatusObject interface { client.Object - GetResourceStatuses() []infrav1alpha.ResourceStatus SetResourceStatuses([]infrav1alpha.ResourceStatus) } @@ -92,13 +96,12 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return err } - _, observedResources, _ := partitionResources(r.resources, r.owner.GetResourceStatuses()) - - for _, spec := range observedResources { + for _, spec := range r.resources { + spec.SetNamespace(r.owner.GetNamespace()) gvk := spec.GroupVersionKind() log.V(4).Info("pausing resource", "resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager")) - if err != nil { + if client.IgnoreNotFound(err) != nil { return fmt.Errorf("failed to patch resource: %w", err) } } @@ -112,26 +115,40 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { var newResourceStatuses []infrav1alpha.ResourceStatus - unobservedResources, observedResources, deletedResources := partitionResources(r.resources, r.owner.GetResourceStatuses()) + ownedKindsValue := r.owner.GetAnnotations()[ownedKindsAnnotation] + ownedKinds, err := parseOwnedKinds(ownedKindsValue) + if err != nil { + return false, fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue) + } + + ownedObjs, err := r.ownedObjs(ctx, ownedKinds) + if err != nil { + return false, fmt.Errorf("failed to get owned objects: %w", err) + } + + unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) - // Newly-defined resources in the CAPZ spec are first recorded in the status without performing a patch - // that would create the resource. CAPZ only patches resources that have already been recorded in status - // to ensure no resources are orphaned. - for _, spec := range unobservedResources { + // Newly-defined types in the CAPZ spec are first recorded in the annotation without performing a + // patch that would create resources of that type. CAPZ only patches resources whose kinds have + // already been recorded to ensure no resources are orphaned. + for _, spec := range unobservedTypeResources { newResourceStatuses = append(newResourceStatuses, infrav1alpha.ResourceStatus{ Resource: statusResource(spec), Ready: false, }) } - for _, spec := range observedResources { + for _, spec := range observedTypeResources { spec.SetNamespace(r.owner.GetNamespace()) if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { return false, fmt.Errorf("failed to set owner reference: %w", err) } - if err := r.watcher.Watch(log, spec, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { + toWatch := meta.AsPartialObjectMetadata(spec) + toWatch.APIVersion = spec.GetAPIVersion() + toWatch.Kind = spec.GetKind() + if err := r.watcher.Watch(log, toWatch, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { return false, fmt.Errorf("failed to watch resource: %w", err) } @@ -152,34 +169,49 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { }) } - for _, status := range deletedResources { - updatedStatus, err := r.deleteResource(ctx, status.Resource) + for _, obj := range deletedResources { + newStatus, err := r.deleteResource(ctx, obj) if err != nil { - return false, err + return false, fmt.Errorf("failed to delete %s %s/%s", obj.GroupVersionKind(), obj.Namespace, obj.Name) } - if updatedStatus != nil { - newResourceStatuses = append(newResourceStatuses, *updatedStatus) + if newStatus != nil { + newResourceStatuses = append(newResourceStatuses, *newStatus) } } + newOwnedKinds := []metav1.TypeMeta{} + for _, status := range newResourceStatuses { + typeMeta := metav1.TypeMeta{ + APIVersion: metav1.GroupVersion{Group: status.Resource.Group, Version: status.Resource.Version}.String(), + Kind: status.Resource.Kind, + } + if !slices.Contains(newOwnedKinds, typeMeta) { + newOwnedKinds = append(newOwnedKinds, typeMeta) + } + } + annotations := r.owner.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + annotations[ownedKindsAnnotation] = getOwnedKindsValue(newOwnedKinds) + if annotations[ownedKindsAnnotation] == "" { + delete(annotations, ownedKindsAnnotation) + } + r.owner.SetAnnotations(annotations) + r.owner.SetResourceStatuses(newResourceStatuses) - return len(unobservedResources) > 0, nil + return len(unobservedTypeResources) > 0, nil } -func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav1alpha.StatusResource) (*infrav1alpha.ResourceStatus, error) { +func (r *ResourceReconciler) deleteResource(ctx context.Context, resource *metav1.PartialObjectMetadata) (*infrav1alpha.ResourceStatus, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.deleteResource") defer done() - spec := &unstructured.Unstructured{} - spec.SetGroupVersionKind(schema.GroupVersionKind{Group: resource.Group, Version: resource.Version, Kind: resource.Kind}) - spec.SetNamespace(r.owner.GetNamespace()) - spec.SetName(resource.Name) - - log = log.WithValues("resource", klog.KObj(spec), "resourceVersion", spec.GroupVersionKind().GroupVersion(), "resourceKind", spec.GetKind()) + log = log.WithValues("resource", klog.KObj(resource), "resourceVersion", resource.APIVersion, "resourceKind", resource.Kind) log.V(4).Info("deleting resource") - err := r.Client.Delete(ctx, spec) + err := r.Client.Delete(ctx, resource) if apierrors.IsNotFound(err) { log.V(4).Info("resource has been deleted") return nil, nil @@ -188,7 +220,7 @@ func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav return nil, fmt.Errorf("failed to delete resource: %w", err) } - err = r.Client.Get(ctx, client.ObjectKeyFromObject(spec), spec) + err = r.Client.Get(ctx, client.ObjectKeyFromObject(resource), resource) if apierrors.IsNotFound(err) { log.V(4).Info("resource has been deleted") return nil, nil @@ -196,17 +228,40 @@ func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav if err != nil { return nil, fmt.Errorf("failed to get resource: %w", err) } - ready, err := readyStatus(ctx, spec) - if err != nil { - return nil, fmt.Errorf("failed to get ready status: %w", err) - } + gvk := resource.GroupVersionKind() return &infrav1alpha.ResourceStatus{ - Resource: resource, - Ready: ready, + Resource: infrav1alpha.StatusResource{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: resource.Name, + }, + Ready: false, }, nil } +func (r *ResourceReconciler) ownedObjs(ctx context.Context, ownedTypes sets.Set[metav1.TypeMeta]) ([]*metav1.PartialObjectMetadata, error) { + var ownedObjs []*metav1.PartialObjectMetadata + + for typeMeta := range ownedTypes { + objs := &metav1.PartialObjectMetadataList{TypeMeta: typeMeta} + err := r.List(ctx, objs, client.InNamespace(r.owner.GetNamespace())) + if err != nil { + return nil, fmt.Errorf("failed to list %s %s: %w", typeMeta.APIVersion, typeMeta.Kind, err) + } + + for _, obj := range objs.Items { + controller := metav1.GetControllerOfNoCopy(&obj) + if controller != nil && controller.UID == r.owner.GetUID() { + ownedObjs = append(ownedObjs, &obj) + } + } + } + + return ownedObjs, nil +} + func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error) { _, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.readyStatus") defer done() @@ -249,37 +304,36 @@ func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error return false, nil } -// partitionResources splits sets of resources defined in spec and status into three groups: -// - unobservedResources exist in spec but not status. -// - observedResources exist in both spec and status. -// - deletedResources exist in status but not spec. +// 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. func partitionResources( + ownedKinds sets.Set[metav1.TypeMeta], specs []*unstructured.Unstructured, - statuses []infrav1alpha.ResourceStatus, + ownedObjs []*metav1.PartialObjectMetadata, ) ( - unobservedResources []*unstructured.Unstructured, - observedResources []*unstructured.Unstructured, - deletedResources []infrav1alpha.ResourceStatus, + unobservedTypeResources []*unstructured.Unstructured, + observedTypeResources []*unstructured.Unstructured, + deletedResources []*metav1.PartialObjectMetadata, ) { -specs: for _, spec := range specs { - for _, status := range statuses { - if statusRefersToResource(status, spec) { - observedResources = append(observedResources, spec) - continue specs - } + typeMeta := metav1.TypeMeta{ + APIVersion: spec.GetAPIVersion(), + Kind: spec.GetKind(), + } + if ownedKinds.Has(typeMeta) { + observedTypeResources = append(observedTypeResources, spec) + } else { + unobservedTypeResources = append(unobservedTypeResources, spec) } - unobservedResources = append(unobservedResources, spec) } -statuses: - for _, status := range statuses { - for _, resource := range specs { - if statusRefersToResource(status, resource) { - continue statuses - } + for _, owned := range ownedObjs { + if !slices.ContainsFunc(specs, metadataRefersToResource(owned)) { + deletedResources = append(deletedResources, owned) } - deletedResources = append(deletedResources, status) } return } @@ -294,11 +348,38 @@ func statusResource(resource *unstructured.Unstructured) infrav1alpha.StatusReso } } -func statusRefersToResource(status infrav1alpha.ResourceStatus, resource *unstructured.Unstructured) bool { - gvk := resource.GroupVersionKind() - // Version is not a stable property of a particular resource. The API version of an ASO resource may - // change in the CAPZ spec from v1 to v2 but that still represents the same underlying resource. - return status.Resource.Group == gvk.Group && - status.Resource.Kind == gvk.Kind && - status.Resource.Name == resource.GetName() +func metadataRefersToResource(metadata *metav1.PartialObjectMetadata) func(*unstructured.Unstructured) bool { + return func(u *unstructured.Unstructured) bool { + // Version is not a stable property of a particular resource. The API version of an ASO resource may + // change in the CAPZ spec from v1 to v2 but that still represents the same underlying resource. + return metadata.GroupVersionKind().GroupKind() == u.GroupVersionKind().GroupKind() && + metadata.Name == u.GetName() + } +} + +func parseOwnedKinds(value string) (sets.Set[metav1.TypeMeta], error) { + ownedKinds := sets.Set[metav1.TypeMeta]{} + if value == "" { + return nil, nil + } + for _, ownedKind := range strings.Split(value, ";") { + i := strings.LastIndexAny(ownedKind, "/") + if i < 0 { + return nil, fmt.Errorf("failed to parse field: %s", ownedKind) + } + apiVersion, kind := ownedKind[:i], ownedKind[i+1:] + ownedKinds.Insert(metav1.TypeMeta{ + APIVersion: apiVersion, + Kind: kind, + }) + } + return ownedKinds, nil +} + +func getOwnedKindsValue(ownedKinds []metav1.TypeMeta) string { + fields := make([]string, 0, len(ownedKinds)) + for _, kind := range ownedKinds { + fields = append(fields, kind.APIVersion+"/"+kind.Kind) + } + return strings.Join(fields, ";") } diff --git a/controllers/resource_reconciler_test.go b/controllers/resource_reconciler_test.go index 73735f6e1f2..b28205d25ea 100644 --- a/controllers/resource_reconciler_test.go +++ b/controllers/resource_reconciler_test.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -92,23 +93,74 @@ func TestResourceReconcilerReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) }) - t.Run("reconcile several resources", func(t *testing.T) { + t.Run("acknowledge new types", func(t *testing.T) { g := NewGomegaWithT(t) w := &FakeWatcher{} c := fakeClientBuilder(). Build() + asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{} + + unpatchedRGs := map[string]struct{}{} + r := &ResourceReconciler{ + Client: &FakeClient{ + Client: c, + patchFunc: func(ctx context.Context, o client.Object, p client.Patch, po ...client.PatchOption) error { + g.Expect(unpatchedRGs).To(HaveKey(o.GetName())) + delete(unpatchedRGs, o.GetName()) + return nil + }, + }, + resources: []*unstructured.Unstructured{ + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rg1", + }, + }), + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rg2", + }, + }), + }, + owner: asoManagedCluster, + watcher: w, + } + + needsRequeue, err := r.Reconcile(ctx) + g.Expect(needsRequeue).To(BeTrue()) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(w.watching).To(BeEmpty()) + g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + + resourcesStatuses := asoManagedCluster.Status.Resources + g.Expect(resourcesStatuses).To(HaveLen(2)) + g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) + g.Expect(resourcesStatuses[0].Ready).To(BeFalse()) + g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) + g.Expect(resourcesStatuses[1].Ready).To(BeFalse()) + }) + + t.Run("create resources with acknowledged types", func(t *testing.T) { + g := NewGomegaWithT(t) + asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{ - Status: infrav1alpha.AzureASOManagedClusterStatus{ - Resources: []infrav1alpha.ResourceStatus{ - rgStatus("rg1"), + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), }, }, } + w := &FakeWatcher{} + c := fakeClientBuilder(). + Build() + unpatchedRGs := map[string]struct{}{ "rg1": {}, + "rg2": {}, } r := &ResourceReconciler{ Client: &FakeClient{ @@ -138,6 +190,14 @@ func TestResourceReconcilerReconcile(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "rg2", }, + Status: asoresourcesv1.ResourceGroup_STATUS{ + Conditions: []conditions.Condition{ + { + Type: conditions.ConditionTypeReady, + Status: metav1.ConditionFalse, + }, + }, + }, }), }, owner: asoManagedCluster, @@ -145,57 +205,69 @@ func TestResourceReconcilerReconcile(t *testing.T) { } needsRequeue, err := r.Reconcile(ctx) - g.Expect(needsRequeue).To(BeTrue()) + g.Expect(needsRequeue).To(BeFalse()) g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(HaveKey("ResourceGroup.resources.azure.com")) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) resourcesStatuses := asoManagedCluster.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(2)) - g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg2")) - g.Expect(resourcesStatuses[0].Ready).To(BeFalse()) - g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg1")) - g.Expect(resourcesStatuses[1].Ready).To(BeTrue()) + g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) + g.Expect(resourcesStatuses[0].Ready).To(BeTrue()) + g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) + g.Expect(resourcesStatuses[1].Ready).To(BeFalse()) }) t.Run("delete stale resources", func(t *testing.T) { g := NewGomegaWithT(t) owner := &infrav1alpha.AzureASOManagedCluster{ - Status: infrav1alpha.AzureASOManagedClusterStatus{ - Resources: []infrav1alpha.ResourceStatus{ - rgStatus("rg0"), - rgStatus("rg1"), - rgStatus("rg2"), - rgStatus("rg3"), + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), }, }, } + ownerGVK, err := apiutil.GVKForObject(owner, s) + g.Expect(err).NotTo(HaveOccurred()) + controlledByOwner := []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + } objs := []client.Object{ &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg0", - Namespace: owner.Namespace, + Name: "rg0", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg1", - Namespace: owner.Namespace, + Name: "rg1", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg2", - Namespace: owner.Namespace, + Name: "rg2", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg3", - Namespace: owner.Namespace, - Finalizers: []string{"still deleting"}, + Name: "rg3", + Namespace: owner.Namespace, + OwnerReferences: controlledByOwner, + Finalizers: []string{"still deleting"}, }, }, } @@ -230,6 +302,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { needsRequeue, err := r.Reconcile(ctx) g.Expect(needsRequeue).To(BeFalse()) g.Expect(err).NotTo(HaveOccurred()) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) resourcesStatuses := owner.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(3)) @@ -237,6 +310,20 @@ func TestResourceReconcilerReconcile(t *testing.T) { g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) g.Expect(resourcesStatuses[2].Resource.Name).To(Equal("rg3")) + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg0"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "err is not a NotFound error") + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg1"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(err).NotTo(HaveOccurred()) + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg2"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(err).NotTo(HaveOccurred()) + + rg3 := &asoresourcesv1.ResourceGroup{} + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg3"}, rg3) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rg3.GetDeletionTimestamp().IsZero()).To(BeFalse(), "resource does not have expected deletion timestamp") }) } @@ -270,25 +357,57 @@ func TestResourceReconcilerPause(t *testing.T) { t.Run("pause several resources", func(t *testing.T) { g := NewGomegaWithT(t) - c := fakeClientBuilder(). - Build() + owner := &infrav1alpha.AzureASOManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + }, + }, + } - asoManagedCluster := &infrav1alpha.AzureASOManagedCluster{ - Status: infrav1alpha.AzureASOManagedClusterStatus{ - Resources: []infrav1alpha.ResourceStatus{ - rgStatus("rg1"), - rgStatus("rg2"), - rgStatus("deleted-from-spec"), + objs := []client.Object{ + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "rg1", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "rg2", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "deleted from spec", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + Finalizers: []string{"still deleting"}, }, }, } + c := fakeClientBuilder(). + WithObjects(objs...). + Build() + var patchedRGs []string r := &ResourceReconciler{ Client: &FakeClient{ Client: c, patchFunc: func(ctx context.Context, o client.Object, p client.Patch, po ...client.PatchOption) error { g.Expect(o.GetAnnotations()).To(HaveKeyWithValue(annotations.ReconcilePolicy, string(annotations.ReconcilePolicySkip))) + if err := c.Get(ctx, client.ObjectKeyFromObject(o), &asoresourcesv1.ResourceGroup{}); err != nil { + // propagate errors like "NotFound" + return err + } patchedRGs = append(patchedRGs, o.GetName()) return nil }, @@ -310,7 +429,7 @@ func TestResourceReconcilerPause(t *testing.T) { }, }), }, - owner: asoManagedCluster, + owner: owner, } g.Expect(r.Pause(ctx)).To(Succeed()) @@ -351,25 +470,32 @@ func TestResourceReconcilerDelete(t *testing.T) { owner := &infrav1alpha.AzureASOManagedCluster{ ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", - }, - Status: infrav1alpha.AzureASOManagedClusterStatus{ - Resources: []infrav1alpha.ResourceStatus{ - rgStatus("still-deleting"), - rgStatus("already-gone"), + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), }, }, } objs := []client.Object{ &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "still-deleting", - Namespace: owner.Namespace, + Name: "still-deleting", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), Finalizers: []string{ "ASO finalizer", }, }, }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "just-deleted", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, } c := fakeClientBuilder(). @@ -384,7 +510,8 @@ func TestResourceReconcilerDelete(t *testing.T) { } g.Expect(r.Delete(ctx)).To(Succeed()) - g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "already-gone"}, &asoresourcesv1.ResourceGroup{}))).To(BeTrue()) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "just-deleted"}, &asoresourcesv1.ResourceGroup{}))).To(BeTrue()) stillDeleting := &asoresourcesv1.ResourceGroup{} g.Expect(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "still-deleting"}, stillDeleting)).To(Succeed()) g.Expect(stillDeleting.GetDeletionTimestamp().IsZero()).To(BeFalse()) @@ -393,6 +520,34 @@ func TestResourceReconcilerDelete(t *testing.T) { g.Expect(owner.Status.Resources[0].Resource.Name).To(Equal("still-deleting")) g.Expect(owner.Status.Resources[0].Ready).To(BeFalse()) }) + + t.Run("done deleting", func(t *testing.T) { + g := NewGomegaWithT(t) + + owner := &infrav1alpha.AzureASOManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + }, + }, + } + + c := fakeClientBuilder(). + Build() + + r := &ResourceReconciler{ + Client: &FakeClient{ + Client: c, + }, + owner: owner, + } + + g.Expect(r.Delete(ctx)).To(Succeed()) + + g.Expect(owner.Annotations).NotTo(HaveKey(ownedKindsAnnotation)) + g.Expect(owner.Status.Resources).To(BeEmpty()) + }) } func TestReadyStatus(t *testing.T) { @@ -598,20 +753,22 @@ func TestReadyStatus(t *testing.T) { }) } +func controlledBy(g *GomegaWithT, s *runtime.Scheme, owner client.Object) []metav1.OwnerReference { + ownerGVK, err := apiutil.GVKForObject(owner, s) + g.Expect(err).NotTo(HaveOccurred()) + return []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + } +} + +var rgTypeMeta = metav1.TypeMeta{ + APIVersion: asoresourcesv1.GroupVersion.Identifier(), + Kind: "ResourceGroup", +} + func rgJSON(g Gomega, scheme *runtime.Scheme, rg *asoresourcesv1.ResourceGroup) *unstructured.Unstructured { rg.SetGroupVersionKind(asoresourcesv1.GroupVersion.WithKind("ResourceGroup")) u := &unstructured.Unstructured{} g.Expect(scheme.Convert(rg, u, nil)).To(Succeed()) return u } - -func rgStatus(name string) infrav1alpha.ResourceStatus { - return infrav1alpha.ResourceStatus{ - Resource: infrav1alpha.StatusResource{ - Group: asoresourcesv1.GroupVersion.Group, - Version: asoresourcesv1.GroupVersion.Version, - Kind: "ResourceGroup", - Name: name, - }, - } -} From 661caf1b450881d43e1d7fe4b360dd6fc904cc73 Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Tue, 24 Jun 2025 14:16:15 -0500 Subject: [PATCH 4/6] fixup! Acknowledge all ASO resources before creating rename variables --- controllers/resource_reconciler.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index f025587a206..516ddf8e331 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -126,19 +126,19 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { return false, fmt.Errorf("failed to get owned objects: %w", err) } - unobservedTypeResources, observedTypeResources, deletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) + unrecordedTypeResources, recordedTypeResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) // Newly-defined types in the CAPZ spec are first recorded in the annotation without performing a // patch that would create resources of that type. CAPZ only patches resources whose kinds have // already been recorded to ensure no resources are orphaned. - for _, spec := range unobservedTypeResources { + for _, spec := range unrecordedTypeResources { newResourceStatuses = append(newResourceStatuses, infrav1.ResourceStatus{ Resource: statusResource(spec), Ready: false, }) } - for _, spec := range observedTypeResources { + for _, spec := range recordedTypeResources { spec.SetNamespace(r.owner.GetNamespace()) if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { @@ -169,7 +169,7 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { }) } - for _, obj := range deletedResources { + for _, obj := range toBeDeletedResources { newStatus, err := r.deleteResource(ctx, obj) if err != nil { return false, fmt.Errorf("failed to delete %s %s/%s", obj.GroupVersionKind(), obj.Namespace, obj.Name) @@ -201,7 +201,7 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { r.owner.SetResourceStatuses(newResourceStatuses) - return len(unobservedTypeResources) > 0, nil + return len(unrecordedTypeResources) > 0, nil } func (r *ResourceReconciler) deleteResource(ctx context.Context, resource *metav1.PartialObjectMetadata) (*infrav1.ResourceStatus, error) { @@ -306,17 +306,17 @@ func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error // 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. +// - unrecordedTypeResources are of a type not yet known to this owning CAPZ resource. +// - recordedTypeResources are of a type already known to this owning CAPZ resource. +// - toBeDeletedResources exist but are not defined in spec. func partitionResources( ownedKinds sets.Set[metav1.TypeMeta], specs []*unstructured.Unstructured, ownedObjs []*metav1.PartialObjectMetadata, ) ( - unobservedTypeResources []*unstructured.Unstructured, - observedTypeResources []*unstructured.Unstructured, - deletedResources []*metav1.PartialObjectMetadata, + unrecordedTypeResources []*unstructured.Unstructured, + recordedTypeResources []*unstructured.Unstructured, + toBeDeletedResources []*metav1.PartialObjectMetadata, ) { for _, spec := range specs { typeMeta := metav1.TypeMeta{ @@ -324,15 +324,15 @@ func partitionResources( Kind: spec.GetKind(), } if ownedKinds.Has(typeMeta) { - observedTypeResources = append(observedTypeResources, spec) + recordedTypeResources = append(recordedTypeResources, spec) } else { - unobservedTypeResources = append(unobservedTypeResources, spec) + unrecordedTypeResources = append(unrecordedTypeResources, spec) } } for _, owned := range ownedObjs { if !slices.ContainsFunc(specs, metadataRefersToResource(owned)) { - deletedResources = append(deletedResources, owned) + toBeDeletedResources = append(toBeDeletedResources, owned) } } return From 759025a172bfa5cf8ea9da59214e9b6ed4abf28d Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Tue, 24 Jun 2025 14:23:45 -0500 Subject: [PATCH 5/6] fixup! Acknowledge all ASO resources before creating tweak annotation format --- controllers/resource_reconciler.go | 40 ++++++++++++++----------- controllers/resource_reconciler_test.go | 19 ++++++------ 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index 516ddf8e331..f96993a6cb4 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -42,7 +43,10 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) -const ownedKindsAnnotation = "sigs.k8s.io/cluster-api-provider-azure-owned-aso-kinds" +const ( + ownedKindsAnnotation = "sigs.k8s.io/cluster-api-provider-azure-owned-aso-kinds" + ownedKindsSep = ";" +) // ResourceReconciler reconciles a set of arbitrary ASO resources. type ResourceReconciler struct { @@ -179,14 +183,15 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { } } - newOwnedKinds := []metav1.TypeMeta{} + newOwnedKinds := []schema.GroupVersionKind{} for _, status := range newResourceStatuses { - typeMeta := metav1.TypeMeta{ - APIVersion: metav1.GroupVersion{Group: status.Resource.Group, Version: status.Resource.Version}.String(), - Kind: status.Resource.Kind, + gvk := schema.GroupVersionKind{ + Group: status.Resource.Group, + Version: status.Resource.Version, + Kind: status.Resource.Kind, } - if !slices.Contains(newOwnedKinds, typeMeta) { - newOwnedKinds = append(newOwnedKinds, typeMeta) + if !slices.Contains(newOwnedKinds, gvk) { + newOwnedKinds = append(newOwnedKinds, gvk) } } annotations := r.owner.GetAnnotations() @@ -362,24 +367,23 @@ func parseOwnedKinds(value string) (sets.Set[metav1.TypeMeta], error) { if value == "" { return nil, nil } - for _, ownedKind := range strings.Split(value, ";") { - i := strings.LastIndexAny(ownedKind, "/") - if i < 0 { - return nil, fmt.Errorf("failed to parse field: %s", ownedKind) + for _, ownedKind := range strings.Split(value, ownedKindsSep) { + gvk, _ := schema.ParseKindArg(ownedKind) + if gvk == nil { + return nil, fmt.Errorf("invalid field %q: expected Kind.version.group", ownedKind) } - apiVersion, kind := ownedKind[:i], ownedKind[i+1:] ownedKinds.Insert(metav1.TypeMeta{ - APIVersion: apiVersion, - Kind: kind, + APIVersion: gvk.GroupVersion().Identifier(), + Kind: gvk.Kind, }) } return ownedKinds, nil } -func getOwnedKindsValue(ownedKinds []metav1.TypeMeta) string { +func getOwnedKindsValue(ownedKinds []schema.GroupVersionKind) string { fields := make([]string, 0, len(ownedKinds)) - for _, kind := range ownedKinds { - fields = append(fields, kind.APIVersion+"/"+kind.Kind) + for _, gvk := range ownedKinds { + fields = append(fields, strings.Join([]string{gvk.Kind, gvk.Version, gvk.Group}, ".")) } - return strings.Join(fields, ";") + return strings.Join(fields, ownedKindsSep) } diff --git a/controllers/resource_reconciler_test.go b/controllers/resource_reconciler_test.go index 68b2b8e68cb..621d4ec3605 100644 --- a/controllers/resource_reconciler_test.go +++ b/controllers/resource_reconciler_test.go @@ -29,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -133,7 +134,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(BeEmpty()) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched - g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) resourcesStatuses := asoManagedCluster.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(2)) @@ -149,7 +150,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { asoManagedCluster := &infrav1.AzureASOManagedCluster{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } @@ -209,7 +210,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(HaveKey("ResourceGroup.resources.azure.com")) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched - g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) resourcesStatuses := asoManagedCluster.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(2)) @@ -226,7 +227,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, Annotations: map[string]string{ - ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } @@ -302,7 +303,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { needsRequeue, err := r.Reconcile(ctx) g.Expect(needsRequeue).To(BeFalse()) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) resourcesStatuses := owner.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(3)) @@ -361,7 +362,7 @@ func TestResourceReconcilerPause(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, Annotations: map[string]string{ - ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } @@ -471,7 +472,7 @@ func TestResourceReconcilerDelete(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", Annotations: map[string]string{ - ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } @@ -510,7 +511,7 @@ func TestResourceReconcilerDelete(t *testing.T) { } g.Expect(r.Delete(ctx)).To(Succeed()) - g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}))) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "just-deleted"}, &asoresourcesv1.ResourceGroup{}))).To(BeTrue()) stillDeleting := &asoresourcesv1.ResourceGroup{} g.Expect(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "still-deleting"}, stillDeleting)).To(Succeed()) @@ -528,7 +529,7 @@ func TestResourceReconcilerDelete(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", Annotations: map[string]string{ - ownedKindsAnnotation: getOwnedKindsValue([]metav1.TypeMeta{{APIVersion: asoresourcesv1.GroupVersion.Identifier(), Kind: "ResourceGroup"}}), + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } From 17a12805bf4e0e0399b0e346bc35e52a4f1095a6 Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Tue, 24 Jun 2025 16:39:07 -0500 Subject: [PATCH 6/6] fixup! Acknowledge all ASO resources before creating don't explicitly requeue --- .../azureasomanagedcluster_controller.go | 9 ++++---- .../azureasomanagedcluster_controller_test.go | 14 +++++------ .../azureasomanagedcontrolplane_controller.go | 4 ++-- ...easomanagedcontrolplane_controller_test.go | 16 ++++++------- .../azureasomanagedmachinepool_controller.go | 4 ++-- ...reasomanagedmachinepool_controller_test.go | 12 +++++----- controllers/resource_reconciler.go | 23 +++++++++---------- controllers/resource_reconciler_test.go | 12 ++++------ 8 files changed, 44 insertions(+), 50 deletions(-) diff --git a/controllers/azureasomanagedcluster_controller.go b/controllers/azureasomanagedcluster_controller.go index 25c856bcc09..e7eb41f7807 100644 --- a/controllers/azureasomanagedcluster_controller.go +++ b/controllers/azureasomanagedcluster_controller.go @@ -57,9 +57,8 @@ type AzureASOManagedClusterReconciler struct { type resourceReconciler interface { // Reconcile reconciles resources defined by this object and updates this object's status to reflect the - // state of the specified resources. It returns a boolean indicating if controllers need to immediately - // requeue the object and an error if one occurred. - Reconcile(context.Context) (bool, error) + // state of the specified resources. + Reconcile(context.Context) error // Pause stops ASO from continuously reconciling the specified resources. Pause(context.Context) error @@ -247,13 +246,13 @@ func (r *AzureASOManagedClusterReconciler) reconcileNormal(ctx context.Context, return ctrl.Result{}, err } resourceReconciler := r.newResourceReconciler(asoManagedCluster, resources) - needsRequeue, err := resourceReconciler.Reconcile(ctx) + err = resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedCluster.Status.Resources { if !status.Ready { - return ctrl.Result{Requeue: needsRequeue}, nil + return ctrl.Result{}, nil } } diff --git a/controllers/azureasomanagedcluster_controller_test.go b/controllers/azureasomanagedcluster_controller_test.go index 8332a186f39..fa82a4de3fb 100644 --- a/controllers/azureasomanagedcluster_controller_test.go +++ b/controllers/azureasomanagedcluster_controller_test.go @@ -39,14 +39,14 @@ import ( type fakeResourceReconciler struct { owner client.Object - reconcileFunc func(context.Context, client.Object) (bool, error) + reconcileFunc func(context.Context, client.Object) error pauseFunc func(context.Context, client.Object) error deleteFunc func(context.Context, client.Object) error } -func (r *fakeResourceReconciler) Reconcile(ctx context.Context) (bool, error) { +func (r *fakeResourceReconciler) Reconcile(ctx context.Context) error { if r.reconcileFunc == nil { - return false, nil + return nil } return r.reconcileFunc(ctx, r.owner) } @@ -208,13 +208,13 @@ func TestAzureASOManagedClusterReconcile(t *testing.T) { newResourceReconciler: func(asoManagedCluster *infrav1.AzureASOManagedCluster, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedCluster, - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + reconcileFunc: func(ctx context.Context, o client.Object) error { asoManagedCluster.SetResourceStatuses([]infrav1.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return false, nil + return nil }, } }, @@ -282,8 +282,8 @@ func TestAzureASOManagedClusterReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1.AzureASOManagedCluster, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, diff --git a/controllers/azureasomanagedcontrolplane_controller.go b/controllers/azureasomanagedcontrolplane_controller.go index c04869031d2..25b8843e303 100644 --- a/controllers/azureasomanagedcontrolplane_controller.go +++ b/controllers/azureasomanagedcontrolplane_controller.go @@ -227,13 +227,13 @@ func (r *AzureASOManagedControlPlaneReconciler) reconcileNormal(ctx context.Cont } resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, resources) - needsRequeue, err := resourceReconciler.Reconcile(ctx) + err = resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedControlPlane.Status.Resources { if !status.Ready { - return ctrl.Result{Requeue: needsRequeue}, nil + return ctrl.Result{}, nil } } diff --git a/controllers/azureasomanagedcontrolplane_controller_test.go b/controllers/azureasomanagedcontrolplane_controller_test.go index cd9e45e0b08..4d3e14a7118 100644 --- a/controllers/azureasomanagedcontrolplane_controller_test.go +++ b/controllers/azureasomanagedcontrolplane_controller_test.go @@ -203,13 +203,13 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { newResourceReconciler: func(asoManagedControlPlane *infrav1.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedControlPlane, - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + reconcileFunc: func(ctx context.Context, o client.Object) error { asoManagedControlPlane.SetResourceStatuses([]infrav1.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return false, nil + return nil }, } }, @@ -317,8 +317,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, @@ -454,8 +454,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, @@ -593,8 +593,8 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { }, newResourceReconciler: func(_ *infrav1.AzureASOManagedControlPlane, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, diff --git a/controllers/azureasomanagedmachinepool_controller.go b/controllers/azureasomanagedmachinepool_controller.go index 1d2bf9228ac..87156d77d12 100644 --- a/controllers/azureasomanagedmachinepool_controller.go +++ b/controllers/azureasomanagedmachinepool_controller.go @@ -232,13 +232,13 @@ func (r *AzureASOManagedMachinePoolReconciler) reconcileNormal(ctx context.Conte } resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, resources) - needsRequeue, err := resourceReconciler.Reconcile(ctx) + err = resourceReconciler.Reconcile(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } for _, status := range asoManagedMachinePool.Status.Resources { if !status.Ready { - return ctrl.Result{Requeue: needsRequeue}, nil + return ctrl.Result{}, nil } } diff --git a/controllers/azureasomanagedmachinepool_controller_test.go b/controllers/azureasomanagedmachinepool_controller_test.go index 93ea177b431..3b915a27f91 100644 --- a/controllers/azureasomanagedmachinepool_controller_test.go +++ b/controllers/azureasomanagedmachinepool_controller_test.go @@ -266,13 +266,13 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { newResourceReconciler: func(asoManagedMachinePool *infrav1.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ owner: asoManagedMachinePool, - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { + reconcileFunc: func(ctx context.Context, o client.Object) error { asoManagedMachinePool.SetResourceStatuses([]infrav1.ResourceStatus{ {Ready: true}, {Ready: false}, {Ready: true}, }) - return false, nil + return nil }, } }, @@ -374,8 +374,8 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, @@ -514,8 +514,8 @@ func TestAzureASOManagedMachinePoolReconcile(t *testing.T) { Client: c, newResourceReconciler: func(_ *infrav1.AzureASOManagedMachinePool, _ []*unstructured.Unstructured) resourceReconciler { return &fakeResourceReconciler{ - reconcileFunc: func(ctx context.Context, o client.Object) (bool, error) { - return false, nil + reconcileFunc: func(ctx context.Context, o client.Object) error { + return nil }, } }, diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index f96993a6cb4..d0b11002c02 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -66,7 +66,7 @@ type resourceStatusObject interface { } // Reconcile creates or updates the specified resources. -func (r *ResourceReconciler) Reconcile(ctx context.Context) (bool, error) { +func (r *ResourceReconciler) Reconcile(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Reconcile") defer done() log.V(4).Info("reconciling resources") @@ -82,8 +82,7 @@ func (r *ResourceReconciler) Delete(ctx context.Context) error { // Delete is a special case of a normal reconciliation which is equivalent to all resources from spec // being deleted. r.resources = nil - _, err := r.reconcile(ctx) - return err + return r.reconcile(ctx) } // Pause pauses reconciliation of the specified resources. @@ -113,7 +112,7 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return nil } -func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { +func (r *ResourceReconciler) reconcile(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.reconcile") defer done() @@ -122,12 +121,12 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { ownedKindsValue := r.owner.GetAnnotations()[ownedKindsAnnotation] ownedKinds, err := parseOwnedKinds(ownedKindsValue) if err != nil { - return false, fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue) + return fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue) } ownedObjs, err := r.ownedObjs(ctx, ownedKinds) if err != nil { - return false, fmt.Errorf("failed to get owned objects: %w", err) + return fmt.Errorf("failed to get owned objects: %w", err) } unrecordedTypeResources, recordedTypeResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) @@ -146,26 +145,26 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { spec.SetNamespace(r.owner.GetNamespace()) if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { - return false, fmt.Errorf("failed to set owner reference: %w", err) + return fmt.Errorf("failed to set owner reference: %w", err) } toWatch := meta.AsPartialObjectMetadata(spec) toWatch.APIVersion = spec.GetAPIVersion() toWatch.Kind = spec.GetKind() if err := r.watcher.Watch(log, toWatch, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { - return false, fmt.Errorf("failed to watch resource: %w", err) + return fmt.Errorf("failed to watch resource: %w", err) } 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) if err != nil { - return false, fmt.Errorf("failed to apply resource: %w", err) + return fmt.Errorf("failed to apply resource: %w", err) } ready, err := readyStatus(ctx, spec) if err != nil { - return false, fmt.Errorf("failed to get ready status: %w", err) + return fmt.Errorf("failed to get ready status: %w", err) } newResourceStatuses = append(newResourceStatuses, infrav1.ResourceStatus{ Resource: statusResource(spec), @@ -176,7 +175,7 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { for _, obj := range toBeDeletedResources { newStatus, err := r.deleteResource(ctx, obj) if err != nil { - return false, fmt.Errorf("failed to delete %s %s/%s", obj.GroupVersionKind(), obj.Namespace, obj.Name) + return fmt.Errorf("failed to delete %s %s/%s", obj.GroupVersionKind(), obj.Namespace, obj.Name) } if newStatus != nil { newResourceStatuses = append(newResourceStatuses, *newStatus) @@ -206,7 +205,7 @@ func (r *ResourceReconciler) reconcile(ctx context.Context) (bool, error) { r.owner.SetResourceStatuses(newResourceStatuses) - return len(unrecordedTypeResources) > 0, nil + return nil } func (r *ResourceReconciler) deleteResource(ctx context.Context, resource *metav1.PartialObjectMetadata) (*infrav1.ResourceStatus, error) { diff --git a/controllers/resource_reconciler_test.go b/controllers/resource_reconciler_test.go index 621d4ec3605..94816bddca9 100644 --- a/controllers/resource_reconciler_test.go +++ b/controllers/resource_reconciler_test.go @@ -89,8 +89,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { resources: []*unstructured.Unstructured{}, owner: &infrav1.AzureASOManagedCluster{}, } - needsRequeue, err := r.Reconcile(ctx) - g.Expect(needsRequeue).To(BeFalse()) + err := r.Reconcile(ctx) g.Expect(err).NotTo(HaveOccurred()) }) @@ -129,8 +128,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: w, } - needsRequeue, err := r.Reconcile(ctx) - g.Expect(needsRequeue).To(BeTrue()) + err := r.Reconcile(ctx) g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(BeEmpty()) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched @@ -205,8 +203,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: w, } - needsRequeue, err := r.Reconcile(ctx) - g.Expect(needsRequeue).To(BeFalse()) + err := r.Reconcile(ctx) g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(HaveKey("ResourceGroup.resources.azure.com")) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched @@ -300,8 +297,7 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: &FakeWatcher{}, } - needsRequeue, err := r.Reconcile(ctx) - g.Expect(needsRequeue).To(BeFalse()) + err = r.Reconcile(ctx) g.Expect(err).NotTo(HaveOccurred()) g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")})))