From c1cca8e5f2624412635fa51c6773ed0eaca2d911 Mon Sep 17 00:00:00 2001 From: Michael Henriksen Date: Fri, 2 Oct 2020 08:09:28 -0400 Subject: [PATCH] fix restore controller memory corruption Signed-off-by: Michael Henriksen --- pkg/virt-controller/watch/snapshot/restore.go | 53 +++++++++++-------- .../watch/snapshot/restore_base.go | 2 +- .../watch/snapshot/restore_test.go | 48 ++++++++++------- .../watch/snapshot/snapshot.go | 8 +-- .../watch/snapshot/snapshot_base.go | 8 +-- tests/restore_test.go | 29 ++++++++++ 6 files changed, 98 insertions(+), 50 deletions(-) diff --git a/pkg/virt-controller/watch/snapshot/restore.go b/pkg/virt-controller/watch/snapshot/restore.go index 58c0848d7be1..4ce0dec82b4f 100644 --- a/pkg/virt-controller/watch/snapshot/restore.go +++ b/pkg/virt-controller/watch/snapshot/restore.go @@ -90,26 +90,29 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual return 0, nil } - complete := false vmRestoreOut := vmRestoreIn.DeepCopy() if vmRestoreOut.Status == nil { - vmRestoreOut.Status = &snapshotv1.VirtualMachineRestoreStatus{} + f := false + vmRestoreOut.Status = &snapshotv1.VirtualMachineRestoreStatus{ + Complete: &f, + } } - vmRestoreOut.Status.Complete = &complete - vmRestoreOut.Status.RestoreTime = nil - target, err := ctrl.getTarget(vmRestoreOut) if err != nil { logger.Reason(err).Error("Error getting restore target") - return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err) + return 0, ctrl.doUpdateError(vmRestoreOut, err) } if len(vmRestoreOut.OwnerReferences) == 0 { target.Own(vmRestoreOut) updateRestoreCondition(vmRestoreOut, newProgressingCondition(corev1.ConditionTrue, "Initializing VirtualMachineRestore")) updateRestoreCondition(vmRestoreOut, newReadyCondition(corev1.ConditionFalse, "Initializing VirtualMachineRestore")) + } + + // let's make sure everything is initialized properly before continuing + if !reflect.DeepEqual(vmRestoreIn, vmRestoreOut) { return 0, ctrl.doUpdate(vmRestoreIn, vmRestoreOut) } @@ -117,7 +120,7 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual updated, err = ctrl.reconcileVolumeRestores(vmRestoreOut, target) if err != nil { logger.Reason(err).Error("Error reconciling VolumeRestores") - return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err) + return 0, ctrl.doUpdateError(vmRestoreIn, err) } if !updated { @@ -125,20 +128,20 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual ready, err = target.Ready() if err != nil { logger.Reason(err).Error("Error checking target ready") - return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err) + return 0, ctrl.doUpdateError(vmRestoreIn, err) } if ready { updated, err = target.Reconcile() if err != nil { logger.Reason(err).Error("Error reconciling target") - return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err) + return 0, ctrl.doUpdateError(vmRestoreIn, err) } if !updated { if err = target.Cleanup(); err != nil { logger.Reason(err).Error("Error cleaning up") - return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err) + return 0, ctrl.doUpdateError(vmRestoreIn, err) } ctrl.Recorder.Eventf( @@ -149,7 +152,8 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual vmRestoreOut.Name, ) - complete = true + t := true + vmRestoreOut.Status.Complete = &t vmRestoreOut.Status.RestoreTime = currentTime() updateRestoreCondition(vmRestoreOut, newProgressingCondition(corev1.ConditionFalse, "Operation complete")) updateRestoreCondition(vmRestoreOut, newReadyCondition(corev1.ConditionTrue, "Operation complete")) @@ -172,18 +176,20 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual return 0, ctrl.doUpdate(vmRestoreIn, vmRestoreOut) } -func (ctrl *VMRestoreController) doUpdateError(original, updated *snapshotv1.VirtualMachineRestore, err error) error { +func (ctrl *VMRestoreController) doUpdateError(restore *snapshotv1.VirtualMachineRestore, err error) error { ctrl.Recorder.Eventf( - updated, + restore, corev1.EventTypeWarning, restoreErrorEvent, "VirtualMachineRestore encountered error %s", err.Error(), ) + updated := restore.DeepCopy() + updateRestoreCondition(updated, newProgressingCondition(corev1.ConditionFalse, err.Error())) updateRestoreCondition(updated, newReadyCondition(corev1.ConditionFalse, err.Error())) - if err2 := ctrl.doUpdate(original, updated); err2 != nil { + if err2 := ctrl.doUpdate(restore, updated); err2 != nil { return err2 } @@ -285,7 +291,7 @@ func (ctrl *VMRestoreController) getBindingMode(pvc *corev1.PersistentVolumeClai return nil, fmt.Errorf("StorageClass %s does not exist", *pvc.Spec.StorageClassName) } - sc := obj.(*storagev1.StorageClass) + sc := obj.(*storagev1.StorageClass).DeepCopy() return sc.VolumeBindingMode, nil } @@ -343,8 +349,13 @@ func (t *vmRestoreTarget) Reconcile() (bool, error) { var deletedDataVolumes []string updatedStatus := false - copy(newTemplates, snapshotVM.Spec.DataVolumeTemplates) - copy(newVolumes, snapshotVM.Spec.Template.Spec.Volumes) + for i, t := range snapshotVM.Spec.DataVolumeTemplates { + t.DeepCopyInto(&newTemplates[i]) + } + + for i, v := range snapshotVM.Spec.Template.Spec.Volumes { + v.DeepCopyInto(&newVolumes[i]) + } for j, v := range snapshotVM.Spec.Template.Spec.Volumes { if v.DataVolume != nil || v.PersistentVolumeClaim != nil { @@ -503,7 +514,7 @@ func (ctrl *VMRestoreController) getSnapshotContent(vmRestore *snapshotv1.Virtua return nil, fmt.Errorf("VMSnapshot %s does not exist", objKey) } - vms := obj.(*snapshotv1.VirtualMachineSnapshot) + vms := obj.(*snapshotv1.VirtualMachineSnapshot).DeepCopy() if !vmSnapshotReady(vms) { return nil, fmt.Errorf("VMSnapshot %s not ready", objKey) } @@ -526,7 +537,7 @@ func (ctrl *VMRestoreController) getSnapshotContent(vmRestore *snapshotv1.Virtua return nil, fmt.Errorf("VMSnapshotContent %s does not exist", objKey) } - vmss := obj.(*snapshotv1.VirtualMachineSnapshotContent) + vmss := obj.(*snapshotv1.VirtualMachineSnapshotContent).DeepCopy() if !vmSnapshotContentReady(vmss) { return nil, fmt.Errorf("VMSnapshotContent %s not ready", objKey) } @@ -545,7 +556,7 @@ func (ctrl *VMRestoreController) getVM(namespace, name string) (*kubevirtv1.Virt return nil, fmt.Errorf("VirtualMachine %s/%s does not exist", namespace, name) } - return obj.(*kubevirtv1.VirtualMachine), nil + return obj.(*kubevirtv1.VirtualMachine).DeepCopy(), nil } func (ctrl *VMRestoreController) getPVC(namespace, name string) (*corev1.PersistentVolumeClaim, error) { @@ -559,7 +570,7 @@ func (ctrl *VMRestoreController) getPVC(namespace, name string) (*corev1.Persist return nil, nil } - return obj.(*corev1.PersistentVolumeClaim), nil + return obj.(*corev1.PersistentVolumeClaim).DeepCopy(), nil } func (ctrl *VMRestoreController) getTarget(vmRestore *snapshotv1.VirtualMachineRestore) (restoreTarget, error) { diff --git a/pkg/virt-controller/watch/snapshot/restore_base.go b/pkg/virt-controller/watch/snapshot/restore_base.go index 29595b5a1630..8a6a329a3bbe 100644 --- a/pkg/virt-controller/watch/snapshot/restore_base.go +++ b/pkg/virt-controller/watch/snapshot/restore_base.go @@ -129,7 +129,7 @@ func (ctrl *VMRestoreController) processVMRestoreWorkItem() bool { return 0, fmt.Errorf("unexpected resource %+v", storeObj) } - return ctrl.updateVMRestore(vmRestore) + return ctrl.updateVMRestore(vmRestore.DeepCopy()) }) } diff --git a/pkg/virt-controller/watch/snapshot/restore_test.go b/pkg/virt-controller/watch/snapshot/restore_test.go index b4e21d807200..95c2d6b6a89d 100644 --- a/pkg/virt-controller/watch/snapshot/restore_test.go +++ b/pkg/virt-controller/watch/snapshot/restore_test.go @@ -51,16 +51,6 @@ var _ = Describe("Restore controlleer", func() { Name: "restore", Namespace: testNamespace, UID: uid, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: kubevirtv1.GroupVersion.String(), - Kind: "VirtualMachine", - Name: vmName, - UID: vmUID, - Controller: &t, - BlockOwnerDeletion: &t, - }, - }, }, Spec: snapshotv1.VirtualMachineRestoreSpec{ Target: corev1.TypedLocalObjectReference{ @@ -73,6 +63,24 @@ var _ = Describe("Restore controlleer", func() { } } + createRestoreWithOwner := func() *snapshotv1.VirtualMachineRestore { + r := createRestore() + r.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: kubevirtv1.GroupVersion.String(), + Kind: "VirtualMachine", + Name: vmName, + UID: vmUID, + Controller: &t, + BlockOwnerDeletion: &t, + }, + } + r.Status = &snapshotv1.VirtualMachineRestoreStatus{ + Complete: &f, + } + return r + } + addVolumeRestores := func(r *snapshotv1.VirtualMachineRestore) { r.Status.Restores = []snapshotv1.VolumeRestore{ { @@ -323,7 +331,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should error if snapshot does not exist", func() { - r := createRestore() + r := createRestoreWithOwner() vm := createModifiedVM() rc := r.DeepCopy() rc.ResourceVersion = "1" @@ -343,7 +351,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should error if different source UID", func() { - r := createRestore() + r := createRestoreWithOwner() vm := createModifiedVM() vm.UID = types.UID("foobar") rc := r.DeepCopy() @@ -363,7 +371,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should update restore status, initializing conditions and add owner", func() { - r := createRestore() + r := createRestoreWithOwner() refs := r.OwnerReferences r.OwnerReferences = nil vm := createModifiedVM() @@ -384,7 +392,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should update restore status with condition and VolumeRestores", func() { - r := createRestore() + r := createRestoreWithOwner() vm := createModifiedVM() rc := r.DeepCopy() rc.ResourceVersion = "1" @@ -403,7 +411,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should create restore PVCs", func() { - r := createRestore() + r := createRestoreWithOwner() vm := createModifiedVM() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, @@ -420,7 +428,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should wait for bound", func() { - r := createRestore() + r := createRestoreWithOwner() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, Conditions: []snapshotv1.Condition{ @@ -443,7 +451,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should update restore status with datavolume", func() { - r := createRestore() + r := createRestoreWithOwner() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, Conditions: []snapshotv1.Condition{ @@ -473,7 +481,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should update PVCs and restores to have datavolumename", func() { - r := createRestore() + r := createRestoreWithOwner() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, Conditions: []snapshotv1.Condition{ @@ -506,7 +514,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should update VM spec", func() { - r := createRestore() + r := createRestoreWithOwner() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, DeletedDataVolumes: getDeletedDataVolumes(createModifiedVM()), @@ -537,7 +545,7 @@ var _ = Describe("Restore controlleer", func() { }) It("should cleanup and complete", func() { - r := createRestore() + r := createRestoreWithOwner() r.Status = &snapshotv1.VirtualMachineRestoreStatus{ Complete: &f, DeletedDataVolumes: getDeletedDataVolumes(createModifiedVM()), diff --git a/pkg/virt-controller/watch/snapshot/snapshot.go b/pkg/virt-controller/watch/snapshot/snapshot.go index ff64c0df777b..1244da3ae737 100644 --- a/pkg/virt-controller/watch/snapshot/snapshot.go +++ b/pkg/virt-controller/watch/snapshot/snapshot.go @@ -503,7 +503,7 @@ func (ctrl *VMSnapshotController) getSnapshotPVC(namespace, volumeName string) ( return nil, nil } - pvc := obj.(*corev1.PersistentVolumeClaim) + pvc := obj.(*corev1.PersistentVolumeClaim).DeepCopy() if pvc.Spec.VolumeName == "" { log.Log.Warningf("Unbound PVC %s/%s", pvc.Namespace, pvc.Name) @@ -533,7 +533,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshotClass(storageClassName string return "", err } - storageClass := obj.(*storagev1.StorageClass) + storageClass := obj.(*storagev1.StorageClass).DeepCopy() var matches []vsv1beta1.VolumeSnapshotClass volumeSnapshotClasses := ctrl.getVolumeSnapshotClasses() @@ -648,7 +648,7 @@ func (ctrl *VMSnapshotController) getVM(vmSnapshot *snapshotv1.VirtualMachineSna return nil, nil } - return obj.(*kubevirtv1.VirtualMachine), nil + return obj.(*kubevirtv1.VirtualMachine).DeepCopy(), nil } func (ctrl *VMSnapshotController) getContent(vmSnapshot *snapshotv1.VirtualMachineSnapshot) (*snapshotv1.VirtualMachineSnapshotContent, error) { @@ -662,7 +662,7 @@ func (ctrl *VMSnapshotController) getContent(vmSnapshot *snapshotv1.VirtualMachi return nil, nil } - return obj.(*snapshotv1.VirtualMachineSnapshotContent), nil + return obj.(*snapshotv1.VirtualMachineSnapshotContent).DeepCopy(), nil } func (s *vmSnapshotSource) UID() types.UID { diff --git a/pkg/virt-controller/watch/snapshot/snapshot_base.go b/pkg/virt-controller/watch/snapshot/snapshot_base.go index 8e68e4db1437..cf4ac13fad32 100644 --- a/pkg/virt-controller/watch/snapshot/snapshot_base.go +++ b/pkg/virt-controller/watch/snapshot/snapshot_base.go @@ -208,7 +208,7 @@ func (ctrl *VMSnapshotController) processVMSnapshotWorkItem() bool { return 0, fmt.Errorf("unexpected resource %+v", storeObj) } - return ctrl.updateVMSnapshot(vmSnapshot) + return ctrl.updateVMSnapshot(vmSnapshot.DeepCopy()) }) } @@ -226,7 +226,7 @@ func (ctrl *VMSnapshotController) processVMSnapshotContentWorkItem() bool { return 0, fmt.Errorf("unexpected resource %+v", storeObj) } - return ctrl.updateVMSnapshotContent(vmSnapshotContent) + return ctrl.updateVMSnapshotContent(vmSnapshotContent.DeepCopy()) }) } @@ -384,7 +384,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshot(namespace, name string) (*vs return nil, err } - return obj.(*vsv1beta1.VolumeSnapshot), nil + return obj.(*vsv1beta1.VolumeSnapshot).DeepCopy(), nil } func (ctrl *VMSnapshotController) getVolumeSnapshotClasses() []vsv1beta1.VolumeSnapshotClass { @@ -399,7 +399,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshotClasses() []vsv1beta1.VolumeS var vscs []vsv1beta1.VolumeSnapshotClass objs := di.informer.GetStore().List() for _, obj := range objs { - vsc := obj.(*vsv1beta1.VolumeSnapshotClass) + vsc := obj.(*vsv1beta1.VolumeSnapshotClass).DeepCopy() vscs = append(vscs, *vsc) } diff --git a/tests/restore_test.go b/tests/restore_test.go index e029667652af..9379de42f0e1 100644 --- a/tests/restore_test.go +++ b/tests/restore_test.go @@ -515,6 +515,35 @@ var _ = Describe("[Serial]VirtualMachineRestore Tests", func() { Expect(err).ToNot(HaveOccurred()) } + It("should restore a vm multiple from the same snapshot", func() { + vm, vmi = createAndStartVM(tests.NewRandomVMWithDataVolumeAndUserDataInStorageClass( + tests.GetUrl(tests.CirrosHttpUrl), + tests.NamespaceTestDefault, + "#!/bin/bash\necho 'hello'\n", + snapshotStorageClass, + )) + + By("Stopping VM") + vm = tests.StopVirtualMachine(vm) + + By("creating snapshot") + snapshot = createSnapshot(vm) + + for i := 0; i < 2; i++ { + By(fmt.Sprintf("Restoring VM iteration %d", i)) + restore = createRestoreDef(vm, snapshot.Name) + + restore, err = virtClient.VirtualMachineRestore(vm.Namespace).Create(restore) + Expect(err).ToNot(HaveOccurred()) + + restore = waitRestoreComplete(restore, vm) + Expect(restore.Status.Restores).To(HaveLen(1)) + + deleteRestore(restore) + restore = nil + } + }) + It("should restore a vm that boots from a datavolumetemplate", func() { vm, vmi = createAndStartVM(tests.NewRandomVMWithDataVolumeAndUserDataInStorageClass( tests.GetUrl(tests.CirrosHttpUrl),