From e5276a3eda3163105bbf4fbc14f03b170359c948 Mon Sep 17 00:00:00 2001 From: Dmitry Rakitin Date: Thu, 19 Dec 2024 14:45:34 +0100 Subject: [PATCH] fix(vdsnapshot,vmsnapshot): unfreeze virtual machines Fix unfreezing of virtual machines Fix VolumeSnapshot deletion Improve processing of concurrent snapshots Signed-off-by: Isteb4k --- .../controller/service/snapshot_service.go | 54 +++++++- .../vdsnapshot/internal/deletion.go | 20 ++- .../vdsnapshot/internal/interfaces.go | 2 +- .../vdsnapshot/internal/life_cycle.go | 20 ++- .../vdsnapshot/internal/life_cycle_test.go | 4 +- .../controller/vdsnapshot/internal/mock.go | 60 ++++----- .../vmsnapshot/internal/interfaces.go | 2 +- .../vmsnapshot/internal/life_cycle.go | 123 ++++++++++++------ .../vmsnapshot/internal/life_cycle_test.go | 20 ++- .../controller/vmsnapshot/internal/mock.go | 74 +++++------ 10 files changed, 243 insertions(+), 136 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/service/snapshot_service.go b/images/virtualization-artifact/pkg/controller/service/snapshot_service.go index 4e6bb3f8a..8106e41c8 100644 --- a/images/virtualization-artifact/pkg/controller/service/snapshot_service.go +++ b/images/virtualization-artifact/pkg/controller/service/snapshot_service.go @@ -77,7 +77,7 @@ func (s *SnapshotService) Freeze(ctx context.Context, name, namespace string) er return nil } -func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { +func (s *SnapshotService) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { if vm == nil || !s.IsFrozen(vm) { return false, nil } @@ -103,7 +103,7 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string } _, ok := vdByName[vdSnapshot.Spec.VirtualDiskName] - if ok { + if ok && vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhaseInProgress { return false, nil } } @@ -117,7 +117,55 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string } for _, vmSnapshot := range vmSnapshots.Items { - if vmSnapshot.Spec.VirtualMachineName == vm.Name { + if vmSnapshot.Spec.VirtualMachineName == vm.Name && vmSnapshot.Status.Phase == virtv2.VirtualMachineSnapshotPhaseInProgress { + return false, nil + } + } + + return true, nil +} + +func (s *SnapshotService) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Context, vmSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { + if vm == nil || !s.IsFrozen(vm) { + return false, nil + } + + vdByName := make(map[string]struct{}) + for _, bdr := range vm.Status.BlockDeviceRefs { + if bdr.Kind == virtv2.DiskDevice { + vdByName[bdr.Name] = struct{}{} + } + } + + var vdSnapshots virtv2.VirtualDiskSnapshotList + err := s.client.List(ctx, &vdSnapshots, &client.ListOptions{ + Namespace: vm.Namespace, + }) + if err != nil { + return false, err + } + + for _, vdSnapshot := range vdSnapshots.Items { + _, ok := vdByName[vdSnapshot.Spec.VirtualDiskName] + if ok && vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhaseInProgress { + return false, nil + } + } + + var vmSnapshots virtv2.VirtualMachineSnapshotList + err = s.client.List(ctx, &vmSnapshots, &client.ListOptions{ + Namespace: vm.Namespace, + }) + if err != nil { + return false, err + } + + for _, vmSnapshot := range vmSnapshots.Items { + if vmSnapshot.Name == vmSnapshotName { + continue + } + + if vmSnapshot.Spec.VirtualMachineName == vm.Name && vmSnapshot.Status.Phase == virtv2.VirtualMachineSnapshotPhaseInProgress { return false, nil } } diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go index 6cbcb73bf..4124bf17d 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go @@ -38,7 +38,7 @@ func NewDeletionHandler(snapshotter *service.SnapshotService) *DeletionHandler { func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot) (reconcile.Result, error) { if vdSnapshot.DeletionTimestamp != nil { - vs, err := h.snapshotter.GetVolumeSnapshot(ctx, vdSnapshot.Name, vdSnapshot.Namespace) + vs, err := h.snapshotter.GetVolumeSnapshot(ctx, vdSnapshot.Status.VolumeSnapshotName, vdSnapshot.Namespace) if err != nil { return reconcile.Result{}, err } @@ -48,24 +48,24 @@ func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *virtv2.VirtualD return reconcile.Result{}, err } - vm, err := getVirtualMachine(ctx, vd, h.snapshotter) - if err != nil { - return reconcile.Result{}, err + var vm *virtv2.VirtualMachine + if vd != nil { + vm, err = getVirtualMachine(ctx, vd, h.snapshotter) + if err != nil { + return reconcile.Result{}, err + } } - var requeue bool - if vs != nil { err = h.snapshotter.DeleteVolumeSnapshot(ctx, vs) if err != nil { return reconcile.Result{}, err } - requeue = true } if vm != nil { var canUnfreeze bool - canUnfreeze, err = h.snapshotter.CanUnfreeze(ctx, vdSnapshot.Name, vm) + canUnfreeze, err = h.snapshotter.CanUnfreezeWithVirtualDiskSnapshot(ctx, vdSnapshot.Name, vm) if err != nil { return reconcile.Result{}, err } @@ -78,10 +78,6 @@ func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *virtv2.VirtualD } } - if requeue { - return reconcile.Result{Requeue: true}, nil - } - controllerutil.RemoveFinalizer(vdSnapshot, virtv2.FinalizerVDSnapshotCleanup) return reconcile.Result{}, nil } diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go index 580b08e62..6fdf9b2bb 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go @@ -35,7 +35,7 @@ type LifeCycleSnapshotter interface { Freeze(ctx context.Context, name, namespace string) error IsFrozen(vm *virtv2.VirtualMachine) bool CanFreeze(vm *virtv2.VirtualMachine) bool - CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) + CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) Unfreeze(ctx context.Context, name, namespace string) error CreateVolumeSnapshot(ctx context.Context, vs *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error) GetPersistentVolumeClaim(ctx context.Context, name, namespace string) (*corev1.PersistentVolumeClaim, error) diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go index 99c121d68..5d5539227 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go @@ -86,6 +86,8 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual switch vdSnapshot.Status.Phase { case "": vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhasePending + case virtv2.VirtualDiskSnapshotPhaseFailed: + return reconcile.Result{}, nil case virtv2.VirtualDiskSnapshotPhaseReady: if vs == nil || vs.Status == nil || vs.Status.ReadyToUse == nil || !*vs.Status.ReadyToUse { vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhaseFailed @@ -134,6 +136,14 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual if h.snapshotter.CanFreeze(vm) { log.Debug("Freeze the virtual machine to take a snapshot") + if vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhasePending { + vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhaseInProgress + condition.Status = metav1.ConditionFalse + condition.Reason = vdscondition.Snapshotting + condition.Message = "The snapshotting process has started." + return reconcile.Result{Requeue: true}, nil + } + err = h.snapshotter.Freeze(ctx, vm.Name, vm.Namespace) if err != nil { setPhaseConditionToFailed(&condition, &vdSnapshot.Status.Phase, err) @@ -164,6 +174,14 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual } } + if vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhasePending { + vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhaseInProgress + condition.Status = metav1.ConditionFalse + condition.Reason = vdscondition.Snapshotting + condition.Message = "The snapshotting process has started." + return reconcile.Result{Requeue: true}, nil + } + log.Debug("The corresponding volume snapshot not found: create the new one") anno := make(map[string]string) @@ -237,7 +255,7 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual vdSnapshot.Status.Consistent = ptr.To(true) var canUnfreeze bool - canUnfreeze, err = h.snapshotter.CanUnfreeze(ctx, vdSnapshot.Name, vm) + canUnfreeze, err = h.snapshotter.CanUnfreezeWithVirtualDiskSnapshot(ctx, vdSnapshot.Name, vm) if err != nil { setPhaseConditionToFailed(&condition, &vdSnapshot.Status.Phase, err) return reconcile.Result{}, err diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle_test.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle_test.go index 56bd06717..7c54672e1 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle_test.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle_test.go @@ -165,6 +165,8 @@ var _ = Describe("LifeCycle handler", func() { var vm *virtv2.VirtualMachine BeforeEach(func() { + vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhaseInProgress + vm = &virtv2.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{Name: "vm"}, Status: virtv2.VirtualMachineStatus{ @@ -185,7 +187,7 @@ var _ = Describe("LifeCycle handler", func() { snapshotter.FreezeFunc = func(_ context.Context, _, _ string) error { return nil } - snapshotter.CanUnfreezeFunc = func(_ context.Context, _ string, _ *virtv2.VirtualMachine) (bool, error) { + snapshotter.CanUnfreezeWithVirtualDiskSnapshotFunc = func(_ context.Context, _ string, _ *virtv2.VirtualMachine) (bool, error) { return true, nil } snapshotter.UnfreezeFunc = func(_ context.Context, _, _ string) error { diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go index 0b39d6b23..b5e9a0e21 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go @@ -102,8 +102,8 @@ var _ LifeCycleSnapshotter = &LifeCycleSnapshotterMock{} // CanFreezeFunc: func(vm *virtv2.VirtualMachine) bool { // panic("mock out the CanFreeze method") // }, -// CanUnfreezeFunc: func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { -// panic("mock out the CanUnfreeze method") +// CanUnfreezeWithVirtualDiskSnapshotFunc: func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { +// panic("mock out the CanUnfreezeWithVirtualDiskSnapshot method") // }, // CreateVolumeSnapshotFunc: func(ctx context.Context, vs *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error) { // panic("mock out the CreateVolumeSnapshot method") @@ -139,8 +139,8 @@ type LifeCycleSnapshotterMock struct { // CanFreezeFunc mocks the CanFreeze method. CanFreezeFunc func(vm *virtv2.VirtualMachine) bool - // CanUnfreezeFunc mocks the CanUnfreeze method. - CanUnfreezeFunc func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) + // CanUnfreezeWithVirtualDiskSnapshotFunc mocks the CanUnfreezeWithVirtualDiskSnapshot method. + CanUnfreezeWithVirtualDiskSnapshotFunc func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) // CreateVolumeSnapshotFunc mocks the CreateVolumeSnapshot method. CreateVolumeSnapshotFunc func(ctx context.Context, vs *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error) @@ -173,8 +173,8 @@ type LifeCycleSnapshotterMock struct { // VM is the vm argument value. VM *virtv2.VirtualMachine } - // CanUnfreeze holds details about calls to the CanUnfreeze method. - CanUnfreeze []struct { + // CanUnfreezeWithVirtualDiskSnapshot holds details about calls to the CanUnfreezeWithVirtualDiskSnapshot method. + CanUnfreezeWithVirtualDiskSnapshot []struct { // Ctx is the ctx argument value. Ctx context.Context // VdSnapshotName is the vdSnapshotName argument value. @@ -249,16 +249,16 @@ type LifeCycleSnapshotterMock struct { Namespace string } } - lockCanFreeze sync.RWMutex - lockCanUnfreeze sync.RWMutex - lockCreateVolumeSnapshot sync.RWMutex - lockFreeze sync.RWMutex - lockGetPersistentVolumeClaim sync.RWMutex - lockGetVirtualDisk sync.RWMutex - lockGetVirtualMachine sync.RWMutex - lockGetVolumeSnapshot sync.RWMutex - lockIsFrozen sync.RWMutex - lockUnfreeze sync.RWMutex + lockCanFreeze sync.RWMutex + lockCanUnfreezeWithVirtualDiskSnapshot sync.RWMutex + lockCreateVolumeSnapshot sync.RWMutex + lockFreeze sync.RWMutex + lockGetPersistentVolumeClaim sync.RWMutex + lockGetVirtualDisk sync.RWMutex + lockGetVirtualMachine sync.RWMutex + lockGetVolumeSnapshot sync.RWMutex + lockIsFrozen sync.RWMutex + lockUnfreeze sync.RWMutex } // CanFreeze calls CanFreezeFunc. @@ -293,10 +293,10 @@ func (mock *LifeCycleSnapshotterMock) CanFreezeCalls() []struct { return calls } -// CanUnfreeze calls CanUnfreezeFunc. -func (mock *LifeCycleSnapshotterMock) CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { - if mock.CanUnfreezeFunc == nil { - panic("LifeCycleSnapshotterMock.CanUnfreezeFunc: method is nil but LifeCycleSnapshotter.CanUnfreeze was just called") +// CanUnfreezeWithVirtualDiskSnapshot calls CanUnfreezeWithVirtualDiskSnapshotFunc. +func (mock *LifeCycleSnapshotterMock) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { + if mock.CanUnfreezeWithVirtualDiskSnapshotFunc == nil { + panic("LifeCycleSnapshotterMock.CanUnfreezeWithVirtualDiskSnapshotFunc: method is nil but LifeCycleSnapshotter.CanUnfreezeWithVirtualDiskSnapshot was just called") } callInfo := struct { Ctx context.Context @@ -307,17 +307,17 @@ func (mock *LifeCycleSnapshotterMock) CanUnfreeze(ctx context.Context, vdSnapsho VdSnapshotName: vdSnapshotName, VM: vm, } - mock.lockCanUnfreeze.Lock() - mock.calls.CanUnfreeze = append(mock.calls.CanUnfreeze, callInfo) - mock.lockCanUnfreeze.Unlock() - return mock.CanUnfreezeFunc(ctx, vdSnapshotName, vm) + mock.lockCanUnfreezeWithVirtualDiskSnapshot.Lock() + mock.calls.CanUnfreezeWithVirtualDiskSnapshot = append(mock.calls.CanUnfreezeWithVirtualDiskSnapshot, callInfo) + mock.lockCanUnfreezeWithVirtualDiskSnapshot.Unlock() + return mock.CanUnfreezeWithVirtualDiskSnapshotFunc(ctx, vdSnapshotName, vm) } -// CanUnfreezeCalls gets all the calls that were made to CanUnfreeze. +// CanUnfreezeWithVirtualDiskSnapshotCalls gets all the calls that were made to CanUnfreezeWithVirtualDiskSnapshot. // Check the length with: // -// len(mockedLifeCycleSnapshotter.CanUnfreezeCalls()) -func (mock *LifeCycleSnapshotterMock) CanUnfreezeCalls() []struct { +// len(mockedLifeCycleSnapshotter.CanUnfreezeWithVirtualDiskSnapshotCalls()) +func (mock *LifeCycleSnapshotterMock) CanUnfreezeWithVirtualDiskSnapshotCalls() []struct { Ctx context.Context VdSnapshotName string VM *virtv2.VirtualMachine @@ -327,9 +327,9 @@ func (mock *LifeCycleSnapshotterMock) CanUnfreezeCalls() []struct { VdSnapshotName string VM *virtv2.VirtualMachine } - mock.lockCanUnfreeze.RLock() - calls = mock.calls.CanUnfreeze - mock.lockCanUnfreeze.RUnlock() + mock.lockCanUnfreezeWithVirtualDiskSnapshot.RLock() + calls = mock.calls.CanUnfreezeWithVirtualDiskSnapshot + mock.lockCanUnfreezeWithVirtualDiskSnapshot.RUnlock() return calls } diff --git a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/interfaces.go b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/interfaces.go index 0bec5f2f8..fa9d2408e 100644 --- a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/interfaces.go +++ b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/interfaces.go @@ -41,5 +41,5 @@ type Snapshotter interface { Unfreeze(ctx context.Context, name, namespace string) error IsFrozen(vm *virtv2.VirtualMachine) bool CanFreeze(vm *virtv2.VirtualMachine) bool - CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) + CanUnfreezeWithVirtualMachineSnapshot(ctx context.Context, vmSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) } diff --git a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle.go b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle.go index bbcacbd11..78e6923fe 100644 --- a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle.go +++ b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle.go @@ -59,6 +59,12 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual cb.Status(metav1.ConditionUnknown).Reason(vmscondition.VirtualMachineSnapshotUnknown) } + vm, err := h.snapshotter.GetVirtualMachine(ctx, vmSnapshot.Spec.VirtualMachineName, vmSnapshot.Namespace) + if err != nil { + setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) + return reconcile.Result{}, err + } + if vmSnapshot.DeletionTimestamp != nil { vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhaseTerminating cb. @@ -66,12 +72,26 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual Reason(vmscondition.VirtualMachineSnapshotUnknown). Message("") + _, err = h.unfreezeVirtualMachineIfCan(ctx, vmSnapshot, vm) + if err != nil { + setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) + return reconcile.Result{}, err + } + return reconcile.Result{}, nil } switch vmSnapshot.Status.Phase { case "": vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhasePending + case virtv2.VirtualMachineSnapshotPhaseFailed: + readyCondition, _ := conditions.GetCondition(vmscondition.VirtualMachineSnapshotReadyType, vmSnapshot.Status.Conditions) + cb. + Status(readyCondition.Status). + //nolint:staticcheck + Reason(conditions.DeprecatedWrappedString(readyCondition.Reason)). + Message(readyCondition.Message) + return reconcile.Result{}, nil case virtv2.VirtualMachineSnapshotPhaseReady: // Ensure vd snapshots aren't lost. var lostVDSnapshots []string @@ -109,12 +129,6 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual return reconcile.Result{}, nil } - vm, err := h.snapshotter.GetVirtualMachine(ctx, vmSnapshot.Spec.VirtualMachineName, vmSnapshot.Namespace) - if err != nil { - setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) - return reconcile.Result{}, err - } - virtualMachineReadyCondition, _ := conditions.GetCondition(vmscondition.VirtualMachineReadyType, vmSnapshot.Status.Conditions) if vm == nil || virtualMachineReadyCondition.Status != metav1.ConditionTrue { vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhasePending @@ -154,26 +168,40 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual return reconcile.Result{}, nil } + needToFreeze := h.needToFreeze(vm) + + isAwaitingConsistency := needToFreeze && !h.snapshotter.CanFreeze(vm) && vmSnapshot.Spec.RequiredConsistency + if isAwaitingConsistency { + vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhasePending + cb. + Status(metav1.ConditionFalse). + Reason(vmscondition.PotentiallyInconsistent). + Message(fmt.Sprintf( + "The snapshotting of virtual machine %q might result in an inconsistent snapshot: "+ + "waiting for the virtual machine to be %s", + vm.Name, virtv2.MachineStopped, + )) + return reconcile.Result{}, nil + } + + if vmSnapshot.Status.Phase == virtv2.VirtualMachineSnapshotPhasePending { + vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhaseInProgress + cb. + Status(metav1.ConditionFalse). + Reason(vmscondition.FileSystemFreezing). + Message("The snapshotting process has started.") + return reconcile.Result{Requeue: true}, nil + } + + var hasFrozen bool + // 3. Ensure the virtual machine is consistent for snapshotting. - hasFrozen, err := h.freezeVirtualMachineIfCan(ctx, vm) - switch { - case err == nil: - case errors.Is(err, ErrPotentiallyInconsistent): - if vmSnapshot.Spec.RequiredConsistency { - vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhasePending - cb. - Status(metav1.ConditionFalse). - Reason(vmscondition.PotentiallyInconsistent). - Message(fmt.Sprintf( - "The snapshotting of virtual machine %q might result in an inconsistent snapshot: "+ - "waiting for the virtual machine to be %s", - vm.Name, virtv2.MachineStopped, - )) - return reconcile.Result{}, nil + if needToFreeze { + hasFrozen, err = h.freezeVirtualMachine(ctx, vm) + if err != nil { + setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) + return reconcile.Result{}, err } - default: - setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) - return reconcile.Result{}, err } if hasFrozen { @@ -233,12 +261,13 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual return reconcile.Result{}, nil } - if vm.Status.Phase == virtv2.MachineStopped || h.snapshotter.IsFrozen(vm) { - vmSnapshot.Status.Consistent = ptr.To(true) + vmSnapshot.Status.Consistent = ptr.To(true) + if !h.areVirtualDiskSnapshotsConsistent(vdSnapshots) { + vmSnapshot.Status.Consistent = nil } // 8. Unfreeze VirtualMachine if can. - unfrozen, err := h.unfreezeVirtualMachineIfCan(ctx, vm) + unfrozen, err := h.unfreezeVirtualMachineIfCan(ctx, vmSnapshot, vm) if err != nil { setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) return reconcile.Result{}, err @@ -374,39 +403,47 @@ func (h LifeCycleHandler) countReadyVirtualDiskSnapshots(vdSnapshots []*virtv2.V return readyCount } -var ErrPotentiallyInconsistent = errors.New("potentially inconsistent") +func (h LifeCycleHandler) areVirtualDiskSnapshotsConsistent(vdSnapshots []*virtv2.VirtualDiskSnapshot) bool { + for _, vdSnapshot := range vdSnapshots { + if vdSnapshot.Status.Consistent == nil || !*vdSnapshot.Status.Consistent { + return false + } + } -func (h LifeCycleHandler) freezeVirtualMachineIfCan(ctx context.Context, vm *virtv2.VirtualMachine) (bool, error) { - switch vm.Status.Phase { - case virtv2.MachineStopped: - return false, nil - case virtv2.MachineRunning: - default: - return false, errors.New("cannot freeze not Running virtual machine") + return true +} + +func (h LifeCycleHandler) needToFreeze(vm *virtv2.VirtualMachine) bool { + if vm.Status.Phase == virtv2.MachineStopped { + return false } if h.snapshotter.IsFrozen(vm) { - return false, nil + return false } - if !h.snapshotter.CanFreeze(vm) { - return false, ErrPotentiallyInconsistent + return true +} + +func (h LifeCycleHandler) freezeVirtualMachine(ctx context.Context, vm *virtv2.VirtualMachine) (bool, error) { + if vm.Status.Phase != virtv2.MachineRunning { + return false, errors.New("cannot freeze not Running virtual machine") } err := h.snapshotter.Freeze(ctx, vm.Name, vm.Namespace) if err != nil { - return false, err + return false, fmt.Errorf("freeze the virtual machine %q: %w", vm.Name, err) } return true, nil } -func (h LifeCycleHandler) unfreezeVirtualMachineIfCan(ctx context.Context, vm *virtv2.VirtualMachine) (bool, error) { - if vm.Status.Phase != virtv2.MachineRunning || !h.snapshotter.IsFrozen(vm) { +func (h LifeCycleHandler) unfreezeVirtualMachineIfCan(ctx context.Context, vmSnapshot *virtv2.VirtualMachineSnapshot, vm *virtv2.VirtualMachine) (bool, error) { + if vm == nil || vm.Status.Phase != virtv2.MachineRunning || !h.snapshotter.IsFrozen(vm) { return false, nil } - canUnfreeze, err := h.snapshotter.CanUnfreeze(ctx, "", vm) + canUnfreeze, err := h.snapshotter.CanUnfreezeWithVirtualMachineSnapshot(ctx, vmSnapshot.Name, vm) if err != nil { return false, err } @@ -417,7 +454,7 @@ func (h LifeCycleHandler) unfreezeVirtualMachineIfCan(ctx context.Context, vm *v err = h.snapshotter.Unfreeze(ctx, vm.Name, vm.Namespace) if err != nil { - return false, err + return false, fmt.Errorf("unfreeze the virtual machine %q: %w", vm.Name, err) } return true, nil diff --git a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle_test.go b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle_test.go index 2107ae9f0..9803e2ed0 100644 --- a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle_test.go +++ b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/life_cycle_test.go @@ -113,7 +113,8 @@ var _ = Describe("LifeCycle handler", func() { vdSnapshot = &virtv2.VirtualDiskSnapshot{ ObjectMeta: metav1.ObjectMeta{Name: getVDSnapshotName(vd.Name, vmSnapshot)}, Status: virtv2.VirtualDiskSnapshotStatus{ - Phase: virtv2.VirtualDiskSnapshotPhaseReady, + Phase: virtv2.VirtualDiskSnapshotPhaseReady, + Consistent: ptr.To(true), }, } @@ -127,9 +128,12 @@ var _ = Describe("LifeCycle handler", func() { IsFrozenFunc: func(_ *virtv2.VirtualMachine) bool { return true }, - CanUnfreezeFunc: func(_ context.Context, _ string, _ *virtv2.VirtualMachine) (bool, error) { + CanUnfreezeWithVirtualMachineSnapshotFunc: func(_ context.Context, _ string, _ *virtv2.VirtualMachine) (bool, error) { return true, nil }, + CanFreezeFunc: func(_ *virtv2.VirtualMachine) bool { + return false + }, UnfreezeFunc: func(ctx context.Context, _, _ string) error { return nil }, @@ -279,6 +283,10 @@ var _ = Describe("LifeCycle handler", func() { }) Context("The virtual machine snapshot is Ready", func() { + BeforeEach(func() { + vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhaseInProgress + }) + It("The snapshot of virtual machine is Ready", func() { h := NewLifeCycleHandler(snapshotter, storer) @@ -315,11 +323,9 @@ var _ = Describe("LifeCycle handler", func() { It("The virtual machine snapshot is potentially inconsistent", func() { vmSnapshot.Spec.RequiredConsistency = false - snapshotter.IsFrozenFunc = func(_ *virtv2.VirtualMachine) bool { - return false - } - snapshotter.CanFreezeFunc = func(_ *virtv2.VirtualMachine) bool { - return false + snapshotter.GetVirtualDiskSnapshotFunc = func(_ context.Context, _, _ string) (*virtv2.VirtualDiskSnapshot, error) { + vdSnapshot.Status.Consistent = nil + return vdSnapshot, nil } h := NewLifeCycleHandler(snapshotter, storer) diff --git a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/mock.go b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/mock.go index bd606db38..90c1cab63 100644 --- a/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/mock.go +++ b/images/virtualization-artifact/pkg/controller/vmsnapshot/internal/mock.go @@ -101,8 +101,8 @@ var _ Snapshotter = &SnapshotterMock{} // CanFreezeFunc: func(vm *virtv2.VirtualMachine) bool { // panic("mock out the CanFreeze method") // }, -// CanUnfreezeFunc: func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { -// panic("mock out the CanUnfreeze method") +// CanUnfreezeWithVirtualMachineSnapshotFunc: func(ctx context.Context, vmSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { +// panic("mock out the CanUnfreezeWithVirtualMachineSnapshot method") // }, // CreateVirtualDiskSnapshotFunc: func(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot) (*virtv2.VirtualDiskSnapshot, error) { // panic("mock out the CreateVirtualDiskSnapshot method") @@ -141,8 +141,8 @@ type SnapshotterMock struct { // CanFreezeFunc mocks the CanFreeze method. CanFreezeFunc func(vm *virtv2.VirtualMachine) bool - // CanUnfreezeFunc mocks the CanUnfreeze method. - CanUnfreezeFunc func(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) + // CanUnfreezeWithVirtualMachineSnapshotFunc mocks the CanUnfreezeWithVirtualMachineSnapshot method. + CanUnfreezeWithVirtualMachineSnapshotFunc func(ctx context.Context, vmSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) // CreateVirtualDiskSnapshotFunc mocks the CreateVirtualDiskSnapshot method. CreateVirtualDiskSnapshotFunc func(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot) (*virtv2.VirtualDiskSnapshot, error) @@ -178,12 +178,12 @@ type SnapshotterMock struct { // VM is the vm argument value. VM *virtv2.VirtualMachine } - // CanUnfreeze holds details about calls to the CanUnfreeze method. - CanUnfreeze []struct { + // CanUnfreezeWithVirtualMachineSnapshot holds details about calls to the CanUnfreezeWithVirtualMachineSnapshot method. + CanUnfreezeWithVirtualMachineSnapshot []struct { // Ctx is the ctx argument value. Ctx context.Context - // VdSnapshotName is the vdSnapshotName argument value. - VdSnapshotName string + // VmSnapshotName is the vmSnapshotName argument value. + VmSnapshotName string // VM is the vm argument value. VM *virtv2.VirtualMachine } @@ -263,17 +263,17 @@ type SnapshotterMock struct { Namespace string } } - lockCanFreeze sync.RWMutex - lockCanUnfreeze sync.RWMutex - lockCreateVirtualDiskSnapshot sync.RWMutex - lockFreeze sync.RWMutex - lockGetPersistentVolumeClaim sync.RWMutex - lockGetSecret sync.RWMutex - lockGetVirtualDisk sync.RWMutex - lockGetVirtualDiskSnapshot sync.RWMutex - lockGetVirtualMachine sync.RWMutex - lockIsFrozen sync.RWMutex - lockUnfreeze sync.RWMutex + lockCanFreeze sync.RWMutex + lockCanUnfreezeWithVirtualMachineSnapshot sync.RWMutex + lockCreateVirtualDiskSnapshot sync.RWMutex + lockFreeze sync.RWMutex + lockGetPersistentVolumeClaim sync.RWMutex + lockGetSecret sync.RWMutex + lockGetVirtualDisk sync.RWMutex + lockGetVirtualDiskSnapshot sync.RWMutex + lockGetVirtualMachine sync.RWMutex + lockIsFrozen sync.RWMutex + lockUnfreeze sync.RWMutex } // CanFreeze calls CanFreezeFunc. @@ -308,43 +308,43 @@ func (mock *SnapshotterMock) CanFreezeCalls() []struct { return calls } -// CanUnfreeze calls CanUnfreezeFunc. -func (mock *SnapshotterMock) CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { - if mock.CanUnfreezeFunc == nil { - panic("SnapshotterMock.CanUnfreezeFunc: method is nil but Snapshotter.CanUnfreeze was just called") +// CanUnfreezeWithVirtualMachineSnapshot calls CanUnfreezeWithVirtualMachineSnapshotFunc. +func (mock *SnapshotterMock) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Context, vmSnapshotName string, vm *virtv2.VirtualMachine) (bool, error) { + if mock.CanUnfreezeWithVirtualMachineSnapshotFunc == nil { + panic("SnapshotterMock.CanUnfreezeWithVirtualMachineSnapshotFunc: method is nil but Snapshotter.CanUnfreezeWithVirtualMachineSnapshot was just called") } callInfo := struct { Ctx context.Context - VdSnapshotName string + VmSnapshotName string VM *virtv2.VirtualMachine }{ Ctx: ctx, - VdSnapshotName: vdSnapshotName, + VmSnapshotName: vmSnapshotName, VM: vm, } - mock.lockCanUnfreeze.Lock() - mock.calls.CanUnfreeze = append(mock.calls.CanUnfreeze, callInfo) - mock.lockCanUnfreeze.Unlock() - return mock.CanUnfreezeFunc(ctx, vdSnapshotName, vm) + mock.lockCanUnfreezeWithVirtualMachineSnapshot.Lock() + mock.calls.CanUnfreezeWithVirtualMachineSnapshot = append(mock.calls.CanUnfreezeWithVirtualMachineSnapshot, callInfo) + mock.lockCanUnfreezeWithVirtualMachineSnapshot.Unlock() + return mock.CanUnfreezeWithVirtualMachineSnapshotFunc(ctx, vmSnapshotName, vm) } -// CanUnfreezeCalls gets all the calls that were made to CanUnfreeze. +// CanUnfreezeWithVirtualMachineSnapshotCalls gets all the calls that were made to CanUnfreezeWithVirtualMachineSnapshot. // Check the length with: // -// len(mockedSnapshotter.CanUnfreezeCalls()) -func (mock *SnapshotterMock) CanUnfreezeCalls() []struct { +// len(mockedSnapshotter.CanUnfreezeWithVirtualMachineSnapshotCalls()) +func (mock *SnapshotterMock) CanUnfreezeWithVirtualMachineSnapshotCalls() []struct { Ctx context.Context - VdSnapshotName string + VmSnapshotName string VM *virtv2.VirtualMachine } { var calls []struct { Ctx context.Context - VdSnapshotName string + VmSnapshotName string VM *virtv2.VirtualMachine } - mock.lockCanUnfreeze.RLock() - calls = mock.calls.CanUnfreeze - mock.lockCanUnfreeze.RUnlock() + mock.lockCanUnfreezeWithVirtualMachineSnapshot.RLock() + calls = mock.calls.CanUnfreezeWithVirtualMachineSnapshot + mock.lockCanUnfreezeWithVirtualMachineSnapshot.RUnlock() return calls }