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 |
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
f2b7e0f to
9183e07
Compare
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
efcca20 to
92103cc
Compare
92103cc to
e6b06c8
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes eviction state transitions and introduces new Nova API-driven gating behavior that warrants a final human review for operational safety and correctness in real deployments.
Pull request overview
This PR improves the hypervisor maintenance/eviction flow by preventing a host from being marked “evicted” while there are still in-flight incoming migrations targeting it, and by restarting eviction if instances appear after an eviction CR reported success. This fits into the operator’s maintenance controller logic to make draining/eviction more correct under Nova live-migration timing behavior.
Changes:
- Added a Nova
/os-migrationsquery + client-side filtering to detect active incoming migrations and optionally abort abortable live-migrations. - Gated eviction on a new
IncomingMigrationsSettledHypervisor condition and requeued reconciliation while migrations are unsettled. - Added a terminal guard that restarts eviction if
NumInstances > 0even after eviction was reported succeeded, plus comprehensive unit tests.
File summaries
| File | Description |
|---|---|
| internal/openstack/migrations.go | Adds Nova migrations listing/abort logic and a “settle incoming migrations” helper. |
| internal/openstack/migrations_test.go | Adds unit tests for migration listing, abort handling, and settle behavior. |
| internal/controller/hypervisor_maintenance_controller.go | Gates eviction on incoming migration settlement; adds requeue; adds terminal guard for late instances. |
| internal/controller/hypervisor_maintenance_controller_test.go | Adds controller tests for IncomingMigrationsSettled condition behavior and eviction restart scenarios. |
| api/v1/eviction_types.go | Introduces the new condition type and reasons for incoming migration settlement reporting. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e6b06c8 to
56c49bc
Compare
5215024 to
af598a8
Compare
af598a8 to
60e54d5
Compare
60e54d5 to
a54e5c8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The controller can keep status.evicted=true while IncomingMigrationsSettled is false due to an early return that doesn’t clear the scalar, which undermines the intended “don’t declare evicted until settled” behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
a54e5c8 to
f559b8c
Compare
| requeue, err := hec.reconcileEviction(ctx, hv, statusCfg) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
When Nova is unreachable and settleIncomingMigrations returns an error, Reconcile returns before Status().Apply runs. The IncomingMigrationsSettled condition is never updated - operators see a stale condition with no indication that Nova is unreachable.
This is a good place to move out status changes from sub function to the reconcile loop to ensure the current state is correctly reflected.
| @@ -81,13 +87,21 @@ func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req c | |||
| return ctrl.Result{}, err | |||
There was a problem hiding this comment.
issues with reconcileComputeService won't be reflected in the Status.
f71f50c to
e06eb68
Compare
e06eb68 to
8d5a9b9
Compare
8d5a9b9 to
270653d
Compare
270653d to
877cbdb
Compare
During a live migration, Nova can still associate the instance with its source while it is already moving to the destination. Relying only on the destination's instance count can therefore declare a host evicted too early. Require incoming migrations to settle before completing eviction, and make an unsuccessful Nova check visible without allowing eviction to continue. Once persisted state conclusively shows that the host is empty and settled, avoid repeatedly verifying the same terminal state. Signed-off-by: Fabian Wiesel <fabian.wiesel@sap.com>
877cbdb to
329ee14
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
|
During a live migration, Nova can still associate an instance with its source while it is already moving to the destination. Relying only on the destination's instance count can therefore declare a host evicted too early.
After disabling the compute service, the maintenance controller checks for in-flight migrations targeting the host. Abortable live migrations are stopped, while post-migrating live migrations and evacuations are allowed to finish. Eviction proceeds only after no incoming migrations remain.
IncomingMigrationsSettledreports whether the host is settled, waiting, aborting, or could not be checked. Nova failures are persisted asUnknown/Failedand returned for retry; they never allow eviction to continue.If an Eviction reports success while instances remain, draining is restarted. Once persisted status conclusively shows that eviction succeeded, the host is empty, and incoming migrations are settled, later reconciliations avoid repeating the Nova migration query.
The migration query uses a six-hour
changes-sincewindow and unsettled migrations are rechecked every ten seconds.