From 4c55bf8d2f96992187feaea2aa8509a0be72fc02 Mon Sep 17 00:00:00 2001 From: "dmitry.lopatin" Date: Tue, 14 Jan 2025 16:41:28 +0300 Subject: [PATCH] fix(vd): fix update generation in conditions Signed-off-by: dmitry.lopatin --- .../controller/cvi/internal/source/errors.go | 2 +- .../cvi/internal/source/object_ref_vd.go | 2 +- .../pkg/controller/vd/internal/inuse.go | 55 +++++++++---------- .../controller/vi/internal/source/errors.go | 2 +- .../vi/internal/source/object_ref_vd.go | 2 +- .../controller/vm/internal/block_device.go | 2 +- 6 files changed, 31 insertions(+), 34 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/errors.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/errors.go index 3de699998..f74b39933 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/errors.go +++ b/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 { diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go index 0e11a9548..64d454fb6 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go +++ b/images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go @@ -243,7 +243,7 @@ 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.UsedForImageCreation.String() && - inUseCondition.ObservedGeneration == vd.Status.ObservedGeneration { + inUseCondition.ObservedGeneration == vd.Generation { return nil } diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/inuse.go b/images/virtualization-artifact/pkg/controller/vd/internal/inuse.go index 12c4ae760..237a6c9f6 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/inuse.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/inuse.go @@ -65,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{ @@ -78,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 { @@ -90,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 } @@ -105,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 } } @@ -129,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 } } @@ -145,46 +145,43 @@ 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.ConditionTrue: - if inUseCondition.Reason != vdcondition.AttachedToVirtualMachine.String() { - cb. - Generation(vd.Generation). - Status(metav1.ConditionTrue). - Reason(vdcondition.AttachedToVirtualMachine). - Message("") - conditions.SetCondition(cb, &vd.Status.Conditions) - return reconcile.Result{}, nil - } - case allowUseForImage && inUseCondition.Status != metav1.ConditionTrue: - if inUseCondition.Reason != vdcondition.UsedForImageCreation.String() { - cb. - Generation(vd.Generation). - Status(metav1.ConditionTrue). - Reason(vdcondition.UsedForImageCreation). - Message("") - conditions.SetCondition(cb, &vd.Status.Conditions) - return reconcile.Result{}, nil - } + 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: setNotInUse := false - if inUseCondition.Reason == vdcondition.AttachedToVirtualMachine.String() && !allowUseForVM { + if inUseCondition.Reason == vdcondition.AttachedToVirtualMachine.String() && !usedByVM { setNotInUse = true } - if inUseCondition.Reason == vdcondition.UsedForImageCreation.String() && !allowUseForImage { + if inUseCondition.Reason == vdcondition.UsedForImageCreation.String() && !usedByImage { setNotInUse = true } 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 } } diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go index 3de699998..f74b39933 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go +++ b/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 { diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go index 8fe993ff0..b20853287 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go +++ b/images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go @@ -396,7 +396,7 @@ 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.UsedForImageCreation.String() && - inUseCondition.ObservedGeneration == vd.Status.ObservedGeneration { + inUseCondition.ObservedGeneration == vd.Generation { return nil } diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go b/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go index 918305c96..170de5100 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go +++ b/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 it is being used for creating an image.").Condition()) changed.Status.Conditions = mgr.Generate() return reconcile.Result{}, nil }