From 48626ac0292cc39c4d546c77238b5fffa0fb768a Mon Sep 17 00:00:00 2001 From: Isteb4k Date: Thu, 19 Dec 2024 12:39:51 +0100 Subject: [PATCH] fix(vdsnapshot,vmsnapshot): unfreeze virtual machines Signed-off-by: Isteb4k --- .../controller/service/snapshot_service.go | 62 +++++++-- .../vdsnapshot/internal/deletion.go | 19 ++- .../vdsnapshot/internal/interfaces.go | 2 +- .../vdsnapshot/internal/life_cycle.go | 29 +++- .../vdsnapshot/internal/life_cycle_test.go | 4 +- .../controller/vdsnapshot/internal/mock.go | 60 ++++----- .../pkg/controller/vm/internal/filesystem.go | 5 +- .../vm/internal/sync_power_state.go | 2 +- .../vmsnapshot/internal/interfaces.go | 2 +- .../vmsnapshot/internal/life_cycle.go | 125 +++++++++++------- .../vmsnapshot/internal/life_cycle_test.go | 20 ++- .../controller/vmsnapshot/internal/mock.go | 74 +++++------ 12 files changed, 246 insertions(+), 158 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/service/snapshot_service.go b/images/virtualization-artifact/pkg/controller/service/snapshot_service.go index 0eb764778..e1aced51e 100644 --- a/images/virtualization-artifact/pkg/controller/service/snapshot_service.go +++ b/images/virtualization-artifact/pkg/controller/service/snapshot_service.go @@ -78,7 +78,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 } @@ -104,10 +104,7 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string } _, ok := vdByName[vdSnapshot.Spec.VirtualDiskName] - if ok && - vdSnapshot.Status.Phase != virtv2.VirtualDiskSnapshotPhaseReady && - vdSnapshot.Status.Phase != virtv2.VirtualDiskSnapshotPhaseFailed && - vdSnapshot.Status.Phase != virtv2.VirtualDiskSnapshotPhaseTerminating { + if ok && vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhaseInProgress { return false, nil } } @@ -121,10 +118,55 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string } for _, vmSnapshot := range vmSnapshots.Items { - if vmSnapshot.Spec.VirtualMachineName == vm.Name && - vmSnapshot.Status.Phase != virtv2.VirtualMachineSnapshotPhaseReady && - vmSnapshot.Status.Phase != virtv2.VirtualMachineSnapshotPhaseFailed && - vmSnapshot.Status.Phase != virtv2.VirtualMachineSnapshotPhaseTerminating { + 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 } } @@ -135,7 +177,7 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string func (s *SnapshotService) Unfreeze(ctx context.Context, name, namespace string) error { err := s.virtClient.VirtualMachines(namespace).Unfreeze(ctx, name) if err != nil { - return fmt.Errorf("failed to unfreeze internal virtual machine %s/%s: %w", namespace, name, err) + return fmt.Errorf("unfreeze virtual machine %s/%s: %w", namespace, name, err) } return nil diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go index 58b707512..ee7e69b8e 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/deletion.go @@ -53,24 +53,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 } @@ -83,11 +83,8 @@ func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *virtv2.VirtualD } } - if requeue { - return reconcile.Result{Requeue: true}, nil - } - log.Info("Deletion observed: remove cleanup finalizer from VirtualDiskSnapshot") + 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 a0fc9fc1b..5ee40020b 100644 --- a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go +++ b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/life_cycle.go @@ -82,15 +82,12 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual switch vdSnapshot.Status.Phase { case "": vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhasePending - case virtv2.VirtualDiskSnapshotPhaseFailed: - vdSnapshotCondition, _ := conditions.GetCondition(vdscondition.VirtualDiskSnapshotReadyType, vdSnapshot.Status.Conditions) - + readyCondition, _ := conditions.GetCondition(vdscondition.VirtualDiskSnapshotReadyType, vdSnapshot.Status.Conditions) cb. Status(metav1.ConditionFalse). - Reason(conditions.CommonReason(vdSnapshotCondition.Reason)). - Message(vdSnapshotCondition.Message) - + Reason(conditions.CommonReason(readyCondition.Reason)). + Message(readyCondition.Message) return reconcile.Result{}, nil case virtv2.VirtualDiskSnapshotPhaseReady: if vs == nil || vs.Status == nil || vs.Status.ReadyToUse == nil || !*vs.Status.ReadyToUse { @@ -144,6 +141,15 @@ 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 + cb. + Status(metav1.ConditionFalse). + Reason(vdscondition.Snapshotting). + 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(cb, &vdSnapshot.Status.Phase, err) @@ -176,6 +182,15 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual } } + if vdSnapshot.Status.Phase == virtv2.VirtualDiskSnapshotPhasePending { + vdSnapshot.Status.Phase = virtv2.VirtualDiskSnapshotPhaseInProgress + cb. + Status(metav1.ConditionFalse). + Reason(vdscondition.Snapshotting). + 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) @@ -252,7 +267,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(cb, &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 661e987f5..753fca950 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/vm/internal/filesystem.go b/images/virtualization-artifact/pkg/controller/vm/internal/filesystem.go index 2b78f81ba..d6dd6d39e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/filesystem.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/filesystem.go @@ -18,7 +18,6 @@ package internal import ( "context" - "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -65,7 +64,7 @@ func (h *FilesystemHandler) Handle(ctx context.Context, s state.VirtualMachineSt if kvvmi == nil { cb.Status(metav1.ConditionFalse). Reason(vmcondition.ReasonFilesystemNotReady). - Message(fmt.Sprintf("The internal virtual machine %q is not running.", changed.Name)) + Message("The virtual machine is not running.") return reconcile.Result{}, nil } @@ -78,7 +77,7 @@ func (h *FilesystemHandler) Handle(ctx context.Context, s state.VirtualMachineSt if kvvmi.Status.FSFreezeStatus == "frozen" { cb.Status(metav1.ConditionFalse). Reason(vmcondition.ReasonFilesystemFrozen). - Message(fmt.Sprintf("The internal virtual machine %q is frozen.", changed.Name)) + Message("The virtual machine is frozen.") return reconcile.Result{}, nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_power_state.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_power_state.go index 0482f6b59..6871c84ff 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_power_state.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_power_state.go @@ -77,7 +77,7 @@ func (h *SyncPowerStateHandler) syncPowerState(ctx context.Context, s state.Virt kvvmi, err := s.KVVMI(ctx) if err != nil { - return fmt.Errorf("find the internal virtual machine instance: %w", err) + return fmt.Errorf("find the virtual machine instance: %w", err) } if runPolicy == virtv2.AlwaysOnUnlessStoppedManually { 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 971996082..853d0fb6e 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(conditions.ReasonUnknown) } + 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,6 +72,12 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual Reason(conditions.ReasonUnknown). Message("") + _, err = h.unfreezeVirtualMachineIfCan(ctx, vmSnapshot, vm) + if err != nil { + setPhaseConditionToFailed(cb, &vmSnapshot.Status.Phase, err) + return reconcile.Result{}, err + } + return reconcile.Result{}, nil } @@ -73,13 +85,11 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmSnapshot *virtv2.Virtual case "": vmSnapshot.Status.Phase = virtv2.VirtualMachineSnapshotPhasePending case virtv2.VirtualMachineSnapshotPhaseFailed: - vmSnapshotCondition, _ := conditions.GetCondition(vmscondition.VirtualMachineSnapshotReadyType, vmSnapshot.Status.Conditions) - + readyCondition, _ := conditions.GetCondition(vmscondition.VirtualMachineSnapshotReadyType, vmSnapshot.Status.Conditions) cb. - Status(metav1.ConditionFalse). - Reason(conditions.CommonReason(vmSnapshotCondition.Reason)). - Message(vmSnapshotCondition.Message) - + Status(readyCondition.Status). + Reason(conditions.CommonReason(readyCondition.Reason)). + Message(readyCondition.Message) return reconcile.Result{}, nil case virtv2.VirtualMachineSnapshotPhaseReady: // Ensure vd snapshots aren't lost. @@ -118,12 +128,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 @@ -163,26 +167,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 { @@ -242,12 +260,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 @@ -382,39 +401,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 } @@ -425,7 +452,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 e846ae453..1d8acebca 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 @@ -112,7 +112,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), }, } @@ -126,9 +127,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 }, @@ -278,6 +282,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) @@ -314,11 +322,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 }