fix(vd): fix condition status updates in VirtualDisk

api/core/v1alpha2/vdcondition/condition.go

@@ -127,8 +127,10 @@ const (
 	// StorageClassNotFound indicates that the storage class is not ready
 	StorageClassNotFound StorageClassReadyReason = "StorageClassNotFound"
 
-	// AllowedForImageUsage indicates that the VirtualDisk is permitted for use in a process of an image creation.
-	AllowedForImageUsage InUseReason = "AllowedForImageUsage"
-	// AllowedForVirtualMachineUsage indicates that the VirtualDisk is permitted for use by a running VirtualMachine.
-	AllowedForVirtualMachineUsage InUseReason = "AllowedForVirtualMachineUsage"
+	// UsedForImageCreation indicates that the VirtualDisk is used for create image.
+	UsedForImageCreation InUseReason = "UsedForImageCreation"
+	// AttachedToVirtualMachine indicates that the VirtualDisk is attached to VirtualMachine.
+	AttachedToVirtualMachine InUseReason = "AttachedToVirtualMachine"
+	// NotInUse indicates that VirtualDisk free for use.
+	NotInUse InUseReason = "NotInUse"
 )

images/virtualization-artifact/pkg/controller/cvi/internal/source/errors.go

@@ -70,7 +70,7 @@ type VirtualDiskNotAllowedForUseError struct {
 }
 
 func (e VirtualDiskNotAllowedForUseError) Error() string {
-	return fmt.Sprintf("use of VirtualDisk %s is not allowed, it may be connected to a running VirtualMachine", e.name)
+	return fmt.Sprintf("the VirtualDisk %s attached to VirtualMachine", e.name)
 }
 
 func NewVirtualDiskNotAllowedForUseError(name string) error {

images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go

@@ -242,8 +242,8 @@ func (ds ObjectRefVirtualDisk) Validate(ctx context.Context, cvi *virtv2.Cluster
 
 	inUseCondition, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 	if inUseCondition.Status == metav1.ConditionTrue &&
-		inUseCondition.Reason == vdcondition.AllowedForImageUsage.String() &&
-		inUseCondition.ObservedGeneration == vd.Status.ObservedGeneration {
+		inUseCondition.Reason == vdcondition.UsedForImageCreation.String() &&
+		inUseCondition.ObservedGeneration == vd.Generation {
 		return nil
 	}
 

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

@@ -36,6 +36,14 @@ import (
 	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
 )
 
+type reasonString struct {
+	value string
+}
+
+func (rs reasonString) String() string {
+	return rs.value
+}
+
 type InUseHandler struct {
 	client client.Client
 }
@@ -57,7 +65,7 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 		inUseCondition = cb.Condition()
 	}
 
-	allowUseForVM, allowUseForImage := false, false
+	usedByVM, usedByImage := false, false
 
 	var vms virtv2.VirtualMachineList
 	err := h.client.List(ctx, &vms, &client.ListOptions{
@@ -70,9 +78,9 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 	for _, vm := range vms.Items {
 		if h.isVDAttachedToVM(vd.GetName(), vm) {
 			if vm.Status.Phase != virtv2.MachineStopped {
-				allowUseForVM = isVMCanStart(vm.Status.Conditions)
+				usedByVM = isVMCanStart(vm.Status.Conditions)
 
-				if allowUseForVM {
+				if usedByVM {
 					break
 				}
 			} else {
@@ -82,7 +90,7 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 				}
 
 				if kvvm != nil && kvvm.Status.StateChangeRequests != nil {
-					allowUseForVM = true
+					usedByVM = true
 					break
 				}
 
@@ -97,7 +105,7 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 
 				for _, pod := range podList.Items {
 					if pod.Status.Phase == corev1.PodRunning {
-						allowUseForVM = true
+						usedByVM = true
 						break
 					}
 				}
@@ -121,7 +129,7 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 			vi.Spec.DataSource.ObjectRef != nil &&
 			vi.Spec.DataSource.ObjectRef.Kind == virtv2.VirtualDiskKind &&
 			vi.Spec.DataSource.ObjectRef.Name == vd.Name {
-			allowUseForImage = true
+			usedByImage = true
 			break
 		}
 	}
@@ -137,48 +145,52 @@ func (h InUseHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (recon
 			cvi.Spec.DataSource.ObjectRef != nil &&
 			cvi.Spec.DataSource.ObjectRef.Kind == virtv2.VirtualDiskKind &&
 			cvi.Spec.DataSource.ObjectRef.Name == vd.Name {
-			allowUseForImage = true
+			usedByImage = true
 		}
 	}
 
 	cb := conditions.NewConditionBuilder(vdcondition.InUseType)
 	switch {
-	case allowUseForVM && inUseCondition.Status == metav1.ConditionUnknown:
-		if inUseCondition.Reason != vdcondition.AllowedForVirtualMachineUsage.String() {
-			cb.
-				Generation(vd.Generation).
-				Status(metav1.ConditionTrue).
-				Reason(vdcondition.AllowedForVirtualMachineUsage).
-				Message("")
-			conditions.SetCondition(cb, &vd.Status.Conditions)
-		}
-	case allowUseForImage && inUseCondition.Status == metav1.ConditionUnknown:
-		if inUseCondition.Reason != vdcondition.AllowedForImageUsage.String() {
-			cb.
-				Generation(vd.Generation).
-				Status(metav1.ConditionTrue).
-				Reason(vdcondition.AllowedForImageUsage).
-				Message("")
-			conditions.SetCondition(cb, &vd.Status.Conditions)
-		}
+	case usedByVM && inUseCondition.Status != metav1.ConditionTrue:
+		cb.
+			Generation(vd.Generation).
+			Status(metav1.ConditionTrue).
+			Reason(vdcondition.AttachedToVirtualMachine).
+			Message("")
+		conditions.SetCondition(cb, &vd.Status.Conditions)
+		return reconcile.Result{}, nil
+	case usedByImage && inUseCondition.Status != metav1.ConditionTrue:
+		cb.
+			Generation(vd.Generation).
+			Status(metav1.ConditionTrue).
+			Reason(vdcondition.UsedForImageCreation).
+			Message("")
+		conditions.SetCondition(cb, &vd.Status.Conditions)
+		return reconcile.Result{}, nil
 	default:
-		setUnknown := false
+		setNotInUse := false
 
-		if inUseCondition.Reason == vdcondition.AllowedForVirtualMachineUsage.String() && !allowUseForVM {
-			setUnknown = true
+		if inUseCondition.Reason == vdcondition.AttachedToVirtualMachine.String() && !usedByVM {
+			setNotInUse = true
 		}
 
-		if inUseCondition.Reason == vdcondition.AllowedForImageUsage.String() && !allowUseForImage {
-			setUnknown = true
+		if inUseCondition.Reason == vdcondition.UsedForImageCreation.String() && !usedByImage {
+			setNotInUse = true
 		}
 
-		if setUnknown {
-			cb.Generation(vd.Generation).Status(metav1.ConditionUnknown).Reason(conditions.ReasonUnknown).Message("")
+		if setNotInUse {
+			cb.Generation(vd.Generation).Status(metav1.ConditionFalse).Reason(vdcondition.NotInUse).Message("")
 			conditions.SetCondition(cb, &vd.Status.Conditions)
+			// TODO redesign the handler's operation to change the logic for updating the Reason on the fly without intermediate switching to False and remove the requeue.
 			return reconcile.Result{Requeue: true}, nil
 		}
 	}
 
+	cb.Generation(vd.Generation).
+		Status(inUseCondition.Status).
+		Message(inUseCondition.Message).
+		Reason(reasonString{inUseCondition.Reason})
+	conditions.SetCondition(cb, &vd.Status.Conditions)
 	return reconcile.Result{}, nil
 }
 

images/virtualization-artifact/pkg/controller/vd/internal/inuse_test.go

@@ -73,6 +73,37 @@ var _ = Describe("InUseHandler", func() {
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
 		})
+
+		It("must set condition generation equal resource generation", func() {
+			vd := &virtv2.VirtualDisk{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-vd",
+					Namespace: "default",
+				},
+				Status: virtv2.VirtualDiskStatus{
+					Conditions: []metav1.Condition{
+						{
+							Type:               vdcondition.InUseType.String(),
+							Reason:             conditions.ReasonUnknown.String(),
+							Status:             metav1.ConditionUnknown,
+							ObservedGeneration: 2,
+						},
+					},
+				},
+			}
+			vd.Generation = 3
+
+			k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(vd).Build()
+			handler = NewInUseHandler(k8sClient)
+
+			result, err := handler.Handle(ctx, vd)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(result).To(Equal(ctrl.Result{}))
+
+			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
+			Expect(cond).ToNot(BeNil())
+			Expect(cond.ObservedGeneration).To(Equal(vd.Generation))
+		})
 	})
 
 	Context("when VirtualDisk is used by running VirtualMachine", func() {
@@ -121,7 +152,7 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForVirtualMachineUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.AttachedToVirtualMachine.String()))
 		})
 	})
 
@@ -217,7 +248,7 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForImageUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.UsedForImageCreation.String()))
 		})
 	})
 
