From 6449b4ad976cb0ed3446c917aaf4c8378e4c972d Mon Sep 17 00:00:00 2001 From: Vishesh Date: Tue, 11 Mar 2025 15:21:02 +0530 Subject: [PATCH 1/3] Requeue instead of throwing an error when onwer of kind can't be found --- controllers/utils/base_reconciler.go | 3 +++ test/e2e/common.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/controllers/utils/base_reconciler.go b/controllers/utils/base_reconciler.go index 65441b17..62dea33c 100644 --- a/controllers/utils/base_reconciler.go +++ b/controllers/utils/base_reconciler.go @@ -464,6 +464,9 @@ func (r *ReconcilerBase) InitFromMgr(mgr ctrl.Manager, client cloud.Client) { func (r *ReconciliationRunner) GetParent(child client.Object, parent client.Object) CloudStackReconcilerMethod { return func() (ctrl.Result, error) { err := GetOwnerOfKind(r.RequestCtx, r.K8sClient, child, parent) + if err != nil && strings.Contains(err.Error(), "couldn't find owner of kind") { + return r.RequeueWithMessage(err.Error()) + } return ctrl.Result{}, err } } diff --git a/test/e2e/common.go b/test/e2e/common.go index ea537200..df87235c 100644 --- a/test/e2e/common.go +++ b/test/e2e/common.go @@ -361,7 +361,7 @@ func CheckAffinityGroupInProject(client *cloudstack.CloudStackClient, clusterNam for _, affinity := range vm.Affinitygroup { affinityIds = append(affinityIds, affinity.Id) - affinity, _, _ := client.AffinityGroup.GetAffinityGroupByID(affinity.Id, cloudstack.WithProject(project)) + affinity, _, err := client.AffinityGroup.GetAffinityGroupByID(affinity.Id, cloudstack.WithProject(project)) if err != nil { Fail("Failed to get affinity group for " + affinity.Id + " : " + err.Error()) } From 61e1fb8ca010765b1d06766ee7bc0c512f3d8207 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Wed, 12 Mar 2025 00:07:42 +0530 Subject: [PATCH 2/3] Add unit test --- controllers/utils/base_reconciler_test.go | 209 ++++++++++++++++++++++ controllers/utils/utils.go | 2 +- controllers/utils/utils_suite_test.go | 29 +++ test/e2e/common.go | 19 +- 4 files changed, 250 insertions(+), 9 deletions(-) create mode 100644 controllers/utils/base_reconciler_test.go create mode 100644 controllers/utils/utils_suite_test.go diff --git a/controllers/utils/base_reconciler_test.go b/controllers/utils/base_reconciler_test.go new file mode 100644 index 00000000..0b6474b3 --- /dev/null +++ b/controllers/utils/base_reconciler_test.go @@ -0,0 +1,209 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils_test + +import ( + "context" + + "github.com/go-logr/logr" + "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" + "sigs.k8s.io/cluster-api-provider-cloudstack/controllers/utils" + dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta3" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// mockConcreteRunner is a minimal implementation of the runner interface +type mockConcreteRunner struct{} + +func (m *mockConcreteRunner) ReconcileDelete() (ctrl.Result, error) { + return ctrl.Result{}, nil +} + +func (m *mockConcreteRunner) Reconcile() (ctrl.Result, error) { + return ctrl.Result{}, nil +} + +var _ = Describe("ReconciliationRunner", func() { + var ( + mockCtrl *gomock.Controller + k8sClient client.Client + scheme *runtime.Scheme + baseRunner *utils.ReconciliationRunner + ctx context.Context + mockRunner *mockConcreteRunner + ) + + BeforeEach(func() { + mockCtrl = gomock.NewController(GinkgoT()) + scheme = runtime.NewScheme() + Expect(infrav1.AddToScheme(scheme)).To(Succeed()) + Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) + + k8sClient = fake.NewClientBuilder().WithScheme(scheme).Build() + ctx = context.Background() + mockRunner = &mockConcreteRunner{} + + // Create the base reconciler + base := utils.ReconcilerBase{ + K8sClient: k8sClient, + Scheme: scheme, + BaseLogger: logr.Discard(), + Recorder: record.NewFakeRecorder(10), + } + + // Create a reconciliation runner with our mock concrete runner + baseRunner = utils.NewRunner(mockRunner, &infrav1.CloudStackMachine{}, "TestController") + baseRunner.UsingBaseReconciler(base) + baseRunner.WithRequestCtx(ctx) + + // Setup a fake Request + baseRunner.ForRequest(ctrl.Request{ + NamespacedName: client.ObjectKey{ + Namespace: "default", + Name: "test-machine", + }, + }) + }) + + AfterEach(func() { + mockCtrl.Finish() + }) + + Describe("GetParent", func() { + var ( + child *infrav1.CloudStackMachine + ) + + BeforeEach(func() { + dummies.SetDummyVars() + + // Set up child object + child = &infrav1.CloudStackMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-child", + Namespace: "default", + }, + } + }) + + Context("when parent exists and is correctly referenced", func() { + var parent *clusterv1.Machine + BeforeEach(func() { + parent = &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-parent", + Namespace: "default", + }, + } + // Set up owner reference + child.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Machine", + Name: parent.Name, + UID: "test-uid", + }, + } + + // Create the objects in the fake client + Expect(k8sClient.Create(ctx, parent)).To(Succeed()) + Expect(k8sClient.Create(ctx, child)).To(Succeed()) + }) + + It("should find the parent successfully", func() { + // Create an empty parent object to be filled + parentToFind := &clusterv1.Machine{} + + // Call GetParent + result, err := baseRunner.GetParent(child, parentToFind)() + + // Check results + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(ctrl.Result{})) + Expect(parentToFind.Name).To(Equal(parent.Name)) + Expect(parentToFind.Namespace).To(Equal(parent.Namespace)) + }) + }) + + Context("when parent doesn't exist", func() { + BeforeEach(func() { + // Set up owner reference to non-existent parent + child.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Machine", + Name: "non-existent-parent", + UID: "test-uid", + }, + } + + // Create only the child in the fake client + Expect(k8sClient.Create(ctx, child)).To(Succeed()) + }) + + It("should return an error", func() { + // Create an empty parent object to be filled + parentToFind := &clusterv1.Machine{} + + // Call GetParent + _, err := baseRunner.GetParent(child, parentToFind)() + + // Check results + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("not found")) + }) + }) + + Context("when no owner reference of requested kind exists", func() { + BeforeEach(func() { + // Set up owner reference to different kind + child.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: "different.api/v1", + Kind: "DifferentKind", + Name: "different-name", + UID: "test-uid", + }, + } + + // Create only the child in the fake client + Expect(k8sClient.Create(ctx, child)).To(Succeed()) + }) + + It("should requeue with error message", func() { + // Create an empty parent object to be filled + parentToFind := &clusterv1.Machine{} + + // Call GetParent + result, err := baseRunner.GetParent(child, parentToFind)() + + // Check results + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(utils.RequeueTimeout)) + }) + }) + }) +}) diff --git a/controllers/utils/utils.go b/controllers/utils/utils.go index 01a2f986..71bb21c3 100644 --- a/controllers/utils/utils.go +++ b/controllers/utils/utils.go @@ -135,7 +135,7 @@ func GetManagementOwnerRef(capiMachine *clusterv1.Machine) *meta.OwnerReference } // GetOwnerOfKind returns the Cluster object owning the current resource of passed kind. -func GetOwnerOfKind(ctx context.Context, c clientPkg.Client, owned clientPkg.Object, owner clientPkg.Object) error { +var GetOwnerOfKind = func(ctx context.Context, c clientPkg.Client, owned clientPkg.Object, owner clientPkg.Object) error { gvks, _, err := c.Scheme().ObjectKinds(owner) if err != nil { return errors.Wrapf(err, "finding owner kind for %s/%s", owned.GetName(), owned.GetNamespace()) diff --git a/controllers/utils/utils_suite_test.go b/controllers/utils/utils_suite_test.go new file mode 100644 index 00000000..1277a7fc --- /dev/null +++ b/controllers/utils/utils_suite_test.go @@ -0,0 +1,29 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestUtils(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Utils Suite") +} diff --git a/test/e2e/common.go b/test/e2e/common.go index df87235c..d1aafea6 100644 --- a/test/e2e/common.go +++ b/test/e2e/common.go @@ -361,18 +361,21 @@ func CheckAffinityGroupInProject(client *cloudstack.CloudStackClient, clusterNam for _, affinity := range vm.Affinitygroup { affinityIds = append(affinityIds, affinity.Id) - affinity, _, err := client.AffinityGroup.GetAffinityGroupByID(affinity.Id, cloudstack.WithProject(project)) + // Store the ID before potential nil issues + affinityID := affinity.Id + // Rename the variable to avoid shadowing + affinityGroup, _, err := client.AffinityGroup.GetAffinityGroupByID(affinityID, cloudstack.WithProject(project)) if err != nil { - Fail("Failed to get affinity group for " + affinity.Id + " : " + err.Error()) + Fail("Failed to get affinity group for " + affinityID + " : " + err.Error()) } - if !strings.Contains(affinity.Name, affinityTypeString) { - Fail(affinity.Name + " does not contain " + affinityTypeString) + if !strings.Contains(affinityGroup.Name, affinityTypeString) { + Fail(affinityGroup.Name + " does not contain " + affinityTypeString) } - if affinityType == "pro" && affinity.Type != "host affinity" { - Fail(affinity.Type + " does not match " + affinityType) + if affinityType == "pro" && affinityGroup.Type != "host affinity" { + Fail(affinityGroup.Type + " does not match " + affinityType) } - if affinityType == "anti" && affinity.Type != "host anti-affinity" { - Fail(affinity.Type + " does not match " + affinityType) + if affinityType == "anti" && affinityGroup.Type != "host anti-affinity" { + Fail(affinityGroup.Type + " does not match " + affinityType) } } } From 5a66366c988984f3cf890954ee8777537a90e3b2 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Wed, 12 Mar 2025 00:15:58 +0530 Subject: [PATCH 3/3] Update controllers/utils/utils.go --- controllers/utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/utils/utils.go b/controllers/utils/utils.go index 71bb21c3..01a2f986 100644 --- a/controllers/utils/utils.go +++ b/controllers/utils/utils.go @@ -135,7 +135,7 @@ func GetManagementOwnerRef(capiMachine *clusterv1.Machine) *meta.OwnerReference } // GetOwnerOfKind returns the Cluster object owning the current resource of passed kind. -var GetOwnerOfKind = func(ctx context.Context, c clientPkg.Client, owned clientPkg.Object, owner clientPkg.Object) error { +func GetOwnerOfKind(ctx context.Context, c clientPkg.Client, owned clientPkg.Object, owner clientPkg.Object) error { gvks, _, err := c.Scheme().ObjectKinds(owner) if err != nil { return errors.Wrapf(err, "finding owner kind for %s/%s", owned.GetName(), owned.GetNamespace())