chore(api): block migration during snapshotting and snapshotting during migration

images/virtualization-artifact/pkg/controller/vdsnapshot/internal/virtual_disk_ready.go

@@ -18,24 +18,32 @@ package internal
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	virtv1 "kubevirt.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
+	"github.com/deckhouse/virtualization-controller/pkg/common/object"
 	"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vdcondition"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vdscondition"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
 )
 
 type VirtualDiskReadyHandler struct {
 	snapshotter VirtualDiskReadySnapshotter
+	client      client.Client
 }
 
-func NewVirtualDiskReadyHandler(snapshotter VirtualDiskReadySnapshotter) *VirtualDiskReadyHandler {
+func NewVirtualDiskReadyHandler(snapshotter VirtualDiskReadySnapshotter, client client.Client) *VirtualDiskReadyHandler {
 	return &VirtualDiskReadyHandler{
 		snapshotter: snapshotter,
+		client:      client,
 	}
 }
 
@@ -94,6 +102,18 @@ func (h VirtualDiskReadyHandler) Handle(ctx context.Context, vdSnapshot *v1alpha
 			return reconcile.Result{}, nil
 		}
 
+		attachedToMigratingVM, err := h.isVDAttachedToMigratingVM(ctx, vd)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+		if attachedToMigratingVM {
+			cb.
+				Status(metav1.ConditionFalse).
+				Reason(vdscondition.VirtualDiskNotReadyForSnapshotting).
+				Message("Snapshot cannot be taken: the virtual disk is attached to a migrating virtual machine.")
+			return reconcile.Result{}, nil
+		}
+
 		cb.
 			Status(metav1.ConditionTrue).
 			Reason(vdscondition.VirtualDiskReady).
@@ -107,3 +127,49 @@ func (h VirtualDiskReadyHandler) Handle(ctx context.Context, vdSnapshot *v1alpha
 		return reconcile.Result{}, nil
 	}
 }
+
+// isVDAttachedToMigratingVM checks that the disk is not attached to a migrating VM.
+// Returns (true, nil) if the disk is attached to a migrating VM (caller should return immediately).
+// Otherwise returns (false, nil) or (false, err).
+func (h VirtualDiskReadyHandler) isVDAttachedToMigratingVM(ctx context.Context, vd *v1alpha2.VirtualDisk) (bool, error) {
+	inUse, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
+	if inUse.Status != metav1.ConditionTrue {
+		return false, nil
+	}
+
+	var vmNames []string
+
+	for _, attachedVM := range vd.Status.AttachedToVirtualMachines {
+		if attachedVM.Mounted {
+			vmNames = append(vmNames, attachedVM.Name)
+		}
+	}
+
+	if len(vmNames) != 1 {
+		return false, errors.New("disk is in InUse state but the number of VMs it is attached to is not 1; this should not happen, please report a bug")
+	}
+
+	vmName := vmNames[0]
+	vm, err := object.FetchObject(ctx, types.NamespacedName{Name: vmName, Namespace: vd.Namespace}, h.client, &v1alpha2.VirtualMachine{})
+	if err != nil {
+		return false, err
+	}
+
+	_, migratingConditionExists := conditions.GetCondition(vmcondition.TypeMigrating, vm.Status.Conditions)
+	if migratingConditionExists {
+		return true, nil
+	}
+
+	kvvmi, err := object.FetchObject(ctx, types.NamespacedName{Name: vmName, Namespace: vd.Namespace}, h.client, &virtv1.VirtualMachineInstance{})
+	if err != nil {
+		return false, err
+	}
+
+	if kvvmi != nil && kvvmi.Status.MigrationState != nil &&
+		kvvmi.Status.MigrationState.StartTimestamp != nil &&
+		kvvmi.Status.MigrationState.EndTimestamp == nil {
+		return true, nil
+	}
+
+	return false, nil
+}

images/virtualization-artifact/pkg/controller/vdsnapshot/internal/virtual_disk_ready_test.go