@@ -264,7 +295,7 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForImageUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.UsedForImageCreation.String()))
 		})
 	})
 
@@ -325,7 +356,7 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForVirtualMachineUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.AttachedToVirtualMachine.String()))
 		})
 	})
 
@@ -372,7 +403,7 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForVirtualMachineUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.AttachedToVirtualMachine.String()))
 		})
 	})
 
@@ -424,12 +455,12 @@ var _ = Describe("InUseHandler", func() {
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
 			Expect(cond.Status).To(Equal(metav1.ConditionTrue))
-			Expect(cond.Reason).To(Equal(vdcondition.AllowedForImageUsage.String()))
+			Expect(cond.Reason).To(Equal(vdcondition.UsedForImageCreation.String()))
 		})
 	})
 
 	Context("when VirtualDisk is not in use after image creation", func() {
-		It("must set status Unknown and reason Unknown", func() {
+		It("must set status False and reason NotInUse", func() {
 			vd := &virtv2.VirtualDisk{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-vd",
@@ -439,7 +470,7 @@ var _ = Describe("InUseHandler", func() {
 					Conditions: []metav1.Condition{
 						{
 							Type:   vdcondition.InUseType.String(),
-							Reason: vdcondition.AllowedForImageUsage.String(),
+							Reason: vdcondition.UsedForImageCreation.String(),
 							Status: metav1.ConditionTrue,
 						},
 					},
@@ -455,12 +486,13 @@ var _ = Describe("InUseHandler", func() {
 
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
-			Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
+			Expect(cond.Status).To(Equal(metav1.ConditionFalse))
+			Expect(cond.Reason).To(Equal(vdcondition.NotInUse.String()))
 		})
 	})
 
 	Context("when VirtualDisk is not in use after VM deletion", func() {
-		It("must set status Unknown and reason Unknown", func() {
+		It("must set status False and reason NotInUse", func() {
 			vd := &virtv2.VirtualDisk{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-vd",
@@ -470,7 +502,7 @@ var _ = Describe("InUseHandler", func() {
 					Conditions: []metav1.Condition{
 						{
 							Type:   vdcondition.InUseType.String(),
-							Reason: vdcondition.AllowedForVirtualMachineUsage.String(),
+							Reason: vdcondition.AttachedToVirtualMachine.String(),
 							Status: metav1.ConditionTrue,
 						},
 					},
@@ -486,7 +518,8 @@ var _ = Describe("InUseHandler", func() {
 
 			cond, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 			Expect(cond).ToNot(BeNil())
-			Expect(cond.Status).To(Equal(metav1.ConditionUnknown))
+			Expect(cond.Status).To(Equal(metav1.ConditionFalse))
+			Expect(cond.Reason).To(Equal(vdcondition.NotInUse.String()))
 		})
 	})
 })

images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go

@@ -70,7 +70,7 @@ type VirtualDiskNotAllowedForUseError struct {
 }
 
 func (e VirtualDiskNotAllowedForUseError) Error() string {
-	return fmt.Sprintf("use of VirtualDisk %s is not allowed, it may be connected to a running VirtualMachine", e.name)
+	return fmt.Sprintf("the VirtualDisk %s attached to VirtualMachine", e.name)
 }
 
 func NewVirtualDiskNotAllowedForUseError(name string) error {

images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go

@@ -395,8 +395,8 @@ func (ds ObjectRefVirtualDisk) Validate(ctx context.Context, vi *virtv2.VirtualI
 
 	inUseCondition, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 	if inUseCondition.Status == metav1.ConditionTrue &&
-		inUseCondition.Reason == vdcondition.AllowedForImageUsage.String() &&
-		inUseCondition.ObservedGeneration == vd.Status.ObservedGeneration {
+		inUseCondition.Reason == vdcondition.UsedForImageCreation.String() &&
+		inUseCondition.ObservedGeneration == vd.Generation {
 		return nil
 	}
 

images/virtualization-artifact/pkg/controller/vm/internal/block_device.go

@@ -172,7 +172,7 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS
 	if !h.areVirtualDisksAllowedToUse(vds) {
 		mgr.Update(cb.Status(metav1.ConditionFalse).
 			Reason(vmcondition.ReasonBlockDevicesNotReady).
-			Message("Virtual disks are not allowed to be used in the virtual machine, check where else they are used.").Condition())
+			Message("Virtual disks cannot be used because they are being used for creating an image.").Condition())
 		changed.Status.Conditions = mgr.Generate()
 		return reconcile.Result{}, nil
 	}
@@ -188,8 +188,8 @@ func (h *BlockDeviceHandler) areVirtualDisksAllowedToUse(vds map[string]*virtv2.
 	for _, vd := range vds {
 		inUseCondition, _ := conditions.GetCondition(vdcondition.InUseType, vd.Status.Conditions)
 		if inUseCondition.Status != metav1.ConditionTrue ||
-			inUseCondition.Reason != vdcondition.AllowedForVirtualMachineUsage.String() ||
-			inUseCondition.ObservedGeneration != vd.Status.ObservedGeneration {
+			inUseCondition.Reason != vdcondition.AttachedToVirtualMachine.String() ||
+			inUseCondition.ObservedGeneration != vd.Generation {
 			return false
 		}
 	}