fix(vmbda): resolve comments

Signed-off-by: Isteb4k <dmitry.rakitin@flant.com>
deckhouse · Jul 5, 2024 · 557f271 · 557f271
1 parent 9c3e734
commit 557f271
Show file tree

Hide file tree

Showing 14 changed files with 129 additions and 98 deletions.
diff --git a/api/pkg/apiserver/api/generated/openapi/zz_generated.openapi.go b/api/pkg/apiserver/api/generated/openapi/zz_generated.openapi.go
diff --git a/crds/doc-ru-virtualimage.yaml b/crds/doc-ru-virtualimage.yaml
@@ -91,20 +91,22 @@ spec:
               properties:
                 conditions:
                   description: |
-                    Последние доступные наблюдения текущего состояния объекта.
-                  properties:
-                    type:
-                      description: Тип состояния.
-                    status:
-                      description: Статус состояния (одно из True, False, Unknown).
-                    message:
-                      description: Cообщение c деталями последнего перехода состояния.
-                    reason:
-                      description: Краткая причина последнего перехода состояния.
-                    lastProbeTime:
-                      description: Время последней проверки состояния.
-                    lastTransitionTime:
-                      description: Время последнего перехода состояния из одного статуса в другой.
+                    Последнее подтвержденное состояние данного ресурса.
+                  items:
+                    properties:
+                      lastProbeTime:
+                        description: Время проверки условия.
+                      lastTransitionTime:
+                        description: Время перехода условия из одного состояния в другое.
+                      message:
+                        description: Удобочитаемое сообщение с подробной информацией о последнем переходе.
+                      reason:
+                        description: Краткая причина последнего перехода состояния.
+                      status:
+                        description: |
+                          Статус условия. Возможные значения: `True`, `False`, `Unknown`.
+                      type:
+                        description: Тип условия.
                 cdrom:
                   description: |
                     Является ли образ форматом, который должен быть смонтирован как cdrom, например iso и т. д.

diff --git a/crds/doc-ru-virtualmachineblockdeviceattachment.yaml b/crds/doc-ru-virtualmachineblockdeviceattachment.yaml
@@ -27,20 +27,22 @@ spec:
               properties:
                 conditions:
                   description: |
-                    Последние доступные наблюдения текущего состояния объекта.
-                  properties:
-                    type:
-                      description: Тип состояния.
-                    status:
-                      description: Статус состояния (одно из True, False, Unknown).
-                    message:
-                      description: Cообщение c деталями последнего перехода состояния.
-                    reason:
-                      description: Краткая причина последнего перехода состояния.
-                    lastProbeTime:
-                      description: Время последней проверки состояния.
-                    lastTransitionTime:
-                      description: Время последнего перехода состояния из одного статуса в другой.
+                    Последнее подтвержденное состояние данного ресурса.
+                  items:
+                    properties:
+                      lastProbeTime:
+                        description: Время проверки условия.
+                      lastTransitionTime:
+                        description: Время перехода условия из одного состояния в другое.
+                      message:
+                        description: Удобочитаемое сообщение с подробной информацией о последнем переходе.
+                      reason:
+                        description: Краткая причина последнего перехода состояния.
+                      status:
+                        description: |
+                          Статус условия. Возможные значения: `True`, `False`, `Unknown`.
+                      type:
+                        description: Тип условия.
                 phase:
                   description: |
                     Фаза ресурса:

diff --git a/crds/virtualimage.yaml b/crds/virtualimage.yaml
@@ -193,40 +193,36 @@ spec:
               type: object
               properties:
                 conditions:
-                  type: array
                   description: |
                     The latest available observations of an object's current state.
+                  type: array
                   items:
                     type: object
                     properties:
-                      type:
-                        type: string
-                        description: Type of condition.
-                      status:
-                        type: string
-                        description: Status of the condition, one of True, False, Unknown.
-                        enum:
-                          - "True"
-                          - "False"
-                          - "Unknown"
-                      reason:
-                        type: string
-                        description: Reason for the condition's last transition.
-                      message:
-                        type: string
-                        description: |
-                          Human readable message indicating details about last transition.
                       lastProbeTime:
