diff --git a/cmd/main.go b/cmd/main.go index a179780..41982a5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,6 +25,7 @@ import ( "fmt" "os" gruntime "runtime" + "strconv" "strings" "github.com/sapcc/go-api-declarations/bininfo" @@ -52,6 +53,7 @@ import ( kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/eviction" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/ready" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/logger" @@ -104,6 +106,14 @@ func main() { flag.StringVar(&agentNamespacesFlag, "agent-namespaces", "", "Comma-separated list of namespaces to search for agent pods (nova-compute, neutron) during offboarding.") + flag.IntVar(&global.EvictionConcurrency, "eviction-concurrency", 1, + "Default maximum number of VM migrations to run concurrently while draining a hypervisor. Defaults to 1 (serial).") + + var evictionTraitConcurrencyFlag string + flag.StringVar(&evictionTraitConcurrencyFlag, "eviction-trait-concurrency", "", + "Comma-separated TRAIT=N overrides for per-host migration concurrency, e.g. "+ + "CUSTOM_HANA_EXCLUSIVE_HOST=1,CUSTOM_FOO=2. The lowest matching trait limit wins.") + flag.StringVar(&certificateNamespace, "certificate-namespace", "monsoon3", "The namespace for the certificates. ") flag.StringVar(&certificateIssuerName, "certificate-issuer-name", "nova-hypervisor-agents-ca-issuer", "Name of the certificate issuer.") @@ -150,6 +160,27 @@ func main() { os.Exit(1) } + if global.EvictionConcurrency < 1 { + setupLog.Error(errors.New("--eviction-concurrency must be >= 1"), "invalid configuration") + os.Exit(1) + } + + for entry := range strings.SplitSeq(evictionTraitConcurrencyFlag, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + trait, value, ok := strings.Cut(entry, "=") + trait = strings.TrimSpace(trait) + n, err := strconv.Atoi(strings.TrimSpace(value)) + if !ok || trait == "" || err != nil || n < 1 { + setupLog.Error(errors.New("invalid --eviction-trait-concurrency entry: "+entry), + "invalid configuration", "expected", "TRAIT=N with N >= 1") + os.Exit(1) + } + global.EvictionTraitConcurrency[trait] = n + } + if certificateIssuerName == "" { setupLog.Error(errors.New("certificate-issuer-name cannot be empty"), "invalid certificate issuer name") os.Exit(1) @@ -297,7 +328,7 @@ func main() { os.Exit(1) } - if err = (&controller.EvictionReconciler{ + if err = (&eviction.EvictionReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { diff --git a/internal/controller/constants.go b/internal/controller/constants.go index f7007fd..fe3585f 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -17,11 +17,16 @@ limitations under the License. package controller +import "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" + // This should contain constants shared between controllers const ( labelHypervisor = "nova.openstack.cloud.sap/virt-driver" testAggregateName = "tenant_filter_tests" + // defaultPollTime is the default requeue interval used while waiting for a + // slow external state transition to settle. + defaultPollTime = global.DefaultPollTime // taintKeyOffboarding is used as a NoExecute taint. nova-compute and // neutron agent pods do not tolerate it (the kvm-node-agent and the // signalling pod do), so applying it forces those agents off the node. diff --git a/internal/controller/eviction_controller.go b/internal/controller/eviction/controller.go similarity index 54% rename from internal/controller/eviction_controller.go rename to internal/controller/eviction/controller.go index b0fa33c..c2a04b3 100644 --- a/internal/controller/eviction_controller.go +++ b/internal/controller/eviction/controller.go @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controller +package eviction import ( "context" @@ -23,7 +23,6 @@ import ( "fmt" "net/http" "strings" - "time" "github.com/gophercloud/gophercloud/v2" "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/hypervisors" @@ -38,6 +37,7 @@ import ( logger "sigs.k8s.io/controller-runtime/pkg/log" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/openstack" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) @@ -51,40 +51,59 @@ type EvictionReconciler struct { const ( EvictionControllerName = "eviction" - shortRetryTime = 1 * time.Second - defaultPollTime = 10 * time.Second ) -// popInstance removes and returns the last instance UUID from the slice. -// Returns the modified slice and the UUID (empty string if slice was empty). -func popInstance(instances []string) (remaining []string, uuid string) { - if len(instances) == 0 { - return instances, "" - } - return instances[:len(instances)-1], instances[len(instances)-1] +// candidate is an instance eligible to start migrating this pass, along with +// the migration mode decided from its already-fetched state. +type candidate struct { + id string + live bool } -// peekInstance returns the last instance UUID without removing it. -// Returns empty string if the slice is empty. -func peekInstance(instances []string) string { - if len(instances) == 0 { - return "" +// removeInstance returns instances with the first occurrence of uuid removed, +// preserving order. Used when a specific VM completes migration, which - once +// migrations run in parallel - is no longer necessarily the tail of the slice. +func removeInstance(instances []string, uuid string) []string { + for i, u := range instances { + if u == uuid { + return append(instances[:i:i], instances[i+1:]...) + } } - return instances[len(instances)-1] + return instances } -// moveToBack moves the last instance to the front of the slice, -// effectively deprioritizing it. Returns the modified slice. -func moveToBack(instances []string) []string { - if len(instances) < 2 { +// deprioritize moves the given instance to the back of the queue (index 0, +// since the queue is processed from the tail). Used to defer an ERROR or +// terminating instance so the eviction retries healthier instances first. If +// the uuid is absent or already at the back, the slice is returned unchanged. +func deprioritize(instances []string, uuid string) []string { + idx := -1 + for i, u := range instances { + if u == uuid { + idx = i + break + } + } + if idx <= 0 { return instances } - uuid := instances[len(instances)-1] - copy(instances[1:], instances[:len(instances)-1]) + // Shift [0:idx] up by one and place uuid at the front (= back of queue). + copy(instances[1:idx+1], instances[0:idx]) instances[0] = uuid return instances } +// isMigrationTaskState reports whether a nova task_state indicates a migration +// (live or cold/resize) is already underway. A freshly-triggered migration +// reports the instance as ACTIVE with such a task_state for a few seconds before +// its Status flips to MIGRATING/RESIZE; counting these prevents the eviction +// loop from over-triggering past the concurrency limit during that window. +// Nova values include "migrating", "migrating-start", "resize_prep", +// "resize_migrating", "resize_migrated", "resize_finish". +func isMigrationTaskState(taskState string) bool { + return strings.Contains(taskState, "migrat") || strings.Contains(taskState, "resize") +} + // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/status,verbs=get;update;patch // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/finalizers,verbs=update @@ -154,7 +173,8 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1. // That should leave us with "Running" and the hypervisor should be deactivated if len(eviction.Status.OutstandingInstances) > 0 { - return r.evictNext(ctx, eviction) + limit := global.ResolveConcurrency(hypervisor.Status.Traits) + return r.evictNext(ctx, eviction, limit) } meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ @@ -203,7 +223,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv }) { return ctrl.Result{}, r.updateStatus(ctx, eviction) } - return ctrl.Result{RequeueAfter: defaultPollTime}, nil // Wait for hypervisor to be disabled + return ctrl.Result{RequeueAfter: global.DefaultPollTime}, nil // Wait for hypervisor to be disabled } // Fetch all virtual machines on the hypervisor @@ -259,145 +279,203 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv return ctrl.Result{}, r.updateStatus(ctx, eviction) } -// Tries to handle the NotFound-error by updating the status -func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1.Eviction, err error) error { +// removeGone drops a specific instance from the outstanding set when it is +// gone (NotFound), recording a success condition. Returns true if the error was +// a NotFound and was handled, false otherwise (caller should treat the original +// error as a real failure). +func (r *EvictionReconciler) removeGone(eviction *kvmv1.Eviction, uuid string, err error) bool { if !gophercloud.ResponseCodeIs(err, http.StatusNotFound) { - return err - } - logger.FromContext(ctx).Info("Instance is gone") - var uuid string - eviction.Status.OutstandingInstances, uuid = popInstance(eviction.Status.OutstandingInstances) - if uuid == "" { - return nil + return false } - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Instance %s is gone", uuid), - Reason: kvmv1.ConditionReasonSucceeded, - }) - return r.updateStatus(ctx, eviction) + eviction.Status.OutstandingInstances = removeInstance(eviction.Status.OutstandingInstances, uuid) + r.setMigrationSucceeded(eviction, fmt.Sprintf("Instance %s is gone", uuid)) + return true } -func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) { - uuid := peekInstance(eviction.Status.OutstandingInstances) - if uuid == "" { - return ctrl.Result{}, nil +// evictNext scans the whole outstanding set once and keeps up to `limit` +// migrations in flight. Instances that have already left the host are removed, +// transient (MIGRATING/RESIZE) and terminating instances are counted as busy, +// and fresh migrations are started only while there is spare concurrency. With +// limit == 1 the behavior is equivalent to the historical one-at-a-time drain. +func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction, limit int) (ctrl.Result, error) { + if limit < 1 { + limit = 1 } - log := logger.FromContext(ctx).WithName("Evict").WithValues("server", uuid) - ctx = logger.IntoContext(ctx, log) - - res := servers.Get(ctx, r.computeClient, uuid) - vm, err := res.Extract() + baseLog := logger.FromContext(ctx).WithName("Evict") - if err != nil { - if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { - return ctrl.Result{}, err2 - } else { - return ctrl.Result{RequeueAfter: shortRetryTime}, nil - } - } + // Snapshot the current queue; we mutate eviction.Status.OutstandingInstances + // as instances complete, so iterate over a copy. + outstanding := append([]string(nil), eviction.Status.OutstandingInstances...) - log = log.WithValues("server_status", vm.Status) - ctx = logger.IntoContext(ctx, log) + inFlight := 0 // migrations currently running (or terminating) + started := 0 // migrations we triggered this pass + var candidates []candidate + var errs []error // errors to surface after the status update - // First, check the transient statuses - switch vm.Status { - case "MIGRATING", "RESIZE": - // wait for the migration to finish - return ctrl.Result{RequeueAfter: defaultPollTime}, nil - case "ERROR": - // Needs manual intervention (or another operator fixes it) - // put it at the end of the list (beginning of array) - eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances) - log.Info("error", "faultMessage", vm.Fault.Message) - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message), - Reason: kvmv1.ConditionReasonFailed, - }) + for _, uuid := range outstanding { + log := baseLog.WithValues("server", uuid) + vmCtx := logger.IntoContext(ctx, log) - return ctrl.Result{}, errors.Join(fmt.Errorf("error migrating instance %v", uuid), - r.updateStatus(ctx, eviction)) - } + vm, err := servers.Get(vmCtx, r.computeClient, uuid).Extract() + if err != nil { + if r.removeGone(eviction, uuid, err) { + continue + } + // Transient Get error - leave the instance queued and retry soon. + errs = append(errs, err) + continue + } - currentHypervisor, _, _ := strings.Cut(vm.HypervisorHostname, ".") - - if currentHypervisor != eviction.Spec.Hypervisor { - log.Info("migrated") - // Don't overwrite a sticky Failed migration condition with Succeeded - // while there are still other outstanding VMs - an earlier ERROR VM - // has been moved to the back of the queue and the eviction is not - // actually clean. The condition is reset only when the whole - // eviction completes (OutstandingInstances becomes empty). - remaining, _ := popInstance(eviction.Status.OutstandingInstances) - prior := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeMigration) - stickyFailure := len(remaining) > 0 && prior != nil && - prior.Status == metav1.ConditionFalse && - prior.Reason == kvmv1.ConditionReasonFailed - if !stickyFailure { + log = log.WithValues("server_status", vm.Status) + + switch vm.Status { + case "MIGRATING", "RESIZE": + // Already draining - occupies an in-flight slot. + inFlight++ + continue + case "ERROR": + // Needs manual intervention (or another operator fixes it); + // deprioritize and record the failure, but don't hold a slot. + eviction.Status.OutstandingInstances = deprioritize(eviction.Status.OutstandingInstances, uuid) + log.Info("error", "faultMessage", vm.Fault.Message) meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ Type: kvmv1.ConditionTypeMigration, Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Migration of instance %s finished", vm.ID), - Reason: kvmv1.ConditionReasonSucceeded, + Message: fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message), + Reason: kvmv1.ConditionReasonFailed, }) + errs = append(errs, fmt.Errorf("error migrating instance %v", uuid)) + continue } - // So, it is already off this one, do we need to verify it? - if vm.Status == "VERIFY_RESIZE" { - err := servers.ConfirmResize(ctx, r.computeClient, vm.ID).ExtractErr() - if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { - // Retry confirm in next reconciliation - return ctrl.Result{}, err2 - } else { - // handled not found without errors - return ctrl.Result{RequeueAfter: shortRetryTime}, nil - } + // A migration we triggered on a previous pass does not flip Status to + // MIGRATING/RESIZE immediately - nova first reports the instance as + // ACTIVE with a migration task_state (e.g. "migrating", + // "migrating-start", "resize_migrating"/"resize_prep"). Count those as + // in-flight too, otherwise the slot looks free and we would trigger a + // second migration, exceeding the concurrency limit. + if isMigrationTaskState(vm.TaskState) { + inFlight++ + continue } - // All done - eviction.Status.OutstandingInstances, _ = popInstance(eviction.Status.OutstandingInstances) - return ctrl.Result{}, r.updateStatus(ctx, eviction) - } + currentHypervisor, _, _ := strings.Cut(vm.HypervisorHostname, ".") + if currentHypervisor != eviction.Spec.Hypervisor { + // It has left this host. Confirm a pending resize first, otherwise + // consider it done and drop it from the outstanding set. + if vm.Status == "VERIFY_RESIZE" { + log.Info("confirm-resize") + err := servers.ConfirmResize(vmCtx, r.computeClient, vm.ID).ExtractErr() + if err != nil && !r.removeGone(eviction, uuid, err) { + // Retry confirm on the next pass. + errs = append(errs, err) + } + // Whether confirmed now or gone, treat as busy this pass so we + // re-check it next time before declaring completion. + inFlight++ + continue + } - if vm.TaskState == "deleting" { //nolint:gocritic - // We just have to wait for it to be gone. Try the next one. - eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances) + log.Info("migrated") + eviction.Status.OutstandingInstances = removeInstance(eviction.Status.OutstandingInstances, uuid) + r.setMigrationSucceeded(eviction, fmt.Sprintf("Migration of instance %s finished", vm.ID)) + continue + } - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID), - Reason: kvmv1.ConditionReasonFailed, + if vm.TaskState == "deleting" { + // Just wait for it to disappear; deprioritize and count as busy so + // we don't spend a fresh slot on it. + eviction.Status.OutstandingInstances = deprioritize(eviction.Status.OutstandingInstances, uuid) + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeMigration, + Status: metav1.ConditionFalse, + Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID), + Reason: kvmv1.ConditionReasonFailed, + }) + inFlight++ + continue + } + + // Otherwise it is a candidate to start migrating this pass. Decide the + // migration mode now from the state we already fetched. + candidates = append(candidates, candidate{ + id: vm.ID, + live: vm.Status == "ACTIVE" || vm.PowerState == 1, }) - if err := r.updateStatus(ctx, eviction); err != nil { - return ctrl.Result{}, fmt.Errorf("could not update status due to %w", err) + } + + // Start fresh migrations until we hit the concurrency limit. + for _, c := range candidates { + if inFlight+started >= limit { + break } - return ctrl.Result{RequeueAfter: defaultPollTime}, nil - } else if vm.Status == "ACTIVE" || vm.PowerState == 1 { - log.Info("trigger live-migration") - if err := r.liveMigrate(ctx, vm.ID, eviction); err != nil { - if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { - return ctrl.Result{}, err2 - } - return ctrl.Result{RequeueAfter: shortRetryTime}, nil + log := baseLog.WithValues("server", c.id) + migrateCtx := logger.IntoContext(ctx, log) + + var migErr error + if c.live { + log.Info("trigger live-migration") + migErr = r.liveMigrate(migrateCtx, c.id, eviction) + } else { + log.Info("trigger cold-migration") + migErr = r.coldMigrate(migrateCtx, c.id, eviction) } - } else { - log.Info("trigger cold-migration") - if err := r.coldMigrate(ctx, vm.ID, eviction); err != nil { - if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { - return ctrl.Result{}, err2 + if migErr != nil { + if r.removeGone(eviction, c.id, migErr) { + continue } - return ctrl.Result{RequeueAfter: shortRetryTime}, nil + errs = append(errs, migErr) + continue } + started++ + } + + // Persisting the status is the only genuinely fatal error here: if it fails + // we must return it (with no result) so controller-runtime retries. Per-VM + // problems (ERROR-state instances, failed migrate triggers, transient Gets) + // are expected and recoverable - they are recorded on the MigratingInstance + // condition and retried via RequeueAfter, so returning them as a reconcile + // error would only discard our RequeueAfter (controller-runtime ignores the + // result when the error is non-nil) and emit a warning. + if err := r.updateStatus(ctx, eviction); err != nil { + return ctrl.Result{}, err + } + + if joined := errors.Join(errs...); joined != nil { + baseLog.Info("instances need attention this pass; will retry", "err", joined.Error()) + } + + baseLog.Info("poll", "inFlight", inFlight, "started", started, + "outstanding", len(eviction.Status.OutstandingInstances), "limit", limit) + + // Requeue while there is still work; use a short retry when nothing is + // actually migrating yet (e.g. all instances errored) so we don't idle. + requeue := global.DefaultPollTime + if inFlight+started == 0 && len(eviction.Status.OutstandingInstances) > 0 { + requeue = global.ShortRetryTime } + return ctrl.Result{RequeueAfter: requeue}, nil +} - // Triggered a migration, give it a generous time to start, so we do not - // see the old state because the migration didn't start - log.Info("poll") - return ctrl.Result{RequeueAfter: defaultPollTime}, nil +// setMigrationSucceeded records a successful migration (with the given message) +// without clobbering a sticky Failed condition while other instances are still +// outstanding - an earlier ERROR instance moved to the back of the queue means +// the eviction is not actually clean yet. The condition is only allowed to flip +// to Succeeded once the whole eviction completes (OutstandingInstances empty). +func (r *EvictionReconciler) setMigrationSucceeded(eviction *kvmv1.Eviction, msg string) { + prior := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeMigration) + stickyFailure := len(eviction.Status.OutstandingInstances) > 0 && prior != nil && + prior.Status == metav1.ConditionFalse && + prior.Reason == kvmv1.ConditionReasonFailed + if stickyFailure { + return + } + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeMigration, + Status: metav1.ConditionFalse, + Message: msg, + Reason: kvmv1.ConditionReasonSucceeded, + }) } func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error { diff --git a/internal/controller/eviction_controller_test.go b/internal/controller/eviction/controller_test.go similarity index 59% rename from internal/controller/eviction_controller_test.go rename to internal/controller/eviction/controller_test.go index 5ef315b..39ca893 100644 --- a/internal/controller/eviction_controller_test.go +++ b/internal/controller/eviction/controller_test.go @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controller +package eviction import ( "fmt" @@ -33,83 +33,75 @@ import ( ctrl "sigs.k8s.io/controller-runtime" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global" ) var _ = Describe("Instance slice helpers", func() { - Describe("peekInstance", func() { - It("returns empty string for empty slice", func() { - Expect(peekInstance([]string{})).To(Equal("")) + Describe("removeInstance", func() { + It("returns unchanged slice for empty slice", func() { + Expect(removeInstance([]string{}, "a")).To(BeEmpty()) }) - It("returns empty string for nil slice", func() { - Expect(peekInstance(nil)).To(Equal("")) + It("returns nil for nil slice", func() { + Expect(removeInstance(nil, "a")).To(BeNil()) }) - It("returns the last element", func() { - Expect(peekInstance([]string{"a", "b", "c"})).To(Equal("c")) + It("removes the matching element preserving order", func() { + Expect(removeInstance([]string{"a", "b", "c"}, "b")).To(Equal([]string{"a", "c"})) }) - It("returns the only element for single-element slice", func() { - Expect(peekInstance([]string{"only"})).To(Equal("only")) + It("removes the first element", func() { + Expect(removeInstance([]string{"a", "b", "c"}, "a")).To(Equal([]string{"b", "c"})) }) - It("does not modify the slice", func() { - s := []string{"a", "b", "c"} - peekInstance(s) - Expect(s).To(Equal([]string{"a", "b", "c"})) + It("removes the last element", func() { + Expect(removeInstance([]string{"a", "b", "c"}, "c")).To(Equal([]string{"a", "b"})) }) - }) - Describe("popInstance", func() { - It("returns empty string and unchanged slice for empty slice", func() { - s, uuid := popInstance([]string{}) - Expect(uuid).To(Equal("")) - Expect(s).To(BeEmpty()) + It("removes only the first occurrence", func() { + Expect(removeInstance([]string{"a", "b", "a"}, "a")).To(Equal([]string{"b", "a"})) }) - It("returns empty string and unchanged slice for nil slice", func() { - s, uuid := popInstance(nil) - Expect(uuid).To(Equal("")) - Expect(s).To(BeNil()) + It("returns the slice unchanged when the uuid is absent", func() { + Expect(removeInstance([]string{"a", "b"}, "z")).To(Equal([]string{"a", "b"})) }) - It("removes and returns the last element", func() { - s, uuid := popInstance([]string{"a", "b", "c"}) - Expect(uuid).To(Equal("c")) - Expect(s).To(Equal([]string{"a", "b"})) - }) - - It("returns empty slice when popping last element", func() { - s, uuid := popInstance([]string{"only"}) - Expect(uuid).To(Equal("only")) - Expect(s).To(BeEmpty()) + It("does not mutate the input backing array", func() { + s := []string{"a", "b", "c"} + _ = removeInstance(s, "b") + Expect(s).To(Equal([]string{"a", "b", "c"})) }) }) - Describe("moveToBack", func() { + Describe("deprioritize", func() { It("returns unchanged slice for empty slice", func() { - s := moveToBack([]string{}) - Expect(s).To(BeEmpty()) + Expect(deprioritize([]string{}, "a")).To(BeEmpty()) }) - It("returns unchanged slice for nil slice", func() { - s := moveToBack(nil) - Expect(s).To(BeNil()) + It("returns nil for nil slice", func() { + Expect(deprioritize(nil, "a")).To(BeNil()) }) It("returns unchanged slice for single-element slice", func() { - s := moveToBack([]string{"only"}) - Expect(s).To(Equal([]string{"only"})) + Expect(deprioritize([]string{"only"}, "only")).To(Equal([]string{"only"})) }) - It("moves last element to front for two elements", func() { - s := moveToBack([]string{"a", "b"}) - Expect(s).To(Equal([]string{"b", "a"})) + It("is a no-op when the uuid is already at the back (index 0)", func() { + Expect(deprioritize([]string{"a", "b", "c"}, "a")).To(Equal([]string{"a", "b", "c"})) }) - It("moves last element to front for multiple elements", func() { - s := moveToBack([]string{"a", "b", "c", "d"}) - Expect(s).To(Equal([]string{"d", "a", "b", "c"})) + It("is a no-op when the uuid is absent", func() { + Expect(deprioritize([]string{"a", "b", "c"}, "z")).To(Equal([]string{"a", "b", "c"})) + }) + + It("moves the tail (head-of-queue) element to the back", func() { + // Queue is processed from the tail; "c" is next, deprioritizing it + // puts it at index 0 (the back). + Expect(deprioritize([]string{"a", "b", "c"}, "c")).To(Equal([]string{"c", "a", "b"})) + }) + + It("moves a specific middle element to the back", func() { + Expect(deprioritize([]string{"a", "b", "c", "d"}, "c")).To(Equal([]string{"c", "a", "b", "d"})) }) }) }) @@ -466,12 +458,12 @@ var _ = Describe("Eviction Controller", func() { liveMigrateCalls = map[string]int{} By("Seeding the eviction status with a list of VMs to evict") - // OutstandingInstances is processed from the END (peekInstance returns - // last). With [good-1, error-1, good-2], processing order is: - // 1) good-2 (last) - migrate, then drop - // 2) error-1 (now last) - skipped via moveToBack - // 3) good-1 - migrate, then drop - // 4) error-1 (alone) - keeps erroring + // With the default concurrency of 1, each reconcile pass scans the + // whole outstanding set but starts at most one new migration. The + // ERROR VM is deprioritized (moved to the back of the queue) and + // never migrated, while + // the two healthy VMs migrate one after another and are removed as + // soon as they report a different host. eviction := &kvmv1.Eviction{} Expect(k8sClient.Get(ctx, typeNamespacedName, eviction)).To(Succeed()) meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ @@ -547,11 +539,11 @@ var _ = Describe("Eviction Controller", func() { By("Running reconciliations until only the errored VM remains") const maxLoops = 20 for i := range maxLoops { - // Tolerate errors here: when the controller hits the ERROR - // VM it returns an error (joined with the status update). - // That is part of the pattern under test, not a test failure. + // Reconcile no longer returns an error for an ERROR-state VM + // (it's recorded on the condition and retried via RequeueAfter), + // so no error is expected here. _, reconcileErr := evictionReconciler.Reconcile(ctx, reconcileRequest) - _ = reconcileErr // expected on ERROR-VM iterations + Expect(reconcileErr).NotTo(HaveOccurred()) Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) // Once both healthy VMs have been migrated and removed, we are @@ -588,15 +580,268 @@ var _ = Describe("Eviction Controller", func() { HaveField("Reason", kvmv1.ConditionReasonSucceeded), ))) - By("Subsequent reconciliations keep retrying the errored VM (and surfacing the error)") + By("Subsequent reconciliations keep retrying the errored VM without surfacing a reconcile error") + result, err := evictionReconciler.Reconcile(ctx, reconcileRequest) + // An ERROR-state instance is an expected, recoverable condition: + // the controller records it on the MigratingInstance condition and + // retries via RequeueAfter, rather than returning a reconcile error + // (which would discard the RequeueAfter and spam warnings). + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + Expect(resource.Status.OutstandingInstances).To(Equal([]string{"error-1"})) + By("The failure remains visible on the MigratingInstance condition") + Expect(resource.Status.Conditions).To(ContainElement(SatisfyAll( + HaveField("Type", kvmv1.ConditionTypeMigration), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", kvmv1.ConditionReasonFailed), + HaveField("Message", ContainSubstring("error-1")), + ))) + }) + }) + + Context("Parallel Eviction", func() { + const serverTpl = `{ + "server": { + "id": "%[1]s", + "status": "%[2]s", + "OS-EXT-SRV-ATTR:hypervisor_hostname": "%[3]s", + "OS-EXT-STS:task_state": "", + "OS-EXT-STS:power_state": 1 + } +}` + + var migrateCalls map[string]int + // seedVMs seeds the eviction with n ACTIVE VMs already past preflight, + // and installs the per-VM mock. A VM that has received a migrate call + // reports a different host on the next GET, so it leaves the queue. + seedVMs := func(ctx SpecContext, n int) []string { + migrateCalls = map[string]int{} + ids := make([]string, n) + for i := range ids { + ids[i] = fmt.Sprintf("vm-%d", i) + } + + eviction := &kvmv1.Eviction{} + Expect(k8sClient.Get(ctx, typeNamespacedName, eviction)).To(Succeed()) + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, Status: metav1.ConditionTrue, + Message: "Running", Reason: kvmv1.ConditionReasonRunning, + }) + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypePreflight, Status: metav1.ConditionTrue, + Message: "preflight passed", Reason: kvmv1.ConditionReasonSucceeded, + }) + eviction.Status.HypervisorServiceId = serviceId + eviction.Status.OutstandingInstances = append([]string(nil), ids...) + Expect(k8sClient.Status().Update(ctx, eviction)).To(Succeed()) + + fakeServer.Mux.HandleFunc("GET /servers/{server_id}", func(w http.ResponseWriter, r *http.Request) { + serverID := r.PathValue("server_id") + hvHost := hypervisorName + ".example.local" + if migrateCalls[serverID] > 0 { + hvHost = "other-host.example.local" + } + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprintf(w, serverTpl, serverID, "ACTIVE", hvHost) + Expect(err).NotTo(HaveOccurred()) + }) + fakeServer.Mux.HandleFunc("POST /servers/{server_id}/action", func(w http.ResponseWriter, r *http.Request) { + migrateCalls[r.PathValue("server_id")]++ + w.WriteHeader(http.StatusAccepted) + }) + return ids + } + + // inFlight counts VMs that have been told to migrate but have not yet + // been observed leaving the host (i.e. still outstanding). + inFlight := func(outstanding []string) int { + n := 0 + for _, id := range outstanding { + if migrateCalls[id] > 0 { + n++ + } + } + return n + } + + It("starts at most `limit` migrations per pass and never exceeds it in flight", func(ctx SpecContext) { + orig := global.EvictionConcurrency + global.EvictionConcurrency = 3 + DeferCleanup(func() { global.EvictionConcurrency = orig }) + + seedVMs(ctx, 5) + resource := &kvmv1.Eviction{} + + By("First pass triggers exactly the limit (3) migrations") _, err := evictionReconciler.Reconcile(ctx, reconcileRequest) - // The controller returns an error when it encounters a VM in ERROR - // state. The reconcile error should mention the errored UUID. - if err != nil { - Expect(err.Error()).To(ContainSubstring("error-1")) + Expect(err).NotTo(HaveOccurred()) + started := 0 + for _, c := range migrateCalls { + started += c } + Expect(started).To(Equal(3), "should start exactly limit migrations in the first pass") + + By("Draining to completion, never exceeding the limit in flight") Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) - Expect(resource.Status.OutstandingInstances).To(Equal([]string{"error-1"})) + for range 20 { + if len(resource.Status.OutstandingInstances) == 0 { + break + } + Expect(inFlight(resource.Status.OutstandingInstances)).To(BeNumerically("<=", 3)) + _, err := evictionReconciler.Reconcile(ctx, reconcileRequest) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + } + + Expect(resource.Status.OutstandingInstances).To(BeEmpty()) + By("Every VM was migrated exactly once") + for id, c := range migrateCalls { + Expect(c).To(Equal(1), "VM %s migrated exactly once", id) + } + Expect(migrateCalls).To(HaveLen(5)) + }) + + It("does not exceed the limit when a triggered migration has not yet flipped to MIGRATING status", func(ctx SpecContext) { + // Regression: nova reports a freshly-triggered instance as ACTIVE + // with task_state "migrating" for a while before Status becomes + // MIGRATING. If those aren't counted as in-flight, the loop keeps + // triggering and exceeds the concurrency limit. + orig := global.EvictionConcurrency + global.EvictionConcurrency = 2 + DeferCleanup(func() { global.EvictionConcurrency = orig }) + + const tmpl = `{ + "server": { + "id": "%[1]s", + "status": "%[2]s", + "OS-EXT-SRV-ATTR:hypervisor_hostname": "%[3]s", + "OS-EXT-STS:task_state": "%[4]s", + "OS-EXT-STS:power_state": 1 + } +}` + calls := map[string]int{} + // polls-since-trigger per VM; a migrated VM lingers ACTIVE with + // task_state=migrating for 2 polls (as nova does), then leaves. + pollsSince := map[string]int{} + + ids := make([]string, 5) + for i := range ids { + ids[i] = fmt.Sprintf("tsvm-%d", i) + } + eviction := &kvmv1.Eviction{} + Expect(k8sClient.Get(ctx, typeNamespacedName, eviction)).To(Succeed()) + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, Status: metav1.ConditionTrue, + Message: "Running", Reason: kvmv1.ConditionReasonRunning, + }) + meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypePreflight, Status: metav1.ConditionTrue, + Message: "preflight passed", Reason: kvmv1.ConditionReasonSucceeded, + }) + eviction.Status.HypervisorServiceId = serviceId + eviction.Status.OutstandingInstances = append([]string(nil), ids...) + Expect(k8sClient.Status().Update(ctx, eviction)).To(Succeed()) + + fakeServer.Mux.HandleFunc("GET /servers/{server_id}", func(w http.ResponseWriter, r *http.Request) { + serverID := r.PathValue("server_id") + host := hypervisorName + ".example.local" + status, task := "ACTIVE", "" + if calls[serverID] > 0 { + pollsSince[serverID]++ + if pollsSince[serverID] <= 2 { + // Still on the source host, ACTIVE, task_state migrating - + // the window that used to be miscounted as a free slot. + task = "migrating" + } else { + host = "other-host.example.local" // finally left + } + } + w.Header().Add("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, err := fmt.Fprintf(w, tmpl, serverID, status, host, task) + Expect(err).NotTo(HaveOccurred()) + }) + fakeServer.Mux.HandleFunc("POST /servers/{server_id}/action", func(w http.ResponseWriter, r *http.Request) { + calls[r.PathValue("server_id")]++ + w.WriteHeader(http.StatusAccepted) + }) + + resource := &kvmv1.Eviction{} + concurrentlyTriggered := func() int { + // VMs triggered but not yet observed off-host. + n := 0 + for id, c := range calls { + if c > 0 && pollsSince[id] <= 2 { + n++ + } + } + return n + } + + By("Draining, asserting no more than `limit` are ever in flight") + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + for range 40 { + if len(resource.Status.OutstandingInstances) == 0 { + break + } + _, err := evictionReconciler.Reconcile(ctx, reconcileRequest) + Expect(err).NotTo(HaveOccurred()) + Expect(concurrentlyTriggered()).To(BeNumerically("<=", 2), + "in-flight migrations must never exceed the limit") + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + } + + Expect(resource.Status.OutstandingInstances).To(BeEmpty()) + By("Every VM migrated exactly once") + for id, c := range calls { + Expect(c).To(Equal(1), "VM %s migrated exactly once", id) + } + Expect(calls).To(HaveLen(5)) + }) + + It("forces serial migration for hosts carrying the exclusive trait", func(ctx SpecContext) { + orig := global.EvictionConcurrency + origMap := global.EvictionTraitConcurrency + global.EvictionConcurrency = 4 + global.EvictionTraitConcurrency = map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1} + DeferCleanup(func() { + global.EvictionConcurrency = orig + global.EvictionTraitConcurrency = origMap + }) + + By("Marking the hypervisor with the exclusive trait") + hv := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: hypervisorName}, hv)).To(Succeed()) + hv.Status.Traits = []string{"CUSTOM_HANA_EXCLUSIVE_HOST"} + Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed()) + + seedVMs(ctx, 4) + resource := &kvmv1.Eviction{} + + By("First pass triggers only ONE migration despite the higher global default") + _, err := evictionReconciler.Reconcile(ctx, reconcileRequest) + Expect(err).NotTo(HaveOccurred()) + started := 0 + for _, c := range migrateCalls { + started += c + } + Expect(started).To(Equal(1), "exclusive-trait host must migrate serially") + + By("Never more than one in flight through to completion") + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + for range 30 { + if len(resource.Status.OutstandingInstances) == 0 { + break + } + Expect(inFlight(resource.Status.OutstandingInstances)).To(BeNumerically("<=", 1)) + _, err := evictionReconciler.Reconcile(ctx, reconcileRequest) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed()) + } + Expect(resource.Status.OutstandingInstances).To(BeEmpty()) + Expect(migrateCalls).To(HaveLen(4)) }) }) }) diff --git a/internal/controller/eviction/suite_test.go b/internal/controller/eviction/suite_test.go new file mode 100644 index 0000000..d4e3bed --- /dev/null +++ b/internal/controller/eviction/suite_test.go @@ -0,0 +1,83 @@ +/* +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 eviction + +import ( + "fmt" + "path/filepath" + "runtime" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" +) + +// The eviction controller reads and writes Hypervisor/Eviction resources +// against a real API server, so its tests run under envtest. This file owns the +// suite runner and the environment bootstrap; controller_test.go holds the +// specs. These tests use Ginkgo - see http://onsi.github.io/ginkgo/. + +var cfg *rest.Config +var k8sClient client.Client +var testEnv *envtest.Environment + +func TestEviction(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Eviction Controller Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "charts", "openstack-hypervisor-operator", "crds")}, + ErrorIfCRDPathMissing: true, + + // Only used when KUBEBUILDER_ASSETS is not set (e.g. running the test + // directly instead of via `make test`). + BinaryAssetsDirectory: filepath.Join("..", "..", "..", "bin", "k8s", + fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + } + + var err error + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + err = kvmv1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + Expect(testEnv.Stop()).To(Succeed()) +}) diff --git a/internal/controller/traits_controller.go b/internal/controller/traits_controller.go index 4e91e21..b079676 100644 --- a/internal/controller/traits_controller.go +++ b/internal/controller/traits_controller.go @@ -22,7 +22,6 @@ import ( "errors" "slices" "strings" - "time" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -66,7 +65,7 @@ func (tc *TraitsController) Reconcile(ctx context.Context, req ctrl.Request) (ct // ensure hypervisorID is set if hv.Status.HypervisorID == "" { - return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + return ctrl.Result{RequeueAfter: defaultPollTime}, nil } if hv.Spec.Maintenance == kvmv1.MaintenanceTermination { diff --git a/internal/global/global.go b/internal/global/global.go index 3a687b6..9e40d29 100644 --- a/internal/global/global.go +++ b/internal/global/global.go @@ -17,6 +17,20 @@ limitations under the License. package global +import "time" + +const ( + // DefaultPollTime is the standard requeue interval controllers use while + // waiting for a slow external state transition (an OpenStack migration, a + // service being disabled, ...) to settle. + DefaultPollTime = 10 * time.Second + + // ShortRetryTime is a brief requeue interval used to retry quickly when a + // reconcile could make progress on the next pass without waiting for a full + // poll cycle. + ShortRetryTime = 1 * time.Second +) + var ( // LabelSelector is a custom label that is used to select resources managed by the operator. LabelSelector = "" @@ -25,4 +39,35 @@ var ( // neutron) are scheduled. The pod list during offboarding is restricted to // these namespaces. Must be non-empty; set via --agent-namespaces. AgentNamespaces []string + + // EvictionConcurrency is the default maximum number of VM migrations that may + // run concurrently while draining a single hypervisor. Defaults to 1 (serial), + // which preserves the historical one-at-a-time behavior. Set via + // --eviction-concurrency. + EvictionConcurrency = 1 + + // EvictionTraitConcurrency maps a Placement trait name to a maximum number of + // concurrent migrations for hosts carrying that trait. It overrides + // EvictionConcurrency for matching hosts. Typical use is forcing exclusive host + // classes (e.g. CUSTOM_HANA_EXCLUSIVE_HOST) to migrate serially. Set via + // --eviction-trait-concurrency. + EvictionTraitConcurrency = map[string]int{} ) + +// ResolveConcurrency returns the maximum number of concurrent migrations for a +// host given its traits. It starts from EvictionConcurrency and, for every trait +// present in EvictionTraitConcurrency, keeps the lowest configured limit. The +// result is always clamped to at least 1 so a misconfiguration can never stall a +// drain entirely. +func ResolveConcurrency(traits []string) int { + limit := EvictionConcurrency + for _, t := range traits { + if traitLimit, ok := EvictionTraitConcurrency[t]; ok && traitLimit < limit { + limit = traitLimit + } + } + if limit < 1 { + limit = 1 + } + return limit +} diff --git a/internal/global/global_test.go b/internal/global/global_test.go new file mode 100644 index 0000000..167cdfc --- /dev/null +++ b/internal/global/global_test.go @@ -0,0 +1,91 @@ +/* +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 global + +import "testing" + +func TestResolveConcurrency(t *testing.T) { + // Save and restore the package globals so tests don't leak into each other. + origDefault := EvictionConcurrency + origMap := EvictionTraitConcurrency + t.Cleanup(func() { + EvictionConcurrency = origDefault + EvictionTraitConcurrency = origMap + }) + + tests := []struct { + name string + defaultVal int + traitMap map[string]int + traits []string + want int + }{ + { + name: "no traits falls back to default", + defaultVal: 3, + traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traits: nil, + want: 3, + }, + { + name: "non-matching trait falls back to default", + defaultVal: 3, + traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traits: []string{"CUSTOM_SOMETHING_ELSE"}, + want: 3, + }, + { + name: "matching trait overrides default", + defaultVal: 5, + traitMap: map[string]int{"CUSTOM_HANA_EXCLUSIVE_HOST": 1}, + traits: []string{"CUSTOM_HANA_EXCLUSIVE_HOST"}, + want: 1, + }, + { + name: "lowest matching trait wins", + defaultVal: 10, + traitMap: map[string]int{"CUSTOM_A": 4, "CUSTOM_B": 2, "CUSTOM_C": 7}, + traits: []string{"CUSTOM_A", "CUSTOM_B", "CUSTOM_C"}, + want: 2, + }, + { + name: "trait limit above default is ignored (default is the cap)", + defaultVal: 2, + traitMap: map[string]int{"CUSTOM_BIG": 8}, + traits: []string{"CUSTOM_BIG"}, + want: 2, + }, + { + name: "result is clamped to at least 1", + defaultVal: 0, + traitMap: map[string]int{}, + traits: nil, + want: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + EvictionConcurrency = tt.defaultVal + EvictionTraitConcurrency = tt.traitMap + if got := ResolveConcurrency(tt.traits); got != tt.want { + t.Errorf("ResolveConcurrency(%v) = %d, want %d", tt.traits, got, tt.want) + } + }) + } +}