Merge pull request #7985 from ShellyKa13/fix-vm-restore-pvc-sizes-0.44

[release-0.44] Backport 'Fix vm restore in case of restore size bigger then PVC requested size'
kubevirt · Jun 27, 2022 · afb1525 · afb1525
2 parents 265db1f + ddb3ed2
commit afb1525
Show file tree

Hide file tree

Showing 7 changed files with 167 additions and 22 deletions.
diff --git a/pkg/virt-controller/watch/application.go b/pkg/virt-controller/watch/application.go
@@ -610,6 +610,7 @@ func (vca *VirtControllerApp) initRestoreController() {
 		DataVolumeInformer:        vca.dataVolumeInformer,
 		PVCInformer:               vca.persistentVolumeClaimInformer,
 		StorageClassInformer:      vca.storageClassInformer,
+		VolumeSnapshotProvider:    vca.snapshotController,
 		Recorder:                  recorder,
 	}
 	vca.restoreController.Init()

diff --git a/pkg/virt-controller/watch/snapshot/restore.go b/pkg/virt-controller/watch/snapshot/restore.go
@@ -663,6 +663,24 @@ func (ctrl *VMRestoreController) createRestorePVC(
 		return fmt.Errorf("missing VolumeSnapshot name")
 	}
 
+	volumeSnapshot, err := ctrl.VolumeSnapshotProvider.GetVolumeSnapshot(vmRestore.Namespace, *volumeBackup.VolumeSnapshotName)
+	if err != nil {
+		return err
+	}
+
+	if volumeSnapshot == nil {
+		log.Log.Errorf("VolumeSnapshot %s is missing", *volumeBackup.VolumeSnapshotName)
+		return fmt.Errorf("missing VolumeSnapshot %s", *volumeBackup.VolumeSnapshotName)
+	}
+
+	if volumeSnapshot.Status != nil && volumeSnapshot.Status.RestoreSize != nil {
+		restorePVCSize, ok := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
+		// Update restore pvc size to be the maximum between the source PVC and the restore size
+		if !ok || restorePVCSize.Cmp(*volumeSnapshot.Status.RestoreSize) < 0 {
+			pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *volumeSnapshot.Status.RestoreSize
+		}
+	}
+
 	if pvc.Annotations == nil {
 		pvc.Annotations = make(map[string]string)
 	}
