Settle incoming migrations before declaring a hypervisor evicted#347
Open
fwiesel wants to merge 4 commits into
Open
Settle incoming migrations before declaring a hypervisor evicted#347fwiesel wants to merge 4 commits into
fwiesel wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIncoming migration discovery and settlement now gate hypervisor eviction. The controller records settlement conditions, requeues while migrations remain, uses SSA for status and Eviction resources, and re-enters draining when eviction completes while instances remain. ChangesIncoming migration eviction flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HypervisorMaintenanceController
participant SettleIncomingMigrations
participant NovaAPI
participant EvictionAPI
HypervisorMaintenanceController->>SettleIncomingMigrations: Settle incoming migrations
SettleIncomingMigrations->>NovaAPI: List active incoming migrations
NovaAPI-->>SettleIncomingMigrations: Migration states
SettleIncomingMigrations->>NovaAPI: Abort eligible live migrations
SettleIncomingMigrations-->>HypervisorMaintenanceController: Settlement status
HypervisorMaintenanceController->>EvictionAPI: Create or apply Eviction
EvictionAPI-->>HypervisorMaintenanceController: Eviction status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Contributor
Author
fwiesel
force-pushed
the
settle-incoming-migrations
branch
from
July 15, 2026 09:59
19e9855 to
ce12efe
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/hypervisor_maintenance_controller.go (1)
183-191: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlways clear the migration-settlement condition when maintenance ends.
If eviction never started because migrations were unsettled,
Evictedis false and noEvictingcondition exists, so this branch leavesIncomingMigrationsSettled=Falsestale after maintenance is cancelled.Proposed fix
case kvmv1.MaintenanceUnset: - if hv.Status.Evicted || meta.RemoveStatusCondition(&hv.Status.Conditions, kvmv1.ConditionTypeEvicting) { + stateChanged := hv.Status.Evicted + stateChanged = meta.RemoveStatusCondition( + &hv.Status.Conditions, kvmv1.ConditionTypeEvicting, + ) || stateChanged + stateChanged = meta.RemoveStatusCondition( + &hv.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled, + ) || stateChanged + if stateChanged { err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)) hv.Status.Evicted = false - meta.RemoveStatusCondition(&hv.Status.Conditions, kvmv1.ConditionTypeIncomingMigrationsSettled) return ctrl.Result{}, err }🤖 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/hypervisor_maintenance_controller.go` around lines 183 - 191, Update the MaintenanceUnset branch in the maintenance reconciliation logic to always remove the ConditionTypeIncomingMigrationsSettled condition when maintenance ends, even when Evicted is false and no Evicting condition exists. Keep eviction deletion conditional as currently implemented, but move the migration-settlement cleanup outside that condition before returning.
🤖 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 `@applyconfigurations/api/v1/hypervisorspec.go`:
- Around line 16-18: Update the source comments for HypervisorSpec fields Reboot
and EvacuateOnReboot to use “requests a reboot” and “requests an evacuation”
wording, then regenerate the derived applyconfigurations HypervisorSpec output
and CRD YAML so all generated documentation matches.
In `@internal/controller/hypervisor_maintenance_controller.go`:
- Around line 237-243: The status-false branch handling remaining instances must
restart eviction instead of leaving the completed Eviction CR in place. In the
hv status reconciliation logic, delete the finished eviction resource, set the
Hypervisor Evicting condition to True with ConditionReasonRunning, and requeue
reconciliation; add regression coverage for an absent or already-Running
condition rather than a pre-seeded Succeeded condition.
In `@internal/openstack/migrations.go`:
- Around line 144-152: The abort branch in SettleIncomingMigrations must only
process migrations whose MigrationType is "live-migration". Update the condition
around AbortMigration so running evacuations remain in waiting without issuing a
DELETE, and add a test covering that behavior.
---
Outside diff comments:
In `@internal/controller/hypervisor_maintenance_controller.go`:
- Around line 183-191: Update the MaintenanceUnset branch in the maintenance
reconciliation logic to always remove the ConditionTypeIncomingMigrationsSettled
condition when maintenance ends, even when Evicted is false and no Evicting
condition exists. Keep eviction deletion conditional as currently implemented,
but move the migration-settlement cleanup outside that condition before
returning.
🪄 Autofix (Beta)
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
Run ID: d6ac979f-7f1c-4d7c-846a-1a65c98e733b
📒 Files selected for processing (24)
api/v1/eviction_types.goapplyconfigurations/api/v1/aggregate.goapplyconfigurations/api/v1/aggregategroup.goapplyconfigurations/api/v1/capabilities.goapplyconfigurations/api/v1/cell.goapplyconfigurations/api/v1/domaincapabilities.goapplyconfigurations/api/v1/eviction.goapplyconfigurations/api/v1/evictionspec.goapplyconfigurations/api/v1/evictionstatus.goapplyconfigurations/api/v1/group.goapplyconfigurations/api/v1/hypervisor.goapplyconfigurations/api/v1/hypervisorspec.goapplyconfigurations/api/v1/hypervisorstatus.goapplyconfigurations/api/v1/hypervisorupdatestatus.goapplyconfigurations/api/v1/instance.goapplyconfigurations/api/v1/operatingsystemstatus.goapplyconfigurations/api/v1/traitgroup.goapplyconfigurations/internal/internal.gocharts/openstack-hypervisor-operator/crds/kvm.cloud.sap_evictions.yamlcharts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yamlinternal/controller/hypervisor_maintenance_controller.gointernal/controller/hypervisor_maintenance_controller_test.gointernal/openstack/migrations.gointernal/openstack/migrations_test.go
fwiesel
force-pushed
the
settle-incoming-migrations
branch
3 times, most recently
from
July 15, 2026 10:24
f2b7e0f to
9183e07
Compare
fwiesel
force-pushed
the
settle-incoming-migrations
branch
from
July 15, 2026 11:02
9183e07 to
090ba86
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/hypervisor_maintenance_controller_test.go (1)
586-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the 10-second requeue contract.
These scenarios verify status and resource state but discard
ctrl.Result. IfRequeueAfterregresses, no Eviction exists to generate another event and the host can remain blocked indefinitely. Capture the reconcile result and assertRequeueAfter == settleRequeueIntervalfor both aborting and waiting cases.Also applies to: 624-657
🤖 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/hypervisor_maintenance_controller_test.go` around lines 586 - 621, The running incoming migration scenarios currently discard the reconcile result, so they do not verify the required retry interval. Update the relevant test cases around the incoming migration aborting and waiting contexts to capture ctrl.Result and assert that RequeueAfter equals settleRequeueInterval, including both cases identified by the existing status/resource assertions.
🤖 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/hypervisor_maintenance_controller.go`:
- Around line 286-298: Update the status condition message in the waiting branch
of the maintenance controller to describe all non-abortable incoming migrations,
including evacuations and other states. Replace the post-migrating-specific
wording in the WithMessage call while preserving the existing count and
condition behavior.
- Around line 186-199: At the start of the active-maintenance branch covering
MaintenanceManual, MaintenanceAuto, and MaintenanceTermination, seed the
existing IncomingMigrationsSettled and Evicting status conditions into statusCfg
before any mutation. Remove the later duplicate seeding in the
unsettled-migration and eviction paths, preserving existing condition timestamps
and avoiding duplicate entries across reconciles.
- Around line 81-84: Update the status construction around HypervisorStatus and
ConditionTypeHypervisorDisabled so this controller does not copy or apply
HypervisorDisabled when HA mode is enabled, avoiding ownership conflicts with
kvm-ha-service. Preserve the existing condition propagation for non-HA mode and
continue applying the evicted status.
---
Nitpick comments:
In `@internal/controller/hypervisor_maintenance_controller_test.go`:
- Around line 586-621: The running incoming migration scenarios currently
discard the reconcile result, so they do not verify the required retry interval.
Update the relevant test cases around the incoming migration aborting and
waiting contexts to capture ctrl.Result and assert that RequeueAfter equals
settleRequeueInterval, including both cases identified by the existing
status/resource assertions.
🪄 Autofix (Beta)
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
Run ID: 2018bcdb-b826-4889-aa0b-0de9caed9776
📒 Files selected for processing (7)
api/v1/eviction_types.gointernal/controller/hypervisor_maintenance_controller.gointernal/controller/hypervisor_maintenance_controller_test.gointernal/openstack/migrations.gointernal/openstack/migrations_test.gointernal/utils/conditions.gointernal/utils/conditions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1/eviction_types.go
fwiesel
force-pushed
the
settle-incoming-migrations
branch
2 times, most recently
from
July 16, 2026 10:12
9caf5f8 to
efcca20
Compare
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.
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.
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).
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.
fwiesel
force-pushed
the
settle-incoming-migrations
branch
from
July 16, 2026 10:24
efcca20 to
92103cc
Compare
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
1 participant
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.
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.Summary by CodeRabbit
New Features
Bug Fixes