From e16f187f3c9f97aa0073191461ef8eba7e39b4cd Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 19 Dec 2025 09:58:32 +0100 Subject: [PATCH 1/2] Normalize tests on SpecContext in leaf The tests before where partly relying one a global context, which means the cancelation from ginkgo wasn't really passed down. The leaf functions can take a context.Context or SpecContext argument. The latter is already imported, so lets go with that one. --- .../controller/aggregates_controller_test.go | 23 +++++---- .../controller/decomission_controller_test.go | 6 +-- .../controller/eviction_controller_test.go | 25 +++++----- ...gardener_node_lifecycle_controller_test.go | 13 ++--- .../hypervisor_maintenance_controller_test.go | 47 +++++++++---------- .../node_certificate_controller_test.go | 6 +-- .../node_eviction_label_controller_test.go | 13 +++-- .../controller/onboarding_controller_test.go | 17 ++++--- internal/controller/suite_test.go | 6 --- internal/controller/traits_controller_test.go | 17 ++++--- 10 files changed, 77 insertions(+), 96 deletions(-) diff --git a/internal/controller/aggregates_controller_test.go b/internal/controller/aggregates_controller_test.go index 3996fe04..3c022dad 100644 --- a/internal/controller/aggregates_controller_test.go +++ b/internal/controller/aggregates_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "net/http" @@ -105,7 +104,7 @@ var _ = Describe("AggregatesController", func() { ) // Setup and teardown - BeforeEach(func(ctx context.Context) { + BeforeEach(func(ctx SpecContext) { By("Setting up the OpenStack http mock server") fakeServer = testhelper.SetupHTTP() DeferCleanup(fakeServer.Teardown) @@ -135,19 +134,19 @@ var _ = Describe("AggregatesController", func() { computeClient: client.ServiceClient(fakeServer), } - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { Expect(tc.Client.Delete(ctx, hypervisor)).To(Succeed()) }) }) - JustBeforeEach(func(ctx context.Context) { + JustBeforeEach(func(ctx SpecContext) { _, err := tc.Reconcile(ctx, ctrl.Request{NamespacedName: hypervisorName}) Expect(err).NotTo(HaveOccurred()) }) // Tests Context("Adding new Aggregate", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { By("Setting a missing aggregate") hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) @@ -189,7 +188,7 @@ var _ = Describe("AggregatesController", func() { }) }) - It("should update Aggregates and set status condition as Aggregates differ", func() { + It("should update Aggregates and set status condition as Aggregates differ", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Aggregates).To(ContainElements("test-aggregate1")) @@ -198,7 +197,7 @@ var _ = Describe("AggregatesController", func() { }) Context("Removing Aggregate", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) // update status to have existing aggregate @@ -233,7 +232,7 @@ var _ = Describe("AggregatesController", func() { fakeServer.Mux.HandleFunc("POST /os-aggregates/99/action", expectRemoveHostFromAggregate) }) - It("should update Aggregates and set status condition when Aggregates differ", func() { + It("should update Aggregates and set status condition when Aggregates differ", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Aggregates).To(BeEmpty()) @@ -242,7 +241,7 @@ var _ = Describe("AggregatesController", func() { }) Context("before onboarding", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) // Remove the onboarding condition @@ -250,7 +249,7 @@ var _ = Describe("AggregatesController", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should neither update Aggregates and nor set status condition", func() { + It("should neither update Aggregates and nor set status condition", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Aggregates).To(BeEmpty()) @@ -259,7 +258,7 @@ var _ = Describe("AggregatesController", func() { }) Context("when terminating", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) // Remove the onboarding condition @@ -272,7 +271,7 @@ var _ = Describe("AggregatesController", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should neither update Aggregates and nor set status condition", func() { + It("should neither update Aggregates and nor set status condition", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Aggregates).To(BeEmpty()) diff --git a/internal/controller/decomission_controller_test.go b/internal/controller/decomission_controller_test.go index 3eeb4a21..867cb779 100644 --- a/internal/controller/decomission_controller_test.go +++ b/internal/controller/decomission_controller_test.go @@ -18,8 +18,6 @@ limitations under the License. package controller import ( - "context" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" @@ -84,7 +82,7 @@ var _ = Describe("Decommission Controller", func() { }) }) - AfterEach(func(ctx context.Context) { + AfterEach(func(ctx SpecContext) { node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName.Name}} By("Cleanup the specific node and hypervisor resource") Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed()) @@ -102,7 +100,7 @@ var _ = Describe("Decommission Controller", func() { }) Context("When reconciling a node", func() { - It("should set the finalizer", func(ctx context.Context) { + It("should set the finalizer", func(ctx SpecContext) { By("reconciling the created resource") _, err := r.Reconcile(ctx, reconcileReq) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/eviction_controller_test.go b/internal/controller/eviction_controller_test.go index d1cf3d77..0affc5e4 100644 --- a/internal/controller/eviction_controller_test.go +++ b/internal/controller/eviction_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "net/http" @@ -97,14 +96,12 @@ var _ = Describe("Eviction Controller", func() { fakeServer testhelper.FakeServer ) - ctx := context.Background() //nolint:govet - BeforeEach(func() { By("Setting up the OpenStack http mock server") fakeServer = testhelper.SetupHTTP() }) - AfterEach(func() { + AfterEach(func(ctx SpecContext) { resource := &kvmv1.Eviction{} err := k8sClient.Get(ctx, typeNamespacedName, resource) if err != nil { @@ -133,7 +130,7 @@ var _ = Describe("Eviction Controller", func() { Describe("API validation", func() { When("creating an eviction without hypervisor", func() { - It("it should fail creating the resource", func() { + It("it should fail creating the resource", func(ctx SpecContext) { resource := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, @@ -149,7 +146,7 @@ var _ = Describe("Eviction Controller", func() { }) When("creating an eviction without reason", func() { - It("it should fail creating the resource", func() { + It("it should fail creating the resource", func(ctx SpecContext) { resource := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, @@ -165,7 +162,7 @@ var _ = Describe("Eviction Controller", func() { }) When("creating an eviction with reason and hypervisor", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { By("creating the hypervisor resource") hypervisor := &kvmv1.Hypervisor{ ObjectMeta: metav1.ObjectMeta{ @@ -174,7 +171,7 @@ var _ = Describe("Eviction Controller", func() { } Expect(ctrlRuntimeClient.IgnoreAlreadyExists(k8sClient.Create(ctx, hypervisor))).To(Succeed()) }) - It("should successfully create the resource", func() { + It("should successfully create the resource", func(ctx SpecContext) { resource := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, @@ -193,7 +190,7 @@ var _ = Describe("Eviction Controller", func() { Describe("Reconciliation", func() { Describe("an eviction for 'test-hypervisor'", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { By("Creating the resource") resource := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{ @@ -224,7 +221,7 @@ var _ = Describe("Eviction Controller", func() { }) }) - It("should fail reconciliation", func() { + It("should fail reconciliation", func(ctx SpecContext) { for range 3 { _, err := controllerReconciler.Reconcile(ctx, reconcileRequest) Expect(err).NotTo(HaveOccurred()) @@ -247,7 +244,7 @@ var _ = Describe("Eviction Controller", func() { }) When("enabled hypervisor has no servers", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { fakeServer.Mux.HandleFunc("GET /os-hypervisors/detail", func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -266,7 +263,7 @@ var _ = Describe("Eviction Controller", func() { } Expect(ctrlRuntimeClient.IgnoreAlreadyExists(k8sClient.Create(ctx, hypervisor))).To(Succeed()) }) - It("should succeed the reconciliation", func() { + It("should succeed the reconciliation", func(ctx SpecContext) { runningCond := &metav1.Condition{ Type: kvmv1.ConditionTypeEvicting, Status: metav1.ConditionTrue, @@ -344,7 +341,7 @@ var _ = Describe("Eviction Controller", func() { }) }) When("disabled hypervisor has no servers", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { fakeServer.Mux.HandleFunc("GET /os-hypervisors/detail", func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -362,7 +359,7 @@ var _ = Describe("Eviction Controller", func() { } Expect(ctrlRuntimeClient.IgnoreAlreadyExists(k8sClient.Create(ctx, hypervisor))).To(Succeed()) }) - It("should succeed the reconciliation", func() { + It("should succeed the reconciliation", func(ctx SpecContext) { for range 3 { _, err := controllerReconciler.Reconcile(ctx, reconcileRequest) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/gardener_node_lifecycle_controller_test.go b/internal/controller/gardener_node_lifecycle_controller_test.go index fdfafadd..1af06de1 100644 --- a/internal/controller/gardener_node_lifecycle_controller_test.go +++ b/internal/controller/gardener_node_lifecycle_controller_test.go @@ -31,7 +31,7 @@ var _ = Describe("Gardener Maintenance Controller", func() { const nodeName = "node-test" var controller *GardenerNodeLifecycleController - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { controller = &GardenerNodeLifecycleController{ Client: k8sClient, Scheme: k8sClient.Scheme(), @@ -49,16 +49,13 @@ var _ = Describe("Gardener Maintenance Controller", func() { }, } Expect(k8sClient.Create(ctx, resource)).To(Succeed()) - }) - - AfterEach(func() { - node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}} - By("Cleanup the specific node") - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed()) + DeferCleanup(func(ctx SpecContext) { + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, resource))).To(Succeed()) + }) }) Context("When reconciling a node", func() { - It("should successfully reconcile the resource", func() { + It("should successfully reconcile the resource", func(ctx SpecContext) { req := ctrl.Request{ NamespacedName: types.NamespacedName{Name: nodeName}, } diff --git a/internal/controller/hypervisor_maintenance_controller_test.go b/internal/controller/hypervisor_maintenance_controller_test.go index 1a56a4bf..07cd7e16 100644 --- a/internal/controller/hypervisor_maintenance_controller_test.go +++ b/internal/controller/hypervisor_maintenance_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "net/http" @@ -79,7 +78,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { } // Setup and teardown - BeforeEach(func(ctx context.Context) { + BeforeEach(func(ctx SpecContext) { By("Setting up the OpenStack http mock server") fakeServer = testhelper.SetupHTTP() DeferCleanup(fakeServer.Teardown) @@ -100,7 +99,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { Spec: kvmv1.HypervisorSpec{}, } Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { By("Deleting the Hypervisor resource") hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) @@ -109,13 +108,13 @@ var _ = Describe("HypervisorMaintenanceController", func() { }) // After the setup in JustBefore, we want to reconcile - JustBeforeEach(func(ctx context.Context) { + JustBeforeEach(func(ctx SpecContext) { req := ctrl.Request{NamespacedName: hypervisorName} _, err := controller.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) }) - AfterEach(func(ctx context.Context) { + AfterEach(func(ctx SpecContext) { eviction := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{ Name: hypervisorName.Name, @@ -127,7 +126,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { // Tests Context("Onboarded Hypervisor", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Status.ServiceID = "1234" @@ -144,7 +143,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { Describe("Enabling or Disabling the Nova Service", func() { Context("Spec.Maintenance=\"\"", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Spec.Maintenance = "" @@ -153,13 +152,13 @@ var _ = Describe("HypervisorMaintenanceController", func() { mockServiceUpdate(expectedBody) }) - It("should set the ConditionTypeHypervisorDisabled to false", func() { + It("should set the ConditionTypeHypervisorDisabled to false", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(meta.IsStatusConditionFalse(updated.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled)).To(BeTrue()) }) - It("should set the ConditionTypeReady to true", func() { + It("should set the ConditionTypeReady to true", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Conditions).To(ContainElement( @@ -173,7 +172,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { for _, mode := range []string{"auto", "manual", "ha"} { Context(fmt.Sprintf("Spec.Maintenance=\"%v\"", mode), func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Spec.Maintenance = mode @@ -182,13 +181,13 @@ var _ = Describe("HypervisorMaintenanceController", func() { mockServiceUpdate(expectedBody) }) - It("should set the ConditionTypeHypervisorDisabled to true", func() { + It("should set the ConditionTypeHypervisorDisabled to true", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled)).To(BeTrue()) }) - It("should set the ConditionTypeReady to false", func() { + It("should set the ConditionTypeReady to false", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Conditions).To(ContainElement( @@ -202,7 +201,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { Describe("Eviction reconciliation", func() { Context("Spec.Maintenance=\"\"", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Spec.Maintenance = "" @@ -230,7 +229,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { Expect(k8sClient.Create(ctx, eviction)).To(Succeed()) }) - It("should delete the created eviction", func() { + It("should delete the created eviction", func(ctx SpecContext) { eviction := &kvmv1.Eviction{} err := k8sClient.Get(ctx, hypervisorName, eviction) By(fmt.Sprintf("%+v", *eviction)) @@ -240,7 +239,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { }) // Spec.Maintenance="" Context("Spec.Maintenance=\"ha\"", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Spec.Maintenance = "ha" @@ -248,7 +247,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { expectedBody := `{"disabled_reason": "Hypervisor CRD: spec.maintenance=ha", "status": "disabled"}` mockServiceUpdate(expectedBody) }) - It("should not create an eviction resource", func() { + It("should not create an eviction resource", func(ctx SpecContext) { eviction := &kvmv1.Eviction{} err := k8sClient.Get(ctx, hypervisorName, eviction) Expect(err).To(HaveOccurred()) @@ -258,7 +257,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { for _, mode := range []string{"auto", "manual"} { Context(fmt.Sprintf("Spec.Maintenance=\"%v\"", mode), func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) hypervisor.Spec.Maintenance = mode @@ -268,12 +267,12 @@ var _ = Describe("HypervisorMaintenanceController", func() { }) When("there is no eviction yet", func() { - It("should create an eviction resource named as the hypervisor", func() { + It("should create an eviction resource named as the hypervisor", func(ctx SpecContext) { eviction := &kvmv1.Eviction{} Expect(k8sClient.Get(ctx, hypervisorName, eviction)).To(Succeed()) }) - It("should create an evicting condition", func() { + It("should create an evicting condition", func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) Expect(hypervisor.Status.Conditions).To(ContainElement( @@ -285,7 +284,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { )) }) - It("should reflect it in the hypervisor evicted status", func() { + It("should reflect it in the hypervisor evicted status", func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) Expect(hypervisor.Status.Evicted).To(BeFalse()) @@ -293,7 +292,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { }) When("there is a finished eviction", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { eviction := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, Spec: kvmv1.EvictionSpec{ @@ -316,7 +315,7 @@ var _ = Describe("HypervisorMaintenanceController", func() { Expect(k8sClient.Status().Update(ctx, eviction)).To(Succeed()) }) - It("should reflect it in the hypervisor evicting condition", func() { + It("should reflect it in the hypervisor evicting condition", func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) Expect(hypervisor.Status.Conditions).To(ContainElement( @@ -328,13 +327,13 @@ var _ = Describe("HypervisorMaintenanceController", func() { )) }) - It("should reflect it in the hypervisor evicted status", func() { + It("should reflect it in the hypervisor evicted status", func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) Expect(hypervisor.Status.Evicted).To(BeTrue()) }) - It("should set the ConditionTypeReady to false and reason to evicted", func() { + It("should set the ConditionTypeReady to false and reason to evicted", func(ctx SpecContext) { updated := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) Expect(updated.Status.Conditions).To(ContainElement( diff --git a/internal/controller/node_certificate_controller_test.go b/internal/controller/node_certificate_controller_test.go index 1e0943da..b704006a 100644 --- a/internal/controller/node_certificate_controller_test.go +++ b/internal/controller/node_certificate_controller_test.go @@ -41,7 +41,7 @@ var _ = Describe("Node Certificate Controller", func() { // Setup - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { By("Setting up the test environment") scheme := runtime.NewScheme() Expect(corev1.AddToScheme(scheme)).To(Succeed()) @@ -72,7 +72,7 @@ var _ = Describe("Node Certificate Controller", func() { Expect(fakeClient.Create(ctx, resource)).To(Succeed()) }) - AfterEach(func() { + AfterEach(func(ctx SpecContext) { node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}} By("Cleanup the specific node") Expect(client.IgnoreAlreadyExists(fakeClient.Delete(ctx, node))).To(Succeed()) @@ -83,7 +83,7 @@ var _ = Describe("Node Certificate Controller", func() { // Tests Context("When reconciling a node with nova virt label", func() { - It("should successfully create a new certificate", func() { + It("should successfully create a new certificate", func(ctx SpecContext) { By("Reconciling the node") req := ctrl.Request{ NamespacedName: types.NamespacedName{Name: nodeName}, diff --git a/internal/controller/node_eviction_label_controller_test.go b/internal/controller/node_eviction_label_controller_test.go index c7c9769e..6866a555 100644 --- a/internal/controller/node_eviction_label_controller_test.go +++ b/internal/controller/node_eviction_label_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "os" "github.com/gophercloud/gophercloud/v2/testhelper" @@ -45,7 +44,7 @@ var _ = Describe("Node Eviction Label Controller", func() { reconciler *NodeEvictionLabelReconciler req = ctrl.Request{NamespacedName: types.NamespacedName{Name: nodeName}} fakeServer testhelper.FakeServer - reconcileNodeLoop = func(ctx context.Context, steps int) (res ctrl.Result, err error) { + reconcileNodeLoop = func(ctx SpecContext, steps int) (res ctrl.Result, err error) { for range steps { res, err = reconciler.Reconcile(ctx, req) if err != nil { @@ -56,7 +55,7 @@ var _ = Describe("Node Eviction Label Controller", func() { } ) - BeforeEach(func(ctx context.Context) { + BeforeEach(func(ctx SpecContext) { fakeServer = testhelper.SetupHTTP() Expect(os.Setenv("KVM_HA_SERVICE_URL", fakeServer.Endpoint())).To(Succeed()) @@ -88,7 +87,7 @@ var _ = Describe("Node Eviction Label Controller", func() { } Expect(k8sClient.Create(ctx, resource)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { By("Cleanup the specific node") Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) }) @@ -106,7 +105,7 @@ var _ = Describe("Node Eviction Label Controller", func() { }, } Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { By("Cleanup the specific hypervisor") Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed()) }) @@ -114,7 +113,7 @@ var _ = Describe("Node Eviction Label Controller", func() { }) Context("When reconciling a node", func() { - BeforeEach(func(ctx context.Context) { + BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: nodeName}, hypervisor)).To(Succeed()) By("updating the hypervisor status sub-resource") @@ -127,7 +126,7 @@ var _ = Describe("Node Eviction Label Controller", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should successfully reconcile the resource", func(ctx context.Context) { + It("should successfully reconcile the resource", func(ctx SpecContext) { By("ConditionType the created resource") _, err := reconcileNodeLoop(ctx, 5) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/onboarding_controller_test.go b/internal/controller/onboarding_controller_test.go index 2f50d935..7cce58f7 100644 --- a/internal/controller/onboarding_controller_test.go +++ b/internal/controller/onboarding_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "net/http" "os" @@ -213,7 +212,7 @@ var _ = Describe("Onboarding Controller", func() { namespacedName = types.NamespacedName{Name: hypervisorName} fakeServer testhelper.FakeServer reconcileReq = ctrl.Request{NamespacedName: namespacedName} - reconcileLoop = func(ctx context.Context, steps int) (err error) { + reconcileLoop = func(ctx SpecContext, steps int) (err error) { for range steps { _, err = onboardingReconciler.Reconcile(ctx, reconcileReq) if err != nil { @@ -224,7 +223,7 @@ var _ = Describe("Onboarding Controller", func() { } ) - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { By("creating the resource for the Kind Hypervisor") hv := &kvmv1.Hypervisor{ ObjectMeta: metav1.ObjectMeta{ @@ -241,7 +240,7 @@ var _ = Describe("Onboarding Controller", func() { } Expect(k8sClient.Create(ctx, hv)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { By("Cleanup the specific hypervisor CRO") Expect(k8sClient.Delete(ctx, hv)).To(Succeed()) }) @@ -389,7 +388,7 @@ var _ = Describe("Onboarding Controller", func() { }) Context("running tests after initial setup", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hv := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) hv.Status.HypervisorID = hypervisorId @@ -497,14 +496,14 @@ var _ = Describe("Onboarding Controller", func() { }) When("SkipTests is set to true", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hv := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) hv.Spec.SkipTests = true Expect(k8sClient.Update(ctx, hv)).To(Succeed()) }) - It("should update the conditions", func() { + It("should update the conditions", func(ctx SpecContext) { By("Reconciling the created resource") err := reconcileLoop(ctx, 3) Expect(err).NotTo(HaveOccurred()) @@ -524,14 +523,14 @@ var _ = Describe("Onboarding Controller", func() { }) }) When("SkipTests is set to false", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { hv := &kvmv1.Hypervisor{} Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) hv.Spec.SkipTests = false Expect(k8sClient.Update(ctx, hv)).To(Succeed()) }) - It("should update the conditions", func() { + It("should update the conditions", func(ctx SpecContext) { By("Reconciling the created resource") err := reconcileLoop(ctx, 3) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index fa36940c..9e039bf9 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "path/filepath" "runtime" @@ -44,8 +43,6 @@ import ( var cfg *rest.Config var k8sClient client.Client var testEnv *envtest.Environment -var ctx context.Context -var cancel context.CancelFunc func TestControllers(t *testing.T) { RegisterFailHandler(Fail) @@ -56,8 +53,6 @@ func TestControllers(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) - ctx, cancel = context.WithCancel(context.TODO()) - By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, @@ -91,7 +86,6 @@ var _ = BeforeSuite(func() { var _ = AfterSuite(func() { By("tearing down the test environment") - cancel() err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) diff --git a/internal/controller/traits_controller_test.go b/internal/controller/traits_controller_test.go index 3c14aacb..40dd5ca5 100644 --- a/internal/controller/traits_controller_test.go +++ b/internal/controller/traits_controller_test.go @@ -18,7 +18,6 @@ limitations under the License. package controller import ( - "context" "fmt" "net/http" @@ -63,7 +62,7 @@ var _ = Describe("TraitsController", func() { // Setup and teardown - BeforeEach(func(ctx context.Context) { + BeforeEach(func(ctx SpecContext) { By("Setting up the OpenStack http mock server") fakeServer = testhelper.SetupHTTP() DeferCleanup(fakeServer.Teardown) @@ -87,7 +86,7 @@ var _ = Describe("TraitsController", func() { } Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { + DeferCleanup(func(ctx SpecContext) { By("Deleting the Hypervisor resource") hypervisor := &kvmv1.Hypervisor{} Expect(tc.Client.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) @@ -98,7 +97,7 @@ var _ = Describe("TraitsController", func() { // Tests Context("Reconcile after onboarding before decommissioning", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { // Mock resourceproviders.GetTraits fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") @@ -146,7 +145,7 @@ var _ = Describe("TraitsController", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should update traits and set status condition when traits differ", func() { + It("should update traits and set status condition when traits differ", func(ctx SpecContext) { req := ctrl.Request{NamespacedName: hypervisorName} _, err := tc.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -159,7 +158,7 @@ var _ = Describe("TraitsController", func() { }) Context("Reconcile before onboarding", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { // Mock resourceproviders.GetTraits fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) { defer GinkgoRecover() @@ -178,7 +177,7 @@ var _ = Describe("TraitsController", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should not update traits", func() { + It("should not update traits", func(ctx SpecContext) { req := ctrl.Request{NamespacedName: hypervisorName} _, err := tc.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -191,7 +190,7 @@ var _ = Describe("TraitsController", func() { }) Context("Reconcile when terminating", func() { - BeforeEach(func() { + BeforeEach(func(ctx SpecContext) { // Mock resourceproviders.GetTraits fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) { defer GinkgoRecover() @@ -221,7 +220,7 @@ var _ = Describe("TraitsController", func() { }) - It("should not update traits", func() { + It("should not update traits", func(ctx SpecContext) { req := ctrl.Request{NamespacedName: hypervisorName} _, err := tc.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) From f625a35343fde248576776f7b32e363c2348d923 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 19 Dec 2025 13:58:27 +0100 Subject: [PATCH 2/2] Hypervisor: Sync terminating to spec on create too If we do not reflect the terminating state right on creation, controllers might try to onboard the new hypervisor until the next reconciliation. --- internal/controller/hypervisor_controller.go | 4 ++ .../controller/hypervisor_controller_test.go | 49 +++++++++++++------ 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/internal/controller/hypervisor_controller.go b/internal/controller/hypervisor_controller.go index 37c80d6e..55ab06fa 100644 --- a/internal/controller/hypervisor_controller.go +++ b/internal/controller/hypervisor_controller.go @@ -147,6 +147,10 @@ func (hv *HypervisorController) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, fmt.Errorf("failed setting controller reference: %w", err) } + if IsNodeConditionPresentAndEqual(node.Status.Conditions, "Terminating", corev1.ConditionTrue) { + hypervisor.Spec.Maintenance = kvmv1.MaintenanceTermination + } + if err := hv.Create(ctx, hypervisor, k8sclient.FieldOwner(HypervisorControllerName)); err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/hypervisor_controller_test.go b/internal/controller/hypervisor_controller_test.go index 8822c438..b98fc39c 100644 --- a/internal/controller/hypervisor_controller_test.go +++ b/internal/controller/hypervisor_controller_test.go @@ -31,8 +31,14 @@ import ( ) var _ = Describe("Hypervisor Controller", func() { - var hypervisorController *HypervisorController - var resource *corev1.Node + const ( + resourceName = "other-node" + ) + var ( + hypervisorController *HypervisorController + resource *corev1.Node + hypervisorName = types.NamespacedName{Name: resourceName} + ) BeforeEach(func(ctx SpecContext) { hypervisorController = &HypervisorController{ @@ -40,6 +46,10 @@ var _ = Describe("Hypervisor Controller", func() { Scheme: k8sClient.Scheme(), } + DeferCleanup(func() { + hypervisorController = nil + }) + By("creating the namespace for the reconciler") ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "monsoon3"}} Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed()) @@ -47,7 +57,7 @@ var _ = Describe("Hypervisor Controller", func() { // pregenerate the resource resource = &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ - Name: "other-node", + Name: resourceName, Labels: map[string]string{corev1.LabelTopologyZone: "test-zone"}, Annotations: map[string]string{annotationCustomTraits: "test-trait"}, }, @@ -55,9 +65,8 @@ var _ = Describe("Hypervisor Controller", func() { }) AfterEach(func(ctx SpecContext) { - node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: resource.Name}} By("Cleanup the specific node") - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed()) + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, resource))).To(Succeed()) hypervisor := &kvmv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: resource.Name}} By("Cleanup the specific hypervisor") @@ -78,11 +87,11 @@ var _ = Describe("Hypervisor Controller", func() { By("should have created the Hypervisor resource") // Get the Hypervisor resource hypervisor := &kvmv1.Hypervisor{} - hypervisorName := types.NamespacedName{Name: resource.Name} - Expect(hypervisorController.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) Expect(hypervisor.Name).To(Equal(resource.Name)) Expect(hypervisor.Labels).ToNot(BeNil()) Expect(hypervisor.Labels[corev1.LabelTopologyZone]).To(Equal("test-zone")) + Expect(hypervisor.Spec.Maintenance).To(BeEmpty()) By("Adding a label annotation to the node and reconciling again") // Add an aggregate annotation to the node @@ -98,11 +107,12 @@ var _ = Describe("Hypervisor Controller", func() { By("should have updated the Hypervisor resource with the aggregate") // Get the Hypervisor resource again updatedHypervisor := &kvmv1.Hypervisor{} - Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed()) + Expect(k8sClient.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed()) Expect(updatedHypervisor.Name).To(Equal(resource.Name)) Expect(updatedHypervisor.Spec.CustomTraits).ToNot(BeNil()) Expect(updatedHypervisor.Spec.CustomTraits).To(ContainElement("test-trait")) Expect(updatedHypervisor.Labels).To(HaveKeyWithValue("worker.garden.sapcloud.io/group", "new-group")) + Expect(updatedHypervisor.Spec.Maintenance).To(BeEmpty()) }) }) @@ -121,8 +131,16 @@ var _ = Describe("Hypervisor Controller", func() { }) Expect(k8sClient.Status().Update(ctx, resource)).To(Succeed()) + _, err := hypervisorController.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: resource.Name}, + }) + Expect(err).NotTo(HaveOccurred()) + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(hypervisor.Spec.Maintenance).To(Equal(kvmv1.MaintenanceTermination)) + By("Reconciling the created resource") - for range 3 { + for range 2 { _, err := hypervisorController.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{Name: resource.Name}, }) @@ -131,17 +149,16 @@ var _ = Describe("Hypervisor Controller", func() { By("should have set the terminating condition on the Hypervisor resource") // Get the Hypervisor resource - hypervisor := &kvmv1.Hypervisor{} - hypervisorName := types.NamespacedName{Name: resource.Name} - Expect(hypervisorController.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) - Expect(hypervisor.Name).To(Equal(resource.Name)) - Expect(hypervisor.Status.Conditions).ToNot(BeNil()) - condition := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeReady) + updatedHypervisor := &kvmv1.Hypervisor{} + Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed()) + Expect(updatedHypervisor.Name).To(Equal(resource.Name)) + Expect(updatedHypervisor.Status.Conditions).ToNot(BeNil()) + condition := meta.FindStatusCondition(updatedHypervisor.Status.Conditions, kvmv1.ConditionTypeReady) Expect(condition).ToNot(BeNil()) Expect(condition.Reason).To(Equal("Terminating")) Expect(condition.Status).To(Equal(metav1.ConditionFalse)) - condition = meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeTerminating) + condition = meta.FindStatusCondition(updatedHypervisor.Status.Conditions, kvmv1.ConditionTypeTerminating) Expect(condition).ToNot(BeNil()) Expect(condition.Reason).To(Equal("Terminating")) Expect(condition.Status).To(Equal(metav1.ConditionTrue))