Eviction: Support parallel VM migrations while draining a hypervisor - #353
Open
notandy wants to merge 1 commit into
Open
Eviction: Support parallel VM migrations while draining a hypervisor#353notandy wants to merge 1 commit into
notandy wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change adds CLI-configurable eviction concurrency with trait-specific limits. It adds shared polling settings and replaces sequential eviction with bounded concurrent processing. Tests cover queue operations, migration states, concurrency limits, trait overrides, and envtest setup. ChangesEviction concurrency
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EvictionReconciler
participant GlobalConfig
participant HypervisorAPI
participant KubernetesAPI
EvictionReconciler->>GlobalConfig: ResolveConcurrency(hypervisor traits)
EvictionReconciler->>HypervisorAPI: inspect outstanding instances
EvictionReconciler->>HypervisorAPI: start migrations up to the resolved limit
EvictionReconciler->>KubernetesAPI: persist status once per pass
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
notandy
force-pushed
the
eviction-parallelism
branch
from
August 11, 2026 20:41
278ecfa to
802169f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cmd/main.go (1)
176-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
entryas a structured field instead of concatenating it into the error string.Static analysis flags this as log injection. The exploitability is low here:
entrycomes from a process flag that the operator controls, and the process exits right after. The structured form is still the idiomatic logr pattern and keeps the log parseable.♻️ Proposed refactor
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") + setupLog.Error(errors.New("invalid --eviction-trait-concurrency entry"), + "invalid configuration", "entry", entry, "expected", "TRAIT=N with N >= 1") os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/main.go` around lines 176 - 180, Update the validation error in the eviction-trait concurrency parsing block to keep the static error message constant and pass the raw entry through the structured logging fields. Preserve the existing “invalid configuration” context and expected-format field, and retain the immediate exit behavior.Source: Linters/SAST tools
internal/controller/eviction/controller_test.go (1)
625-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mock never reports
MIGRATING, so the in-flight accounting is untested.A VM flips directly from this host to another host as soon as a migrate call lands. The
case "MIGRATING", "RESIZE"branch inevictNextnever runs. The tests therefore verify only thestartedcounter within a single pass, not theinFlight + started >= limitbound that enforces the limit across passes. That bound is the core of this feature.Add a state where a VM reports
MIGRATINGon the host for one or more passes before it moves, so a later pass must count it as in flight.♻️ Suggested mock change
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 { + status := "ACTIVE" + // Report MIGRATING on the source host for `settleDelay` polls so + // the reconciler must count the VM as in flight before it moves. + switch { + case migrateCalls[serverID] == 0: + case polls[serverID] < settleDelay: + polls[serverID]++ + status = "MIGRATING" + default: hvHost = "other-host.example.local" } w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, err := fmt.Fprintf(w, serverTpl, serverID, "ACTIVE", hvHost) + _, err := fmt.Fprintf(w, serverTpl, serverID, status, hvHost) Expect(err).NotTo(HaveOccurred()) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/eviction/controller_test.go` around lines 625 - 635, Update the GET /servers/{server_id} mock in the eviction tests so a migrated VM reports the existing hypervisor with state “MIGRATING” for at least one polling pass before switching to the other host as “ACTIVE”. Ensure migrateCalls-based responses exercise evictNext’s “MIGRATING”, “RESIZE” branch and validate the inFlight + started limit across passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/eviction/controller_test.go`:
- Around line 731-732: Strengthen the exclusive-trait spec’s migration
assertions by verifying each VM’s migrate call count equals one, matching the
per-VM assertion used in the first spec. Update the assertions around
migrateCalls so duplicate migrations cannot pass merely because the map contains
four entries, while preserving the existing resource status and total-entry
checks.
In `@internal/controller/eviction/controller.go`:
- Around line 321-333: Update internal/controller/eviction/controller.go lines
321-333 and 359-371 to replace moveToBack calls with a helper that moves the
specific uuid being processed to the end of OutstandingInstances, preserving
queue order during full-queue scans. In
internal/controller/eviction/controller_test.go lines 456-460, revise the
outdated moveToBack comment and add coverage asserting the resulting order when
two ERROR instances are present.
- Around line 270-277: Update removeGone to route the migration condition
through the shared setMigrationCompleted helper instead of writing a succeeded
condition directly. Extract or reuse the sticky-failure guard currently in
setMigrationSucceeded so a Failed condition remains while outstanding instances
exist, and have setMigrationSucceeded delegate to that helper with its existing
message.
- Around line 156-157: Update handleRunning before calling evictNext to require
the hypervisor’s TraitsUpdated condition to be True, returning without draining
when traits are stale or the traits controller reports failure. Only resolve the
concurrency limit from hypervisor.Status.Traits and invoke evictNext after that
condition is satisfied.
---
Nitpick comments:
In `@cmd/main.go`:
- Around line 176-180: Update the validation error in the eviction-trait
concurrency parsing block to keep the static error message constant and pass the
raw entry through the structured logging fields. Preserve the existing “invalid
configuration” context and expected-format field, and retain the immediate exit
behavior.
In `@internal/controller/eviction/controller_test.go`:
- Around line 625-635: Update the GET /servers/{server_id} mock in the eviction
tests so a migrated VM reports the existing hypervisor with state “MIGRATING”
for at least one polling pass before switching to the other host as “ACTIVE”.
Ensure migrateCalls-based responses exercise evictNext’s “MIGRATING”, “RESIZE”
branch and validate the inFlight + started limit across passes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90777f3a-506b-4cf7-9260-8ed8607cd549
📒 Files selected for processing (8)
cmd/main.gointernal/controller/constants.gointernal/controller/eviction/controller.gointernal/controller/eviction/controller_test.gointernal/controller/eviction/suite_test.gointernal/controller/traits_controller.gointernal/global/global.gointernal/global/global_test.go
Comment on lines
+156
to
+157
| limit := global.ResolveConcurrency(hypervisor.Status.Traits) | ||
| return r.evictNext(ctx, eviction, limit) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace population of Hypervisor Status.Traits and any eviction-time gating on it.
set -euo pipefail
# Where Status.Traits is assigned.
rg -nP --type=go -C4 '\.Status\.Traits\s*='
# Whether any eviction path checks the traits condition before draining.
rg -nP --type=go -C4 'ConditionTypeTraitsUpdated'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(controller|api|types|hypervisor|eviction).*\.go$' | head -200
printf '%s\n' '--- all trait references ---'
rg -n -i --type=go -C3 'traits|TraitsUpdated|ResolveConcurrency|handleRunning|evictNext' . | head -400Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 33446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- eviction controller outline ---'
ast-grep outline internal/controller/eviction/controller.go
printf '%s\n' '--- traits controller outline ---'
ast-grep outline internal/controller/traits_controller.go
printf '%s\n' '--- eviction controller ---'
sed -n '1,260p' internal/controller/eviction/controller.go
printf '%s\n' '--- traits controller ---'
sed -n '1,280p' internal/controller/traits_controller.go
printf '%s\n' '--- lifecycle and eviction references ---'
rg -n --type=go -C5 'EvictionInProgress|WaitingForTraits|ConditionTypeEviction|handleRunning|eviction' internal/controller api/v1 | head -500Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 50400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- concurrency resolver and tests ---'
rg -n -C8 'func ResolveConcurrency|ResolveConcurrency|exclusive|Trait' internal/global internal/controller/eviction
printf '%s\n' '--- maintenance reconcile flow ---'
sed -n '58,220p' internal/controller/hypervisor_maintenance_controller.go
printf '%s\n' '--- onboarding handover and termination flow ---'
sed -n '280,370p' internal/controller/onboarding_controller.go
sed -n '90,180p' internal/controller/hypervisor_controller.go
printf '%s\n' '--- all production uses of traits conditions and waiting reason ---'
rg -n -C6 --type=go 'ConditionTypeTraitsUpdated|ConditionReasonWaitingForTraits|Status\.Traits' \
internal/controller api/v1 --glob '!**/*_test.go'Repository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 33060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- controller registration order ---'
sed -n '330,390p' cmd/main.go
printf '%s\n' '--- maintenance and onboarding condition tests ---'
rg -n -C8 --type=go \
'MaintenanceTermination|ConditionTypeOnboarding|ConditionTypeTraitsUpdated|Status\.Traits' \
internal/controller/hypervisor_maintenance_controller_test.go \
internal/controller/onboarding_controller_test.go \
internal/controller/traits_controller_test.go
printf '%s\n' '--- deterministic guard check ---'
python3 - <<'PY'
from pathlib import Path
maintenance = Path("internal/controller/hypervisor_maintenance_controller.go").read_text()
eviction = Path("internal/controller/eviction/controller.go").read_text()
onboarding = Path("internal/controller/onboarding_controller.go").read_text()
checks = {
"maintenance blocks while onboarding is true":
'if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)' in maintenance,
"maintenance does not check TraitsUpdated":
'ConditionTypeTraitsUpdated' not in maintenance,
"eviction handleRunning resolves from Status.Traits":
'global.ResolveConcurrency(hypervisor.Status.Traits)' in eviction,
"eviction does not check TraitsUpdated":
'ConditionTypeTraitsUpdated' not in eviction,
"onboarding completion requires TraitsUpdated":
'meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTraitsUpdated)' in onboarding,
}
for name, value in checks.items():
print(f"{name}: {value}")
PYRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 40704
Gate eviction on TraitsUpdated=True.
Onboarding gates only initial eviction. After onboarding, maintenance can create an eviction while Status.Traits is stale or while TraitsController reports an update failure. handleRunning can then apply the global limit instead of the trait-specific limit. Wait for TraitsUpdated=True before draining.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/eviction/controller.go` around lines 156 - 157, Update
handleRunning before calling evictNext to require the hypervisor’s TraitsUpdated
condition to be True, returning without draining when traits are stale or the
traits controller reports failure. Only resolve the concurrency limit from
hypervisor.Status.Traits and invoke evictNext after that condition is satisfied.
notandy
force-pushed
the
eviction-parallelism
branch
from
August 11, 2026 20:58
802169f to
f88f0cb
Compare
notandy
force-pushed
the
eviction-parallelism
branch
from
August 11, 2026 21:12
f88f0cb to
abd5299
Compare
Contributor
Author
|
Addressed the review findings in the amended commit (
Also fixed a separate concurrency-limit overshoot (freshly-triggered migrations weren't counted as in-flight until nova flipped their status to MIGRATING, so the loop could exceed the limit — verified live with 3 concurrent migrations at limit 2). Now counts instances with a migration |
There was a problem hiding this comment.
Pull request overview
Adds configurable parallel VM migration during hypervisor eviction while preserving serial defaults.
Changes:
- Adds global and trait-specific concurrency limits.
- Refactors eviction logic to track multiple migrations.
- Moves eviction controller into a dedicated package and expands tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cmd/main.go |
Adds concurrency flags and validation. |
internal/global/global.go |
Defines limits, resolution, and retry intervals. |
internal/global/global_test.go |
Tests concurrency resolution. |
internal/controller/constants.go |
Shares the global polling interval. |
internal/controller/traits_controller.go |
Uses the shared polling interval. |
internal/controller/eviction/controller.go |
Implements parallel eviction. |
internal/controller/eviction/controller_test.go |
Tests helpers and parallel migrations. |
internal/controller/eviction/suite_test.go |
Bootstraps the relocated test suite. |
Suppressed comments (2)
internal/controller/eviction/controller.go:446
- Returning a non-nil joined error discards this
RequeueAfterin controller-runtime. Because this pass can successfully start migrations and also collect an error from another VM (for example, an existingERRORVM or one failed POST), the request may be retried immediately before Nova exposes a migration task state, causing duplicate migration actions; persistent VM errors then drive exponential backoff and delay the healthy migrations. Separate fatal reconciliation/status errors from per-VM blocked errors, and preserve the poll delay whenever migrations were started or remain in flight; only return an error when no progress can safely continue.
internal/controller/eviction/controller.go:315 - This scans from index 0, while
deprioritizeand its tests define index 0 as the back of a queue processed from the tail. Consequently the serial limit now starts the opposite end of the historical queue, and a VM moved to index 0 after an error is selected first as soon as it becomes eligible instead of remaining deprioritized. Iterate the snapshot from the tail so candidate ordering matches the queue contract while still scanning every VM.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Previously a hypervisor drain migrated one VM at a time. This adds a configurable concurrency limit so multiple migrations can run in flight. - --eviction-concurrency sets the default max concurrent migrations (1 = serial, preserving prior behavior). - --eviction-trait-concurrency allows per-trait overrides (e.g. CUSTOM_HANA_EXCLUSIVE_HOST=1); the lowest matching limit wins. - evictNext scans the whole outstanding set each pass, counts in-flight migrations, and starts new ones only up to the limit; completed and gone instances are removed regardless of queue position. - Move the eviction controller into its own package and share poll/retry interval constants via internal/global. Signed-off-by: Andrew Karpow <andrew.karpow@sap.com>
notandy
force-pushed
the
eviction-parallelism
branch
from
August 12, 2026 18:22
abd5299 to
3f42690
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/controller/eviction/controller_test.go (1)
843-844: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the per-VM migration-count assertion.
Line 844 only checks that four VM IDs were called. A duplicate call to one VM still passes when all four IDs exist. Assert that every
migrateCallsvalue equals one.Proposed fix
Expect(resource.Status.OutstandingInstances).To(BeEmpty()) + for id, c := range migrateCalls { + Expect(c).To(Equal(1), "VM %s migrated exactly once", id) + } Expect(migrateCalls).To(HaveLen(4))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/eviction/controller_test.go` around lines 843 - 844, Update the assertions in the test around migrateCalls to verify each VM ID has exactly one migration call, not just that four IDs are present. Retain the existing length assertion if useful, and add per-entry count validation so duplicate calls to any VM fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/controller/eviction/controller_test.go`:
- Around line 843-844: Update the assertions in the test around migrateCalls to
verify each VM ID has exactly one migration call, not just that four IDs are
present. Retain the existing length assertion if useful, and add per-entry count
validation so duplicate calls to any VM fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72421bb6-ff78-48f3-a5fe-002e455e31f2
📒 Files selected for processing (2)
internal/controller/eviction/controller.gointernal/controller/eviction/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/eviction/controller.go
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously a hypervisor drain migrated VMs strictly one at a time. This adds a configurable concurrency limit so multiple migrations can run in flight, cutting drain time on hosts with many instances while keeping the safe serial behavior as the default.
--eviction-concurrencysets the default maximum concurrent migrations. Defaults to1, preserving the historical one-at-a-time behavior.--eviction-trait-concurrencytakesTRAIT=Noverrides (e.g.CUSTOM_HANA_EXCLUSIVE_HOST=1) so exclusive-host classes can be forced to drain serially. The lowest matching trait limit wins, resolved per host viaglobal.ResolveConcurrency.evictNextnow scans the entire outstanding set each pass: it counts in-flight (MIGRATING/RESIZE/terminating) instances, removes completed or gone ones regardless of their queue position, and starts fresh migrations only up to the limit.removeInstancereplaces the old tail-drop since completions are no longer ordered.internal/controller/evictionpackage, and the poll/retry requeue intervals are shared as constants ininternal/global.Summary by CodeRabbit
New Features
Bug Fixes