Fix cleanup revisited

internal/controller/onboarding_controller.go

@@ -53,6 +53,8 @@ var errRequeue = errors.New("requeue requested")
 const (
 	defaultWaitTime           = 1 * time.Minute
 	ConditionTypeOnboarding   = "Onboarding"
+	ConditionReasonAborted    = "aborted"
+	ConditionReasonError      = "error"
 	ConditionReasonInitial    = "initial"
 	ConditionReasonOnboarding = "onboarding"
 	ConditionReasonTesting    = "testing"
@@ -87,17 +89,18 @@ func (r *OnboardingController) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
 	}
 
+	computeHost := hv.Name
+
 	// check if lifecycle management is enabled
 	if !hv.Spec.LifecycleEnabled {
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost)
 	}
 
 	// check if hv is terminating
 	if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) {
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, r.abortOnboarding(ctx, hv, computeHost)
 	}
 
-	computeHost := hv.Name
 	// We bail here out, because the openstack api is not the best to poll
 	if hv.Status.HypervisorID == "" || hv.Status.ServiceID == "" {
 		if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
@@ -144,6 +147,46 @@ func (r *OnboardingController) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 }
 
+func (r *OnboardingController) abortOnboarding(ctx context.Context, hv *kvmv1.Hypervisor, computeHost string) error {
+	status := meta.FindStatusCondition(hv.Status.Conditions, ConditionTypeOnboarding)
+	// Never onboarded
+	if status == nil {
+		return nil
+	}
+
+	changed := meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+		Type:    kvmv1.ConditionTypeReady,
+		Status:  metav1.ConditionFalse,
+		Reason:  ConditionReasonOnboarding,
+		Message: "Onboarding aborted",
+	})
+
+	if meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+		Type:    ConditionTypeOnboarding,
+		Status:  metav1.ConditionFalse,
+		Reason:  ConditionReasonAborted,
+		Message: "Aborted due to LifecycleEnabled being false",
+	}) {
+		changed = true
+	}
+
+	if !changed {
+		// Already aborted
+		return nil
+	}
+	if err := r.deleteTestServers(ctx, computeHost); err != nil {
+		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+			Type:    ConditionTypeOnboarding,
+			Status:  metav1.ConditionTrue, // No cleanup, so we are still "onboarding"
+			Reason:  ConditionReasonAborted,
+			Message: err.Error(),
+		})
+
+		return errors.Join(err, r.Status().Update(ctx, hv))
+	}
+	return r.Status().Update(ctx, hv)
+}
+
 func (r *OnboardingController) initialOnboarding(ctx context.Context, hv *kvmv1.Hypervisor, host string) error {
 	zone, found := hv.Labels[corev1.LabelTopologyZone]
 	if !found || zone == "" {
@@ -261,27 +304,18 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis
 func (r *OnboardingController) completeOnboarding(ctx context.Context, host string, hv *kvmv1.Hypervisor) (ctrl.Result, error) {
 	log := logger.FromContext(ctx)
 
-	serverPrefix := fmt.Sprintf("%v-%v", testPrefixName, host)
-
-	serverPages, err := servers.ListSimple(r.testComputeClient, servers.ListOpts{
-		Name: serverPrefix,
-	}).AllPages(ctx)
-
-	if err != nil && !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
-		return ctrl.Result{}, err
-	}
-
-	serverList, err := servers.ExtractServers(serverPages)
+	err := r.deleteTestServers(ctx, host)
 	if err != nil {
-		return ctrl.Result{}, err
-	}
-
-	for _, server := range serverList {
-		log.Info("deleting server", "name", server.Name)
-		err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr()
-		if err != nil && !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
-			return ctrl.Result{}, err
+		if meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+			Type:    ConditionTypeOnboarding,
+			Status:  metav1.ConditionTrue, // No cleanup, so we are still "onboarding"
+			Reason:  ConditionReasonAborted,
+			Message: err.Error(),
+		}) {
+			return ctrl.Result{}, errors.Join(err, r.Status().Update(ctx, hv))
 		}
+
+		return ctrl.Result{}, err
 	}
 
 	aggs, err := aggregatesByName(ctx, r.computeClient)