@@ -23,19 +23,27 @@ import (
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/deckhouse/virtualization-controller/pkg/common/testutil"
 	"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vdcondition"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vdscondition"
+	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
 )
 
 var _ = Describe("VirtualDiskReady handler", func() {
 	var snapshotter *VirtualDiskReadySnapshotterMock
 	var vd *v1alpha2.VirtualDisk
 	var vdSnapshot *v1alpha2.VirtualDiskSnapshot
+	var fakeClient client.Client
 
 	BeforeEach(func() {
+		var err error
+		fakeClient, err = testutil.NewFakeClientWithObjects()
+		Expect(err).NotTo(HaveOccurred())
+
 		vd = &v1alpha2.VirtualDisk{
 			ObjectMeta: metav1.ObjectMeta{Name: "vd-01"},
 			Status: v1alpha2.VirtualDiskStatus{
@@ -61,9 +69,50 @@ var _ = Describe("VirtualDiskReady handler", func() {
 		}
 	})
 
+	Context("condition VirtualDiskReady is Unknown", func() {
+		It("The virtual disk snapshot is being deleted", func() {
+			vdSnapshot.DeletionTimestamp = ptr.To(metav1.Now())
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionUnknown))
+			Expect(ready.Reason).To(Equal(conditions.ReasonUnknown.String()))
+			Expect(ready.Message).To(BeEmpty())
+		})
+	})
+
 	Context("condition VirtualDiskReady is True", func() {
 		It("The virtual disk is ready for snapshotting", func() {
-			h := NewVirtualDiskReadyHandler(snapshotter)
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionTrue))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskReady.String()))
+			Expect(ready.Message).To(BeEmpty())
+		})
+
+		It("The virtual disk snapshot is already in Ready phase", func() {
+			vdSnapshot.Status.Phase = v1alpha2.VirtualDiskSnapshotPhaseReady
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionTrue))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskReady.String()))
+			Expect(ready.Message).To(BeEmpty())
+		})
+
+		It("The virtual disk has no snapshotting condition", func() {
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				vd.Status.Conditions = nil
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
 
 			_, err := h.Handle(testContext(), vdSnapshot)
 			Expect(err).To(BeNil())
@@ -72,14 +121,79 @@ var _ = Describe("VirtualDiskReady handler", func() {
 			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskReady.String()))
 			Expect(ready.Message).To(BeEmpty())
 		})
