From 7f8410a39756119c3e4c428eddee15e546191ff2 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Thu, 25 Jul 2024 13:34:11 +0200 Subject: [PATCH] Improve MP unit test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../controllers/machinepool_controller.go | 19 +- .../machinepool_controller_noderef.go | 3 +- .../machinepool_controller_test.go | 252 +++++++++++++----- 3 files changed, 188 insertions(+), 86 deletions(-) diff --git a/exp/internal/controllers/machinepool_controller.go b/exp/internal/controllers/machinepool_controller.go index f32a696cd1ea..aae6fa66885d 100644 --- a/exp/internal/controllers/machinepool_controller.go +++ b/exp/internal/controllers/machinepool_controller.go @@ -302,10 +302,6 @@ func (r *MachinePoolReconciler) reconcileDeleteNodes(ctx context.Context, cluste return nil } - if r.Tracker == nil { - return errors.New("Cannot establish cluster client to delete nodes") - } - clusterClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster)) if err != nil { return err @@ -328,13 +324,9 @@ func (r *MachinePoolReconciler) isMachinePoolNodeDeleteTimeoutPassed(machinePool // reconcileDeleteExternal tries to delete external references, returning true if it cannot find any. func (r *MachinePoolReconciler) reconcileDeleteExternal(ctx context.Context, machinePool *expv1.MachinePool) (bool, error) { objects := []*unstructured.Unstructured{} - references := []*corev1.ObjectReference{} - // check for external ref - if machinePool.Spec.Template.Spec.Bootstrap.ConfigRef != nil { - references = append(references, machinePool.Spec.Template.Spec.Bootstrap.ConfigRef) - } - if machinePool.Spec.Template.Spec.InfrastructureRef != (corev1.ObjectReference{}) { - references = append(references, &machinePool.Spec.Template.Spec.InfrastructureRef) + references := []*corev1.ObjectReference{ + machinePool.Spec.Template.Spec.Bootstrap.ConfigRef, + &machinePool.Spec.Template.Spec.InfrastructureRef, } // Loop over the references and try to retrieve it with the client. @@ -374,11 +366,6 @@ func (r *MachinePoolReconciler) watchClusterNodes(ctx context.Context, cluster * return nil } - // If there is no tracker, don't watch remote nodes - if r.Tracker == nil { - return nil - } - return r.Tracker.Watch(ctx, remote.WatchInput{ Name: "machinepool-watchNodes", Cluster: util.ObjectKey(cluster), diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index e6c27b42ce93..790f2280e688 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -23,6 +23,7 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" @@ -153,7 +154,7 @@ func (r *MachinePoolReconciler) deleteRetiredNodes(ctx context.Context, c client delete(nodeRefsMap, providerID) } for _, node := range nodeRefsMap { - if err := c.Delete(ctx, node); err != nil { + if err := c.Delete(ctx, node); err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "failed to delete Node") } } diff --git a/exp/internal/controllers/machinepool_controller_test.go b/exp/internal/controllers/machinepool_controller_test.go index 03ae2605fac5..aa9de43469e9 100644 --- a/exp/internal/controllers/machinepool_controller_test.go +++ b/exp/internal/controllers/machinepool_controller_test.go @@ -17,6 +17,8 @@ limitations under the License. package controllers import ( + "context" + "fmt" "testing" "time" @@ -30,6 +32,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -255,9 +258,9 @@ func TestMachinePoolOwnerReference(t *testing.T) { } func TestReconcileMachinePoolRequest(t *testing.T) { - infraConfig := unstructured.Unstructured{ + infraMachinePool := unstructured.Unstructured{ Object: map[string]interface{}{ - "kind": builder.TestInfrastructureMachineTemplateKind, + "kind": builder.TestInfrastructureMachinePoolKind, "apiVersion": builder.InfrastructureGroupVersion.String(), "metadata": map[string]interface{}{ "name": "infra-config1", @@ -303,13 +306,14 @@ func TestReconcileMachinePoolRequest(t *testing.T) { err bool } testCases := []struct { - name string - machinePool expv1.MachinePool - expected expected - withTracker bool + name string + machinePool expv1.MachinePool + nodes []corev1.Node + errOnDeleteNode bool + expected expected }{ { - name: "Create machine Pool", + name: "Successfully reconcile MachinePool", machinePool: expv1.MachinePool{ ObjectMeta: metav1.ObjectMeta{ Name: "created", @@ -325,7 +329,7 @@ func TestReconcileMachinePoolRequest(t *testing.T) { Spec: clusterv1.MachineSpec{ InfrastructureRef: corev1.ObjectReference{ APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.TestInfrastructureMachineTemplateKind, + Kind: builder.TestInfrastructureMachinePoolKind, Name: "infra-config1", }, Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, @@ -346,10 +350,9 @@ func TestReconcileMachinePoolRequest(t *testing.T) { result: reconcile.Result{}, err: false, }, - withTracker: true, }, { - name: "Delete machinePool with nodeDeleteTimeout not passed", + name: "Successfully reconcile MachinePool with deletionTimestamp & NodeDeletionTimeout not passed when Nodes can be deleted (MP should go away)", machinePool: expv1.MachinePool{ ObjectMeta: metav1.ObjectMeta{ Name: "deleted", @@ -359,7 +362,7 @@ func TestReconcileMachinePoolRequest(t *testing.T) { }, Finalizers: []string{expv1.MachinePoolFinalizer}, CreationTimestamp: timeNow, - DeletionTimestamp: &metav1.Time{Time: timeNow.Add(time.Hour * 1)}, + DeletionTimestamp: &timeNow, }, Spec: expv1.MachinePoolSpec{ ClusterName: "test-cluster", @@ -368,11 +371,11 @@ func TestReconcileMachinePoolRequest(t *testing.T) { Spec: clusterv1.MachineSpec{ InfrastructureRef: corev1.ObjectReference{ APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.TestInfrastructureMachineTemplateKind, - Name: "infra-config1", + Kind: builder.TestInfrastructureMachinePoolKind, + Name: "infra-config1-already-deleted", // Use an InfrastructureMachinePool that doesn't exist, so reconcileDelete doesn't get stuck on deletion }, Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, - NodeDeletionTimeout: &metav1.Duration{Duration: 1 * time.Minute}, + NodeDeletionTimeout: &metav1.Duration{Duration: 10 * time.Minute}, }, }, ProviderIDList: []string{"aws:///us-test-2a/i-013ab00756982217f"}, @@ -387,32 +390,49 @@ func TestReconcileMachinePoolRequest(t *testing.T) { }, }, }, + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + }, + Spec: corev1.NodeSpec{ + // This providerID is not in the list above, so the reconciler will try (and fail) to delete it + ProviderID: "aws:///us-test-2c/i-013ab00756982217f", + }, + }, + }, + errOnDeleteNode: false, // Node can be deleted expected: expected{ - mpExist: true, + mpExist: false, result: reconcile.Result{}, err: false, }, - withTracker: true, }, { - name: "Delete machinePool with nodeDeleteTimeout passed", + name: "Fail reconcile MachinePool with deletionTimestamp & NodeDeletionTimeout not passed when Nodes cannot be deleted (MP should stay around)", machinePool: expv1.MachinePool{ ObjectMeta: metav1.ObjectMeta{ - Name: "deleted-timeoutNode", + Name: "deleted", Namespace: metav1.NamespaceDefault, Labels: map[string]string{ clusterv1.MachineControlPlaneLabel: "", }, Finalizers: []string{expv1.MachinePoolFinalizer}, - CreationTimestamp: metav1.Time{Time: timeNow.Add(time.Minute * -2)}, - DeletionTimestamp: &metav1.Time{Time: timeNow.Add(time.Minute * -1)}, + CreationTimestamp: timeNow, + DeletionTimestamp: &timeNow, }, Spec: expv1.MachinePoolSpec{ ClusterName: "test-cluster", Replicas: ptr.To[int32](1), Template: clusterv1.MachineTemplateSpec{ Spec: clusterv1.MachineSpec{ - NodeDeletionTimeout: &metav1.Duration{Duration: 10 * time.Second}, + InfrastructureRef: corev1.ObjectReference{ + APIVersion: builder.InfrastructureGroupVersion.String(), + Kind: builder.TestInfrastructureMachinePoolKind, + Name: "infra-config1-already-deleted", // Use an InfrastructureMachinePool that doesn't exist, so reconcileDelete doesn't get stuck on deletion + }, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + NodeDeletionTimeout: &metav1.Duration{Duration: 10 * time.Minute}, }, }, ProviderIDList: []string{"aws:///us-test-2a/i-013ab00756982217f"}, @@ -427,17 +447,29 @@ func TestReconcileMachinePoolRequest(t *testing.T) { }, }, }, + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + }, + Spec: corev1.NodeSpec{ + // This providerID is not in the list above, so the reconciler will try (and fail) to delete it + ProviderID: "aws:///us-test-2c/i-013ab00756982217f", + }, + }, + }, + errOnDeleteNode: true, // Node cannot be deleted expected: expected{ - mpExist: false, + mpExist: true, result: reconcile.Result{}, - err: false, + err: true, }, }, { - name: "Try delete machinePool with nodeDeleteTimeout set to zero", + name: "Successfully reconcile MachinePool with deletionTimestamp & NodeDeletionTimeout passed when Nodes cannot be deleted (MP should go away)", machinePool: expv1.MachinePool{ ObjectMeta: metav1.ObjectMeta{ - Name: "try-delete", + Name: "deleted", Namespace: metav1.NamespaceDefault, Labels: map[string]string{ clusterv1.MachineControlPlaneLabel: "", @@ -451,7 +483,13 @@ func TestReconcileMachinePoolRequest(t *testing.T) { Replicas: ptr.To[int32](1), Template: clusterv1.MachineTemplateSpec{ Spec: clusterv1.MachineSpec{ - NodeDeletionTimeout: &metav1.Duration{Duration: 0 * time.Second}, + InfrastructureRef: corev1.ObjectReference{ + APIVersion: builder.InfrastructureGroupVersion.String(), + Kind: builder.TestInfrastructureMachinePoolKind, + Name: "infra-config1-already-deleted", // Use an InfrastructureMachinePool that doesn't exist, so reconcileDelete doesn't get stuck on deletion + }, + Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")}, + NodeDeletionTimeout: &metav1.Duration{Duration: 10 * time.Second}, // timeout passed }, }, ProviderIDList: []string{"aws:///us-test-2a/i-013ab00756982217f"}, @@ -466,36 +504,56 @@ func TestReconcileMachinePoolRequest(t *testing.T) { }, }, }, + nodes: []corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + }, + Spec: corev1.NodeSpec{ + // This providerID is not in the list above, so the reconciler will try (and fail) to delete it + ProviderID: "aws:///us-test-2c/i-013ab00756982217f", + }, + }, + }, + errOnDeleteNode: true, // Node cannot be deleted expected: expected{ - mpExist: true, + mpExist: false, result: reconcile.Result{}, - err: true, + err: false, }, }, } - for i := range testCases { - tc := testCases[i] + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) clientFake := fake.NewClientBuilder().WithObjects( &testCluster, &tc.machinePool, - &infraConfig, + &infraMachinePool, bootstrapConfig, builder.TestBootstrapConfigCRD, - builder.TestInfrastructureMachineTemplateCRD, + builder.TestInfrastructureMachinePoolCRD, ).WithStatusSubresource(&expv1.MachinePool{}).Build() + trackerObjects := []client.Object{} + for _, node := range tc.nodes { + trackerObjects = append(trackerObjects, &node) + } + trackerClientFake := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteOption) error { + if tc.errOnDeleteNode { + return fmt.Errorf("node deletion failed") + } + return nil + }, + }).WithObjects(trackerObjects...).Build() + r := &MachinePoolReconciler{ Client: clientFake, APIReader: clientFake, - } - - // Set the tracker. - if tc.withTracker { - r.Tracker = remote.NewTestClusterCacheTracker(ctrl.LoggerFrom(ctx), clientFake, clientFake, clientFake.Scheme(), client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}) + Tracker: remote.NewTestClusterCacheTracker(ctrl.LoggerFrom(ctx), clientFake, trackerClientFake, clientFake.Scheme(), client.ObjectKey{Name: testCluster.Name, Namespace: testCluster.Namespace}), } result, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: util.ObjectKey(&tc.machinePool)}) @@ -509,50 +567,106 @@ func TestReconcileMachinePoolRequest(t *testing.T) { // Check machinePool test cases key := client.ObjectKey{Namespace: tc.machinePool.Namespace, Name: tc.machinePool.Name} if tc.expected.mpExist { - g.Consistently(func(g Gomega) { - g.Expect(r.Client.Get(ctx, key, &expv1.MachinePool{})).To(Succeed()) - }).WithTimeout(2 * time.Second).Should(Succeed()) + g.Expect(r.Client.Get(ctx, key, &expv1.MachinePool{})).To(Succeed()) } else { - // We expect to fail retrieve the machinePool as it is deleted - g.Eventually(func(g Gomega) { - g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, key, &expv1.MachinePool{}))).To(BeTrue()) - }).WithTimeout(5 * time.Second).Should(Succeed()) + g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, key, &expv1.MachinePool{}))).To(BeTrue()) } }) } } func TestMachinePoolNodeDeleteTimeoutPassed(t *testing.T) { - g := NewWithT(t) - r := &MachinePoolReconciler{} timeNow := metav1.Now() - machinePool := expv1.MachinePool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machinepool", - Namespace: metav1.NamespaceDefault, - CreationTimestamp: timeNow, + testCases := []struct { + name string + machinePool *expv1.MachinePool + want bool + }{ + { + name: "false if deletionTimestamp not set", + machinePool: &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool", + Namespace: metav1.NamespaceDefault, + }, + }, + want: false, + }, + { + name: "false if deletionTimestamp set to now and NodeDeletionTimeout not set", + machinePool: &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool", + Namespace: metav1.NamespaceDefault, + DeletionTimestamp: &timeNow, + }, + }, + want: false, + }, + { + name: "false if deletionTimestamp set to now and NodeDeletionTimeout set to 0", + machinePool: &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool", + Namespace: metav1.NamespaceDefault, + DeletionTimestamp: &timeNow, + }, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + NodeDeletionTimeout: &metav1.Duration{Duration: 0 * time.Second}, + }, + }, + }, + }, + want: false, + }, + { + name: "false if deletionTimestamp set to now and NodeDeletionTimeout set to 1m", + machinePool: &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool", + Namespace: metav1.NamespaceDefault, + DeletionTimestamp: &timeNow, + }, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + NodeDeletionTimeout: &metav1.Duration{Duration: 1 * time.Minute}, + }, + }, + }, + }, + want: false, + }, + { + name: "true if deletionTimestamp set to now-1m and NodeDeletionTimeout set to 10s", + machinePool: &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool", + Namespace: metav1.NamespaceDefault, + DeletionTimestamp: &metav1.Time{Time: timeNow.Add(time.Minute * -1)}, + }, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + NodeDeletionTimeout: &metav1.Duration{Duration: 10 * time.Second}, + }, + }, + }, + }, + want: true, }, } - // MachienPool not deleted expected false - g.Expect(r.isMachinePoolNodeDeleteTimeoutPassed(&machinePool)).To(BeFalse()) - - // MachienPool deleted and NodeDeleteTimeout not set expected false - machinePool.DeletionTimestamp = &timeNow - g.Expect(r.isMachinePoolNodeDeleteTimeoutPassed(&machinePool)).To(BeFalse()) - - // MachienPool deleted and NodeDeleteTimeout set to zero expected false - machinePool.Spec.Template.Spec.NodeDeletionTimeout = &metav1.Duration{Duration: 0 * time.Second} - g.Expect(r.isMachinePoolNodeDeleteTimeoutPassed(&machinePool)).To(BeFalse()) - - // MachienPool deleted (at timeNow) and NodeDeleteTimeout set to 1 min expected false - machinePool.Spec.Template.Spec.NodeDeletionTimeout = &metav1.Duration{Duration: 1 * time.Minute} - g.Expect(r.isMachinePoolNodeDeleteTimeoutPassed(&machinePool)).To(BeFalse()) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) - // MachienPool deleted (at timeNow -1 min) and NodeDeleteTimeout set to 10 Second expected true - machinePool.DeletionTimestamp = &metav1.Time{Time: timeNow.Add(time.Minute * -1)} - machinePool.Spec.Template.Spec.NodeDeletionTimeout = &metav1.Duration{Duration: 10 * time.Second} - g.Expect(r.isMachinePoolNodeDeleteTimeoutPassed(&machinePool)).To(BeTrue()) + timeoutPassed := (&MachinePoolReconciler{}).isMachinePoolNodeDeleteTimeoutPassed(tc.machinePool) + g.Expect(timeoutPassed).To(Equal(tc.want)) + }) + } } func TestReconcileMachinePoolDeleteExternal(t *testing.T) {