Aggregates: Only reconcile after onboarding and before decomissioning

Onboarding needs to set its own aggregates for testing purposes,
and decomissining needs to remove the aggregates.
To avoid these controllers fighting, let's limit this controller
to the actual active lifetime of the hypervisor.

Unfortunately, that means we have to reconcile on every status update.

Author: fwiesel

internal/controller/aggregates_controller.go

@@ -30,7 +30,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	logger "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	"github.com/gophercloud/gophercloud/v2"
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/aggregates"
@@ -63,6 +62,12 @@ func (ac *AggregatesController) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
 	}
 
+	/// On- and off-boarding need to mess with the aggregates, so let's get out of their way
+	if !meta.IsStatusConditionFalse(hv.Status.Conditions, ConditionTypeOnboarding) ||
+		meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) {
+		return ctrl.Result{}, nil
+	}
+
 	if slices.Equal(hv.Spec.Aggregates, hv.Status.Aggregates) {
 		// Nothing to be done
 		return ctrl.Result{}, nil
@@ -154,7 +159,6 @@ func (ac *AggregatesController) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		Named(AggregatesControllerName).
 		For(&kvmv1.Hypervisor{}, builder.WithPredicates(utils.LifecycleEnabledPredicate)).
-		WithEventFilter(predicate.GenerationChangedPredicate{}).
 		Complete(ac)
 }
 

internal/controller/aggregates_controller_test.go

