Rework Onboarding

internal/controller/onboarding_controller.go

@@ -235,17 +235,19 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis
 	zone := hv.Labels[corev1.LabelTopologyZone]
 	server, err := r.createOrGetTestServer(ctx, zone, host, hv.UID)
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to create or get test instance: %w", err)
-	}
-
-	setTestingCondition := func(message string) error {
-		utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions,
+		// Surface the failure in the Onboarding condition so the user can see
+		// the latest error and confirm the controller is still retrying.
+		// Returning a bare error here would leave the previously-set message
+		// in place and the resource looks frozen even though we keep retrying.
+		if err2 := r.applyOnboardingCondition(ctx, hv,
 			*k8sacmetav1.Condition().
 				WithType(kvmv1.ConditionTypeOnboarding).
 				WithStatus(metav1.ConditionTrue).
 				WithReason(kvmv1.ConditionReasonTesting).
-				WithMessage(message))
-		return apply()
+				WithMessage(fmt.Sprintf("failed to create or get test instance: %v", err))); err2 != nil {
+			return ctrl.Result{}, err2
+		}
+		return ctrl.Result{RequeueAfter: defaultWaitTime}, nil
 	}
 
 	switch server.Status {
@@ -258,8 +260,16 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis
 			return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil
 		}
 
