Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c9db3d1
fix(vm): allow volume migration for restart-required VMs
danilrwx Jul 10, 2026
fef6b10
fix(vm): migrate attached volumes even with pending structural change
danilrwx Jul 10, 2026
b261277
chore(build): point 3p-kubevirt to volume-migration branch
danilrwx Jul 10, 2026
470d57d
chore(vd): TEMP debug log for volume migration revert decision
danilrwx Jul 10, 2026
c2df4a8
fix(vd): prevent volume migration revert loop under pending restart
danilrwx Jul 10, 2026
ccd0d6b
fix(vd): do not let stale migration state block a new volume migration
danilrwx Jul 10, 2026
f1a0b8f
fix(vd): prepare volume migration target only while awaiting disks
danilrwx Jul 10, 2026
1155df6
refactor(vm): extract restart-time volume migration into a method
danilrwx Jul 10, 2026
fbbd895
refactor(vm): return only error from migrateAttachedVolumesDelayingSt…
danilrwx Jul 10, 2026
6828e4f
refactor(vd): fetch migrating operation inside migratingIsWaitingForD…
danilrwx Jul 10, 2026
6b4f11e
style(vd): add blank lines in migratingIsWaitingForDisks
danilrwx Jul 10, 2026
a628fbb
refactor(vm): drop structural-change volume migration handling
danilrwx Jul 10, 2026
03c381d
fix(vd): restore source PVC when reverting a volume migration
danilrwx Jul 10, 2026
dd2ba8f
Revert "fix(vd): restore source PVC when reverting a volume migration"
danilrwx Jul 10, 2026
f250323
fix(vm): let volume migration revert proceed under pending restart
danilrwx Jul 10, 2026
a1d2295
test(vm): cover volume migration revert under pending restart
danilrwx Jul 10, 2026
52da264
test(vm): add table test for isStructuralVolumeChange
danilrwx Jul 10, 2026
bdc4493
test(e2e): migrate local RWO disks while a restart is pending
danilrwx Jul 10, 2026
38e23f4
refactor(e2e): align restart-pending migration test with conventions
danilrwx Jul 13, 2026
89acf42
test(vd): cover volume migration revert-loop guard in getAction
danilrwx Jul 13, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/components/versions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ firmware:
libvirt: v10.9.0
edk2: stable202411
core:
3p-kubevirt: v1.6.2-v12n.60
3p-kubevirt: v1.6.2-v12n.61
distribution: 3.1.1
package:
acl: v2.3.1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import (
"github.com/deckhouse/virtualization/api/core/v1alpha2"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vdcondition"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmopcondition"
)

const migrationHandlerName = "MigrationHandler"
Expand Down Expand Up @@ -192,6 +193,19 @@ func (h MigrationHandler) getAction(ctx context.Context, vd *v1alpha2.VirtualDis
disksShouldBeMigrating := vmMigratable.Reason == vmcondition.ReasonDisksShouldBeMigrating.String()

if disksShouldBeMigrating {
// Prepare disk targets only while the operation is still waiting for the
// disks to be ready to migrate. Once the compute migration has started or
// finished, a new prepare would race with the operation finalization: it
// overwrites the disk migration timestamp, breaks isMigrationsMatched on
// the next reconcile and gets reverted.
waiting, err := h.migratingIsWaitingForDisks(ctx, vm)
if err != nil {
return none, err
}
if !waiting {
log.Info("Operation is not waiting for disks to migrate, skip starting a new volume migration.")
return none, nil
}
return h.getActionIfDisksShouldBeMigrating(ctx, vd, log)
}
}
Expand Down Expand Up @@ -644,6 +658,25 @@ func (h MigrationHandler) getInProgressMigratingVMOP(ctx context.Context, vm *v1
return nil, nil
}

// migratingIsWaitingForDisks reports whether the in-progress migrating operation
// is in the phase where it waits for the disks to become ready to migrate. Only
// then a new disk target should be prepared; later phases mean the compute
// migration has already started or is being finalized.
func (h MigrationHandler) migratingIsWaitingForDisks(ctx context.Context, vm *v1alpha2.VirtualMachine) (bool, error) {
vmop, err := h.getInProgressMigratingVMOP(ctx, vm)
if err != nil {
return false, err
}

if vmop == nil {
return false, nil
}

completed, _ := conditions.GetCondition(vmopcondition.TypeCompleted, vmop.Status.Conditions)

return completed.Reason == vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate.String(), nil
}

