From d09ff8f1d66fc0e98cabc8ef9e1a58597082ad32 Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 11:38:38 +0300 Subject: [PATCH 1/6] e2e,storage migration,UpdateVMWithPVC: Replace client update with patch Patch enable mutating objects in precise manner, changing spesific fields, making the change more predictive, and reducing potential for confilcts because it doesnt change the resource-verison. Signed-off-by: Or Mergi --- tests/storage/migration.go | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index 3db50dfcfcee..202c910397ec 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -34,12 +34,14 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" virtv1 "kubevirt.io/api/core/v1" "kubevirt.io/client-go/kubecli" cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" + "kubevirt.io/kubevirt/pkg/apimachinery/patch" "kubevirt.io/kubevirt/pkg/controller" "kubevirt.io/kubevirt/pkg/libvmi" "kubevirt.io/kubevirt/pkg/pointer" @@ -207,28 +209,33 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { var replacedIndex int vm, err := virtClient.VirtualMachine(ns).Get(context.Background(), vmName, metav1.GetOptions{}) Expect(err).ShouldNot(HaveOccurred()) - // Remove datavolume templates - vm.Spec.DataVolumeTemplates = []virtv1.DataVolumeTemplateSpec{} // Replace dst pvc for i, v := range vm.Spec.Template.Spec.Volumes { if v.Name == volName { By(fmt.Sprintf("Replacing volume %s with PVC %s", volName, claim)) - vm.Spec.Template.Spec.Volumes[i].VolumeSource.PersistentVolumeClaim = &virtv1.PersistentVolumeClaimVolumeSource{ - PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ - ClaimName: claim, - }, - } - vm.Spec.Template.Spec.Volumes[i].VolumeSource.DataVolume = nil replacedIndex = i break } } - vm.Spec.UpdateVolumesStrategy = pointer.P(virtv1.UpdateVolumesStrategyMigration) - vm, err = virtClient.VirtualMachine(ns).Update(context.Background(), vm, metav1.UpdateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + + updatedVolume := virtv1.Volume{ + Name: volName, + VolumeSource: virtv1.VolumeSource{PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: k8sv1.PersistentVolumeClaimVolumeSource{ + ClaimName: claim, + }}}} + + p, err := patch.New( + patch.WithReplace("/spec/dataVolumeTemplates", []virtv1.DataVolumeTemplateSpec{}), + patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", replacedIndex), updatedVolume), + patch.WithReplace("/spec/updateVolumesStrategy", virtv1.UpdateVolumesStrategyMigration), + ).GeneratePayload() + Expect(err).ToNot(HaveOccurred()) + vm, err = virtClient.VirtualMachine(vm.Namespace).Patch(context.Background(), vm.Name, types.JSONPatchType, p, metav1.PatchOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(vm.Spec.Template.Spec.Volumes[replacedIndex].VolumeSource.PersistentVolumeClaim. PersistentVolumeClaimVolumeSource.ClaimName).To(Equal(claim)) - } // TODO: right now, for simplicity, this function assumes the DV in the first position in the datavolumes templata list. Otherwise, we need // to pass the old name of the DV to be replaces. From dc43b3421cda5e65af93dcd18fa8282678404537 Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 16:18:25 +0300 Subject: [PATCH 2/6] e2e,migration-support,updateVMWithPVC: Pass VM object Signed-off-by: Or Mergi --- tests/storage/migration.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index 202c910397ec..0bb48c4cc5ac 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -205,10 +205,8 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { return vm } - updateVMWithPVC := func(vmName, volName, claim string) { + updateVMWithPVC := func(vm *virtv1.VirtualMachine, volName, claim string) { var replacedIndex int - vm, err := virtClient.VirtualMachine(ns).Get(context.Background(), vmName, metav1.GetOptions{}) - Expect(err).ShouldNot(HaveOccurred()) // Replace dst pvc for i, v := range vm.Spec.Template.Spec.Volumes { if v.Name == volName { @@ -278,7 +276,7 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { Fail("Unrecognized mode") } By("Update volumes") - updateVMWithPVC(vm.Name, volName, destPVC) + updateVMWithPVC(vm, volName, destPVC) Eventually(func() bool { vmi, err := virtClient.VirtualMachineInstance(ns).Get(context.Background(), vm.Name, metav1.GetOptions{}) @@ -329,7 +327,7 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { libwait.WaitForSuccessfulVMIStart(vmi) By("Update volumes") - updateVMWithPVC(vm.Name, volName, destPVC) + updateVMWithPVC(vm, volName, destPVC) Eventually(func() bool { vmi, err := virtClient.VirtualMachineInstance(ns).Get(context.Background(), vm.Name, metav1.GetOptions{}) @@ -353,11 +351,11 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { // Create dest PVC createUnschedulablePVC(destPVC, ns, size) By("Update volumes") - updateVMWithPVC(vm.Name, volName, destPVC) + updateVMWithPVC(vm, volName, destPVC) waitMigrationToExist(vm.Name, ns) waitVMIToHaveVolumeChangeCond(vm.Name, ns) By("Cancel the volume migration") - updateVMWithPVC(vm.Name, volName, dv.Name) + updateVMWithPVC(vm, volName, dv.Name) // After the volume migration abortion the VMI should have: // 1. the source volume restored // 2. condition VolumesChange set to false @@ -384,7 +382,7 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { vm := createVMWithDV(createDV(), volName) createSmallImageForDestinationMigration(vm, destPVC, size) By("Update volume") - updateVMWithPVC(vm.Name, volName, destPVC) + updateVMWithPVC(vm, volName, destPVC) // let the workload updater creates some migration time.Sleep(2 * time.Minute) ls := labels.Set{virtv1.VolumesUpdateMigration: vm.Name} From 438fd46040a5003fe182c5f11636457a2975a4f7 Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 15:56:39 +0300 Subject: [PATCH 3/6] e2e,storage migration,UpdateVMWithPVC: Fetch volume name using slices Change signature to get VM object instead of name to save additional Get operation. Signed-off-by: Or Mergi --- tests/storage/migration.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index 0bb48c4cc5ac..684f5cf28228 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -22,6 +22,7 @@ package storage import ( "context" "fmt" + "slices" "strconv" "time" @@ -206,15 +207,12 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { } updateVMWithPVC := func(vm *virtv1.VirtualMachine, volName, claim string) { - var replacedIndex int // Replace dst pvc - for i, v := range vm.Spec.Template.Spec.Volumes { - if v.Name == volName { - By(fmt.Sprintf("Replacing volume %s with PVC %s", volName, claim)) - replacedIndex = i - break - } - } + i := slices.IndexFunc(vm.Spec.Template.Spec.Volumes, func(volume virtv1.Volume) bool { + return volume.Name == volName + }) + Expect(i).To(BeNumerically(">", -1)) + By(fmt.Sprintf("Replacing volume %s with PVC %s", volName, claim)) updatedVolume := virtv1.Volume{ Name: volName, @@ -225,14 +223,14 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { p, err := patch.New( patch.WithReplace("/spec/dataVolumeTemplates", []virtv1.DataVolumeTemplateSpec{}), - patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", replacedIndex), updatedVolume), + patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", i), updatedVolume), patch.WithReplace("/spec/updateVolumesStrategy", virtv1.UpdateVolumesStrategyMigration), ).GeneratePayload() Expect(err).ToNot(HaveOccurred()) vm, err = virtClient.VirtualMachine(vm.Namespace).Patch(context.Background(), vm.Name, types.JSONPatchType, p, metav1.PatchOptions{}) Expect(err).ToNot(HaveOccurred()) - Expect(vm.Spec.Template.Spec.Volumes[replacedIndex].VolumeSource.PersistentVolumeClaim. + Expect(vm.Spec.Template.Spec.Volumes[i].VolumeSource.PersistentVolumeClaim. PersistentVolumeClaimVolumeSource.ClaimName).To(Equal(claim)) } // TODO: right now, for simplicity, this function assumes the DV in the first position in the datavolumes templata list. Otherwise, we need From 8fa35eca05dd6e343de340b798583529876ccbd0 Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 15:57:29 +0300 Subject: [PATCH 4/6] e2e,storage migration,UpdateVMWithDV: Replace client update with patch Patch enable mutating objects in precise manner, changing spesific fields, making the change more predictive, and reducing potential for confilcts because it doesnt change the resource-verison. Signed-off-by: Or Mergi --- tests/storage/migration.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index 684f5cf28228..446b1f64c959 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -239,19 +239,28 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { var replacedIndex int vm, err := virtClient.VirtualMachine(ns).Get(context.Background(), vmName, metav1.GetOptions{}) Expect(err).ShouldNot(HaveOccurred()) - vm.Spec.DataVolumeTemplates[0].Name = name for i, v := range vm.Spec.Template.Spec.Volumes { if v.Name == volName { - vm.Spec.Template.Spec.Volumes[i].VolumeSource.DataVolume = &virtv1.DataVolumeSource{ - Name: name, - } replacedIndex = i break } } - vm.Spec.UpdateVolumesStrategy = pointer.P(virtv1.UpdateVolumesStrategyMigration) - vm, err = virtClient.VirtualMachine(ns).Update(context.Background(), vm, metav1.UpdateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + + updatedVolume := virtv1.Volume{ + Name: volName, + VolumeSource: virtv1.VolumeSource{DataVolume: &virtv1.DataVolumeSource{ + Name: name, + }}} + + p, err := patch.New( + patch.WithReplace("/spec/dataVolumeTemplates/0/metadata/name", name), + patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", replacedIndex), updatedVolume), + patch.WithReplace("/spec/updateVolumesStrategy", virtv1.UpdateVolumesStrategyMigration), + ).GeneratePayload() + Expect(err).ToNot(HaveOccurred()) + vm, err = virtClient.VirtualMachine(vm.Namespace).Patch(context.Background(), vm.Name, types.JSONPatchType, p, metav1.PatchOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(vm.Spec.Template.Spec.Volumes[replacedIndex].VolumeSource.DataVolume.Name).To(Equal(name)) } From 2ecc0c6f79baf11c3a2f7d35e570319e60f56cab Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 16:27:40 +0300 Subject: [PATCH 5/6] e2e,migration-support,updateVMWithDV: Pass VM object Change signature to get VM object instead of name to save additional Get operation. Signed-off-by: Or Mergi --- tests/storage/migration.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index 446b1f64c959..ca1b21e49632 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -235,10 +235,8 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { } // TODO: right now, for simplicity, this function assumes the DV in the first position in the datavolumes templata list. Otherwise, we need // to pass the old name of the DV to be replaces. - updateVMWithDV := func(vmName, volName, name string) { + updateVMWithDV := func(vm *virtv1.VirtualMachine, volName, name string) { var replacedIndex int - vm, err := virtClient.VirtualMachine(ns).Get(context.Background(), vmName, metav1.GetOptions{}) - Expect(err).ShouldNot(HaveOccurred()) for i, v := range vm.Spec.Template.Spec.Volumes { if v.Name == volName { replacedIndex = i @@ -302,7 +300,7 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { vm := createVMWithDV(createDV(), volName) destDV := createBlankDV() By("Update volumes") - updateVMWithDV(vm.Name, volName, destDV.Name) + updateVMWithDV(vm, volName, destDV.Name) Eventually(func() bool { vmi, err := virtClient.VirtualMachineInstance(ns).Get(context.Background(), vm.Name, metav1.GetOptions{}) @@ -425,7 +423,7 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { Eventually(matcher.ThisVM(vm), 360*time.Second, 1*time.Second).Should(matcher.BeReady()) libwait.WaitForSuccessfulVMIStart(vmi) By("Update volumes") - updateVMWithDV(vm.Name, volName, destDV.Name) + updateVMWithDV(vm, volName, destDV.Name) Eventually(func() []virtv1.VirtualMachineCondition { vm, err := virtClient.VirtualMachine(vm.Namespace).Get(context.Background(), vm.Name, metav1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) From 98f52f6c6edacb35687a5b6b3dac6ae2e3b4a702 Mon Sep 17 00:00:00 2001 From: Or Mergi Date: Mon, 29 Jul 2024 16:01:12 +0300 Subject: [PATCH 6/6] e2e,storage migration,UpdateVMWithDV: Fetch volume name using slices Signed-off-by: Or Mergi --- tests/storage/migration.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/storage/migration.go b/tests/storage/migration.go index ca1b21e49632..40d5ea3889e9 100644 --- a/tests/storage/migration.go +++ b/tests/storage/migration.go @@ -236,13 +236,11 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { // TODO: right now, for simplicity, this function assumes the DV in the first position in the datavolumes templata list. Otherwise, we need // to pass the old name of the DV to be replaces. updateVMWithDV := func(vm *virtv1.VirtualMachine, volName, name string) { - var replacedIndex int - for i, v := range vm.Spec.Template.Spec.Volumes { - if v.Name == volName { - replacedIndex = i - break - } - } + i := slices.IndexFunc(vm.Spec.Template.Spec.Volumes, func(volume virtv1.Volume) bool { + return volume.Name == volName + }) + Expect(i).To(BeNumerically(">", -1)) + By(fmt.Sprintf("Replacing volume %s with DV %s", volName, name)) updatedVolume := virtv1.Volume{ Name: volName, @@ -252,14 +250,14 @@ var _ = SIGDescribe("[Serial]Volumes update with migration", Serial, func() { p, err := patch.New( patch.WithReplace("/spec/dataVolumeTemplates/0/metadata/name", name), - patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", replacedIndex), updatedVolume), + patch.WithReplace(fmt.Sprintf("/spec/template/spec/volumes/%d", i), updatedVolume), patch.WithReplace("/spec/updateVolumesStrategy", virtv1.UpdateVolumesStrategyMigration), ).GeneratePayload() Expect(err).ToNot(HaveOccurred()) vm, err = virtClient.VirtualMachine(vm.Namespace).Patch(context.Background(), vm.Name, types.JSONPatchType, p, metav1.PatchOptions{}) Expect(err).ToNot(HaveOccurred()) - Expect(vm.Spec.Template.Spec.Volumes[replacedIndex].VolumeSource.DataVolume.Name).To(Equal(name)) + Expect(vm.Spec.Template.Spec.Volumes[i].VolumeSource.DataVolume.Name).To(Equal(name)) } BeforeEach(func() {