+
+		It("The virtual disk is InUse but attached VM is not migrating", func() {
+			const namespace = "default"
+			vmName := "vm-running"
+			vd.Namespace = namespace
+			vd.Status.Conditions = []metav1.Condition{
+				{Type: vdcondition.SnapshottingType.String(), Status: metav1.ConditionTrue},
+				{Type: vdcondition.InUseType.String(), Status: metav1.ConditionTrue},
+			}
+			vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{{Name: vmName, Mounted: true}}
+			vdSnapshot.Namespace = namespace
+
+			vm := &v1alpha2.VirtualMachine{
+				ObjectMeta: metav1.ObjectMeta{Name: vmName, Namespace: namespace},
+				Status:     v1alpha2.VirtualMachineStatus{},
+			}
+			fakeClient, err := testutil.NewFakeClientWithObjects(vm)
+			Expect(err).NotTo(HaveOccurred())
+
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err = h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionTrue))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskReady.String()))
+			Expect(ready.Message).To(BeEmpty())
+		})
+
+		It("The virtual disk has multiple attached VMs but only one is mounted and it is not migrating", func() {
+			const namespace = "default"
+			vmName := "vm-mounted"
+			vd.Namespace = namespace
+			vd.Status.Conditions = []metav1.Condition{
+				{Type: vdcondition.SnapshottingType.String(), Status: metav1.ConditionTrue},
+				{Type: vdcondition.InUseType.String(), Status: metav1.ConditionTrue},
+			}
+			vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{
+				{Name: "vm-unmounted", Mounted: false},
+				{Name: vmName, Mounted: true},
+			}
+			vdSnapshot.Namespace = namespace
+
+			vm := &v1alpha2.VirtualMachine{
+				ObjectMeta: metav1.ObjectMeta{Name: vmName, Namespace: namespace},
+				Status:     v1alpha2.VirtualMachineStatus{},
+			}
+			fakeClient, err := testutil.NewFakeClientWithObjects(vm)
+			Expect(err).NotTo(HaveOccurred())
+
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err = h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionTrue))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskReady.String()))
+			Expect(ready.Message).To(BeEmpty())
+		})
 	})
 
 	Context("condition VirtualDiskReady is False", func() {
 		It("The virtual disk not found", func() {
 			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
 				return nil, nil
 			}
-			h := NewVirtualDiskReadyHandler(snapshotter)
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
 
 			_, err := h.Handle(testContext(), vdSnapshot)
 			Expect(err).To(BeNil())
@@ -94,7 +208,7 @@ var _ = Describe("VirtualDiskReady handler", func() {
 				vd.DeletionTimestamp = ptr.To(metav1.Now())
 				return vd, nil
 			}
-			h := NewVirtualDiskReadyHandler(snapshotter)
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
 
 			_, err := h.Handle(testContext(), vdSnapshot)
 			Expect(err).To(BeNil())
@@ -109,7 +223,7 @@ var _ = Describe("VirtualDiskReady handler", func() {
 				vd.Status.Phase = v1alpha2.DiskPending
 				return vd, nil
 			}
-			h := NewVirtualDiskReadyHandler(snapshotter)
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
 
 			_, err := h.Handle(testContext(), vdSnapshot)
 			Expect(err).To(BeNil())
@@ -130,7 +244,7 @@ var _ = Describe("VirtualDiskReady handler", func() {
 				})
 				return vd, nil
 			}
-			h := NewVirtualDiskReadyHandler(snapshotter)
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
 
 			_, err := h.Handle(testContext(), vdSnapshot)
 			Expect(err).To(BeNil())
@@ -139,5 +253,108 @@ var _ = Describe("VirtualDiskReady handler", func() {
 			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskNotReadyForSnapshotting.String()))
 			Expect(ready.Message).ToNot(BeEmpty())
 		})
+
+		It("The virtual disk snapshotting condition is stale", func() {
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				vd.Generation = 2
+				vd.Status.Conditions = []metav1.Condition{
+					{
+						Type:               vdcondition.SnapshottingType.String(),
+						Status:             metav1.ConditionTrue,
+						ObservedGeneration: 1,
+					},
+				}
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionFalse))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskNotReadyForSnapshotting.String()))
+		})
+
+		It("The virtual disk is attached to a migrating VM", func() {
+			const namespace = "default"
+			vmName := "vm-migrating"
+			vd.Namespace = namespace
+			vd.Status.Conditions = []metav1.Condition{
+				{Type: vdcondition.SnapshottingType.String(), Status: metav1.ConditionTrue},
+				{Type: vdcondition.InUseType.String(), Status: metav1.ConditionTrue},
+			}
+			vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{{Name: vmName, Mounted: true}}
+			vdSnapshot.Namespace = namespace
+
+			vm := &v1alpha2.VirtualMachine{
+				ObjectMeta: metav1.ObjectMeta{Name: vmName, Namespace: namespace},
+				Status: v1alpha2.VirtualMachineStatus{
+					Conditions: []metav1.Condition{
+						{Type: vmcondition.TypeMigrating.String(), Status: metav1.ConditionTrue},
+					},
+				},
+			}
+			fakeClient, err := testutil.NewFakeClientWithObjects(vm)
+			Expect(err).NotTo(HaveOccurred())
+
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err = h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(BeNil())
+			ready, _ := conditions.GetCondition(vdscondition.VirtualDiskReadyType, vdSnapshot.Status.Conditions)
+			Expect(ready.Status).To(Equal(metav1.ConditionFalse))
+			Expect(ready.Reason).To(Equal(vdscondition.VirtualDiskNotReadyForSnapshotting.String()))
+			Expect(ready.Message).To(ContainSubstring("migrating"))
+		})
+	})
+
+	Context("returns an error", func() {
+		It("The virtual disk is InUse but has no mounted VMs", func() {
+			const namespace = "default"
+			vd.Namespace = namespace
+			vd.Status.Conditions = []metav1.Condition{
+				{Type: vdcondition.SnapshottingType.String(), Status: metav1.ConditionTrue},
+				{Type: vdcondition.InUseType.String(), Status: metav1.ConditionTrue},
+			}
+			vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{
+				{Name: "vm-01", Mounted: false},
+			}
+			vdSnapshot.Namespace = namespace
+
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("please report a bug"))
+		})
+
+		It("The virtual disk is InUse with multiple mounted VMs", func() {
+			const namespace = "default"
+			vd.Namespace = namespace
+			vd.Status.Conditions = []metav1.Condition{
+				{Type: vdcondition.SnapshottingType.String(), Status: metav1.ConditionTrue},
+				{Type: vdcondition.InUseType.String(), Status: metav1.ConditionTrue},
+			}
+			vd.Status.AttachedToVirtualMachines = []v1alpha2.AttachedVirtualMachine{
+				{Name: "vm-01", Mounted: true},
+				{Name: "vm-02", Mounted: true},
+			}
+			vdSnapshot.Namespace = namespace
+
+			snapshotter.GetVirtualDiskFunc = func(_ context.Context, _, _ string) (*v1alpha2.VirtualDisk, error) {
+				return vd, nil
+			}
+			h := NewVirtualDiskReadyHandler(snapshotter, fakeClient)
+
+			_, err := h.Handle(testContext(), vdSnapshot)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("please report a bug"))
+		})
 	})
 })