-                        type: string
                         description: Last time the condition was checked.
                         format: date-time
-                      lastTransitionTime:
                         type: string
+                      lastTransitionTime:
                         description: Last time the condition transit from one status to another.
                         format: date-time
+                        type: string
+                      message:
+                        description: Human readable message indicating details about last transition.
+                        type: string
+                      reason:
+                        description: (brief) reason for the condition's last transition.
+                        type: string
+                      status:
+                        description: Status of the condition, one of True, False, Unknown.
+                        type: string
+                        enum: ["True", "False", "Unknown"]
+                      type:
+                        description: Type of condition.
+                        type: string
                     required:
-                      - type
                       - status
+                      - type
                 downloadSpeed:
                   type: object
                   description: |
@@ -325,7 +321,7 @@ spec:
                 observedGeneration:
                   type: integer
                   description: |
-                    Represents the .metadata.generation that the status was set based upon.
+                    The generation last processed by the controller.
       additionalPrinterColumns:
         - name: Phase
           type: string

diff --git a/crds/virtualmachineblockdeviceattachment.yaml b/crds/virtualmachineblockdeviceattachment.yaml
@@ -116,7 +116,7 @@ spec:
                 observedGeneration:
                   type: integer
                   description: |
-                    Represents the .metadata.generation that the status was set based upon.
+                    The generation last processed by the controller.
       additionalPrinterColumns:
         - name: Phase
           type: string

