Onboarding: Remove additional aggregates

If the offboarding failed for any reason, we might be have left-over
associations with other aggregates. For robustness reasons, let's
handle that case here as well.

Author: fwiesel

internal/controller/onboarding_controller.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"slices"
 	"strings"
 	"time"
 
@@ -205,6 +206,22 @@ func (r *OnboardingController) initialOnboarding(ctx context.Context, hv *kvmv1.
 		return fmt.Errorf("failed to agg to test aggregate %w", err)
 	}
 
+	var errs []error
+	for aggregateName, aggregate := range aggs {
+		if aggregateName == testAggregateName || aggregateName == zone {
+			continue
+		}
+		if slices.Contains(aggregate.Hosts, host) {
+			if err := removeFromAggregate(ctx, r.computeClient, aggs, host, aggregateName); err != nil {
+				errs = append(errs, err)
+			}
+		}
+	}
+
+	if len(errs) > 0 {
+		return fmt.Errorf("failed to remove host %v from aggregates due to %w", host, errors.Join(errs...))
+	}
+
 	// The service may be forced down previously due to an HA event,
 	// so we need to ensure it not only enabled, but also not forced to be down.
 	falseVal := false

internal/controller/onboarding_controller_test.go

@@ -62,6 +62,32 @@ var _ = Describe("Onboarding Controller", func() {
 	]
 }`
 
+		aggregatesBodyUnexpected = `{
+	"aggregates": [
+		{
+			"name": "test-az",
+			"availability_zone": "test-az",
+			"deleted": false,
+			"id": 100001,
+			"hosts": []
+		},
+		{
+			"name": "tenant_filter_tests",
+			"availability_zone": "",
+			"deleted": false,
+			"id": 99,
+			"hosts": []
+		},
+		{
+			"name": "unexpected",
+			"availability_zone": "",
+			"deleted": false,
+			"id": -1,
+			"hosts": ["test-host"]
+		}
+	]
+}`
+
 		aggregatesBodySetup = `{
     "aggregates": [
 		{
@@ -249,12 +275,6 @@ var _ = Describe("Onboarding Controller", func() {
 				w.WriteHeader(http.StatusOK)
 				Expect(fmt.Fprintf(w, HypervisorWithServers, serviceId, "", hypervisorName)).ToNot(BeNil())
 			})
-			fakeServer.Mux.HandleFunc("GET /os-aggregates", func(w http.ResponseWriter, r *http.Request) {
-				w.Header().Add("Content-Type", "application/json")
-				w.WriteHeader(http.StatusOK)
-				_, err := fmt.Fprint(w, aggregatesBodyInitial)
-				Expect(err).NotTo(HaveOccurred())
-			})
 
 			fakeServer.Mux.HandleFunc("POST /os-aggregates/100001/action", func(w http.ResponseWriter, r *http.Request) {
 				w.Header().Add("Content-Type", "application/json")
@@ -278,34 +298,93 @@ var _ = Describe("Onboarding Controller", func() {
 			})
 		})
 
-		It("should set the Service- and HypervisorId from Nova", func(ctx SpecContext) {
-			By("Reconciling the created resource")
-			err := reconcileLoop(ctx, 1)
-			Expect(err).NotTo(HaveOccurred())
-			hv := &kvmv1.Hypervisor{}
-			Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
-			Expect(hv.Status.ServiceID).To(Equal(serviceId))
-			Expect(hv.Status.HypervisorID).To(Equal(hypervisorId))
+		When("it is a clean setup", func() {
+			BeforeEach(func() {
+				fakeServer.Mux.HandleFunc("GET /os-aggregates", func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Add("Content-Type", "application/json")
+					w.WriteHeader(http.StatusOK)
+					_, err := fmt.Fprint(w, aggregatesBodyInitial)
+					Expect(err).NotTo(HaveOccurred())
+				})
+			})
+
+			It("should set the Service- and HypervisorId from Nova", func(ctx SpecContext) {
+				By("Reconciling the created resource")
+				err := reconcileLoop(ctx, 1)
+				Expect(err).NotTo(HaveOccurred())
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				Expect(hv.Status.ServiceID).To(Equal(serviceId))
+				Expect(hv.Status.HypervisorID).To(Equal(hypervisorId))
+			})
+
+			It("should update the status accordingly", func(ctx SpecContext) {
+				By("Reconciling the created resource")
+				err := reconcileLoop(ctx, 2)
+				Expect(err).NotTo(HaveOccurred())
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				Expect(hv.Status.Conditions).To(ContainElements(
+					SatisfyAll(
+						HaveField("Type", kvmv1.ConditionTypeReady),
+						HaveField("Status", metav1.ConditionFalse),
+						HaveField("Reason", ConditionReasonOnboarding),
+					),
+					SatisfyAll(
+						HaveField("Type", ConditionTypeOnboarding),
+						HaveField("Status", metav1.ConditionTrue),
+						HaveField("Reason", ConditionReasonTesting),
+					),
+				))
+			})
 		})
 
-		It("should update the status accordingly", func(ctx SpecContext) {
-			By("Reconciling the created resource")
-			err := reconcileLoop(ctx, 2)
-			Expect(err).NotTo(HaveOccurred())
-			hv := &kvmv1.Hypervisor{}
-			Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
-			Expect(hv.Status.Conditions).To(ContainElements(
-				SatisfyAll(
-					HaveField("Type", kvmv1.ConditionTypeReady),
-					HaveField("Status", metav1.ConditionFalse),
-					HaveField("Reason", ConditionReasonOnboarding),
-				),
-				SatisfyAll(
-					HaveField("Type", ConditionTypeOnboarding),
-					HaveField("Status", metav1.ConditionTrue),
-					HaveField("Reason", ConditionReasonTesting),
-				),
-			))
+		When("it the host is already in an unexpected aggregate", func() {
+			BeforeEach(func() {
+				fakeServer.Mux.HandleFunc("GET /os-aggregates", func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Add("Content-Type", "application/json")
+					w.WriteHeader(http.StatusOK)
+					_, err := fmt.Fprint(w, aggregatesBodyUnexpected)
+					Expect(err).NotTo(HaveOccurred())
+				})
+
+				fakeServer.Mux.HandleFunc("POST /os-aggregates/-1/action", func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Add("Content-Type", "application/json")
+					w.WriteHeader(http.StatusOK)
+					_, err := fmt.Fprint(w, addedHostToTestBody)
+					Expect(err).NotTo(HaveOccurred())
+				})
+			})
+
+			It("should set the Service- and HypervisorId from Nova", func(ctx SpecContext) {
+				By("Reconciling the created resource")
+				err := reconcileLoop(ctx, 1)
+				Expect(err).NotTo(HaveOccurred())
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				Expect(hv.Status.ServiceID).To(Equal(serviceId))
+				Expect(hv.Status.HypervisorID).To(Equal(hypervisorId))
+			})
+
+			It("should update the status accordingly", func(ctx SpecContext) {
+				By("Reconciling the created resource")
+				err := reconcileLoop(ctx, 2)
+				Expect(err).NotTo(HaveOccurred())
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				Expect(hv.Status.Conditions).To(ContainElements(
+					SatisfyAll(
+						HaveField("Type", kvmv1.ConditionTypeReady),
+						HaveField("Status", metav1.ConditionFalse),
+						HaveField("Reason", ConditionReasonOnboarding),
+					),
+					SatisfyAll(
+						HaveField("Type", ConditionTypeOnboarding),
+						HaveField("Status", metav1.ConditionTrue),
+						HaveField("Reason", ConditionReasonTesting),
+					),
+				))
+			})
 		})
 	})