func (h MigrationHandler) createTargetPersistentVolumeClaim(ctx context.Context, vd *v1alpha2.VirtualDisk, sc *storagev1.StorageClass, size resource.Quantity, targetPVCName, sourcePVCName string, volumeMode corev1.PersistentVolumeMode, accessMode corev1.PersistentVolumeAccessMode) (*corev1.PersistentVolumeClaim, error) {
pvcs, err := listPersistentVolumeClaims(ctx, vd, h.client)
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"github.com/deckhouse/virtualization/api/core/v1alpha2"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vdcondition"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmopcondition"
)

type fakeStorageClassValidator struct {
Expand Down Expand Up @@ -266,6 +267,85 @@ var _ = Describe("MigrationHandler", func() {
Expect(action).To(Equal(migratePrepareTarget))
})
})

Context("when disks should be migrating", func() {
createMigratingVMOP := func(completedReason vmopcondition.ReasonCompleted) {
vmop := &v1alpha2.VirtualMachineOperation{
ObjectMeta: metav1.ObjectMeta{Name: "test-vmop", Namespace: "default"},
Spec: v1alpha2.VirtualMachineOperationSpec{
Type: v1alpha2.VMOPTypeMigrate,
VirtualMachine: "test-vm",
},
Status: v1alpha2.VirtualMachineOperationStatus{
Phase: v1alpha2.VMOPPhaseInProgress,
Conditions: []metav1.Condition{
{
Type: vmopcondition.TypeCompleted.String(),
Status: metav1.ConditionFalse,
Reason: completedReason.String(),
},
},
},
}
Expect(fakeClient.Create(ctx, vmop)).To(Succeed())
}

BeforeEach(func() {
vd.Status.Conditions = []metav1.Condition{
{
Type: vdcondition.InUseType.String(),
Status: metav1.ConditionTrue,
Reason: vdcondition.AttachedToVirtualMachine.String(),
},
{
Type: vdcondition.ReadyType.String(),
Status: metav1.ConditionTrue,
Reason: vdcondition.Ready.String(),
},
}
vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{
{Name: "test-vm", Mounted: true},
}
// Keep the storage class unchanged so getAction takes the
// disks-should-be-migrating branch, not the storage-class-changed one.
vd.Spec.PersistentVolumeClaim.StorageClass = ptr.To("default-sc")
vd.Status.StorageClassName = "default-sc"

vm.Status.Conditions = []metav1.Condition{
{
Type: vmcondition.TypeMigrating.String(),
Status: metav1.ConditionTrue,
Reason: vmcondition.ReasonMigratingPending.String(),
},
{
Type: vmcondition.TypeMigratable.String(),
Status: metav1.ConditionTrue,
Reason: vmcondition.ReasonDisksShouldBeMigrating.String(),
},
}
Expect(fakeClient.Create(ctx, vm)).To(Succeed())
Expect(fakeClient.Create(ctx, pvc)).To(Succeed())
})

It("should prepare the target when the operation is waiting for disks", func() {
createMigratingVMOP(vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate)

action, err := migrationHandler.getAction(ctx, vd, log)
Expect(err).NotTo(HaveOccurred())
Expect(action).To(Equal(migratePrepareTarget))
})

It("should not start a new migration once the operation is past waiting for disks", func() {
// The compute migration has already started or is finalizing: starting a
// new disk migration here would overwrite the migration state and cause a
// revert loop.
createMigratingVMOP(vmopcondition.ReasonMigrationRunning)

action, err := migrationHandler.getAction(ctx, vd, log)
Expect(err).NotTo(HaveOccurred())
Expect(action).To(Equal(none))
})
})
})

