From 2994697415d161cecf784975db1f893d1cfc25ba Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Thu, 1 Dec 2022 14:12:25 +0000 Subject: [PATCH 1/2] Add regression test for bug #8890 Signed-off-by: Lee Yarwood --- pkg/storage/snapshot/restore_test.go | 107 +++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/pkg/storage/snapshot/restore_test.go b/pkg/storage/snapshot/restore_test.go index eda9ddcfc0d4..7413ef217aaa 100644 --- a/pkg/storage/snapshot/restore_test.go +++ b/pkg/storage/snapshot/restore_test.go @@ -965,6 +965,8 @@ var _ = Describe("Restore controller", func() { preferenceOriginalCR *appsv1.ControllerRevision ) + const vmCreationFailureMessage = "something failed during VirtualMachine creation" + expectCreateVM := func(vm *v1.VirtualMachine) { newVMUID := vm.UID vm.UID = "" @@ -998,6 +1000,28 @@ var _ = Describe("Restore controller", func() { expectVMRestoreUpdate(kubevirtClient, expectedUpdatedRestore) } + expectUpdateVMRestoreFailure := func(vmRestore *snapshotv1.VirtualMachineRestore, resourceVersion, failureReason string) { + expectedUpdatedRestore := vmRestore.DeepCopy() + expectedUpdatedRestore.ResourceVersion = resourceVersion + expectedUpdatedRestore.Status.Conditions = []snapshotv1.Condition{ + newProgressingCondition(corev1.ConditionFalse, failureReason), + newReadyCondition(corev1.ConditionFalse, failureReason), + } + expectVMRestoreUpdate(kubevirtClient, expectedUpdatedRestore) + } + + expectCreateVMFailure := func(vm *v1.VirtualMachine) { + newVMUID := vm.UID + vm.UID = "" + vm.ResourceVersion = "" + vm.Annotations = map[string]string{"restore.kubevirt.io/lastRestoreUID": "restore-uid"} + vmInterface.EXPECT(). + Create(vm). + Do(func(objs ...interface{}) { + vm.UID = newVMUID + }).Return(vm, fmt.Errorf(vmCreationFailureMessage)) + } + getInstancetypeOriginalCR := func() *appsv1.ControllerRevision { return instancetypeOriginalCR } getPreferenceOriginalCR := func() *appsv1.ControllerRevision { return preferenceOriginalCR } nilInstancetypeMatcher := func() *v1.InstancetypeMatcher { return nil } @@ -1163,6 +1187,89 @@ var _ = Describe("Restore controller", func() { }, getPreferenceOriginalCR, ), ) + DescribeTable("with a failure during VirtualMachine creation", + func(getVMInstancetypeMatcher, getSnapshotInstancetypeMatcher func() *v1.InstancetypeMatcher, getVMPreferenceMatcher, getSnapshotPreferenceMatcher func() *v1.PreferenceMatcher, getExpectedCR func() *appsv1.ControllerRevision) { + originalVM.Spec.Instancetype = getVMInstancetypeMatcher() + originalVM.Spec.Preference = getVMPreferenceMatcher() + vmSource.Add(originalVM) + + vmSnapshotContent.Spec.Source.VirtualMachine.Spec.Instancetype = getSnapshotInstancetypeMatcher() + vmSnapshotContent.Spec.Source.VirtualMachine.Spec.Preference = getSnapshotPreferenceMatcher() + vmSnapshotContentSource.Add(vmSnapshotContent) + + // Ensure we restore into a new VM + newVM := originalVM.DeepCopy() + newVM.Name = "newvm" + newVM.UID = "newvm-uid" + restore.Spec.Target.Name = newVM.Name + + originalCR := getExpectedCR() + expectedCreatedCR := originalCR.DeepCopy() + expectedCreatedCR.Name = strings.Replace(expectedCreatedCR.Name, originalVM.Name, newVM.Name, 1) + expectedCreatedCR.OwnerReferences = nil + expectControllerRevisionCreate(k8sClient, expectedCreatedCR) + + // We need to be able to find the created CR from the controller so add it to the source + expectedCreatedCR.Namespace = testNamespace + crSource.Add(expectedCreatedCR) + + expectedUpdatedCR := expectedCreatedCR.DeepCopy() + expectedUpdatedCR.ResourceVersion = "5" + expectedUpdatedCR.OwnerReferences = []metav1.OwnerReference{*metav1.NewControllerRef(newVM, kubevirtv1.VirtualMachineGroupVersionKind)} + expectControllerRevisionUpdate(k8sClient, expectedUpdatedCR) + + if newVM.Spec.Instancetype != nil { + newVM.Spec.Instancetype.RevisionName = expectedCreatedCR.Name + } + if newVM.Spec.Preference != nil { + newVM.Spec.Preference.RevisionName = expectedCreatedCR.Name + } + + expectCreateVMFailure(newVM) + expectUpdateVMRestoreFailure(restore, "1", vmCreationFailureMessage) + + addVirtualMachineRestore(restore) + controller.processVMRestoreWorkItem() + + // This is bug #8890, the fact that the CR already exists shouldn't cause the reconcile to fail + expectCreateControllerRevisionAlreadyExists(k8sClient, expectedCreatedCR) + expectUpdateVMRestoreFailure(restore, "2", fmt.Sprintf(" \"%s\" already exists", expectedCreatedCR.Name)) + + addVirtualMachineRestore(restore) + controller.processVMRestoreWorkItem() + }, + Entry("and referenced instancetype", + func() *v1.InstancetypeMatcher { + return &v1.InstancetypeMatcher{ + Name: instancetypeObj.Name, + Kind: instancetypeapi.SingularResourceName, + RevisionName: instancetypeOriginalCR.Name, + } + }, func() *v1.InstancetypeMatcher { + return &v1.InstancetypeMatcher{ + Name: instancetypeObj.Name, + Kind: instancetypeapi.SingularResourceName, + RevisionName: instancetypeSnapshotCR.Name, + } + }, nilPrefrenceMatcher, nilPrefrenceMatcher, getInstancetypeOriginalCR, + ), + Entry("and referenced preference", nilInstancetypeMatcher, nilInstancetypeMatcher, + func() *v1.PreferenceMatcher { + return &v1.PreferenceMatcher{ + Name: preferenceObj.Name, + Kind: instancetypeapi.SingularPreferenceResourceName, + RevisionName: preferenceOriginalCR.Name, + } + }, + func() *v1.PreferenceMatcher { + return &v1.PreferenceMatcher{ + Name: preferenceObj.Name, + Kind: instancetypeapi.SingularPreferenceResourceName, + RevisionName: preferenceSnapshotCR.Name, + } + }, getPreferenceOriginalCR, + ), + ) }) }) }) From 3fea23fd47622cf1ec7b20492ded6d7452eed544 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Thu, 1 Dec 2022 13:12:14 +0000 Subject: [PATCH 2/2] instancetype: Ignore unexpected existing CRs during restore 5cc62c0f8f1fc368ffc920ab29fda7b55272728f failed to accommodate repeat attempts to reconcile a VirtualMachineRestore that in turn lead to multiple calls to restoreInstancetypeControllerRevision being made for the same ControllerRevision. This change handles this case by ignoring any existing ControllerRevisions found during the restore, assuming that the existing ControllerRevision was created by a previous call to restoreInstancetypeControllerRevision. Future changes will check the contents of this ControllerRevision against that of the VirtualMachineSnapshot to ensure they match. Signed-off-by: Lee Yarwood --- pkg/storage/snapshot/restore.go | 4 +++- pkg/storage/snapshot/restore_test.go | 13 +++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/storage/snapshot/restore.go b/pkg/storage/snapshot/restore.go index 0ecba76df504..2afe9b0a45c1 100644 --- a/pkg/storage/snapshot/restore.go +++ b/pkg/storage/snapshot/restore.go @@ -658,7 +658,9 @@ func (t *vmRestoreTarget) restoreInstancetypeControllerRevision(vmSnapshotRevisi restoredCR.Name = restoredCRName restoredCR, err = t.controller.Client.AppsV1().ControllerRevisions(vm.Namespace).Create(context.Background(), restoredCR, metav1.CreateOptions{}) - if err != nil { + // This might not be our first time through the reconcile loop so accommodate previous calls to restoreInstancetypeControllerRevision by ignoring unexpected existing CRs for now. + // TODO - Check the contents of the existing CR here against that of the snapshot CR + if err != nil && !errors.IsAlreadyExists(err) { return nil, err } diff --git a/pkg/storage/snapshot/restore_test.go b/pkg/storage/snapshot/restore_test.go index 7413ef217aaa..f7243bd0525b 100644 --- a/pkg/storage/snapshot/restore_test.go +++ b/pkg/storage/snapshot/restore_test.go @@ -990,9 +990,9 @@ var _ = Describe("Restore controller", func() { }).Return(expectedUpdatedVM, nil) } - expectUpdateVMRestoreUpdatingTargetSpec := func(vmRestore *snapshotv1.VirtualMachineRestore) { + expectUpdateVMRestoreUpdatingTargetSpec := func(vmRestore *snapshotv1.VirtualMachineRestore, resourceVersion string) { expectedUpdatedRestore := vmRestore.DeepCopy() - expectedUpdatedRestore.ResourceVersion = "1" + expectedUpdatedRestore.ResourceVersion = resourceVersion expectedUpdatedRestore.Status.Conditions = []snapshotv1.Condition{ newProgressingCondition(corev1.ConditionTrue, "Updating target spec"), newReadyCondition(corev1.ConditionFalse, "Waiting for target update"), @@ -1074,7 +1074,7 @@ var _ = Describe("Restore controller", func() { expectCreateControllerRevisionAlreadyExists(k8sClient, getExpectedCR()) expectUpdateVMRestoreInProgress(originalVM) expectUpdateVMRestored(originalVM) - expectUpdateVMRestoreUpdatingTargetSpec(restore) + expectUpdateVMRestoreUpdatingTargetSpec(restore, "1") addVirtualMachineRestore(restore) controller.processVMRestoreWorkItem() @@ -1150,7 +1150,7 @@ var _ = Describe("Restore controller", func() { newVM.Spec.Preference.RevisionName = expectedCreatedCR.Name } expectCreateVM(newVM) - expectUpdateVMRestoreUpdatingTargetSpec(restore) + expectUpdateVMRestoreUpdatingTargetSpec(restore, "1") addVirtualMachineRestore(restore) controller.processVMRestoreWorkItem() @@ -1231,9 +1231,10 @@ var _ = Describe("Restore controller", func() { addVirtualMachineRestore(restore) controller.processVMRestoreWorkItem() - // This is bug #8890, the fact that the CR already exists shouldn't cause the reconcile to fail + // We have already created the ControllerRevision but that shouldn't stop the reconcile from progressing expectCreateControllerRevisionAlreadyExists(k8sClient, expectedCreatedCR) - expectUpdateVMRestoreFailure(restore, "2", fmt.Sprintf(" \"%s\" already exists", expectedCreatedCR.Name)) + expectCreateVM(newVM) + expectUpdateVMRestoreUpdatingTargetSpec(restore, "2") addVirtualMachineRestore(restore) controller.processVMRestoreWorkItem()