-		// Set condition back to testing
-		if err = setTestingCondition("Server ended up in error state: " + server.Fault.Message); err != nil {
+		// Set condition back to testing. Include the server ID so each retry
+		// (which creates a fresh instance with a new UUID) yields a different
+		// message — otherwise SetApplyConfigurationStatusCondition would treat
+		// the update as a no-op when Nova keeps reporting the same fault text.
+		if err = r.applyOnboardingCondition(ctx, hv,
+			*k8sacmetav1.Condition().
+				WithType(kvmv1.ConditionTypeOnboarding).
+				WithStatus(metav1.ConditionTrue).
+				WithReason(kvmv1.ConditionReasonTesting).
+				WithMessage(fmt.Sprintf("Server %s ended up in error state: %s", id, server.Fault.Message))); err != nil {
 			return ctrl.Result{}, err
 		}
 
@@ -275,15 +285,25 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis
 			ShowConsoleOutput(ctx, r.testComputeClient, server.ID, servers.ShowConsoleOutputOpts{Length: 11}).
 			Extract()
 		if err != nil {
-			if err2 := setTestingCondition(fmt.Sprintf("could not get console output %v", err)); err2 != nil {
+			if err2 := r.applyOnboardingCondition(ctx, hv,
+				*k8sacmetav1.Condition().
+					WithType(kvmv1.ConditionTypeOnboarding).
+					WithStatus(metav1.ConditionTrue).
+					WithReason(kvmv1.ConditionReasonTesting).
+					WithMessage(fmt.Sprintf("could not get console output for server %s: %v", server.ID, err))); err2 != nil {
 				return ctrl.Result{}, err2
 			}
 			return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil
 		}
 
 		if !strings.Contains(consoleOutput, server.Name) {
 			if !server.LaunchedAt.IsZero() && r.Clock.Now().After(server.LaunchedAt.Add(smokeTestTimeout)) {
-				if err2 := setTestingCondition(fmt.Sprintf("timeout waiting for console output since %v", server.LaunchedAt)); err2 != nil {
+				if err2 := r.applyOnboardingCondition(ctx, hv,
+					*k8sacmetav1.Condition().
+						WithType(kvmv1.ConditionTypeOnboarding).
+						WithStatus(metav1.ConditionTrue).
+						WithReason(kvmv1.ConditionReasonTesting).
+						WithMessage(fmt.Sprintf("timeout waiting for console output of server %s since %v", server.ID, server.LaunchedAt))); err2 != nil {
 					return ctrl.Result{}, err2
 				}
 				if err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr(); err != nil {
@@ -296,7 +316,12 @@ func (r *OnboardingController) smokeTest(ctx context.Context, hv *kvmv1.Hypervis
 		}
 
 		if err = servers.Delete(ctx, r.testComputeClient, server.ID).ExtractErr(); err != nil {
-			if err2 := setTestingCondition(fmt.Sprintf("failed to terminate instance %v", err)); err2 != nil {
+			if err2 := r.applyOnboardingCondition(ctx, hv,
+				*k8sacmetav1.Condition().
+					WithType(kvmv1.ConditionTypeOnboarding).
+					WithStatus(metav1.ConditionTrue).
+					WithReason(kvmv1.ConditionReasonTesting).
+					WithMessage(fmt.Sprintf("failed to terminate instance %s: %v", server.ID, err))); err2 != nil {
 				return ctrl.Result{}, err2
 			}
 			return ctrl.Result{RequeueAfter: r.getRequeueInterval()}, nil
@@ -356,6 +381,22 @@ func (r *OnboardingController) completeOnboarding(ctx context.Context, host stri
 	return ctrl.Result{}, apply()
 }
 
+// applyOnboardingCondition applies a single Onboarding condition via SSA.
+// It seeds the previously-stored Onboarding condition so it is not pruned,
+// then upserts the new value preserving LastTransitionTime when Status is unchanged.
+func (r *OnboardingController) applyOnboardingCondition(ctx context.Context, hv *kvmv1.Hypervisor, cond k8sacmetav1.ConditionApplyConfiguration) error {
+	statusCfg := apiv1.HypervisorStatus().
+		WithHypervisorID(hv.Status.HypervisorID).
+		WithServiceID(hv.Status.ServiceID)
+	if c := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding); c != nil {
+		statusCfg.WithConditions(utils.ConditionFromStatus(*c))
+	}
+	utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, cond)
+	return r.Status().Apply(ctx,
+		apiv1.Hypervisor(hv.Name).WithStatus(statusCfg),
+		k8sclient.ForceOwnership, k8sclient.FieldOwner(OnboardingControllerName))
+}
+
 func (r *OnboardingController) deleteTestServers(ctx context.Context, host string) error {
 	log := logger.FromContext(ctx)
 	serverPrefix := fmt.Sprintf("%v-%v", testPrefixName, host)

internal/controller/onboarding_controller_test.go

@@ -336,6 +336,7 @@ var _ = Describe("Onboarding Controller", func() {
 	Context("running tests after initial setup", func() {
 		var (
 			serverActionHandler func(http.ResponseWriter, *http.Request)
+			serverCreateHandler func(http.ResponseWriter, *http.Request)
 			serverDeleteHandler func(http.ResponseWriter, *http.Request)
 			serverDetailHandler func(http.ResponseWriter, *http.Request)
 		)
@@ -397,11 +398,14 @@ var _ = Describe("Onboarding Controller", func() {
 				Expect(err).NotTo(HaveOccurred())
 			})
 
-			fakeServer.Mux.HandleFunc("POST /servers", func(w http.ResponseWriter, r *http.Request) {
+			serverCreateHandler = func(w http.ResponseWriter, _ *http.Request) {
 				w.Header().Add("Content-Type", "application/json")
 				w.WriteHeader(http.StatusOK)
 				_, err := fmt.Fprint(w, createServerBody)
 				Expect(err).NotTo(HaveOccurred())
+			}
+			fakeServer.Mux.HandleFunc("POST /servers", func(w http.ResponseWriter, r *http.Request) {
+				serverCreateHandler(w, r)
 			})
 
 			serverActionHandler = func(w http.ResponseWriter, _ *http.Request) {
@@ -632,13 +636,126 @@ var _ = Describe("Onboarding Controller", func() {
 				By("Verifying the timed-out server was deleted")
 				Expect(serverDeletedCalled).To(BeTrue())
 
-				By("Verifying the onboarding condition message indicates a timeout")
+				By("Verifying the onboarding condition message indicates a timeout and includes the server ID")
 				hv := &kvmv1.Hypervisor{}
 				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
 				onboardingCond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
 				Expect(onboardingCond).NotTo(BeNil())
 				Expect(onboardingCond.Reason).To(Equal(kvmv1.ConditionReasonTesting))
 				Expect(onboardingCond.Message).To(ContainSubstring("timeout"))
+				Expect(onboardingCond.Message).To(ContainSubstring("server-id"))
+			})
+		})
+
+		When("test server creation fails", func() {
+			BeforeEach(func(ctx SpecContext) {
+				By("Overriding HV status to Testing state")
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+					Type:    kvmv1.ConditionTypeOnboarding,
+					Status:  metav1.ConditionTrue,
+					Reason:  kvmv1.ConditionReasonTesting,
+					Message: "previously stuck on something else",
+				})
+				Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed())
+			})
+
+			It("should surface the error in the Onboarding condition and requeue without returning an error", func(ctx SpecContext) {
+				By("Overriding the POST /servers handler to return 500")
+				serverCreateHandler = func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(http.StatusInternalServerError)
+					_, err := fmt.Fprint(w, `{"computeFault": {"message": "Cinder volume backend is degraded"}}`)
+					Expect(err).NotTo(HaveOccurred())
+				}
+
+				By("Reconciling: createOrGetTestServer should fail at POST /servers")
+				result, err := onboardingReconciler.Reconcile(ctx, reconcileReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result.RequeueAfter).To(Equal(defaultWaitTime))
+
+				By("Verifying the Onboarding condition reflects the create error")
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				onboardingCond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
+				Expect(onboardingCond).NotTo(BeNil())
+				Expect(onboardingCond.Status).To(Equal(metav1.ConditionTrue))
+				Expect(onboardingCond.Reason).To(Equal(kvmv1.ConditionReasonTesting))
+				Expect(onboardingCond.Message).To(ContainSubstring("failed to create or get test instance"))
+			})
+		})
+
+		When("test server is in ERROR state", func() {
+			var (
+				serverGetCalled    bool
+				serverDeleteCalled bool
+			)
+
+			BeforeEach(func(ctx SpecContext) {
+				By("Overriding HV status to Testing state")
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
+					Type:    kvmv1.ConditionTypeOnboarding,
+					Status:  metav1.ConditionTrue,
+					Reason:  kvmv1.ConditionReasonTesting,
+					Message: "Running onboarding tests",
+				})
+				Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed())
+
+				serverName := fmt.Sprintf("%s-%s-%s", testPrefixName, hypervisorName, string(hv.UID))
+				serverGetCalled = false
+				serverDeleteCalled = false
+
+				// GET /servers/detail returns a single ERROR-state server with the
+				// expected name so createOrGetTestServer returns it as foundServer.
+				serverDetailHandler = func(w http.ResponseWriter, _ *http.Request) {
+					_, err := fmt.Fprintf(w,
+						`{"servers": [{"id": "server-id", "name": %q, "status": "ERROR"}], "servers_links": []}`,
+						serverName)
+					Expect(err).NotTo(HaveOccurred())
+				}
+
+				// GET /servers/server-id returns the ERROR server with a fault
+				// message so smokeTest can record it.
+				fakeServer.Mux.HandleFunc("GET /servers/server-id", func(w http.ResponseWriter, _ *http.Request) {
+					serverGetCalled = true
+					w.Header().Add("Content-Type", "application/json")
+					w.WriteHeader(http.StatusOK)
+					_, err := fmt.Fprintf(w,
+						`{"server": {"id": "server-id", "name": %q, "status": "ERROR", "fault": {"message": "Build of instance aborted: volume creation failed"}}}`,
+						serverName)
+					Expect(err).NotTo(HaveOccurred())
+				})
+
+				serverDeleteHandler = func(w http.ResponseWriter, _ *http.Request) {
+					serverDeleteCalled = true
+					w.WriteHeader(http.StatusAccepted)
+				}
+			})
+
+			It("should record the failure with the server UUID and issue a delete, without returning an error", func(ctx SpecContext) {
+				By("Reconciling once to enter the ERROR branch")
+				result, err := onboardingReconciler.Reconcile(ctx, reconcileReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result.RequeueAfter).To(Equal(defaultWaitTime))
+
+				By("Verifying GET on the server happened so the fault could be read")
+				Expect(serverGetCalled).To(BeTrue())
+
+				By("Verifying the server delete was attempted")
+				Expect(serverDeleteCalled).To(BeTrue())
+
+				By("Verifying the Onboarding condition message includes the server UUID and the fault text")
+				hv := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed())
+				onboardingCond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
+				Expect(onboardingCond).NotTo(BeNil())
+				Expect(onboardingCond.Status).To(Equal(metav1.ConditionTrue))
+				Expect(onboardingCond.Reason).To(Equal(kvmv1.ConditionReasonTesting))
+				Expect(onboardingCond.Message).To(ContainSubstring("server-id"))
+				Expect(onboardingCond.Message).To(ContainSubstring("ended up in error state"))
+				Expect(onboardingCond.Message).To(ContainSubstring("Build of instance aborted"))
 			})
 		})