@@ -35,18 +35,15 @@ import (
 )
 
 var _ = Describe("AggregatesController", func() {
-	var (
-		tc         *AggregatesController
-		fakeServer testhelper.FakeServer
-	)
-	const EOF = "EOF"
-	const AggregateListBodyEmpty = `
+	const (
+		EOF                    = "EOF"
+		AggregateListBodyEmpty = `
 {
     "aggregates": []
 }
 `
 
-	const AggregateListBodyFull = `
+		AggregateListBodyFull = `
 {
     "aggregates": [
 		{
@@ -67,7 +64,7 @@ var _ = Describe("AggregatesController", func() {
 }
 `
 
-	const AggregatesPostBody = `
+		AggregatesPostBody = `
 {
     "aggregate": {
 		"name": "test-aggregate1",
@@ -77,7 +74,7 @@ var _ = Describe("AggregatesController", func() {
     }
 }`
 
-	const AggregateRemoveHostBody = `
+		AggregateRemoveHostBody = `
 {
         "aggregate": {
 			"name": "test-aggregate3",
@@ -87,7 +84,7 @@ var _ = Describe("AggregatesController", func() {
 		}
 }`
 
-	const AggregateAddHostBody = `
+		AggregateAddHostBody = `
 {
     "aggregate": {
             "name": "test-aggregate1",
@@ -99,47 +96,63 @@ var _ = Describe("AggregatesController", func() {
             "id": 42
         }
 }`
+	)
 
+	var (
+		tc             *AggregatesController
+		fakeServer     testhelper.FakeServer
+		hypervisorName = types.NamespacedName{Name: "hv-test"}
+	)
 	// Setup and teardown
 
 	BeforeEach(func(ctx context.Context) {
 		By("Setting up the OpenStack http mock server")
 		fakeServer = testhelper.SetupHTTP()
+		DeferCleanup(fakeServer.Teardown)
+
+		hypervisor := &kvmv1.Hypervisor{
+			ObjectMeta: v1.ObjectMeta{
+				Name: hypervisorName.Name,
+			},
+			Spec: kvmv1.HypervisorSpec{
+				LifecycleEnabled: true,
+			},
+		}
+		Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
+		Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+		meta.SetStatusCondition(&hypervisor.Status.Conditions, v1.Condition{
+			Type:    ConditionTypeOnboarding,
+			Status:  v1.ConditionFalse,
+			Reason:  "dontcare",
+			Message: "dontcare",
+		})
+		Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
 
 		By("Creating the AggregatesController")
 		tc = &AggregatesController{
 			Client:        k8sClient,
 			Scheme:        k8sClient.Scheme(),
 			computeClient: client.ServiceClient(fakeServer),
 		}
-	})
 
-	AfterEach(func() {
-		By("Tearing down the OpenStack http mock server")
-		fakeServer.Teardown()
+		DeferCleanup(func(ctx context.Context) {
+			Expect(tc.Client.Delete(ctx, hypervisor)).To(Succeed())
+		})
+	})
 
-		By("Deleting the Hypervisor resource")
-		hypervisor := &kvmv1.Hypervisor{}
-		Expect(tc.Client.Get(ctx, types.NamespacedName{Name: "hv-test", Namespace: "default"}, hypervisor)).To(Succeed())
-		Expect(tc.Client.Delete(ctx, hypervisor)).To(Succeed())
+	JustBeforeEach(func(ctx context.Context) {
+		_, err := tc.Reconcile(ctx, ctrl.Request{NamespacedName: hypervisorName})
+		Expect(err).NotTo(HaveOccurred())
 	})
 
 	// Tests
-
 	Context("Adding new Aggregate", func() {
 		BeforeEach(func() {
-			By("Creating a Hypervisor resource")
-			hypervisor := &kvmv1.Hypervisor{
-				ObjectMeta: v1.ObjectMeta{
-					Name:      "hv-test",
-					Namespace: "default",
-				},
-				Spec: kvmv1.HypervisorSpec{
-					LifecycleEnabled: true,
-					Aggregates:       []string{"test-aggregate1"},
-				},
-			}
-			Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
+			By("Setting a missing aggregate")
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+			hypervisor.Spec.Aggregates = []string{"test-aggregate1"}
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
 
 			// Mock resourceproviders.GetAggregates
 			fakeServer.Mux.HandleFunc("GET /os-aggregates", func(w http.ResponseWriter, r *http.Request) {
@@ -176,32 +189,18 @@ var _ = Describe("AggregatesController", func() {
 			})
 		})
 
-		It("should update Aggregates and set status condition when Aggregates differ", func() {
-			req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "hv-test", Namespace: "default"}}
-			_, err := tc.Reconcile(ctx, req)
-			Expect(err).NotTo(HaveOccurred())
-
+		It("should update Aggregates and set status condition as Aggregates differ", func() {
 			updated := &kvmv1.Hypervisor{}
-			Expect(tc.Client.Get(ctx, req.NamespacedName, updated)).To(Succeed())
+			Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
 			Expect(updated.Status.Aggregates).To(ContainElements("test-aggregate1"))
 			Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, ConditionTypeAggregatesUpdated)).To(BeTrue())
 		})
 	})
 
 	Context("Removing Aggregate", func() {
 		BeforeEach(func() {
-			By("Creating a Hypervisor resource")
-			hypervisor := &kvmv1.Hypervisor{
-				ObjectMeta: v1.ObjectMeta{
-					Name:      "hv-test",
-					Namespace: "default",
-				},
-				Spec: kvmv1.HypervisorSpec{
-					LifecycleEnabled: true,
-				},
-			}
-			Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
-
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
 			// update status to have existing aggregate
 			hypervisor.Status.Aggregates = []string{"test-aggregate2", "test-aggregate3"}
 			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
@@ -235,14 +234,49 @@ var _ = Describe("AggregatesController", func() {
 		})
 
 		It("should update Aggregates and set status condition when Aggregates differ", func() {
-			req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "hv-test", Namespace: "default"}}
-			_, err := tc.Reconcile(ctx, req)
-			Expect(err).NotTo(HaveOccurred())
-
 			updated := &kvmv1.Hypervisor{}
-			Expect(tc.Client.Get(ctx, req.NamespacedName, updated)).To(Succeed())
+			Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
 			Expect(updated.Status.Aggregates).To(BeEmpty())
 			Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, ConditionTypeAggregatesUpdated)).To(BeTrue())
 		})
 	})
+
+	Context("before onboarding", func() {
+		BeforeEach(func() {
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+			// Remove the onboarding condition
+			hypervisor.Status.Conditions = []v1.Condition{}
+			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
+		})
+
+		It("should neither update Aggregates and nor set status condition", func() {
+			updated := &kvmv1.Hypervisor{}
+			Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Status.Aggregates).To(BeEmpty())
+			Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, ConditionTypeAggregatesUpdated)).To(BeFalse())
+		})
+	})
+
+	Context("when terminating", func() {
+		BeforeEach(func() {
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+			// Remove the onboarding condition
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, v1.Condition{
+				Type:    kvmv1.ConditionTypeTerminating,
+				Status:  v1.ConditionTrue,
+				Reason:  "dontcare",
+				Message: "dontcare",
+			})
+			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
+		})
+
+		It("should neither update Aggregates and nor set status condition", func() {
+			updated := &kvmv1.Hypervisor{}
+			Expect(tc.Client.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Status.Aggregates).To(BeEmpty())
+			Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, ConditionTypeAggregatesUpdated)).To(BeFalse())
+		})
+	})
 })