@@ -319,6 +353,35 @@ func (r *OnboardingController) completeOnboarding(ctx context.Context, host stri
 	return ctrl.Result{}, r.Status().Update(ctx, hv)
 }
 
+func (r *OnboardingController) deleteTestServers(ctx context.Context, host string) error {
+	log := logger.FromContext(ctx)
+	serverPrefix := fmt.Sprintf("%v-%v", testPrefixName, host)
+
+	serverPages, err := servers.ListSimple(r.testComputeClient, servers.ListOpts{
+		Name: serverPrefix,
+	}).AllPages(ctx)
+
+	if err != nil && !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
+		return err
+	}
+
+	serverList, err := servers.ExtractServers(serverPages)
+	if err != nil {
+		return err
+	}
+
+	errs := make([]error, 0)
+	for _, server := range serverList {
+		log.Info("deleting server", "name", server.Name)
+		err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr()
+		if err != nil && !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
+			errs = append(errs, err)
+		}
+	}
+
+	return errors.Join(errs...)
+}
+
 func (r *OnboardingController) ensureNovaProperties(ctx context.Context, hv *kvmv1.Hypervisor) error {
 	hypervisorAddress := hv.Labels[corev1.LabelHostname]
 	if hypervisorAddress == "" {
@@ -392,7 +455,7 @@ func (r *OnboardingController) createOrGetTestServer(ctx context.Context, zone,
 			continue
 		}
 		if server.Name != serverName || foundServer != nil {
-			log.Info("deleting server", "name", server.Name)
+			log.Info("deleting outdated server", "name", server.Name)
 			err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr()
 			if err != nil && !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
 				log.Error(err, "failed deleting instance due to", "instance", server.ID)

internal/controller/onboarding_controller_test.go

@@ -31,19 +31,20 @@ import (
 )
 
 var _ = Describe("Onboarding Controller", func() {
+	const (
+		hypervisorName = "some-test"
+	)
+
 	var (
 		onboardingReconciler *OnboardingController
+		namespacedName       = types.NamespacedName{Name: hypervisorName}
 	)
 
 	Context("When reconciling a hypervisor", func() {
-		const hypervisorName = "some-test"
-
 		ctx := context.Background() //nolint:govet
 
 		reconcileLoop := func(steps int) (res ctrl.Result, err error) {
-			req := ctrl.Request{
-				NamespacedName: types.NamespacedName{Name: hypervisorName},
-			}
+			req := ctrl.Request{NamespacedName: namespacedName}
 			for range steps {
 				res, err = onboardingReconciler.Reconcile(ctx, req)
 				if err != nil {
@@ -60,19 +61,18 @@ var _ = Describe("Onboarding Controller", func() {
 			}
 
 			By("creating the resource for the Kind Hypervisor")
-			resource := &kvmv1.Hypervisor{
+			hv := &kvmv1.Hypervisor{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: hypervisorName,
 				},
 				Spec: kvmv1.HypervisorSpec{},
 			}
-			Expect(k8sClient.Create(ctx, resource)).To(Succeed())
-		})
+			Expect(k8sClient.Create(ctx, hv)).To(Succeed())
 
-		AfterEach(func() {
-			hv := &kvmv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: hypervisorName}}
-			By("Cleanup the specific hypervisor CRO")
-			Expect(client.IgnoreAlreadyExists(k8sClient.Delete(ctx, hv))).To(Succeed())
+			DeferCleanup(func(ctx context.Context) {
+				By("Cleanup the specific hypervisor CRO")
+				Expect(client.IgnoreAlreadyExists(k8sClient.Delete(ctx, hv))).To(Succeed())
+			})
 		})
 
 		It("should successfully reconcile the resource", func() {