Propagate more errors

Both in the traits controller as well as in the placement-api
calls, some errors were swallowed.

Author: fwiesel

internal/controller/traits_controller.go

@@ -106,7 +106,7 @@ func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		base := hv.DeepCopy()
 		if meta.SetStatusCondition(&hv.Status.Conditions,
 			getTraitCondition(err, "Failed to get current traits from placement")) {
-			err = errors.Join(tc.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
+			err = errors.Join(err, tc.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
 				k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(TraitsControllerName)))
 		}
 		return ctrl.Result{}, err

internal/controller/traits_controller_test.go

@@ -266,6 +266,41 @@ var _ = Describe("TraitsController", func() {
 		})
 	})
 
+	Context("Reconcile when GetTraits returns an error", func() {
+		BeforeEach(func(ctx SpecContext) {
+			// Mock resourceproviders.GetTraits to return a server error
+			fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(http.StatusInternalServerError)
+				fmt.Fprint(w, `{"error": "Internal Server Error"}`)
+			})
+			// UpdateTraits must not be called
+			fakeServer.Mux.HandleFunc("PUT /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) {
+				defer GinkgoRecover()
+				Fail("should not be called")
+			})
+
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
+				Type:   kvmv1.ConditionTypeOnboarding,
+				Status: metav1.ConditionFalse,
+				Reason: "UnitTest",
+			})
+			hypervisor.Status.HypervisorID = "1234"
+			hypervisor.Status.Traits = []string{"CUSTOM_FOO", "HW_CPU_X86_VMX"}
+			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
+		})
+
+		It("should return the GetTraits error", func(ctx SpecContext) {
+			req := ctrl.Request{NamespacedName: hypervisorName}
+			_, err := traitsController.Reconcile(ctx, req)
+			// Bug: err is reassigned to errors.Join(patchErr) instead of
+			// errors.Join(originalErr, patchErr), so when the status patch
+			// succeeds the original GetTraits error is silently dropped.
+			Expect(err).To(HaveOccurred())
+		})
+	})
+
 	Context("Reconcile when terminating", func() {
 		BeforeEach(func(ctx SpecContext) {
 			// Mock resourceproviders.GetTraits

internal/openstack/placement.go

@@ -151,7 +151,9 @@ func CleanupResourceProvider(ctx context.Context, client *gophercloud.ServiceCli
 
 		// The consumer actually doesn't have *any* allocations, so it is just
 		// inconsistent, and we can drop them all
-		deleteConsumerAllocations(ctx, client, consumerID)
+		if err := deleteConsumerAllocations(ctx, client, consumerID).Err; err != nil {
+			return err
+		}
 	}
 
 	// We are done, let's clean it up

internal/openstack/placement_test.go

@@ -298,6 +298,49 @@ var _ = Describe("Placement API", func() {
 			})
 		})
 
+		Context("when deleting consumer allocations fails", func() {
+			BeforeEach(func() {
+				fakeServer.Mux.HandleFunc(providerAllocsURL, func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Set("Content-Type", "application/json")
+					w.WriteHeader(http.StatusOK)
+					fmt.Fprint(w, `{"allocations": {"consumer-1": {}}}`)
+				})
+
+				fakeServer.Mux.HandleFunc("/allocations/consumer-1", func(w http.ResponseWriter, r *http.Request) {
+					switch r.Method {
+					case http.MethodGet:
+						// Consumer has no allocations of its own — proceed to delete
+						w.Header().Set("Content-Type", "application/json")
+						w.WriteHeader(http.StatusOK)
+						fmt.Fprint(w, `{"allocations": {}, "consumer_generation": 0}`)
+					case http.MethodDelete:
+						// Deletion fails
+						w.WriteHeader(http.StatusInternalServerError)
+						fmt.Fprint(w, `{"error": "Internal Server Error"}`)
+					}
+				})
+
+				// The provider delete succeeds, so that it cannot accidentally surface
+				// the error for us — only the consumer allocation delete error should be returned.
+				fakeServer.Mux.HandleFunc(deleteProviderURL, func(w http.ResponseWriter, r *http.Request) {
+					w.WriteHeader(http.StatusNoContent)
+				})
+			})
+
+			It("should return an error when consumer allocation deletion fails", func() {
+				provider := &resourceproviders.ResourceProvider{
+					UUID: providerUUID,
+					Name: "test-provider",
+				}
+
+				err := CleanupResourceProvider(ctx, client.ServiceClient(fakeServer), provider)
+				// Bug: deleteConsumerAllocations return value is discarded, so this
+				// error is silently swallowed and the function proceeds to delete the
+				// provider and returns nil instead of the deletion error.
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
 		Context("when deleting provider fails", func() {
 			BeforeEach(func() {
 				fakeServer.Mux.HandleFunc(providerAllocsURL, func(w http.ResponseWriter, r *http.Request) {