From 40e4306347d34e0900558962cb9ddb41303c47d5 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:45:35 +0200 Subject: [PATCH 1/4] Add SSA conditions helpers in internal/utils Introduce ConditionFromStatus and SetApplyConfigurationStatusCondition as the foundation for migrating all controllers to server-side apply of status subresources. ConditionFromStatus converts a single metav1.Condition (as stored in a status subresource) to a ConditionApplyConfiguration suitable for SSA. ObservedGeneration is preserved so gating logic (e.g. the taint controller's skip-check) keeps working. SetApplyConfigurationStatusCondition mirrors the semantics of meta.SetStatusCondition on the apply-configuration slice: append or update by Type, refresh LastTransitionTime only when Status changes, and guard against nil/empty Type to prevent panics. Together they enable per-condition seeding so each controller claims field ownership only over the conditions it actually manages, without disturbing conditions owned by other field managers on Apply. --- internal/utils/conditions.go | 136 ++++++++++++++ internal/utils/conditions_test.go | 285 ++++++++++++++++++++++++++++++ 2 files changed, 421 insertions(+) create mode 100644 internal/utils/conditions.go create mode 100644 internal/utils/conditions_test.go diff --git a/internal/utils/conditions.go b/internal/utils/conditions.go new file mode 100644 index 0000000..98f5e7d --- /dev/null +++ b/internal/utils/conditions.go @@ -0,0 +1,136 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" +) + +// ConditionFromStatus converts a single metav1.Condition into a +// *k8sacmetav1.ConditionApplyConfiguration, preserving all fields verbatim +// (including LastTransitionTime and ObservedGeneration). +// +// Controllers must seed their apply-config payload with the conditions they +// own before mutating them. With +listType=map / +listMapKey=type on the CRD, +// SSA merges conditions by the "type" key: conditions owned by other field +// managers are left untouched, and conditions previously written by this +// field manager that are absent from the payload are pruned. Therefore each +// controller should include all conditions it has previously set — and only +// those — in every Apply call. +func ConditionFromStatus(c metav1.Condition) *k8sacmetav1.ConditionApplyConfiguration { + return &k8sacmetav1.ConditionApplyConfiguration{ + Type: &c.Type, + Status: &c.Status, + Reason: &c.Reason, + Message: &c.Message, + LastTransitionTime: &c.LastTransitionTime, + ObservedGeneration: &c.ObservedGeneration, + } +} + +// SetApplyConfigurationStatusCondition sets the corresponding condition in +// conditions to newCondition and returns true if the conditions are changed by +// this call. It mirrors the behaviour of k8s.io/apimachinery/pkg/api/meta.SetStatusCondition: +// +// 1. If a condition of the specified type does not exist, newCondition is +// appended. LastTransitionTime is set to now if not provided. +// 2. If a condition of the specified type already exists, all fields are +// updated. LastTransitionTime is updated to now (or the provided value) +// only when Status changes; otherwise the existing LastTransitionTime is +// preserved. +func SetApplyConfigurationStatusCondition(conditions *[]k8sacmetav1.ConditionApplyConfiguration, newCondition k8sacmetav1.ConditionApplyConfiguration) (changed bool) { + if conditions == nil { + return false + } + if newCondition.Type == nil || *newCondition.Type == "" { + return false + } + + // Find existing entry by type + var existing *k8sacmetav1.ConditionApplyConfiguration + for i := range *conditions { + if (*conditions)[i].Type != nil && *(*conditions)[i].Type == *newCondition.Type { + existing = &(*conditions)[i] + break + } + } + + if existing == nil { + // New condition: set LastTransitionTime if not provided + if newCondition.LastTransitionTime == nil { + now := metav1.Now() + newCondition.LastTransitionTime = &now + } + *conditions = append(*conditions, newCondition) + return true + } + + // Existing condition: update fields, handle LastTransitionTime + statusChanged := (existing.Status == nil) != (newCondition.Status == nil) || + (existing.Status != nil && *existing.Status != *newCondition.Status) + + if statusChanged { + existing.Status = newCondition.Status + if newCondition.LastTransitionTime != nil { + existing.LastTransitionTime = newCondition.LastTransitionTime + } else { + now := metav1.Now() + existing.LastTransitionTime = &now + } + changed = true + } + // When status is unchanged, LastTransitionTime is intentionally preserved. + + if strChanged(existing.Reason, newCondition.Reason) { + existing.Reason = newCondition.Reason + changed = true + } + if strChanged(existing.Message, newCondition.Message) { + existing.Message = newCondition.Message + changed = true + } + if int64Changed(existing.ObservedGeneration, newCondition.ObservedGeneration) { + existing.ObservedGeneration = newCondition.ObservedGeneration + changed = true + } + + return changed +} + +// strChanged reports whether the pointer value of b differs from a. +func strChanged(a, b *string) bool { + if a == nil && b == nil { + return false + } + if a == nil || b == nil { + return true + } + return *a != *b +} + +// int64Changed reports whether the pointer value of b differs from a. +func int64Changed(a, b *int64) bool { + if a == nil && b == nil { + return false + } + if a == nil || b == nil { + return true + } + return *a != *b +} diff --git a/internal/utils/conditions_test.go b/internal/utils/conditions_test.go new file mode 100644 index 0000000..b9a4f80 --- /dev/null +++ b/internal/utils/conditions_test.go @@ -0,0 +1,285 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" +) + +func TestUtils(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Utils Suite") +} + +var _ = Describe("ConditionFromStatus", func() { + It("copies all fields verbatim", func() { + ts := metav1.NewTime(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)) + gen := int64(42) + c := metav1.Condition{ + Type: "Ready", + Status: metav1.ConditionTrue, + Reason: "AllGood", + Message: "everything is fine", + LastTransitionTime: ts, + ObservedGeneration: gen, + } + got := ConditionFromStatus(c) + Expect(*got.Type).To(Equal("Ready")) + Expect(*got.Status).To(Equal(metav1.ConditionTrue)) + Expect(*got.Reason).To(Equal("AllGood")) + Expect(*got.Message).To(Equal("everything is fine")) + Expect(*got.LastTransitionTime).To(Equal(ts)) + Expect(*got.ObservedGeneration).To(Equal(gen)) + }) + + It("includes ObservedGeneration even when zero", func() { + c := metav1.Condition{Type: "T", Status: metav1.ConditionFalse, Reason: "R"} + got := ConditionFromStatus(c) + Expect(got.ObservedGeneration).NotTo(BeNil()) + Expect(*got.ObservedGeneration).To(Equal(int64(0))) + }) +}) + +var _ = Describe("SetApplyConfigurationStatusCondition", func() { + ptr := func(s string) *string { return &s } + condStatus := func(s metav1.ConditionStatus) *metav1.ConditionStatus { return &s } + + Describe("nil / empty-type guards", func() { + It("returns false and does not panic on nil slice pointer", func() { + changed := SetApplyConfigurationStatusCondition(nil, + *k8sacmetav1.Condition().WithType("T").WithStatus(metav1.ConditionTrue).WithReason("R")) + Expect(changed).To(BeFalse()) + }) + + It("returns false when Type is nil", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + changed := SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{}) + Expect(changed).To(BeFalse()) + Expect(conditions).To(BeEmpty()) + }) + + It("returns false when Type is empty string", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition().WithType("")) + Expect(changed).To(BeFalse()) + Expect(conditions).To(BeEmpty()) + }) + }) + + Describe("appending a new condition", func() { + It("appends when the type is not yet present", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK")) + Expect(changed).To(BeTrue()) + Expect(conditions).To(HaveLen(1)) + Expect(*conditions[0].Type).To(Equal("Ready")) + }) + + It("sets LastTransitionTime to now when not provided", func() { + before := time.Now() + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("T"). + WithStatus(metav1.ConditionTrue). + WithReason("R")) + Expect(conditions[0].LastTransitionTime).NotTo(BeNil()) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("preserves a caller-supplied LastTransitionTime", func() { + ts := metav1.NewTime(time.Date(2020, 6, 1, 0, 0, 0, 0, time.UTC)) + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("T"). + WithStatus(metav1.ConditionTrue). + WithReason("R"). + WithLastTransitionTime(ts)) + Expect(*conditions[0].LastTransitionTime).To(Equal(ts)) + }) + }) + + Describe("updating an existing condition", func() { + var ( + oldTS metav1.Time + conditions []k8sacmetav1.ConditionApplyConfiguration + ) + + BeforeEach(func() { + oldTS = metav1.NewTime(time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)) + conditions = []k8sacmetav1.ConditionApplyConfiguration{ + { + Type: ptr("Ready"), + Status: condStatus(metav1.ConditionFalse), + Reason: ptr("NotReady"), + Message: ptr("waiting"), + LastTransitionTime: &oldTS, + }, + } + }) + + It("updates Status and refreshes LastTransitionTime when Status changes", func() { + before := time.Now() + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK"). + WithMessage("done")) + Expect(changed).To(BeTrue()) + Expect(*conditions[0].Status).To(Equal(metav1.ConditionTrue)) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("preserves LastTransitionTime when Status is unchanged", func() { + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("StillNotReady"). + WithMessage("still waiting")) + Expect(changed).To(BeTrue()) // Reason/Message changed + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("returns false when nothing changed", func() { + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(changed).To(BeFalse()) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("updates Reason independently of Status", func() { + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("DifferentReason"). + WithMessage("waiting")) + Expect(changed).To(BeTrue()) + Expect(*conditions[0].Reason).To(Equal("DifferentReason")) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("uses a caller-supplied LastTransitionTime when Status changes", func() { + newTS := metav1.NewTime(time.Date(2025, 6, 1, 0, 0, 0, 0, time.UTC)) + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK"). + WithLastTransitionTime(newTS)) + Expect(*conditions[0].LastTransitionTime).To(Equal(newTS)) + }) + + It("does not affect other conditions in the slice", func() { + otherTS := metav1.NewTime(time.Date(2019, 1, 1, 0, 0, 0, 0, time.UTC)) + conditions = append(conditions, k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Other"), + Status: condStatus(metav1.ConditionTrue), + LastTransitionTime: &otherTS, + }) + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK")) + Expect(*conditions[1].LastTransitionTime).To(Equal(otherTS)) + }) + + It("updates ObservedGeneration and reports changed", func() { + gen := int64(5) + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting"). + WithObservedGeneration(gen)) + Expect(changed).To(BeTrue()) + Expect(*conditions[0].ObservedGeneration).To(Equal(gen)) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("detects change when existing Status is nil", func() { + conditions[0].Status = nil + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(changed).To(BeTrue()) + Expect(*conditions[0].Status).To(Equal(metav1.ConditionFalse)) + }) + + It("does not detect a change when both Status values are nil", func() { + conditions[0].Status = nil + changed := SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Ready"), + Reason: ptr("NotReady"), + Message: ptr("waiting"), + // Status intentionally nil + }) + Expect(changed).To(BeFalse()) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("detects change when existing Reason is nil", func() { + conditions[0].Reason = nil + changed := SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(changed).To(BeTrue()) + Expect(*conditions[0].Reason).To(Equal("NotReady")) + }) + + It("detects change when new Message is nil but existing is not", func() { + changed := SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Ready"), + Status: condStatus(metav1.ConditionFalse), + Reason: ptr("NotReady"), + // Message intentionally nil + }) + Expect(changed).To(BeTrue()) + Expect(conditions[0].Message).To(BeNil()) + }) + }) +}) From 98a6751c6b6672f8fe75434908bda6aaba983cb3 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:56:25 +0200 Subject: [PATCH 2/4] Migrate HypervisorMaintenanceController to SSA status updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeHypervisorDisabled, ConditionTypeEvicting, and the Evicted scalar. ConditionTypeEvicting is removed by omitting it from the apply — SSA prunes map-list entries no longer claimed by the sole owner. The status config is built once in Reconcile and passed to reconcileComputeService and reconcileEviction which mutate it directly. This eliminates intermediate condition return values and makes the single-apply-per-reconcile explicit. The early-return guard for already-enabled/disabled state is inlined into each branch. Seed only the conditions owned by this controller so SSA does not claim ownership over conditions written by others. For the delete-by-omission case, owned conditions are seeded selectively so the target condition is simply absent from the payload. Also clear the forced_down flag when enabling a compute service so an earlier HA event that set forced_down=true does not leave the service reachable but treated as down. SetupWithManager now delegates to registerWithManager instead of inlining the same ctrl.NewControllerManagedBy call, which had left registerWithManager unreachable. --- .../hypervisor_maintenance_controller.go | 177 +++++++++--------- .../hypervisor_maintenance_controller_test.go | 23 +++ 2 files changed, 109 insertions(+), 91 deletions(-) diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 5c82487..9eefa88 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -24,11 +24,11 @@ import ( "context" "fmt" - "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -38,6 +38,7 @@ import ( "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + apiv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/applyconfigurations/api/v1" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/openstack" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) @@ -58,7 +59,6 @@ type HypervisorMaintenanceController struct { func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hv := &kvmv1.Hypervisor{} if err := hec.Get(ctx, req.NamespacedName, hv); err != nil { - // OnboardingReconciler not found errors, could be deleted return ctrl.Result{}, k8sclient.IgnoreNotFound(err) } @@ -69,148 +69,144 @@ func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req c return ctrl.Result{}, nil } - old := hv.DeepCopy() - - if err := hec.reconcileComputeService(ctx, hv); err != nil { - return ctrl.Result{}, err + // Build status apply config upfront; sub-functions mutate it directly. + // Seed only the HypervisorDisabled condition (the only one always retained). + // reconcileEviction conditionally seeds ConditionTypeEvicting: it is included + // when maintenance is active, and intentionally omitted when MaintenanceUnset + // so that SSA prunes it from the field manager's managed fields. + statusCfg := apiv1.HypervisorStatus().WithEvicted(hv.Status.Evicted) + if c := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled); c != nil { + statusCfg.WithConditions(utils.ConditionFromStatus(*c)) } - if err := hec.reconcileEviction(ctx, hv); err != nil { + if err := hec.reconcileComputeService(ctx, hv, statusCfg); err != nil { return ctrl.Result{}, err } - if equality.Semantic.DeepEqual(hv, old) { - return ctrl.Result{}, nil + if err := hec.reconcileEviction(ctx, hv, statusCfg); err != nil { + return ctrl.Result{}, err } - // Capture only the fields this controller owns - disabledCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) - evictingCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) - evicted := hv.Status.Evicted - - return ctrl.Result{}, utils.PatchHypervisorStatusWithRetry(ctx, hec.Client, hv.Name, HypervisorMaintenanceControllerName, func(h *kvmv1.Hypervisor) { - if disabledCondition != nil { - meta.SetStatusCondition(&h.Status.Conditions, *disabledCondition) - } - if evictingCondition != nil { - meta.SetStatusCondition(&h.Status.Conditions, *evictingCondition) - } else { - meta.RemoveStatusCondition(&h.Status.Conditions, kvmv1.ConditionTypeEvicting) - } - h.Status.Evicted = evicted - }) + return ctrl.Result{}, hec.Status().Apply(ctx, + apiv1.Hypervisor(hv.Name).WithStatus(statusCfg), + k8sclient.ForceOwnership, k8sclient.FieldOwner(HypervisorMaintenanceControllerName)) } -func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.Context, hv *kvmv1.Hypervisor) error { +// reconcileComputeService enables/disables the nova-compute service based on +// hv.Spec.Maintenance and sets the HypervisorDisabled condition on statusCfg. +func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) error { log := logger.FromContext(ctx) serviceId := hv.Status.ServiceID - // We can only do something here, if there is a service to begin with. - // The onboarding should take care of that if serviceId == "" { + // We can only do something here, if there is a service to begin with. + // The onboarding should take care of that. return nil } switch hv.Spec.Maintenance { case kvmv1.MaintenanceUnset: - // Enable the compute service (in case we haven't done so already) - if !meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeHypervisorDisabled, - Status: metav1.ConditionFalse, - Message: "Hypervisor is enabled", - Reason: kvmv1.ConditionReasonSucceeded, - }) { - // Spec matches status - return nil + existing := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) + if existing == nil || existing.Status != metav1.ConditionFalse { + // We need to enable the host as per spec. + // Also clear forced_down in case a previous HA event set it. + falseVal := false + enableService := openstack.UpdateServiceOpts{ + Status: services.ServiceEnabled, + ForcedDown: &falseVal, + } + log.Info("Enabling hypervisor", "id", serviceId) + if _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract(); err != nil { + return fmt.Errorf("failed to enable hypervisor due to %w", err) + } } + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeHypervisorDisabled). + WithStatus(metav1.ConditionFalse). + WithMessage("Hypervisor is enabled"). + WithReason(kvmv1.ConditionReasonSucceeded)) - // We need to enable the host as per spec and clear forced_down - // in case it was set by the HA service during maintenance. - falseVal := false - enableService := openstack.UpdateServiceOpts{ - Status: services.ServiceEnabled, - ForcedDown: &falseVal, - } - log.Info("Enabling hypervisor", "id", serviceId) - _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract() - if err != nil { - return fmt.Errorf("failed to enable hypervisor due to %w", err) - } case kvmv1.MaintenanceManual, kvmv1.MaintenanceAuto, kvmv1.MaintenanceTermination: // Disable the compute service. - if !meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeHypervisorDisabled, - Status: metav1.ConditionTrue, - Message: "Hypervisor is disabled", - Reason: kvmv1.ConditionReasonSucceeded, - }) { - // Spec matches status - return nil - } - - // We need to disable the host as per spec - enableService := services.UpdateOpts{ - Status: services.ServiceDisabled, - DisabledReason: "Hypervisor CRD: spec.maintenance=" + hv.Spec.Maintenance, - } - log.Info("Disabling hypervisor", "id", serviceId) - _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract() - if err != nil { - return fmt.Errorf("failed to disable hypervisor due to %w", err) + existing := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) + if existing == nil || existing.Status != metav1.ConditionTrue { + disableService := services.UpdateOpts{ + Status: services.ServiceDisabled, + DisabledReason: "Hypervisor CRD: spec.maintenance=" + hv.Spec.Maintenance, + } + // We need to disable the host as per spec + log.Info("Disabling hypervisor", "id", serviceId) + if _, err := services.Update(ctx, hec.computeClient, serviceId, disableService).Extract(); err != nil { + return fmt.Errorf("failed to disable hypervisor due to %w", err) + } } + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeHypervisorDisabled). + WithStatus(metav1.ConditionTrue). + WithMessage("Hypervisor is disabled"). + WithReason(kvmv1.ConditionReasonSucceeded)) } return nil } -func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor) error { +// reconcileEviction creates/deletes the Eviction CR and sets the ConditionTypeEvicting +// condition and Evicted scalar on statusCfg. When eviction should be removed, the +// condition entry is filtered out so SSA prunes it. +func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) error { eviction := &kvmv1.Eviction{ - ObjectMeta: metav1.ObjectMeta{ - Name: hv.Name, - }, + ObjectMeta: metav1.ObjectMeta{Name: hv.Name}, } switch hv.Spec.Maintenance { case kvmv1.MaintenanceUnset: // Avoid deleting the eviction over and over. - if hv.Status.Evicted || meta.RemoveStatusCondition(&hv.Status.Conditions, kvmv1.ConditionTypeEvicting) { - err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)) - hv.Status.Evicted = false + if !hv.Status.Evicted && meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) == nil { + return nil + } + if err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)); err != nil { return err } - return nil + // ConditionTypeEvicting is intentionally absent from statusCfg — SSA + // will prune it from this field manager's managed fields on Apply. + statusCfg.WithEvicted(false) + case kvmv1.MaintenanceManual, kvmv1.MaintenanceAuto, kvmv1.MaintenanceTermination: - // In case of "ha", the host gets emptied from the HA service + // In case of "ha", the host gets emptied from the HA service. + // Seed the existing evicting condition so SSA does not prune it, + // regardless of whether we take the short-circuit below. if cond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting); cond != nil { + statusCfg.WithConditions(utils.ConditionFromStatus(*cond)) if cond.Reason == kvmv1.ConditionReasonSucceeded { - // We are done here, no need to look at the eviction any more + // We are done here, no need to look at the eviction any more. return nil } } + status, err := hec.ensureEviction(ctx, eviction, hv) if err != nil { return err } - var reason, message string + var reason, message string if status == metav1.ConditionFalse { message = "Evicted" reason = kvmv1.ConditionReasonSucceeded - hv.Status.Evicted = true + statusCfg.WithEvicted(true) } else { message = "Evicting" reason = kvmv1.ConditionReasonRunning - hv.Status.Evicted = false + statusCfg.WithEvicted(false) } - meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: status, - Reason: reason, - Message: message, - }) - - return nil + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(status). + WithReason(reason). + WithMessage(message)) } return nil @@ -242,9 +238,8 @@ func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, // check if we are still evicting (defaulting to yes) if meta.IsStatusConditionFalse(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting) { return metav1.ConditionFalse, nil - } else { - return metav1.ConditionTrue, nil } + return metav1.ConditionTrue, nil } // registerWithManager registers the controller with the Manager without acquiring OpenStack clients. diff --git a/internal/controller/hypervisor_maintenance_controller_test.go b/internal/controller/hypervisor_maintenance_controller_test.go index bdbe615..4af542b 100644 --- a/internal/controller/hypervisor_maintenance_controller_test.go +++ b/internal/controller/hypervisor_maintenance_controller_test.go @@ -396,6 +396,29 @@ var _ = Describe("HypervisorMaintenanceController", func() { HaveField("Status", metav1.ConditionFalse), ))) }) + + It("should keep the evicting condition on a repeated reconcile (must not be pruned by SSA)", func(ctx SpecContext) { + // Reconciling again with the succeeded state already + // recorded on the Hypervisor takes the early-return + // branch in reconcileEviction. The apply must still + // seed the succeeded evicting condition — otherwise + // SSA prunes it because this controller is its sole + // owner. + req := ctrl.Request{NamespacedName: hypervisorName} + _, err := controller.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + + updated := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) + Expect(updated.Status.Conditions).To(ContainElement( + SatisfyAll( + HaveField("Type", kvmv1.ConditionTypeEvicting), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", kvmv1.ConditionReasonSucceeded), + ), + )) + Expect(updated.Status.Evicted).To(BeTrue()) + }) }) }) // Spec.Maintenance="" } From 93b87bea149ca2761ccfee0d6667b7d80aabc60a Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:57:32 +0200 Subject: [PATCH 3/4] Migrate HypervisorMaintenanceController Eviction creation to SSA Replace Create + SetControllerReference with Apply for the Eviction CR. The owner reference and labels are set in the apply configuration metadata, and SSA handles the upsert. A Get is still performed after the apply to read the current eviction status conditions. The apply configuration constructor for the cluster-scoped Eviction resource takes only a name (no namespace). --- .../hypervisor_maintenance_controller.go | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 9eefa88..05ec3a2 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -24,14 +24,12 @@ import ( "context" "fmt" - k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" @@ -214,25 +212,39 @@ func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Contex func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, eviction *kvmv1.Eviction, hypervisor *kvmv1.Hypervisor) (metav1.ConditionStatus, error) { log := logger.FromContext(ctx) - if err := hec.Get(ctx, k8sclient.ObjectKeyFromObject(eviction), eviction); err != nil { - if !k8serrors.IsNotFound(err) { - return metav1.ConditionUnknown, fmt.Errorf("failed to get eviction due to %w", err) - } - if err := controllerutil.SetControllerReference(hypervisor, eviction, hec.Scheme); err != nil { - return metav1.ConditionUnknown, err - } - log.Info("Creating new eviction", "name", eviction.Name) - eviction.Spec = kvmv1.EvictionSpec{ - Hypervisor: hypervisor.Name, - Reason: "openstack-hypervisor-operator maintenance", + + // Build labels to transport from hypervisor (e.g. label-selector, if set) + evictionLabels := make(map[string]string) + for _, label := range transferLabels { + if v, ok := hypervisor.Labels[label]; ok { + evictionLabels[label] = v } + } - // This also transports the label-selector, if set - transportLabels(&hypervisor.ObjectMeta, &eviction.ObjectMeta) + ownerRef := k8sacmetav1.OwnerReference(). + WithAPIVersion(kvmv1.GroupVersion.String()). + WithKind("Hypervisor"). + WithName(hypervisor.Name). + WithUID(hypervisor.UID). + WithController(true). + WithBlockOwnerDeletion(true) + + evictionApplyCfg := apiv1.Eviction(eviction.Name). + WithLabels(evictionLabels). + WithOwnerReferences(ownerRef). + WithSpec(apiv1.EvictionSpec(). + WithHypervisor(hypervisor.Name). + WithReason("openstack-hypervisor-operator maintenance")) + + log.Info("Applying eviction", "name", eviction.Name) + if err := hec.Apply(ctx, evictionApplyCfg, + k8sclient.ForceOwnership, k8sclient.FieldOwner(HypervisorMaintenanceControllerName)); err != nil { + return metav1.ConditionUnknown, fmt.Errorf("failed to apply eviction due to %w", err) + } - if err = hec.Create(ctx, eviction); err != nil { - return metav1.ConditionUnknown, fmt.Errorf("failed to create eviction due to %w", err) - } + // Re-fetch to read current eviction status + if err := hec.Get(ctx, k8sclient.ObjectKeyFromObject(eviction), eviction); err != nil { + return metav1.ConditionUnknown, fmt.Errorf("failed to get eviction status due to %w", err) } // check if we are still evicting (defaulting to yes) From 92103cc3065b9de3aa9dd5cf48c8670c60b178c5 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Wed, 15 Jul 2026 11:45:53 +0200 Subject: [PATCH 4/4] Settle incoming migrations before declaring a hypervisor evicted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During the running phase of a live-migration, Nova's Instance.host still points at the source. If the operator queries the destination at that moment, it sees zero servers and declares it evicted — while a VM is about to land. A terminal guard now restarts eviction when NumInstances > 0 despite the Eviction CR reporting success. After disabling the compute service, the maintenance controller now queries os-migrations for in-flight migrations targeting the host, aborts abortable ones, and waits for post-migrating ones to complete. Eviction only proceeds once no incoming migrations remain. The query uses a 6h changes-since window. Nova updates Migration.updated_at every ~5s during live-migration, so any active migration is guaranteed to appear. 6h also covers the post-migrating phase, which writes updated_at only once on entry. New condition: IncomingMigrationsSettled. Missing/False blocks terminal state. Requeues every 10s while unsettled. --- api/v1/eviction_types.go | 16 + .../hypervisor_maintenance_controller.go | 155 ++++- .../hypervisor_maintenance_controller_test.go | 312 ++++++++- internal/openstack/migrations.go | 173 +++++ internal/openstack/migrations_test.go | 618 ++++++++++++++++++ 5 files changed, 1250 insertions(+), 24 deletions(-) create mode 100644 internal/openstack/migrations.go create mode 100644 internal/openstack/migrations_test.go diff --git a/api/v1/eviction_types.go b/api/v1/eviction_types.go index b96ad00..5f6039f 100644 --- a/api/v1/eviction_types.go +++ b/api/v1/eviction_types.go @@ -53,6 +53,13 @@ const ( // ConditionTypeHypervisorDisabled is the type of condition for hypervisor disabled status ConditionTypeHypervisorDisabled = "HypervisorDisabled" + // ConditionTypeIncomingMigrationsSettled is set on the Hypervisor CR by the + // maintenance controller. True means no live-migrations or evacuations are + // currently in flight targeting this host. False/Unknown/Missing means the + // controller has observed outstanding incoming migrations (or hasn't checked yet) + // and the host must not be declared evicted. + ConditionTypeIncomingMigrationsSettled = "IncomingMigrationsSettled" + // ConditionTypeHaEnabled is the type of condition for signalling if HA is enabled / disabled for the hypervisor ConditionTypeHaEnabled = "HaEnabled" @@ -70,6 +77,15 @@ const ( // ConditionReasonSucceeded means the eviction has succeeded ConditionReasonSucceeded string = "Succeeded" + + // ConditionReasonSettled means all incoming migrations have reached a terminal state + ConditionReasonSettled string = "Settled" + + // ConditionReasonAborting means the controller is aborting incoming migrations + ConditionReasonAborting string = "AbortingIncomingMigrations" + + // ConditionReasonWaiting means the controller is waiting for non-abortable incoming migrations + ConditionReasonWaiting string = "WaitingForIncomingMigrations" ) // EvictionStatus defines the observed state of Eviction diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 05ec3a2..25bbc72 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -23,6 +23,7 @@ package controller import ( "context" "fmt" + "time" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,6 +44,10 @@ import ( const ( HypervisorMaintenanceControllerName = "HypervisorMaintenance" + + // settleRequeueInterval is the interval at which the controller re-checks + // incoming migrations when they haven't settled yet. + settleRequeueInterval = 10 * time.Second ) type HypervisorMaintenanceController struct { @@ -69,9 +74,10 @@ func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req c // Build status apply config upfront; sub-functions mutate it directly. // Seed only the HypervisorDisabled condition (the only one always retained). - // reconcileEviction conditionally seeds ConditionTypeEvicting: it is included - // when maintenance is active, and intentionally omitted when MaintenanceUnset - // so that SSA prunes it from the field manager's managed fields. + // reconcileEviction conditionally seeds ConditionTypeEvicting and + // ConditionTypeIncomingMigrationsSettled: they are included when maintenance + // is active, and intentionally omitted when MaintenanceUnset so that SSA + // prunes them from the field manager's managed fields. statusCfg := apiv1.HypervisorStatus().WithEvicted(hv.Status.Evicted) if c := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled); c != nil { statusCfg.WithConditions(utils.ConditionFromStatus(*c)) @@ -81,13 +87,15 @@ func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req c return ctrl.Result{}, err } - if err := hec.reconcileEviction(ctx, hv, statusCfg); err != nil { + result, err := hec.reconcileEviction(ctx, hv, statusCfg) + if err != nil { return ctrl.Result{}, err } - return ctrl.Result{}, hec.Status().Apply(ctx, + applyErr := hec.Status().Apply(ctx, apiv1.Hypervisor(hv.Name).WithStatus(statusCfg), k8sclient.ForceOwnership, k8sclient.FieldOwner(HypervisorMaintenanceControllerName)) + return result, applyErr } // reconcileComputeService enables/disables the nova-compute service based on @@ -151,9 +159,10 @@ func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context. } // reconcileEviction creates/deletes the Eviction CR and sets the ConditionTypeEvicting -// condition and Evicted scalar on statusCfg. When eviction should be removed, the -// condition entry is filtered out so SSA prunes it. -func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) error { +// and ConditionTypeIncomingMigrationsSettled conditions plus the Evicted scalar on +// statusCfg. When eviction should be removed, the condition entries are filtered out +// so SSA prunes them. +func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) (ctrl.Result, error) { eviction := &kvmv1.Eviction{ ObjectMeta: metav1.ObjectMeta{Name: hv.Name}, } @@ -161,35 +170,87 @@ func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Contex switch hv.Spec.Maintenance { case kvmv1.MaintenanceUnset: // Avoid deleting the eviction over and over. - if !hv.Status.Evicted && meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) == nil { - return nil + if !hv.Status.Evicted && + meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) == nil && + meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled) == nil { + return ctrl.Result{}, nil } if err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)); err != nil { - return err + return ctrl.Result{}, err } - // ConditionTypeEvicting is intentionally absent from statusCfg — SSA - // will prune it from this field manager's managed fields on Apply. + // ConditionTypeEvicting and ConditionTypeIncomingMigrationsSettled are + // intentionally absent from statusCfg — SSA will prune them from this + // field manager's managed fields on Apply. statusCfg.WithEvicted(false) case kvmv1.MaintenanceManual, kvmv1.MaintenanceAuto, kvmv1.MaintenanceTermination: - // In case of "ha", the host gets emptied from the HA service. - // Seed the existing evicting condition so SSA does not prune it, - // regardless of whether we take the short-circuit below. + // Gate: ensure all incoming migrations are settled before proceeding. + // This runs on every reconcile (defense in depth). + settled, err := hec.settleIncomingMigrations(ctx, hv, statusCfg) + if err != nil { + return ctrl.Result{}, err + } + if !settled { + // Cannot proceed with eviction while incoming migrations are outstanding. + // Seed the existing evicting condition so SSA does not prune it. + if cond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting); cond != nil { + statusCfg.WithConditions(utils.ConditionFromStatus(*cond)) + } + return ctrl.Result{RequeueAfter: settleRequeueInterval}, nil + } + + // Terminal guard: if Evicting was already Succeeded, verify the host is truly empty. if cond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting); cond != nil { - statusCfg.WithConditions(utils.ConditionFromStatus(*cond)) if cond.Reason == kvmv1.ConditionReasonSucceeded { - // We are done here, no need to look at the eviction any more. - return nil + if hv.Status.NumInstances > 0 { + // Instance appeared after eviction completed. + // Delete the Eviction CR so ensureEviction recreates a fresh one. + log := logger.FromContext(ctx) + log.Info("Eviction reported succeeded but instances remain on host; re-entering drain", + "numInstances", hv.Status.NumInstances) + if delErr := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)); delErr != nil { + return ctrl.Result{}, delErr + } + statusCfg.WithEvicted(false) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionTrue). + WithReason(kvmv1.ConditionReasonRunning). + WithMessage(fmt.Sprintf("Re-entering drain: %d instance(s) still on host", hv.Status.NumInstances))) + return ctrl.Result{}, nil + } + // Truly done: host is empty and migrations are settled. + statusCfg.WithConditions(utils.ConditionFromStatus(*cond)) + return ctrl.Result{}, nil } } status, err := hec.ensureEviction(ctx, eviction, hv) if err != nil { - return err + return ctrl.Result{}, err } var reason, message string if status == metav1.ConditionFalse { + if hv.Status.NumInstances > 0 { + // Eviction CR says done but instances exist (race with in-flight migration). + // Delete the Eviction CR and restart drain. + log := logger.FromContext(ctx) + log.Info("Eviction finished but instances remain on host; restarting drain", + "numInstances", hv.Status.NumInstances) + if delErr := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)); delErr != nil { + return ctrl.Result{}, delErr + } + statusCfg.WithEvicted(false) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionTrue). + WithReason(kvmv1.ConditionReasonRunning). + WithMessage(fmt.Sprintf("Restarting drain: %d instance(s) still on host", hv.Status.NumInstances))) + return ctrl.Result{}, nil + } message = "Evicted" reason = kvmv1.ConditionReasonSucceeded statusCfg.WithEvicted(true) @@ -207,7 +268,59 @@ func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Contex WithMessage(message)) } - return nil + return ctrl.Result{}, nil +} + +// settleIncomingMigrations checks for in-flight migrations targeting this host, +// aborts those that can be aborted, and reports whether the host is settled. +// It sets the IncomingMigrationsSettled condition on statusCfg. +// Returns true if settled (no incoming migrations), false otherwise. +func (hec *HypervisorMaintenanceController) settleIncomingMigrations(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) (bool, error) { + log := logger.FromContext(ctx) + + aborted, waiting, err := openstack.SettleIncomingMigrations(ctx, hec.computeClient, hv.Name) + if err != nil { + return false, fmt.Errorf("settling incoming migrations for %s: %w", hv.Name, err) + } + + if len(waiting) > 0 { + instanceUUIDs := make([]string, len(waiting)) + for i, m := range waiting { + instanceUUIDs[i] = m.InstanceUUID + } + log.Info("Waiting for non-abortable incoming migrations", "host", hv.Name, "instances", instanceUUIDs) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeIncomingMigrationsSettled). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonWaiting). + WithMessage(fmt.Sprintf("Waiting for %d non-abortable incoming migration(s) to complete", len(waiting)))) + return false, nil + } + + if len(aborted) > 0 { + instanceUUIDs := make([]string, len(aborted)) + for i, m := range aborted { + instanceUUIDs[i] = m.InstanceUUID + } + log.Info("Aborted incoming migrations", "host", hv.Name, "instances", instanceUUIDs) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeIncomingMigrationsSettled). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonAborting). + WithMessage(fmt.Sprintf("Aborted %d incoming migration(s); verifying on next reconcile", len(aborted)))) + return false, nil + } + + // No incoming migrations — settled. + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeIncomingMigrationsSettled). + WithStatus(metav1.ConditionTrue). + WithReason(kvmv1.ConditionReasonSettled). + WithMessage("No incoming migrations targeting this host")) + return true, nil } func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, eviction *kvmv1.Eviction, hypervisor *kvmv1.Hypervisor) (metav1.ConditionStatus, error) { diff --git a/internal/controller/hypervisor_maintenance_controller_test.go b/internal/controller/hypervisor_maintenance_controller_test.go index 4af542b..7120b78 100644 --- a/internal/controller/hypervisor_maintenance_controller_test.go +++ b/internal/controller/hypervisor_maintenance_controller_test.go @@ -20,6 +20,7 @@ package controller import ( "fmt" "net/http" + "time" "github.com/gophercloud/gophercloud/v2/testhelper" "github.com/gophercloud/gophercloud/v2/testhelper/client" @@ -28,18 +29,22 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + applymetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + applyv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/applyconfigurations/api/v1" ) var _ = Describe("HypervisorMaintenanceController", func() { var ( - controller *HypervisorMaintenanceController - fakeServer testhelper.FakeServer - hypervisorName = types.NamespacedName{Name: "hv-test"} + controller *HypervisorMaintenanceController + fakeServer testhelper.FakeServer + hypervisorName = types.NamespacedName{Name: "hv-test"} + migrationsResponse string // mutable: tests can set this to control /os-migrations responses + deletedMigrations []string ) const ( @@ -83,6 +88,28 @@ var _ = Describe("HypervisorMaintenanceController", func() { fakeServer = testhelper.SetupHTTP() DeferCleanup(fakeServer.Teardown) + // Default: no incoming migrations + migrationsResponse = `{"migrations": []}` + deletedMigrations = nil + + fakeServer.Mux.HandleFunc("GET /os-migrations", func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + mt := r.URL.Query().Get("migration_type") + // Only return migrations for the live-migration type query; + // evacuation query returns empty by default. + if mt == "evacuation" { + fmt.Fprint(w, `{"migrations": []}`) + } else { + fmt.Fprint(w, migrationsResponse) + } + }) + + fakeServer.Mux.HandleFunc("DELETE /servers/", func(w http.ResponseWriter, r *http.Request) { + deletedMigrations = append(deletedMigrations, r.URL.Path) + w.WriteHeader(http.StatusAccepted) + }) + By("Creating the HypervisorMaintenanceController") controller = &HypervisorMaintenanceController{ Client: k8sClient, @@ -238,6 +265,42 @@ var _ = Describe("HypervisorMaintenanceController", func() { }) }) // Spec.Maintenance="" + Context("Spec.Maintenance=\"\" with only stale IncomingMigrationsSettled condition", func() { + BeforeEach(func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + + // First, simulate that the controller previously owned the condition + // by applying it via SSA with the same field manager. + statusCfg := applyv1.HypervisorStatus(). + WithEvicted(false). + WithConditions(applymetav1.Condition(). + WithType(kvmv1.ConditionTypeIncomingMigrationsSettled). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonWaiting). + WithMessage("incoming migrations still pending"). + WithLastTransitionTime(metav1.Now())) + Expect(k8sClient.Status().Apply(ctx, + applyv1.Hypervisor(hypervisor.Name).WithStatus(statusCfg), + k8sclient.ForceOwnership, + k8sclient.FieldOwner(HypervisorMaintenanceControllerName), + )).To(Succeed()) + + // Now cancel maintenance. + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + hypervisor.Spec.Maintenance = "" + Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed()) + expectedBody := `{"status": "enabled", "forced_down": false}` + mockServiceUpdate(expectedBody) + }) + + It("should remove the stale IncomingMigrationsSettled condition", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled)).To(BeNil()) + }) + }) + Context("Spec.Maintenance=\"ha\"", func() { BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} @@ -482,4 +545,247 @@ var _ = Describe("HypervisorMaintenanceController", func() { Expect(hypervisor.Status.ServiceID).To(BeEmpty()) }) }) + + Context("Incoming migrations settling", func() { + BeforeEach(func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + hypervisor.Status.ServiceID = "1234" + meta.SetStatusCondition(&hypervisor.Status.Conditions, + metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: metav1.StatusSuccess, + Message: "Onboarded", + }, + ) + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + + // Re-read to get fresh resourceVersion, then update spec + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + hypervisor.Spec.Maintenance = "auto" + Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed()) + + // Permissive service mock: accept any enable/disable call + fakeServer.Mux.HandleFunc("PUT /os-services/1234", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, ServiceEnabledResponse) + }) + }) + + Context("when there are no incoming migrations", func() { + It("should set IncomingMigrationsSettled=True and proceed to create eviction", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(meta.IsStatusConditionTrue(hypervisor.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled)).To(BeTrue()) + // Eviction should be created + eviction := &kvmv1.Eviction{} + Expect(k8sClient.Get(ctx, hypervisorName, eviction)).To(Succeed()) + }) + }) + + Context("when there is a running incoming migration", func() { + BeforeEach(func(_ SpecContext) { + migrationsResponse = `{"migrations": [ + { + "id": 42, + "uuid": "mig-uuid-1", + "instance_uuid": "inst-uuid-1", + "status": "running", + "source_compute": "node003", + "dest_compute": "hv-test", + "migration_type": "live-migration" + } + ]}` + }) + + It("should set IncomingMigrationsSettled=False with Aborting reason", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + cond := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonAborting)) + }) + + It("should issue a DELETE to abort the migration", func(_ SpecContext) { + Expect(deletedMigrations).To(HaveLen(1)) + Expect(deletedMigrations[0]).To(ContainSubstring("inst-uuid-1")) + Expect(deletedMigrations[0]).To(ContainSubstring("42")) + }) + + It("should not create an eviction resource", func(ctx SpecContext) { + eviction := &kvmv1.Eviction{} + err := k8sClient.Get(ctx, hypervisorName, eviction) + Expect(err).To(HaveOccurred()) + Expect(k8sclient.IgnoreNotFound(err)).To(Succeed()) + }) + + It("should requeue after settleRequeueInterval", func(ctx SpecContext) { + req := ctrl.Request{NamespacedName: hypervisorName} + result, err := controller.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + }) + }) + + Context("when there is a post-migrating incoming migration", func() { + BeforeEach(func(_ SpecContext) { + migrationsResponse = `{"migrations": [ + { + "id": 99, + "uuid": "mig-uuid-2", + "instance_uuid": "inst-uuid-2", + "status": "post-migrating", + "source_compute": "node003", + "dest_compute": "hv-test", + "migration_type": "live-migration" + } + ]}` + }) + + It("should set IncomingMigrationsSettled=False with Waiting reason", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + cond := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonWaiting)) + }) + + It("should not issue any DELETE", func(_ SpecContext) { + Expect(deletedMigrations).To(BeEmpty()) + }) + + It("should not create an eviction resource", func(ctx SpecContext) { + eviction := &kvmv1.Eviction{} + err := k8sClient.Get(ctx, hypervisorName, eviction) + Expect(err).To(HaveOccurred()) + Expect(k8sclient.IgnoreNotFound(err)).To(Succeed()) + }) + + It("should requeue after settleRequeueInterval", func(ctx SpecContext) { + req := ctrl.Request{NamespacedName: hypervisorName} + result, err := controller.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(10 * time.Second)) + }) + }) + + Context("when eviction succeeded but instances remain (incident regression)", func() { + BeforeEach(func(ctx SpecContext) { + // Simulate: eviction CR exists and reports Succeeded + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + + eviction := &kvmv1.Eviction{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: kvmv1.EvictionSpec{ + Hypervisor: hypervisorName.Name, + Reason: "test", + }, + } + Expect(controllerutil.SetControllerReference(hypervisor, eviction, controller.Scheme)).To(Succeed()) + Expect(k8sClient.Create(ctx, eviction)).To(Succeed()) + + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionFalse, + Message: "done", + Reason: kvmv1.ConditionReasonSucceeded, + }) + Expect(k8sClient.Status().Update(ctx, eviction)).To(Succeed()) + + // Set NumInstances=1 on the Hypervisor (simulating late-arriving instance) + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + hypervisor.Status.NumInstances = 1 + meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonSucceeded, + Message: "Evicted", + }) + hypervisor.Status.Evicted = true + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + }) + + It("should flip Evicted back to false", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(hypervisor.Status.Evicted).To(BeFalse()) + }) + + It("should set Evicting back to Running", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + cond := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeEvicting) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonRunning)) + }) + + It("should delete the eviction CR to re-enter drain", func(ctx SpecContext) { + eviction := &kvmv1.Eviction{} + err := k8sClient.Get(ctx, hypervisorName, eviction) + Expect(err).To(HaveOccurred()) + Expect(k8sclient.IgnoreNotFound(err)).To(Succeed()) + }) + }) + + Context("when eviction just finished but instances remain (no pre-existing Succeeded condition)", func() { + BeforeEach(func(ctx SpecContext) { + // Simulate: eviction CR exists and reports finished, but the + // Hypervisor does NOT have Evicting=Succeeded yet (first reconcile + // after the Eviction CR transitioned). + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + + eviction := &kvmv1.Eviction{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: kvmv1.EvictionSpec{ + Hypervisor: hypervisorName.Name, + Reason: "test", + }, + } + Expect(controllerutil.SetControllerReference(hypervisor, eviction, controller.Scheme)).To(Succeed()) + Expect(k8sClient.Create(ctx, eviction)).To(Succeed()) + + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionFalse, + Message: "done", + Reason: kvmv1.ConditionReasonSucceeded, + }) + Expect(k8sClient.Status().Update(ctx, eviction)).To(Succeed()) + + // Set NumInstances=1 but leave Evicting condition as Running (not Succeeded) + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + hypervisor.Status.NumInstances = 1 + meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonRunning, + Message: "Evicting", + }) + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + }) + + It("should set Evicting to True/Running and not declare Evicted", func(ctx SpecContext) { + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed()) + Expect(hypervisor.Status.Evicted).To(BeFalse()) + cond := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeEvicting) + Expect(cond).NotTo(BeNil()) + Expect(cond.Status).To(Equal(metav1.ConditionTrue)) + Expect(cond.Reason).To(Equal(kvmv1.ConditionReasonRunning)) + }) + + It("should delete the eviction CR to restart drain", func(ctx SpecContext) { + eviction := &kvmv1.Eviction{} + err := k8sClient.Get(ctx, hypervisorName, eviction) + Expect(err).To(HaveOccurred()) + Expect(k8sclient.IgnoreNotFound(err)).To(Succeed()) + }) + }) + }) }) diff --git a/internal/openstack/migrations.go b/internal/openstack/migrations.go new file mode 100644 index 0000000..9a92ee2 --- /dev/null +++ b/internal/openstack/migrations.go @@ -0,0 +1,173 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "context" + "fmt" + "net/http" + "net/url" + "strconv" + "time" + + "github.com/gophercloud/gophercloud/v2" +) + +// IncomingMigrationsLookbackWindow defines how far back we query Nova for +// migrations. Nova's live-migration monitor updates Migration.updated_at every +// ~5 seconds on both libvirt/kvm and libvirt/cloud-hypervisor backends, so any +// in-flight migration is guaranteed to appear in this window. +const IncomingMigrationsLookbackWindow = 6 * time.Hour + +// MigrationInfo holds the subset of Nova migration fields relevant to settling +// incoming migrations on a host being drained. +type MigrationInfo struct { + ID int `json:"id"` + UUID string `json:"uuid"` + InstanceUUID string `json:"instance_uuid"` + Status string `json:"status"` + SourceCompute string `json:"source_compute"` + DestCompute string `json:"dest_compute"` + MigrationType string `json:"migration_type"` +} + +// migrationsResponse mirrors Nova's GET /os-migrations JSON envelope. +type migrationsResponse struct { + Migrations []MigrationInfo `json:"migrations"` +} + +// terminalStatuses are migration statuses that no longer represent an in-flight +// migration. Derived from nova/db/main/api.py migration_get_in_progress_by_host_and_node. +var terminalStatuses = map[string]bool{ + "confirmed": true, + "reverted": true, + "error": true, + "failed": true, + "completed": true, + "cancelled": true, + "done": true, +} + +// abortableStatuses are migration statuses where a DELETE (abort) is supported. +// queued and preparing require microversion >= 2.65; running is always abortable. +// The operator uses microversion 2.90 which covers all three. +var abortableStatuses = map[string]bool{ + "queued": true, + "preparing": true, + "running": true, +} + +// ListActiveIncomingMigrations queries Nova for migrations that target the +// given host and are still in a non-terminal state. It issues a single call +// with a changes-since window of IncomingMigrationsLookbackWindow, then +// filters client-side for dest_compute==host and non-terminal status. +func ListActiveIncomingMigrations(ctx context.Context, sc *gophercloud.ServiceClient, host string) ([]MigrationInfo, error) { + changesSince := time.Now().UTC().Add(-IncomingMigrationsLookbackWindow).Format(time.RFC3339) + + migrations, err := listMigrationsForHost(ctx, sc, host, changesSince) + if err != nil { + return nil, fmt.Errorf("listing migrations for host %s: %w", host, err) + } + + var result []MigrationInfo + for _, m := range migrations { + if m.DestCompute != host { + continue + } + if terminalStatuses[m.Status] { + continue + } + result = append(result, m) + } + + return result, nil +} + +// AbortMigration issues DELETE /servers/{instanceUUID}/migrations/{migrationID} +// to abort a live-migration. Treats HTTP 404 and 409 as non-fatal (the +// migration already transitioned past an abortable state or was deleted). +func AbortMigration(ctx context.Context, sc *gophercloud.ServiceClient, instanceUUID string, migrationID int) error { + deleteURL := sc.ServiceURL("servers", instanceUUID, "migrations", strconv.Itoa(migrationID)) + + resp, err := sc.Delete(ctx, deleteURL, &gophercloud.RequestOpts{ + OkCodes: []int{http.StatusAccepted, http.StatusNoContent}, + }) + if err != nil { + if gophercloud.ResponseCodeIs(err, http.StatusNotFound) || gophercloud.ResponseCodeIs(err, http.StatusConflict) { + // Migration already completed/cancelled or not found — non-fatal. + return nil + } + return fmt.Errorf("aborting migration %d for instance %s: %w", migrationID, instanceUUID, err) + } + // gophercloud closes the response body when JSONResponse is nil, + // but close defensively if it wasn't. + if resp != nil && resp.Body != nil { + resp.Body.Close() + } + return nil +} + +// SettleIncomingMigrations combines listing and aborting: it lists all active +// incoming migrations for the host, aborts those in abortable states, and +// returns both the aborted and the waiting (post-migrating) sets. +// +// The caller should treat: +// - len(aborted) > 0 as "just issued aborts, recheck on next reconcile" +// - len(waiting) > 0 as "cannot abort, must wait for completion" +// - both empty as "no incoming migrations; host is settled" +func SettleIncomingMigrations(ctx context.Context, sc *gophercloud.ServiceClient, host string) (aborted, waiting []MigrationInfo, err error) { + active, err := ListActiveIncomingMigrations(ctx, sc, host) + if err != nil { + return nil, nil, err + } + + for _, m := range active { + if abortableStatuses[m.Status] && m.MigrationType == "live-migration" { + if abortErr := AbortMigration(ctx, sc, m.InstanceUUID, m.ID); abortErr != nil { + return aborted, waiting, abortErr + } + aborted = append(aborted, m) + } else { + // Non-abortable: either post-migrating, or an evacuation (which Nova + // does not support aborting). Must wait for completion. + waiting = append(waiting, m) + } + } + + return aborted, waiting, nil +} + +// listMigrationsForHost queries GET /os-migrations with host and changes-since filters. +func listMigrationsForHost(ctx context.Context, sc *gophercloud.ServiceClient, host, changesSince string) ([]MigrationInfo, error) { + query := url.Values{} + query.Set("host", host) + query.Set("changes-since", changesSince) + + requestURL := sc.ServiceURL("os-migrations") + "?" + query.Encode() + + var parsed migrationsResponse + //nolint:bodyclose // gophercloud closes the body when JSONResponse is non-nil + _, err := sc.Get(ctx, requestURL, &parsed, &gophercloud.RequestOpts{ + OkCodes: []int{http.StatusOK}, + }) + if err != nil { + return nil, err + } + + return parsed.Migrations, nil +} diff --git a/internal/openstack/migrations_test.go b/internal/openstack/migrations_test.go new file mode 100644 index 0000000..d22b916 --- /dev/null +++ b/internal/openstack/migrations_test.go @@ -0,0 +1,618 @@ +/* +SPDX-FileCopyrightText: Copyright 2025 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "context" + "fmt" + "net/http" + + "github.com/gophercloud/gophercloud/v2/testhelper" + "github.com/gophercloud/gophercloud/v2/testhelper/client" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Migrations", func() { + var ( + fakeServer testhelper.FakeServer + ctx context.Context + ) + + BeforeEach(func() { + fakeServer = testhelper.SetupHTTP() + ctx = context.Background() + }) + + AfterEach(func() { + fakeServer.Teardown() + }) + + // migrationsHandler returns a handler that responds to GET /os-migrations. + migrationsHandler := func(data string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, data) + } + } + + emptyMigrations := `{"migrations": []}` + + Describe("ListActiveIncomingMigrations", func() { + Context("when there are no migrations", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(emptyMigrations)) + }) + + It("should return an empty list", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(BeEmpty()) + }) + }) + + Context("when there is a running migration targeting the host", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 42, + "uuid": "mig-uuid-1", + "instance_uuid": "inst-uuid-1", + "status": "running", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should return the migration", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(HaveLen(1)) + Expect(migrations[0].ID).To(Equal(42)) + Expect(migrations[0].InstanceUUID).To(Equal("inst-uuid-1")) + Expect(migrations[0].Status).To(Equal("running")) + Expect(migrations[0].DestCompute).To(Equal("node009-bb549")) + }) + }) + + Context("when there is a post-migrating migration targeting the host", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 99, + "uuid": "mig-uuid-2", + "instance_uuid": "inst-uuid-2", + "status": "post-migrating", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should return the migration as active incoming", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(HaveLen(1)) + Expect(migrations[0].Status).To(Equal("post-migrating")) + }) + }) + + Context("when there is an outbound migration (source_compute == host)", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 50, + "uuid": "mig-uuid-3", + "instance_uuid": "inst-uuid-3", + "status": "running", + "source_compute": "node009-bb549", + "dest_compute": "node005-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should ignore outbound migrations", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(BeEmpty()) + }) + }) + + Context("when there is an incoming evacuation", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 77, + "uuid": "mig-uuid-4", + "instance_uuid": "inst-uuid-4", + "status": "running", + "source_compute": "node001-bb549", + "dest_compute": "node009-bb549", + "migration_type": "evacuation" + } + ]}`)) + }) + + It("should include incoming evacuations", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(HaveLen(1)) + Expect(migrations[0].MigrationType).To(Equal("evacuation")) + Expect(migrations[0].DestCompute).To(Equal("node009-bb549")) + }) + }) + + Context("when migrations have terminal statuses", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 10, + "uuid": "mig-t1", + "instance_uuid": "inst-t1", + "status": "completed", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 11, + "uuid": "mig-t2", + "instance_uuid": "inst-t2", + "status": "cancelled", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 12, + "uuid": "mig-t3", + "instance_uuid": "inst-t3", + "status": "error", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 13, + "uuid": "mig-t4", + "instance_uuid": "inst-t4", + "status": "failed", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 14, + "uuid": "mig-t5", + "instance_uuid": "inst-t5", + "status": "confirmed", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 15, + "uuid": "mig-t6", + "instance_uuid": "inst-t6", + "status": "reverted", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 16, + "uuid": "mig-t7", + "instance_uuid": "inst-t7", + "status": "done", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should filter out all terminal statuses", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(BeEmpty()) + }) + }) + + Context("when Nova returns a server error", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"error": "Internal Server Error"}`) + }) + }) + + It("should propagate the error", func() { + sc := client.ServiceClient(fakeServer) + _, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).To(HaveOccurred()) + }) + }) + + Context("with a mixed set of migrations", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 1, + "uuid": "mig-1", + "instance_uuid": "inst-1", + "status": "running", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 2, + "uuid": "mig-2", + "instance_uuid": "inst-2", + "status": "running", + "source_compute": "node007-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 3, + "uuid": "mig-3", + "instance_uuid": "inst-3", + "status": "post-migrating", + "source_compute": "node002-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 4, + "uuid": "mig-4", + "instance_uuid": "inst-4", + "status": "running", + "source_compute": "node009-bb549", + "dest_compute": "node005-bb549", + "migration_type": "live-migration" + }, + { + "id": 5, + "uuid": "mig-5", + "instance_uuid": "inst-5", + "status": "completed", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should return only active incoming migrations", func() { + sc := client.ServiceClient(fakeServer) + migrations, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(migrations).To(HaveLen(3)) + ids := []int{migrations[0].ID, migrations[1].ID, migrations[2].ID} + Expect(ids).To(ConsistOf(1, 2, 3)) + }) + }) + + Context("verifies query parameters", func() { + var receivedQueries []string + + BeforeEach(func() { + receivedQueries = nil + fakeServer.Mux.HandleFunc("GET /os-migrations", func(w http.ResponseWriter, r *http.Request) { + receivedQueries = append(receivedQueries, r.URL.RawQuery) + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, `{"migrations": []}`) + }) + }) + + It("should send host and changes-since parameters", func() { + sc := client.ServiceClient(fakeServer) + _, err := ListActiveIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + // Single call without migration_type filter + Expect(receivedQueries).To(HaveLen(1)) + Expect(receivedQueries[0]).To(ContainSubstring("host=node009-bb549")) + Expect(receivedQueries[0]).To(ContainSubstring("changes-since=")) + Expect(receivedQueries[0]).NotTo(ContainSubstring("migration_type")) + }) + }) + }) + + Describe("AbortMigration", func() { + Context("when abort succeeds", func() { + var deleteCalled bool + + BeforeEach(func() { + deleteCalled = false + fakeServer.Mux.HandleFunc("DELETE /servers/inst-uuid-1/migrations/42", func(w http.ResponseWriter, r *http.Request) { + deleteCalled = true + w.WriteHeader(http.StatusAccepted) + }) + }) + + It("should issue DELETE and return nil", func() { + sc := client.ServiceClient(fakeServer) + err := AbortMigration(ctx, sc, "inst-uuid-1", 42) + Expect(err).NotTo(HaveOccurred()) + Expect(deleteCalled).To(BeTrue()) + }) + }) + + Context("when Nova returns 404 (migration already gone)", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("DELETE /servers/inst-uuid-1/migrations/42", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, `{"itemNotFound": {"message": "Migration not found", "code": 404}}`) + }) + }) + + It("should treat as non-fatal", func() { + sc := client.ServiceClient(fakeServer) + err := AbortMigration(ctx, sc, "inst-uuid-1", 42) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when Nova returns 409 (migration not abortable)", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("DELETE /servers/inst-uuid-1/migrations/42", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusConflict) + fmt.Fprint(w, `{"conflictingRequest": {"message": "Migration status is post-migrating", "code": 409}}`) + }) + }) + + It("should treat as non-fatal", func() { + sc := client.ServiceClient(fakeServer) + err := AbortMigration(ctx, sc, "inst-uuid-1", 42) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("when Nova returns a server error", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("DELETE /servers/inst-uuid-1/migrations/42", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"error": "Internal Server Error"}`) + }) + }) + + It("should propagate the error", func() { + sc := client.ServiceClient(fakeServer) + err := AbortMigration(ctx, sc, "inst-uuid-1", 42) + Expect(err).To(HaveOccurred()) + }) + }) + }) + + Describe("SettleIncomingMigrations", func() { + Context("when there are no incoming migrations", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(emptyMigrations)) + }) + + It("should return empty aborted and waiting", func() { + sc := client.ServiceClient(fakeServer) + aborted, waiting, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(aborted).To(BeEmpty()) + Expect(waiting).To(BeEmpty()) + }) + }) + + Context("when there are abortable migrations", func() { + var deleteCalls int + + BeforeEach(func() { + deleteCalls = 0 + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 1, + "uuid": "mig-1", + "instance_uuid": "inst-1", + "status": "running", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 2, + "uuid": "mig-2", + "instance_uuid": "inst-2", + "status": "queued", + "source_compute": "node007-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + + fakeServer.Mux.HandleFunc("DELETE /servers/inst-1/migrations/1", func(w http.ResponseWriter, r *http.Request) { + deleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + + fakeServer.Mux.HandleFunc("DELETE /servers/inst-2/migrations/2", func(w http.ResponseWriter, r *http.Request) { + deleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + }) + + It("should abort all and return them in aborted list", func() { + sc := client.ServiceClient(fakeServer) + aborted, waiting, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(aborted).To(HaveLen(2)) + Expect(waiting).To(BeEmpty()) + Expect(deleteCalls).To(Equal(2)) + }) + }) + + Context("when there is a post-migrating migration (not abortable)", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 99, + "uuid": "mig-99", + "instance_uuid": "inst-99", + "status": "post-migrating", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + }) + + It("should return in waiting list without issuing DELETE", func() { + sc := client.ServiceClient(fakeServer) + aborted, waiting, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(aborted).To(BeEmpty()) + Expect(waiting).To(HaveLen(1)) + Expect(waiting[0].ID).To(Equal(99)) + }) + }) + + Context("with a mixed set (abortable + post-migrating + outbound + completed)", func() { + var deleteCalls int + + BeforeEach(func() { + deleteCalls = 0 + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 1, + "uuid": "mig-1", + "instance_uuid": "inst-1", + "status": "running", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 2, + "uuid": "mig-2", + "instance_uuid": "inst-2", + "status": "preparing", + "source_compute": "node007-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 3, + "uuid": "mig-3", + "instance_uuid": "inst-3", + "status": "post-migrating", + "source_compute": "node002-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + }, + { + "id": 4, + "uuid": "mig-4", + "instance_uuid": "inst-4", + "status": "running", + "source_compute": "node009-bb549", + "dest_compute": "node005-bb549", + "migration_type": "live-migration" + }, + { + "id": 5, + "uuid": "mig-5", + "instance_uuid": "inst-5", + "status": "completed", + "source_compute": "node003-bb549", + "dest_compute": "node009-bb549", + "migration_type": "live-migration" + } + ]}`)) + + fakeServer.Mux.HandleFunc("DELETE /servers/inst-1/migrations/1", func(w http.ResponseWriter, r *http.Request) { + deleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + + fakeServer.Mux.HandleFunc("DELETE /servers/inst-2/migrations/2", func(w http.ResponseWriter, r *http.Request) { + deleteCalls++ + w.WriteHeader(http.StatusAccepted) + }) + }) + + It("should abort 2, wait on 1, ignore outbound and completed", func() { + sc := client.ServiceClient(fakeServer) + aborted, waiting, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(aborted).To(HaveLen(2)) + Expect(waiting).To(HaveLen(1)) + Expect(waiting[0].ID).To(Equal(3)) + Expect(deleteCalls).To(Equal(2)) + }) + }) + + Context("when Nova list returns an error", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, `{"error": "Internal Server Error"}`) + }) + }) + + It("should propagate the error", func() { + sc := client.ServiceClient(fakeServer) + _, _, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).To(HaveOccurred()) + }) + }) + + Context("when there is a running evacuation (not abortable via migration-abort)", func() { + BeforeEach(func() { + fakeServer.Mux.HandleFunc("GET /os-migrations", migrationsHandler(`{"migrations": [ + { + "id": 88, + "uuid": "mig-evac-1", + "instance_uuid": "inst-evac-1", + "status": "running", + "source_compute": "node001-bb549", + "dest_compute": "node009-bb549", + "migration_type": "evacuation" + } + ]}`)) + }) + + It("should place the evacuation in waiting without issuing DELETE", func() { + sc := client.ServiceClient(fakeServer) + aborted, waiting, err := SettleIncomingMigrations(ctx, sc, "node009-bb549") + Expect(err).NotTo(HaveOccurred()) + Expect(aborted).To(BeEmpty()) + Expect(waiting).To(HaveLen(1)) + Expect(waiting[0].ID).To(Equal(88)) + Expect(waiting[0].MigrationType).To(Equal("evacuation")) + }) + }) + }) +})