fix(vm): allow volume migration while restart is pending#2634
Open
danilrwx wants to merge 18 commits into
Open
fix(vm): allow volume migration while restart is pending#2634danilrwx wants to merge 18 commits into
danilrwx wants to merge 18 commits into
Conversation
danilrwx
requested review from
nevermarine and
universal-itengineer
as code owners
Volume migration for local (RWO) disks was fully skipped whenever the VM had the AwaitingRestartToApplyConfiguration condition. This created a deadlock: a spurious RestartRequired (e.g. memory defaulting drift after an upgrade) blocked volume migration, so target PVCs never synced, the migration stayed Pending forever and firmware/workload-update VMOPs hung. RestartRequired and volume migration are orthogonal: patchVolumes only touches volumes/affinity/updateVolumesStrategy and never applies non-live-updatable domain fields. Narrow the early skip to the single branch that would propagate a structural volume change (added/removed disk, which may require a restart) to KVVM, so genuine volume migration proceeds while awaiting a restart. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
The previous fix let volume migration proceed under a pending restart, but still skipped it entirely when the desired volume set also differed structurally from the running one (a disk added or removed). That meant a node drain could be blocked by an unrelated pending disk change on a VM. Keep delaying the structural change until the restart, but no longer block the migration of already-attached disks: rebuild the volume patch from the running KVVM structure and only redirect migrating disks to their target PVCs. A structural change without an active migration still waits for the restart, unchanged. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Temporary: build 3p-kubevirt from the fix/volume-migration-restart-required branch to test the volume-migration RestartRequired fix end-to-end. Revert to a tagged version before merge. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
danilrwx
force-pushed
the
fix/volume-migration-restart-required
branch
from
83a52e0 to
49cb516
Compare
Temporary diagnostic log in getActionIfMigrationInProgress to capture why a local volume migration reverts under KubeVirt-side RestartRequired. Revert this commit before merge. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Under a KubeVirt-side RestartRequired, a local (RWO) disk migration could enter a revert loop: right after the compute migration of a cycle succeeded and the disk migration completed, a new volume migration was started because the RWO disk still reports DisksShouldBeMigrating while the VMOP was being finalized. The new prepare overwrote the disk migration start timestamp, so isMigrationsMatched no longer held on the next reconcile, the migration was reverted, and the cleared target made the disk eligible for migration again. Fix it in two places: - getAction: do not start a new volume migration while the current cycle's compute migration is finalizing (its end timestamp is set), so the restart-induced race window no longer triggers a re-prepare. - getActionIfMigrationInProgress: if the compute migration already succeeded, finalize the disk migration instead of blindly reverting, so a concurrent re-prepare cannot drop the already-migrated target. Also removes the temporary diagnostic log added to investigate this. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
The previous guard skipped starting a volume migration whenever vm.Status.MigrationState had an end timestamp. That state is not cleared after a migration finishes, so any subsequent migration (e.g. a new Evict operation) was skipped forever and stuck in WaitingForVirtualMachineToBeReadyToMigrate. Compare the compute migration end time against the current operation's creation time: only skip the re-prepare when the migration that finished belongs to the operation still being finalized, not a stale one from a previous migration. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Replace the timestamp-based guard against the volume migration revert loop with a phase check. A local (RWO) disk always reports DisksShouldBeMigrating, so getAction would re-prepare a target right after a migration completed (while the operation was being finalized) — overwriting the disk migration timestamp, breaking isMigrationsMatched and reverting the migration in a loop under a pending restart. Now a new target is prepared only while the operation is in the WaitingForVirtualMachineToBeReadyToMigrate phase, i.e. while it actually waits for disks. Once the compute migration has started or finished, no new prepare is triggered, so the loop cannot form. This also drops the earlier timestamp comparison and the complete-vs-revert safety net, which relied on a stale MigrationState and were the ugly part of the previous fix. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Move the structural-change-under-restart handling out of the large SyncVolumes body into migrateAttachedVolumesDelayingStructural, and inline the single-use rebase helpers into it. Same behavior: an in-progress volume migration of the attached disks proceeds while a pending structural change waits for the restart. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
…ructural The method always returned an empty reconcile.Result (unparam). Return just error and build the Result at the call site. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
…isks Move the in-progress migrating VMOP lookup into migratingIsWaitingForDisks so getAction no longer threads the operation through. Same behavior. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Remove migrateAttachedVolumesDelayingStructural. It only served the rare non-virtio edge case where a structural disk change (add/remove) coincides with an in-progress migration. Now a structural change under a pending restart simply waits for the restart; genuine volume migration (unchanged structure) still proceeds through the branches below. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
handleRevert deleted the target PVC and marked the migration failed but never pointed the disk back at its source PVC (unlike the revert branches in handleComplete). The disk was left with a dangling/empty target, so the VM got stuck: no valid target and no new migration could start. Set the active PVC back to the source on revert so the disk recovers to a consistent state. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
The revert did not resolve the stuck state: vd.Status.Target stayed empty even though handleRevert set it back to the source PVC. The real cause is elsewhere in the Target lifecycle; reverting to keep history honest while investigating. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
A restart-required VM whose KVVM was left pointing at a dead migration target PVC (a previous volume migration that failed and was reverted on the disk side but not on the KVVM) could never recover: the restart-required guard in SyncVolumes delayed *any* volume-set difference, including the PVC swap needed to revert KVVM back to the source. Every later migration then hung in Pending forever. Only a structural change (a disk added or removed) can require a restart. A difference that is just a PVC swap on the same set of disks is a volume migration or its revert and must proceed regardless of restart. Gate the delay on isStructuralVolumeChange so reverts and migrations are no longer blocked. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Update the structural-change test to model a real structural change (a disk added) instead of a same-disk PVC swap, and add a case asserting that a PVC swap (a migration revert) is applied even while a restart is pending. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Cover the helper directly: added/removed/renamed disks are structural, while a PVC swap on the same set of volume names (a migration or its revert) is not. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
Add an e2e case to RWOVirtualDiskMigration that makes the VM restart-required via a delayed configuration change and then migrates it. Guards the regression where volume migration of local disks hung whenever the VM had a pending restart, and asserts the delayed change is neither lost nor applied without a restart. Signed-off-by: Daniil Antoshin <daniil.antoshin@flant.com>
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.
Description
Virtual machines with local (RWO) disks could get stuck unable to migrate whenever they had a pending restart. The "restart required to apply configuration" state can appear on its own after a module upgrade, without any user change; while it was set, volume migration of the VM's local disks was skipped entirely. Migrations initiated by evacuation (node drain), firmware updates and other workload updates then hung indefinitely — target disks were never prepared and the migration stayed Pending.
The same over-broad skip also blocked recovery: if a volume migration was interrupted, the VM could be left running while its disk still pointed at the discarded target, and it could never migrate again.
The pending-restart state and volume migration are unrelated: volume migration only moves a disk between PVCs and never applies the changes that actually require a restart. The skip is now narrowed so that only a genuine structural disk change (a disk added or removed, which may need a restart) waits for the restart, while a real volume migration — and the roll-back of an interrupted one — proceeds regardless of a pending restart.
Why do we need it, and what problem does it solve?
Local-disk VMs could end up in a state where a node cannot be drained and firmware/workload updates never finish, with no user-visible reason — the VM just reports it needs a restart and its migration hangs forever. An interrupted migration could leave the VM permanently unable to migrate. Both dead-ends are removed here, so evacuation, updates and repeated migrations work for local-disk VMs without a manual restart.
What is the expected result?
For a running VM with local (RWO) disks that has the "restart required to apply configuration" condition:
Evictoperation). The volume migration completes instead of staying Pending.Checklist
Changelog entries