Skip to content

Fix offline migration with a hotplugged volume #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
awels wants to merge 1 commit into
base: main
Choose a base branch
from
+151 −6
Open

Fix offline migration with a hotplugged volume #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 14 additions & 6 deletions internal/controller/storagemig/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,11 @@ func (t *Task) prepareVMForStorageMigration(vm *virtv1.VirtualMachine, planVM mi

vmCopy := vm.DeepCopy()

// Track which volumes are present in the DataVolume templates and being migrated
volumesInTemplate := make(map[string]bool)
Copy link
Copy Markdown
Contributor

@akalenyu akalenyu Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just me or we don't usually use the "template" term when reasoning about vmis? so variable names/comments here are bit confusing

Copy link
Copy Markdown
Member Author

@awels awels Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a VM not a VMI, so not sure what you mean, but I reworded it a little bit to make sure we mention DataVolumeTemplates

for i, volume := range vm.Spec.Template.Spec.Volumes {
if planPVC, ok := planTargetPVCMap[volume.Name]; ok {
volumesInTemplate[volume.Name] = true
if volume.PersistentVolumeClaim != nil {
vmCopy.Spec.Template.Spec.Volumes[i].PersistentVolumeClaim.ClaimName = *planPVC.DestinationPVC.Name
}
Expand All @@ -252,15 +255,20 @@ func (t *Task) prepareVMForStorageMigration(vm *virtv1.VirtualMachine, planVM mi
for _, pvc := range planVM.SourcePVCs {
planPVCNameMap[pvc.Name] = pvc.VolumeName
}
// Only update DataVolumeTemplates if the corresponding volume is in the template.
// Hotplugged volumes will not be defined in the DataVolume templates section.
for i, dvTemplate := range vmCopy.Spec.DataVolumeTemplates {
if _, ok := planPVCNameMap[dvTemplate.Name]; ok {
volumeName := planPVCNameMap[dvTemplate.Name]
if targetPVC, ok := planTargetPVCMap[volumeName]; ok {
vmCopy.Spec.DataVolumeTemplates[i].Name = *targetPVC.DestinationPVC.Name
if vmCopy.Spec.DataVolumeTemplates[i].Spec.Storage != nil {
vmCopy.Spec.DataVolumeTemplates[i].Spec.Storage.StorageClassName = targetPVC.DestinationPVC.StorageClassName
} else if vmCopy.Spec.DataVolumeTemplates[i].Spec.PVC != nil {
vmCopy.Spec.DataVolumeTemplates[i].Spec.PVC.StorageClassName = targetPVC.DestinationPVC.StorageClassName
// Only update if this volume is actually in the datavolume template
if volumesInTemplate[volumeName] {
if targetPVC, ok := planTargetPVCMap[volumeName]; ok {
vmCopy.Spec.DataVolumeTemplates[i].Name = *targetPVC.DestinationPVC.Name
if vmCopy.Spec.DataVolumeTemplates[i].Spec.Storage != nil {
vmCopy.Spec.DataVolumeTemplates[i].Spec.Storage.StorageClassName = targetPVC.DestinationPVC.StorageClassName
} else if vmCopy.Spec.DataVolumeTemplates[i].Spec.PVC != nil {
vmCopy.Spec.DataVolumeTemplates[i].Spec.PVC.StorageClassName = targetPVC.DestinationPVC.StorageClassName
}
}
}
}
Expand Down
137 changes: 137 additions & 0 deletions internal/controller/storagemig/vm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,143 @@ var _ = Describe("StorageMigration VM", func() {
Expect(t.updateVMForOfflineStorageMigration(ctx, nil, migrations.VirtualMachineStorageMigrationPlanStatusVirtualMachine{})).To(MatchError("vm is nil or empty"))
Expect(t.updateVMForOfflineStorageMigration(ctx, &virtv1.VirtualMachine{}, migrations.VirtualMachineStorageMigrationPlanStatusVirtualMachine{})).To(MatchError("vm is nil or empty"))
})

DescribeTable("should handle offline migration with hotplugged volumes and DataVolumeTemplates",
func(regularDVTargetName, hotplugDVTargetName string) {
// Scenario: VM has a regular disk with DVTemplate + hotplugged disk without DVTemplate
// Both are being migrated. The hotplugged volume is in spec.template.spec.volumes
// with hotpluggable:true, but has NO DataVolumeTemplate entry.
vm := &virtv1.VirtualMachine{
ObjectMeta: metav1.ObjectMeta{
Name: testVM,
Namespace: testNamespace,
},
Spec: virtv1.VirtualMachineSpec{
DataVolumeTemplates: []virtv1.DataVolumeTemplateSpec{
{
ObjectMeta: metav1.ObjectMeta{
Name: "regular-dv",
},
Spec: cdiv1.DataVolumeSpec{
Storage: &cdiv1.StorageSpec{
StorageClassName: ptr.To("old-sc"),
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceStorage: resource.MustParse("1Gi"),
},
},
},
},
},
// No DVTemplate for hotplugged volume!
},
Template: &virtv1.VirtualMachineInstanceTemplateSpec{
Spec: virtv1.VirtualMachineInstanceSpec{
Volumes: []virtv1.Volume{
{
Name: "disk0",
VolumeSource: virtv1.VolumeSource{
DataVolume: &virtv1.DataVolumeSource{
Name: "regular-dv",
},
},
},
{
Name: "hotplug-disk",
VolumeSource: virtv1.VolumeSource{
DataVolume: &virtv1.DataVolumeSource{
Name: "hotplug-dv",
Hotpluggable: true, // Hotplugged!
},
},
},
},
},
},
},
}
Expect(k8sClient.Create(ctx, vm)).To(Succeed())

t := &Task{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
}

// Migration plan includes BOTH volumes with target names already populated
// (as the plan controller would have done, either from explicit names or generated from suffix)
planVM := migrations.VirtualMachineStorageMigrationPlanStatusVirtualMachine{
SourcePVCs: []migrations.VirtualMachineStorageMigrationPlanSourcePVC{
{
VolumeName: "disk0",
Name: "regular-dv",
Namespace: testNamespace,
SourcePVC: corev1.PersistentVolumeClaim{},
},
{
VolumeName: "hotplug-disk",
Name: "hotplug-dv",
Namespace: testNamespace,
SourcePVC: corev1.PersistentVolumeClaim{},
},
},
VirtualMachineStorageMigrationPlanVirtualMachine: migrations.VirtualMachineStorageMigrationPlanVirtualMachine{
Name: testVM,
TargetMigrationPVCs: []migrations.VirtualMachineStorageMigrationPlanTargetMigrationPVC{
{
VolumeName: "disk0",
DestinationPVC: migrations.VirtualMachineStorageMigrationPlanDestinationPVC{
Name: ptr.To(regularDVTargetName),
StorageClassName: ptr.To("new-sc"),
},
},
{
VolumeName: "hotplug-disk",
DestinationPVC: migrations.VirtualMachineStorageMigrationPlanDestinationPVC{
Name: ptr.To(hotplugDVTargetName),
StorageClassName: ptr.To("new-sc"),
},
},
},
},
}

// This should succeed without triggering the KubeVirt webhook error
Expect(t.updateVMForOfflineStorageMigration(ctx, vm, planVM)).To(Succeed())

updatedVM := &virtv1.VirtualMachine{}
Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: testVM}, updatedVM)).To(Succeed())

// Verify volumes are updated
Expect(updatedVM.Spec.Template.Spec.Volumes).To(HaveLen(2))

// Regular volume should point to new DV
Expect(updatedVM.Spec.Template.Spec.Volumes[0].Name).To(Equal("disk0"))
Expect(updatedVM.Spec.Template.Spec.Volumes[0].VolumeSource.DataVolume.Name).To(Equal(regularDVTargetName))

// Hotplugged volume should point to new DV and preserve hotpluggable flag
Expect(updatedVM.Spec.Template.Spec.Volumes[1].Name).To(Equal("hotplug-disk"))
Expect(updatedVM.Spec.Template.Spec.Volumes[1].VolumeSource.DataVolume.Name).To(Equal(hotplugDVTargetName))
Expect(updatedVM.Spec.Template.Spec.Volumes[1].VolumeSource.DataVolume.Hotpluggable).To(BeTrue())

// CRITICAL: Verify DVTemplate behavior
// Only the regular volume's DVTemplate should be updated
// The hotplugged volume never had a DVTemplate, so there's nothing to update
Expect(updatedVM.Spec.DataVolumeTemplates).To(HaveLen(1))
Expect(updatedVM.Spec.DataVolumeTemplates[0].Name).To(Equal(regularDVTargetName))
Expect(updatedVM.Spec.DataVolumeTemplates[0].Spec.Storage.StorageClassName).To(Equal(ptr.To("new-sc")))

// Verify UpdateVolumesStrategy is NOT set for offline migration
Expect(updatedVM.Spec.UpdateVolumesStrategy).To(BeNil())
},
Entry("both volumes have explicit target names",
"regular-dv-new", "hotplug-dv-new"),
Entry("regular volume has explicit name, hotplug uses generated suffix name",
"regular-dv-new", "hotplug-dv-mig-abcd"),
Entry("regular volume uses generated suffix name, hotplug has explicit name",
"regular-dv-mig-xyz9", "hotplug-dv-new"),
Entry("both volumes use generated suffix names",
"regular-dv-mig-1234", "hotplug-dv-mig-1234"),
)
})

var _ = Describe("StorageMigration offline completion and getPlanVMByName", func() {
Expand Down