From 0dc7aa427e85a475b8842a26d79c93759d5146bf Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Fri, 30 Oct 2026 14:21:22 +0100 Subject: [PATCH] Set TraitsUpdated when the recorded traits match the spec The reconcile returned early when the desired and the recorded custom traits matched, without ever setting TraitsUpdated. That condition gates the handover phase, so hypervisors without custom traits could never finish onboarding. Report success from the recorded state in that case. Placement is only queried when the spec and the recorded traits diverge, as before, and the comparison is now order-insensitive. While such a sync runs, the applied set is read from placement instead of the status cache, so stray custom traits are removed in a single pass. Signed-off-by: Fabian Wiesel --- internal/controller/traits_controller.go | 40 ++++++---- internal/controller/traits_controller_test.go | 79 +++++++++++++++++++ 2 files changed, 105 insertions(+), 14 deletions(-) diff --git a/internal/controller/traits_controller.go b/internal/controller/traits_controller.go index ef031870..600902f0 100644 --- a/internal/controller/traits_controller.go +++ b/internal/controller/traits_controller.go @@ -85,29 +85,30 @@ func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, nil } - customTraitsApplied := slices.Collect(func(yield func(string) bool) { - for _, trait := range hv.Status.Traits { - if strings.HasPrefix(trait, customPrefix) && !yield(trait) { - return - } - } - }) + statusCustomTraits := customTraitsOf(hv.Status.Traits) - if slices.Equal(hv.Spec.CustomTraits, customTraitsApplied) { - // Nothing to be done - return ctrl.Result{}, nil + if slicesEqualUnordered(hv.Spec.CustomTraits, statusCustomTraits) { + // Nothing to apply; only ensure the success condition is set, + // since it gates the handover phase. + if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated) { + return ctrl.Result{}, nil + } + return ctrl.Result{}, tc.applyTraitsStatus(ctx, hv, hv.Status.Traits, + getTraitCondition(nil, "Traits in sync")) } - toAdd := utils.Difference(customTraitsApplied, hv.Spec.CustomTraits) - toRemove := utils.Difference(hv.Spec.CustomTraits, customTraitsApplied) - - // fetch current traits, to ensure we don't add duplicates + // fetch the traits currently set in placement current, err := resourceproviders.GetTraits(ctx, tc.serviceClient, hv.Status.HypervisorID).Extract() if err != nil { return ctrl.Result{}, errors.Join(err, tc.applyTraitsStatus(ctx, hv, hv.Status.Traits, getTraitCondition(err, "Failed to get current traits from placement"))) } + // the applied set is what placement currently reports + appliedCustomTraits := customTraitsOf(current.Traits) + toAdd := utils.Difference(appliedCustomTraits, hv.Spec.CustomTraits) + toRemove := utils.Difference(hv.Spec.CustomTraits, appliedCustomTraits) + var targetTraits []string slices.Sort(current.Traits) for _, trait := range current.Traits { @@ -185,6 +186,17 @@ func getTraitCondition(err error, msg string) metav1.Condition { } } +// customTraitsOf returns the subset of traits carrying the CUSTOM_ prefix. +func customTraitsOf(traits []string) []string { + return slices.Collect(func(yield func(string) bool) { + for _, trait := range traits { + if strings.HasPrefix(trait, customPrefix) && !yield(trait) { + return + } + } + }) +} + // registerWithManager registers the controller with the Manager without acquiring OpenStack clients. // This is useful for testing where clients are injected directly. func (tc *TraitsController) registerWithManager(mgr ctrl.Manager) error { diff --git a/internal/controller/traits_controller_test.go b/internal/controller/traits_controller_test.go index ec37094e..72cfffbd 100644 --- a/internal/controller/traits_controller_test.go +++ b/internal/controller/traits_controller_test.go @@ -264,6 +264,85 @@ var _ = Describe("TraitsController", func() { }) }) + Context("Reconcile during onboarding Handover phase with no custom traits", func() { + BeforeEach(func(ctx SpecContext) { + // Mock resourceproviders.GetTraits + fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) { + defer GinkgoRecover() + Fail("should not be called") + }) + // Mock resourceproviders.UpdateTraits + 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()) + hypervisor.Spec.CustomTraits = nil + Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed()) + + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonHandover, + }) + hypervisor.Status.HypervisorID = "1234" + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + }) + + It("should set the TraitsUpdated condition without contacting placement", func(ctx SpecContext) { + req := ctrl.Request{NamespacedName: hypervisorName} + _, err := traitsController.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + + updated := &kvmv1.Hypervisor{} + Expect(traitsController.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) + Expect(updated.Status.Traits).To(BeEmpty()) + Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated)).To(BeTrue()) + Expect(meta.FindStatusCondition(updated.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated).Reason).To(Equal(kvmv1.ConditionReasonSucceeded)) + }) + }) + + Context("Reconcile when the recorded custom traits already match the spec", func() { + BeforeEach(func(ctx SpecContext) { + // Mock resourceproviders.GetTraits + fakeServer.Mux.HandleFunc("GET /resource_providers/1234/traits", func(w http.ResponseWriter, r *http.Request) { + defer GinkgoRecover() + Fail("should not be called") + }) + // Mock resourceproviders.UpdateTraits + 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.ConditionTrue, + Reason: kvmv1.ConditionReasonHandover, + }) + hypervisor.Status.HypervisorID = "1234" + hypervisor.Status.Traits = []string{"CUSTOM_BAR", "HW_CPU_X86_VMX", "CUSTOM_FOO"} + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + }) + + It("should set the TraitsUpdated condition without contacting placement", func(ctx SpecContext) { + req := ctrl.Request{NamespacedName: hypervisorName} + _, err := traitsController.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + + updated := &kvmv1.Hypervisor{} + Expect(traitsController.Client.Get(ctx, hypervisorName, updated)).To(Succeed()) + Expect(updated.Status.Traits).To(Equal([]string{"CUSTOM_BAR", "HW_CPU_X86_VMX", "CUSTOM_FOO"})) + Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated)).To(BeTrue()) + Expect(meta.FindStatusCondition(updated.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated).Reason).To(Equal(kvmv1.ConditionReasonSucceeded)) + }) + }) + Context("Reconcile when GetTraits returns an error", func() { BeforeEach(func(ctx SpecContext) { // Mock resourceproviders.GetTraits to return a server error