diff --git a/images/virtualization-artifact/pkg/controller/service/resource.go b/images/virtualization-artifact/pkg/controller/service/resource.go
@@ -161,7 +161,7 @@ func MergeResults(results ...reconcile.Result) reconcile.Result {
 		if r.IsZero() {
 			continue
 		}
-		if r.Requeue {
+		if r.Requeue && r.RequeueAfter == 0 {
 			return r
 		}
 		if result.IsZero() && r.RequeueAfter > 0 {

diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go b/images/virtualization-artifact/pkg/controller/vm/internal/block_device.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
-	"slices"
 	"strings"
 	"time"
 
@@ -109,28 +108,55 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS
 	}
 
 	if kvvmi != nil {
-		specDeviceMap := make(map[virtv2.BlockDeviceSpecRef]struct{}, len(changed.Spec.BlockDeviceRefs))
+		// Fill BlockDeviceRefs every time without knowledge of previously kept BlockDeviceRefs.
+		changed.Status.BlockDeviceRefs = nil
+
+		// Set BlockDeviceRef in the status if the disk exists in KVVMI.
 		for _, ref := range changed.Spec.BlockDeviceRefs {
-			specDeviceMap[ref] = struct{}{}
-			idx, bds := h.findAttachedBlockDevice(changed, ref)
-			newBds := h.createAttachedBlockDevice(ref, bdState, kvvmi)
-			if newBds == nil {
+			bd := h.createAttachedBlockDevice(ref, bdState, kvvmi)
+			if bd == nil {
 				continue
 			}
-			if bds != nil {
-				changed.Status.BlockDeviceRefs[idx] = *newBds
+
+			changed.Status.BlockDeviceRefs = append(
+				changed.Status.BlockDeviceRefs,
+				*bd,
+			)
+		}
+
+		// Set BlockDeviceRef `Hotpluggable: true` in the status if KVVMI has a hotplugged disk.
+		for _, vs := range kvvmi.Status.VolumeStatus {
+			if vs.HotplugVolume == nil {
 				continue
 			}
+
+			vdName, ok := kvbuilder.GerOriginalDiskName(vs.Name)
+			if !ok {
+				h.logger.Warn("volume %s was hot plugged to VirtualMachineInstance %s, but it is not a VirtualDisk.", vdName, kvvmi.Name)
+				h.recorder.Eventf(changed, corev1.EventTypeNormal, virtv2.ReasonUnknownHotPluggedVolume, "Volume %s was hot plugged to VirtualMachineInstance %s, but it is not a VirtualDisk.", vdName, kvvmi.Name)
+				continue
+			}
+
+			var vd *virtv2.VirtualDisk
+			vd, err = s.VirtualDisk(ctx, vdName)
+			if err != nil {
+				return reconcile.Result{}, err
+			}
+
+			if vd == nil {
+				h.logger.Warn("VirtualDisk %s not found but pvc hot plugged into VirtualMachineInstance %s", vdName, kvvmi.Name)
+				h.recorder.Eventf(changed, corev1.EventTypeNormal, virtv2.ReasonUnknownHotPluggedVolume, "VirtualDisk %s not found but pvc hot plugged into VirtualMachineInstance %s", vdName, kvvmi.Name)
+				continue
+			}
+
 			changed.Status.BlockDeviceRefs = append(
 				changed.Status.BlockDeviceRefs,
-				*newBds)
+				h.getHotPluggedDiskStatusRef(vs, vd),
+			)
 		}
-
-		changed.Status.BlockDeviceRefs = slices.DeleteFunc(changed.Status.BlockDeviceRefs, func(ref virtv2.BlockDeviceStatusRef) bool {
-			_, ok := specDeviceMap[virtv2.BlockDeviceSpecRef{Kind: ref.Kind, Name: ref.Name}]
-			return !ok && h.findVolumeStatus(GenerateDiskName(ref.Kind, ref.Name), kvvmi) == nil
-		})
 	}
+
+	// Update the BlockDevicesReady condition.
 	countBD := len(current.Spec.BlockDeviceRefs)
 	if ready, count := h.countReadyBlockDevices(current, bdState); !ready {
 		// Wait until block devices are ready.
@@ -267,6 +293,16 @@ func (h *BlockDeviceHandler) updateFinalizers(ctx context.Context, vm *virtv2.Vi
 	return nil
 }
 
+func (h *BlockDeviceHandler) getHotPluggedDiskStatusRef(vs virtv1.VolumeStatus, vd *virtv2.VirtualDisk) virtv2.BlockDeviceStatusRef {
+	return virtv2.BlockDeviceStatusRef{
+		Kind:         virtv2.DiskDevice,
+		Name:         vd.Name,
+		Target:       vs.Target,
+		Size:         vd.Status.Capacity,
+		Hotpluggable: true,
+	}
+}
+
 func (h *BlockDeviceHandler) createAttachedBlockDevice(spec virtv2.BlockDeviceSpecRef, state BlockDevicesState, kvvmi *virtv1.VirtualMachineInstance) *virtv2.BlockDeviceStatusRef {
 	if kvvmi == nil {
 		return nil
@@ -329,17 +365,6 @@ func (h *BlockDeviceHandler) createAttachedBlockDevice(spec virtv2.BlockDeviceSp
 	return nil
 }
 
-func (h *BlockDeviceHandler) findAttachedBlockDevice(vm *virtv2.VirtualMachine, spec virtv2.BlockDeviceSpecRef) (int, *virtv2.BlockDeviceStatusRef) {
-	for i := range vm.Status.BlockDeviceRefs {
-		bda := &vm.Status.BlockDeviceRefs[i]
-		if bda.Kind == spec.Kind && bda.Name == spec.Name {
-			return i, bda
-		}
-	}
-
-	return -1, nil
-}
-
 func (h *BlockDeviceHandler) findVolumeStatus(volumeName string, kvvmi *virtv1.VirtualMachineInstance) *virtv1.VolumeStatus {
 	if kvvmi != nil {
 		for i := range kvvmi.Status.VolumeStatus {
@@ -369,11 +394,11 @@ type BlockDevicesState struct {
 }
 
 func (s *BlockDevicesState) Reload(ctx context.Context) error {
-	viByName, err := s.s.VirtualImageByName(ctx)
+	viByName, err := s.s.VirtualImagesByName(ctx)
 	if err != nil {
 		return err
 	}
-	ciByName, err := s.s.ClusterVirtualImageByName(ctx)
+	ciByName, err := s.s.ClusterVirtualImagesByName(ctx)
 	if err != nil {
 		return err
 	}

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/block_device_ready.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/block_device_ready.go
@@ -97,7 +97,7 @@ func (h BlockDeviceReadyHandler) Handle(ctx context.Context, vmbda *virtv2.Virtu
 		if vd.Status.Target.PersistentVolumeClaim == "" {
 			condition.Status = metav1.ConditionFalse
 			condition.Reason = vmbdacondition.BlockDeviceNotReady
-			condition.Message = "Waiting for the underlying PersistentVolumeClaim name will be known."
+			condition.Message = "Waiting until VirtualDisk has associated PersistentVolumeClaim name."
 			return reconcile.Result{}, nil
 		}
 

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/life_cycle.go
@@ -69,6 +69,9 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vmbda *virtv2.VirtualMachi
 
 	if vmbda.DeletionTimestamp != nil {
 		vmbda.Status.Phase = virtv2.BlockDeviceAttachmentPhaseTerminating
+		condition.Status = metav1.ConditionUnknown
+		condition.Reason = ""
+		condition.Message = ""
 
 		err = h.attacher.UnplugDisk(ctx, vd, vm)
 		if err != nil {

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/kvvmi_watcher.go
@@ -51,7 +51,7 @@ func NewKVVMIWatcher(logger *slog.Logger, client client.Client) *KVVMIWatcher {
 	}
 }
 
-func (w KVVMIWatcher) Run(mgr manager.Manager, ctr controller.Controller) error {
+func (w KVVMIWatcher) Watch(mgr manager.Manager, ctr controller.Controller) error {
 	return ctr.Watch(
 		source.Kind(mgr.GetCache(), &virtv1.VirtualMachineInstance{}),
 		NewKVVMIEventHandler(w.client, w.logger),

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vd_watcher.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vd_watcher.go
@@ -48,7 +48,7 @@ func NewVirtualDiskWatcher(logger *slog.Logger, client client.Client) *VirtualDi
 	}
 }
 
-func (w VirtualDiskWatcher) Run(mgr manager.Manager, ctr controller.Controller) error {
+func (w VirtualDiskWatcher) Watch(mgr manager.Manager, ctr controller.Controller) error {
 	return ctr.Watch(
 		source.Kind(mgr.GetCache(), &virtv2.VirtualDisk{}),
 		handler.EnqueueRequestsFromMapFunc(w.enqueueRequests),

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vm_watcher.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vm_watcher.go
@@ -48,7 +48,7 @@ func NewVirtualMachineWatcher(logger *slog.Logger, client client.Client) *Virtua
 	}
 }
 
-func (w VirtualMachineWatcher) Run(mgr manager.Manager, ctr controller.Controller) error {
+func (w VirtualMachineWatcher) Watch(mgr manager.Manager, ctr controller.Controller) error {
 	return ctr.Watch(
 		source.Kind(mgr.GetCache(), &virtv2.VirtualMachine{}),
 		handler.EnqueueRequestsFromMapFunc(w.enqueueRequests),

diff --git a/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vmbda_watcher.go b/images/virtualization-artifact/pkg/controller/vmbda/internal/watcher/vmbda_watcher.go
@@ -42,7 +42,7 @@ func NewVirtualMachineBlockDeviceAttachmentWatcher(logger *slog.Logger, client c
 	}
 }
 
-func (w VirtualMachineBlockDeviceAttachmentWatcher) Run(mgr manager.Manager, ctr controller.Controller) error {
+func (w VirtualMachineBlockDeviceAttachmentWatcher) Watch(mgr manager.Manager, ctr controller.Controller) error {
 	return ctr.Watch(
 		source.Kind(mgr.GetCache(), &virtv2.VirtualMachineBlockDeviceAttachment{}),
 		&handler.EnqueueRequestForObject{},