@@ -686,7 +704,7 @@ func (ctrl *VMRestoreController) createRestorePVC(
 
 	target.Own(pvc)
 
-	_, err := ctrl.Client.CoreV1().PersistentVolumeClaims(vmRestore.Namespace).Create(context.Background(), pvc, metav1.CreateOptions{})
+	_, err = ctrl.Client.CoreV1().PersistentVolumeClaims(vmRestore.Namespace).Create(context.Background(), pvc, metav1.CreateOptions{})
 	if err != nil {
 		return err
 	}

diff --git a/pkg/virt-controller/watch/snapshot/restore_base.go b/pkg/virt-controller/watch/snapshot/restore_base.go
@@ -50,6 +50,8 @@ type VMRestoreController struct {
 	PVCInformer               cache.SharedIndexInformer
 	StorageClassInformer      cache.SharedIndexInformer
 
+	VolumeSnapshotProvider VolumeSnapshotProvider
+
 	Recorder record.EventRecorder
 
 	vmRestoreQueue workqueue.RateLimitingInterface

diff --git a/pkg/virt-controller/watch/snapshot/restore_test.go b/pkg/virt-controller/watch/snapshot/restore_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/golang/mock/gomock"
+	vsv1beta1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
@@ -161,6 +162,17 @@ var _ = Describe("Restore controlleer", func() {
 		}
 	}
 
+	createVolumeSnapshot := func(name string, restoreSize resource.Quantity) *vsv1beta1.VolumeSnapshot {
+		return &vsv1beta1.VolumeSnapshot{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+			Status: &vsv1beta1.VolumeSnapshotStatus{
+				RestoreSize: &restoreSize,
+			},
+		}
+	}
+
 	Context("One valid Restore controller given", func() {
 
 		var ctrl *gomock.Controller
@@ -195,6 +207,7 @@ var _ = Describe("Restore controlleer", func() {
 		var controller *VMRestoreController
 		var recorder *record.FakeRecorder
 		var mockVMRestoreQueue *testutils.MockWorkQueue
+		var fakeVolumeSnapshotProvider *MockVolumeSnapshotProvider
 
 		var kubevirtClient *kubevirtfake.Clientset
 		var k8sClient *k8sfake.Clientset
@@ -250,6 +263,10 @@ var _ = Describe("Restore controlleer", func() {
 			recorder = record.NewFakeRecorder(100)
 			recorder.IncludeObject = true
 
+			fakeVolumeSnapshotProvider = &MockVolumeSnapshotProvider{
+				volumeSnapshots: []*vsv1beta1.VolumeSnapshot{},
+			}
+
 			controller = &VMRestoreController{
 				Client:                    virtClient,
 				VMRestoreInformer:         vmRestoreInformer,
@@ -262,6 +279,7 @@ var _ = Describe("Restore controlleer", func() {
 				DataVolumeInformer:        dataVolumeInformer,
 				Recorder:                  recorder,
 				vmStatusUpdater:           status.NewVMStatusUpdater(virtClient),
+				VolumeSnapshotProvider:    fakeVolumeSnapshotProvider,
 			}
 			controller.Init()
 
@@ -439,8 +457,54 @@ var _ = Describe("Restore controlleer", func() {
 				}
 				vmSource.Add(vm)
 				addVolumeRestores(r)
+				pvcSize := resource.MustParse("2Gi")
+				vs := createVolumeSnapshot(r.Status.Restores[0].VolumeSnapshotName, pvcSize)
+				fakeVolumeSnapshotProvider.Add(vs)
+				expectUpdateVMRestoreInProgress(vm)
+				expectPVCCreates(k8sClient, r, pvcSize)
+				addVirtualMachineRestore(r)
+				controller.processVMRestoreWorkItem()
+			})
+
+			It("should create restore PVC with volume snapshot size if bigger then PVC size", func() {
+				r := createRestoreWithOwner()
+				vm := createModifiedVM()
+				r.Status = &snapshotv1.VirtualMachineRestoreStatus{
+					Complete: &f,
+					Conditions: []snapshotv1.Condition{
+						newProgressingCondition(corev1.ConditionTrue, "Creating new PVCs"),
+						newReadyCondition(corev1.ConditionFalse, "Waiting for new PVCs"),
+					},
+				}
+				vmSource.Add(vm)
+				addVolumeRestores(r)
+				q := resource.MustParse("3Gi")
+				vs := createVolumeSnapshot(r.Status.Restores[0].VolumeSnapshotName, q)
+				fakeVolumeSnapshotProvider.Add(vs)
 				expectUpdateVMRestoreInProgress(vm)
-				expectPVCCreates(k8sClient, r)
+				expectPVCCreates(k8sClient, r, q)
+				addVirtualMachineRestore(r)
+				controller.processVMRestoreWorkItem()
+			})
+
+			It("should create restore PVC with pvc size if restore size is smaller", func() {
+				r := createRestoreWithOwner()
+				vm := createModifiedVM()
+				r.Status = &snapshotv1.VirtualMachineRestoreStatus{
+					Complete: &f,
+					Conditions: []snapshotv1.Condition{
+						newProgressingCondition(corev1.ConditionTrue, "Creating new PVCs"),
+						newReadyCondition(corev1.ConditionFalse, "Waiting for new PVCs"),
+					},
+				}
+				vmSource.Add(vm)
+				addVolumeRestores(r)
+				q := resource.MustParse("1Gi")
+				vs := createVolumeSnapshot(r.Status.Restores[0].VolumeSnapshotName, q)
+				fakeVolumeSnapshotProvider.Add(vs)
+				expectUpdateVMRestoreInProgress(vm)
+				pvcSize := resource.MustParse("2Gi")
+				expectPVCCreates(k8sClient, r, pvcSize)
 				addVirtualMachineRestore(r)
 				controller.processVMRestoreWorkItem()
 			})
@@ -651,7 +715,7 @@ var _ = Describe("Restore controlleer", func() {
 	})
 })
 
-func expectPVCCreates(client *k8sfake.Clientset, vmRestore *snapshotv1.VirtualMachineRestore) {
+func expectPVCCreates(client *k8sfake.Clientset, vmRestore *snapshotv1.VirtualMachineRestore, expectedSize resource.Quantity) {
 	client.Fake.PrependReactor("create", "persistentvolumeclaims", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
 		create, ok := action.(testing.CreateAction)
 		Expect(ok).To(BeTrue())
@@ -660,6 +724,7 @@ func expectPVCCreates(client *k8sfake.Clientset, vmRestore *snapshotv1.VirtualMa
 		found := false
 		for _, vr := range vmRestore.Status.Restores {
 			if vr.PersistentVolumeClaimName == createObj.Name {
+				Expect(createObj.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(expectedSize))
 				found = true
 				break
 			}
@@ -719,3 +784,21 @@ func expectDataVolumeDeletes(client *cdifake.Clientset, names []string) {
 		return true, nil, nil
 	})
 }
+
+// A mock to implement volumeSnapshotProvider interface
+type MockVolumeSnapshotProvider struct {
+	volumeSnapshots []*vsv1beta1.VolumeSnapshot
+}
+
+func (v *MockVolumeSnapshotProvider) GetVolumeSnapshot(namespace, name string) (*vsv1beta1.VolumeSnapshot, error) {
+	if len(v.volumeSnapshots) == 0 {
+		return nil, nil
+	}
+	vs := v.volumeSnapshots[0]
+	v.volumeSnapshots = v.volumeSnapshots[1:]
+	return vs, nil
+}
+
+func (v *MockVolumeSnapshotProvider) Add(s *vsv1beta1.VolumeSnapshot) {
+	v.volumeSnapshots = append(v.volumeSnapshots, s)
+}
diff --git a/pkg/virt-controller/watch/snapshot/snapshot.go b/pkg/virt-controller/watch/snapshot/snapshot.go
@@ -223,7 +223,7 @@ func (ctrl *VMSnapshotController) updateVMSnapshotContent(content *snapshotv1.Vi
 
 		vsName := *volumeBackup.VolumeSnapshotName
 
-		volumeSnapshot, err := ctrl.getVolumeSnapshot(content.Namespace, vsName)
+		volumeSnapshot, err := ctrl.GetVolumeSnapshot(content.Namespace, vsName)
 		if err != nil {
 			return 0, err
 		}

diff --git a/pkg/virt-controller/watch/snapshot/snapshot_base.go b/pkg/virt-controller/watch/snapshot/snapshot_base.go
@@ -511,24 +511,6 @@ func (ctrl *VMSnapshotController) handleDV(obj interface{}) {
 	}
 }
 
-func (ctrl *VMSnapshotController) getVolumeSnapshot(namespace, name string) (*vsv1beta1.VolumeSnapshot, error) {
-	di := ctrl.dynamicInformerMap[volumeSnapshotCRD]
-	di.mutex.Lock()
-	defer di.mutex.Unlock()
-
-	if di.informer == nil {
-		return nil, nil
-	}
-
-	key := fmt.Sprintf("%s/%s", namespace, name)
-	obj, exists, err := di.informer.GetStore().GetByKey(key)
-	if !exists || err != nil {
-		return nil, err
-	}
-
-	return obj.(*vsv1beta1.VolumeSnapshot).DeepCopy(), nil
-}
-
 func (ctrl *VMSnapshotController) getVolumeSnapshotClasses() []vsv1beta1.VolumeSnapshotClass {
 	di := ctrl.dynamicInformerMap[volumeSnapshotClassCRD]
 	di.mutex.Lock()
@@ -595,3 +577,25 @@ func (ctrl *VMSnapshotController) deleteDynamicInformer(name string) (time.Durat
 
 	return 0, nil
 }
+
+type VolumeSnapshotProvider interface {
+	GetVolumeSnapshot(string, string) (*vsv1beta1.VolumeSnapshot, error)
+}
+
+func (ctrl *VMSnapshotController) GetVolumeSnapshot(namespace, name string) (*vsv1beta1.VolumeSnapshot, error) {
+	di := ctrl.dynamicInformerMap[volumeSnapshotCRD]
+	di.mutex.Lock()
+	defer di.mutex.Unlock()
+
+	if di.informer == nil {
+		return nil, nil
+	}
+
+	key := fmt.Sprintf("%s/%s", namespace, name)
+	obj, exists, err := di.informer.GetStore().GetByKey(key)
+	if !exists || err != nil {
+		return nil, err
+	}
+
+	return obj.(*vsv1beta1.VolumeSnapshot).DeepCopy(), nil
+}
diff --git a/tests/storage/restore.go b/tests/storage/restore.go
@@ -599,6 +599,43 @@ var _ = SIGDescribe("[Serial]VirtualMachineRestore Tests", func() {
 				}
 			})
 
+			// This test is relevant to provisioner which round up the recieved size of
+			// the PVC. Currently we only test vmsnapshot tests which ceph which has this
+			// behavior. In case of running this test with other provisioner or if ceph
+			// will change this behavior it will fail.
+			It("should restore a vm with restore size bigger then PVC size", func() {
+				vm = tests.NewRandomVMWithDataVolumeAndUserDataInStorageClass(
+					tests.GetUrl(tests.CirrosHttpUrl),
+					util.NamespaceTestDefault,
+					"#!/bin/bash\necho 'hello'\n",
+					snapshotStorageClass,
+				)
+				quantity, err := resource.ParseQuantity("1528Mi")
+				Expect(err).ToNot(HaveOccurred())
+				vm.Spec.DataVolumeTemplates[0].Spec.PVC.Resources.Requests["storage"] = quantity
+				vm, vmi = createAndStartVM(vm)
+				expectedCapacity, err := resource.ParseQuantity("2Gi")
+				Expect(err).ToNot(HaveOccurred())
+				pvc, err := virtClient.CoreV1().PersistentVolumeClaims(vm.Namespace).Get(context.Background(), vm.Spec.DataVolumeTemplates[0].Name, metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(pvc.Status.Capacity["storage"]).To(Equal(expectedCapacity))
+
+				doRestore("", console.LoginToCirros, false)
+
+				content, err := virtClient.VirtualMachineSnapshotContent(vm.Namespace).Get(context.Background(), *snapshot.Status.VirtualMachineSnapshotContentName, metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(content.Spec.VolumeBackups[0].PersistentVolumeClaim.Spec.Resources.Requests["storage"]).To(Equal(quantity))
+				vs, err := virtClient.KubernetesSnapshotClient().SnapshotV1beta1().VolumeSnapshots(vm.Namespace).Get(context.Background(), *content.Spec.VolumeBackups[0].VolumeSnapshotName, metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(*vs.Status.RestoreSize).To(Equal(expectedCapacity))
+
+				pvc, err = virtClient.CoreV1().PersistentVolumeClaims(vm.Namespace).Get(context.Background(), restore.Status.Restores[0].PersistentVolumeClaimName, metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(pvc.Status.Capacity["storage"]).To(Equal(expectedCapacity))
+				Expect(pvc.Spec.Resources.Requests["storage"]).To(Equal(expectedCapacity))
+
+			})
+
 			It("[test_id:5260]should restore a vm that boots from a datavolumetemplate", func() {
 				vm, vmi = createAndStartVM(tests.NewRandomVMWithDataVolumeAndUserDataInStorageClass(
 					tests.GetUrl(tests.CirrosHttpUrl),