diff --git a/internal/controller/traits_controller.go b/internal/controller/traits_controller.go index 19b71ff..f56866f 100644 --- a/internal/controller/traits_controller.go +++ b/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 diff --git a/internal/controller/traits_controller_test.go b/internal/controller/traits_controller_test.go index 54772db..776ec2e 100644 --- a/internal/controller/traits_controller_test.go +++ b/internal/controller/traits_controller_test.go @@ -266,6 +266,38 @@ 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) + Expect(err).To(HaveOccurred()) + }) + }) + Context("Reconcile when terminating", func() { BeforeEach(func(ctx SpecContext) { // Mock resourceproviders.GetTraits diff --git a/internal/openstack/placement.go b/internal/openstack/placement.go index 77695d2..c73bae6 100644 --- a/internal/openstack/placement.go +++ b/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 fmt.Errorf("deleteConsumerAllocations failed for consumer %s: %w", consumerID, err) + } } // We are done, let's clean it up diff --git a/internal/openstack/placement_test.go b/internal/openstack/placement_test.go index ccbb734..ab83635 100644 --- a/internal/openstack/placement_test.go +++ b/internal/openstack/placement_test.go @@ -298,6 +298,46 @@ 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) + Expect(err).To(HaveOccurred()) + }) + }) + Context("when deleting provider fails", func() { BeforeEach(func() { fakeServer.Mux.HandleFunc(providerAllocsURL, func(w http.ResponseWriter, r *http.Request) {