Describe("handleMigrate", func() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,6 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.

vm := vmState.VirtualMachine().Changed()

// TODO: refactor syncKVVM and allow migration
if restartRequired {
log.Info("Virtualmachine is restart required, skip volume migration.")
return reconcile.Result{}, nil
}

// not syncing if migrating
migrating, _ := conditions.GetCondition(vmcondition.TypeMigrating, vm.Status.Conditions)
if migrating.Status == metav1.ConditionTrue {
Expand Down Expand Up @@ -155,6 +149,18 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.
}

if !equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) {
// A difference here (ignoring migration target PVCs, which live in
// builtKVVMWithMigrationVolumes) means the desired volume set differs from
// the running one. Only a structural change (a disk added or removed) may
// require a restart, so it must not be propagated to KVVM while the VM
// awaits restart. A difference that is only a PVC swap on the same disks is
// a volume migration or a revert of one: it keeps the structure intact and
// must proceed regardless of restart, otherwise a KVVM left pointing at a
// dead migration target can never be reverted back to the source.
if restartRequired && isStructuralVolumeChange(builtKVVM, kvvmiInCluster) {
log.Info("Virtualmachine is restart required, delay structural volume changes to KVVM.")
return reconcile.Result{}, nil
}
return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM)
}

Expand Down Expand Up @@ -218,6 +224,34 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.
return reconcile.Result{}, nil
}

// isStructuralVolumeChange reports whether the desired and running volume sets
// differ structurally, i.e. a volume was added or removed. A difference that is
// only a PersistentVolumeClaim swap on the same set of volume names is a volume
// migration or its revert, not a structural change.
func isStructuralVolumeChange(builtKVVM *virtv1.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) bool {
desired := make(map[string]struct{}, len(builtKVVM.Spec.Template.Spec.Volumes))
for _, v := range builtKVVM.Spec.Template.Spec.Volumes {
desired[v.Name] = struct{}{}
}

running := make(map[string]struct{}, len(kvvmi.Spec.Volumes))
for _, v := range kvvmi.Spec.Volumes {
running[v.Name] = struct{}{}
}

if len(desired) != len(running) {
return true
}

for name := range desired {
if _, ok := running[name]; !ok {
return true
}
}

return false
}

func getVolumesByName(vmiSpec *virtv1.VirtualMachineInstanceSpec) map[string]*virtv1.Volume {
volumes := map[string]*virtv1.Volume{}
for _, vol := range vmiSpec.Volumes {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,104 @@ var _ = Describe("MigrationVolumesService", func() {
return state.New(fakeClient, resource)
}

newKVVMIWithVolume := func(pvcName string) *virtv1.VirtualMachineInstance {
return &virtv1.VirtualMachineInstance{
TypeMeta: metav1.TypeMeta{
APIVersion: virtv1.GroupVersion.String(),
Kind: "VirtualMachineInstance",
},
ObjectMeta: metav1.ObjectMeta{
Name: vmName,
Namespace: namespace,
},
Spec: virtv1.VirtualMachineInstanceSpec{
Volumes: []virtv1.Volume{
{
Name: "rootdisk",
VolumeSource: virtv1.VolumeSource{
PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
},
},
},
},
}
}

appendVolume := func(kvvm *virtv1.VirtualMachine, name, pvcName string) *virtv1.VirtualMachine {
kvvm.Spec.Template.Spec.Volumes = append(kvvm.Spec.Template.Spec.Volumes, virtv1.Volume{
Name: name,
VolumeSource: virtv1.VolumeSource{
PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
},
})
return kvvm
}

It("does not apply structural volume changes to kvvm while restart is required", func() {
ctx := testutil.ContextBackgroundWithNoOpLogger()

vm := newVM()
kvvmInCluster := newKVVMWithVolume(sourcePVC, nil, "source-node")
kvvmi := newKVVMIWithVolume(sourcePVC)
// The desired spec adds a second disk: a structural change that may require
// a restart and must not be propagated to KVVM while the VM awaits restart.
desiredKVVM := appendVolume(newKVVMWithVolume(sourcePVC, nil, "source-node"), "extradisk", "disk-extra")
vmState := setupState(vm, kvvmInCluster, kvvmi)

service := NewMigrationVolumesService(
vmState.Client(),
func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) {
return desiredKVVM.DeepCopy(), nil
},
10*time.Second,
)

_, err := service.SyncVolumes(ctx, vmState, true)
Expect(err).NotTo(HaveOccurred())

updatedKVVM := &virtv1.VirtualMachine{}
Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed())
Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1))
Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC))
})

It("reverts a migration PVC swap to the source even while restart is required", func() {
ctx := testutil.ContextBackgroundWithNoOpLogger()

vm := newVM()
// KVVM/KVVMI are left pointing at a migration target PVC that must be
// reverted back to the source. It is not a structural change (same disk),
// so the revert must proceed despite the pending restart.
kvvmInCluster := newKVVMWithVolume(targetPVC, nil, "target-node")
kvvmi := newKVVMIWithVolume(targetPVC)
desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "source-node")
vmState := setupState(vm, kvvmInCluster, kvvmi)

service := NewMigrationVolumesService(
vmState.Client(),
func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) {
return desiredKVVM.DeepCopy(), nil
},
10*time.Second,
)

_, err := service.SyncVolumes(ctx, vmState, true)
Expect(err).NotTo(HaveOccurred())

updatedKVVM := &virtv1.VirtualMachine{}
Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed())
Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1))
Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC))
})

It("forces volume rollback when kvvmi is missing", func() {
ctx := testutil.ContextBackgroundWithNoOpLogger()
migrationStrategy := virtv1.UpdateVolumesStrategyMigration
Expand Down Expand Up @@ -153,3 +251,54 @@ var _ = Describe("MigrationVolumesService", func() {
Expect(updatedKVVM.Spec.Template.Spec.Affinity).To(Equal(desiredKVVM.Spec.Template.Spec.Affinity))
})
})

var _ = Describe("isStructuralVolumeChange", func() {
// volumes builds a volume list from name -> claim pairs; the claim only
// exists to prove that isStructuralVolumeChange ignores it and looks at names.
volumes := func(nameToClaim map[string]string) []virtv1.Volume {
vols := make([]virtv1.Volume, 0, len(nameToClaim))
for name, claim := range nameToClaim {
vols = append(vols, virtv1.Volume{
Name: name,
VolumeSource: virtv1.VolumeSource{
PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{
PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{
ClaimName: claim,
},
},
},
})
}
return vols
}

built := func(nameToClaim map[string]string) *virtv1.VirtualMachine {
return &virtv1.VirtualMachine{
Spec: virtv1.VirtualMachineSpec{
Template: &virtv1.VirtualMachineInstanceTemplateSpec{
Spec: virtv1.VirtualMachineInstanceSpec{Volumes: volumes(nameToClaim)},
},
},
}
}

running := func(nameToClaim map[string]string) *virtv1.VirtualMachineInstance {
return &virtv1.VirtualMachineInstance{
Spec: virtv1.VirtualMachineInstanceSpec{Volumes: volumes(nameToClaim)},
}
}

DescribeTable("distinguishes structural changes from PVC swaps",
func(desired, current map[string]string, expected bool) {
Expect(isStructuralVolumeChange(built(desired), running(current))).To(Equal(expected))
},
Entry("identical single disk", map[string]string{"root": "a"}, map[string]string{"root": "a"}, false),
Entry("PVC swap on the same disk (migration/revert)", map[string]string{"root": "src"}, map[string]string{"root": "tgt"}, false),
Entry("PVC swap on some of many disks", map[string]string{"root": "a", "data": "new"}, map[string]string{"root": "a", "data": "old"}, false),
Entry("reordered volumes", map[string]string{"a": "1", "b": "2"}, map[string]string{"b": "2", "a": "1"}, false),
Entry("both empty", map[string]string{}, map[string]string{}, false),
Entry("disk added", map[string]string{"root": "a", "extra": "b"}, map[string]string{"root": "a"}, true),
Entry("disk removed", map[string]string{"root": "a"}, map[string]string{"root": "a", "extra": "b"}, true),
Entry("disk renamed (same count, different name)", map[string]string{"root": "a"}, map[string]string{"data": "a"}, true),
)
})
Loading
Loading