fix(vdsnapshot): fix no bootable device error

1. Fix no bootable device error (wrong api group for pvc) 2. Fix unfreeze for disk devices 3. Improve crd description Signed-off-by: Isteb4k <[email protected]>
deckhouse · Oct 4, 2024 · a22de20 · a22de20
1 parent e4426b7
commit a22de20
Show file tree

Hide file tree

Showing 11 changed files with 77 additions and 83 deletions.
diff --git a/crds/doc-ru-virtualdisksnapshots.yaml b/crds/doc-ru-virtualdisksnapshots.yaml
@@ -20,7 +20,7 @@ spec:
                   description: |
                     Создавать снимок диска подключённой виртуальной машины только в том случае, если возможно заморозить её через агента.
 
-                    Если значение установлено в true, снимок виртуального диска будет создан только в следующих случаях:
+                    Если значение установлено в true, снимок виртуального диска будет создан, если выполняется хотя бы одно из следующих правил:
                     - виртуальный диск не подключен ни к одной виртуальной машине;
                     - виртуальный диск подключен к виртуальной машине, которая выключена;
                     - виртуальный диск подключен к виртуальной машине с агентом, и операция заморозки прошла успешно.

diff --git a/crds/virtualdisksnapshots.yaml b/crds/virtualdisksnapshots.yaml
@@ -53,7 +53,7 @@ spec:
                   description: |
                     Create a snapshot of a connected virtual machine's disk only if it is possible to freeze the machine through the agent.
 
-                    If value is true, the snapshot of the virtual disk will be taken only in the following scenarios:
+                    If the value is set to true, a virtual disk snapshot will be created if at least one of the following rules is met:
                     - the virtual disk is not attached to any virtual machine.
                     - the virtual disk is attached to a virtual machine that is powered off.
                     - the virtual disk is attached to a virtual machine with an agent, and the freeze operation was successful.

diff --git a/images/virtualization-artifact/pkg/controller/service/snapshot_service.go b/images/virtualization-artifact/pkg/controller/service/snapshot_service.go
@@ -19,7 +19,6 @@ package service
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -85,7 +84,7 @@ func (s *SnapshotService) CanUnfreeze(ctx context.Context, vdSnapshotName string
 
 	vdByName := make(map[string]struct{})
 	for _, bdr := range vm.Status.BlockDeviceRefs {
-		if bdr.Kind != virtv2.DiskDevice {
+		if bdr.Kind == virtv2.DiskDevice {
 			vdByName[bdr.Name] = struct{}{}
 		}
 	}
@@ -121,46 +120,18 @@ func (s *SnapshotService) Unfreeze(ctx context.Context, name, namespace string)
 	return nil
 }
 
-func (s *SnapshotService) CreateVolumeSnapshot(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot, pvc *corev1.PersistentVolumeClaim) (*vsv1.VolumeSnapshot, error) {
-	anno := make(map[string]string)
-	if pvc.Spec.StorageClassName != nil && *pvc.Spec.StorageClassName != "" {
-		anno["storageClass"] = *pvc.Spec.StorageClassName
-		accessModes := make([]string, 0, len(pvc.Status.AccessModes))
-		for _, accessMode := range pvc.Status.AccessModes {
-			accessModes = append(accessModes, string(accessMode))
-		}
-
-		anno["accessModes"] = strings.Join(accessModes, ",")
-	}
-
-	volumeSnapshot := vsv1.VolumeSnapshot{
-		ObjectMeta: metav1.ObjectMeta{
-			Annotations: anno,
-			Name:        vdSnapshot.Name,
-			Namespace:   vdSnapshot.Namespace,
-			OwnerReferences: []metav1.OwnerReference{
-				MakeOwnerReference(vdSnapshot),
-			},
-		},
-		Spec: vsv1.VolumeSnapshotSpec{
-			Source: vsv1.VolumeSnapshotSource{
-				PersistentVolumeClaimName: &pvc.Name,
-			},
-			VolumeSnapshotClassName: &vdSnapshot.Spec.VolumeSnapshotClassName,
-		},
-	}
-
-	err := s.client.Create(ctx, &volumeSnapshot)
+func (s *SnapshotService) CreateVolumeSnapshot(ctx context.Context, vs *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error) {
+	err := s.client.Create(ctx, vs)
 	if err != nil && !k8serrors.IsAlreadyExists(err) {
 		return nil, err
 	}
 
-	err = s.protection.AddProtection(ctx, &volumeSnapshot)
+	err = s.protection.AddProtection(ctx, vs)
 	if err != nil {
 		return nil, err
 	}
 
-	return &volumeSnapshot, nil
+	return vs, nil
 }
 
 func (s *SnapshotService) DeleteVolumeSnapshot(ctx context.Context, vs *vsv1.VolumeSnapshot) error {

diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/source/object_ref_vdsnapshot.go b/images/virtualization-artifact/pkg/controller/vd/internal/source/object_ref_vdsnapshot.go
@@ -80,6 +80,7 @@ func (ds ObjectRefVirtualDiskSnapshot) Sync(ctx context.Context, vd *virtv2.Virt
 		namespacedName := supplements.NewGenerator(common.VDShortName, vd.Name, vd.Namespace, vd.UID).PersistentVolumeClaim()
 
 		storageClassName := vs.Annotations["storageClass"]
+		volumeMode := vs.Annotations["volumeMode"]
 		accessModesStr := strings.Split(vs.Annotations["accessModes"], ",")
 		accessModes := make([]corev1.PersistentVolumeAccessMode, 0, len(accessModesStr))
 		for _, accessModeStr := range accessModesStr {
@@ -89,7 +90,7 @@ func (ds ObjectRefVirtualDiskSnapshot) Sync(ctx context.Context, vd *virtv2.Virt
 		spec := corev1.PersistentVolumeClaimSpec{
 			AccessModes: accessModes,
 			DataSource: &corev1.TypedLocalObjectReference{
-				APIGroup: ptr.To(vs.GroupVersionKind().GroupVersion().String()),
+				APIGroup: ptr.To(vs.GroupVersionKind().Group),
 				Kind:     vs.Kind,
 				Name:     vd.Spec.DataSource.ObjectRef.Name,
 			},
@@ -99,6 +100,10 @@ func (ds ObjectRefVirtualDiskSnapshot) Sync(ctx context.Context, vd *virtv2.Virt
 			spec.StorageClassName = &storageClassName
 		}
 
+		if volumeMode != "" {
+			spec.VolumeMode = ptr.To(corev1.PersistentVolumeMode(volumeMode))
+		}
+
 		if vs.Status != nil && vs.Status.RestoreSize != nil {
 			spec.Resources = corev1.VolumeResourceRequirements{
 				Requests: corev1.ResourceList{

diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/interfaces.go
@@ -37,7 +37,7 @@ type LifeCycleSnapshotter interface {
 	CanFreeze(vm *virtv2.VirtualMachine) bool
 	CanUnfreeze(ctx context.Context, vdSnapshotName string, vm *virtv2.VirtualMachine) (bool, error)
 	Unfreeze(ctx context.Context, name, namespace string) error
-	CreateVolumeSnapshot(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot, pvc *corev1.PersistentVolumeClaim) (*vsv1.VolumeSnapshot, error)
+	CreateVolumeSnapshot(ctx context.Context, vs *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error)
 	GetPersistentVolumeClaim(ctx context.Context, name, namespace string) (*corev1.PersistentVolumeClaim, error)
 	GetVirtualDisk(ctx context.Context, name, namespace string) (*virtv2.VirtualDisk, error)
 	GetVirtualMachine(ctx context.Context, name, namespace string) (*virtv2.VirtualMachine, 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
@@ -19,7 +19,9 @@ package internal
 import (
 	"context"
 	"fmt"
+	"strings"
 
+	vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
@@ -164,7 +166,40 @@ func (h LifeCycleHandler) Handle(ctx context.Context, vdSnapshot *virtv2.Virtual
 
 		log.Debug("The corresponding volume snapshot not found: create the new one")
 
-		vs, err = h.snapshotter.CreateVolumeSnapshot(ctx, vdSnapshot, pvc)
+		anno := make(map[string]string)
+		if pvc.Spec.StorageClassName != nil && *pvc.Spec.StorageClassName != "" {
+			anno["storageClass"] = *pvc.Spec.StorageClassName
+		}
+
+		if pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode != "" {
+			anno["volumeMode"] = string(*pvc.Spec.VolumeMode)
+		}
+
+		accessModes := make([]string, 0, len(pvc.Status.AccessModes))
+		for _, accessMode := range pvc.Status.AccessModes {
+			accessModes = append(accessModes, string(accessMode))
+		}
+
+		anno["accessModes"] = strings.Join(accessModes, ",")
+
+		vs = &vsv1.VolumeSnapshot{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: anno,
+				Name:        vdSnapshot.Name,
+				Namespace:   vdSnapshot.Namespace,
+				OwnerReferences: []metav1.OwnerReference{
+					service.MakeOwnerReference(vdSnapshot),
+				},
+			},
+			Spec: vsv1.VolumeSnapshotSpec{
+				Source: vsv1.VolumeSnapshotSource{
+					PersistentVolumeClaimName: &pvc.Name,
+				},
+				VolumeSnapshotClassName: &vdSnapshot.Spec.VolumeSnapshotClassName,
+			},
+		}
+
+		vs, err = h.snapshotter.CreateVolumeSnapshot(ctx, vs)
 		if err != nil {
 			setPhaseConditionToFailed(&condition, &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
@@ -73,7 +73,7 @@ var _ = Describe("LifeCycle handler", func() {
 		}
 
 		snapshotter = &LifeCycleSnapshotterMock{
-			CreateVolumeSnapshotFunc: func(ctx context.Context, vdSnapshot *virtv2.VirtualDiskSnapshot, pvc *corev1.PersistentVolumeClaim) (*vsv1.VolumeSnapshot, error) {
+			CreateVolumeSnapshotFunc: func(_ context.Context, _ *vsv1.VolumeSnapshot) (*vsv1.VolumeSnapshot, error) {
 				return vs, nil
 			},
 			GetPersistentVolumeClaimFunc: func(_ context.Context, _, _ string) (*corev1.PersistentVolumeClaim, error) {

diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/internal/mock.go
diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/vdsnapshot_reconciler.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/vdsnapshot_reconciler.go
@@ -56,14 +56,14 @@ func NewReconciler(client client.Client, handlers ...Handler) *Reconciler {
 func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
 	log := logger.FromContext(ctx)
 
-	vdsnapshot := service.NewResource(req.NamespacedName, r.client, r.factory, r.statusGetter)
+	vdSnapshot := service.NewResource(req.NamespacedName, r.client, r.factory, r.statusGetter)
 
-	err := vdsnapshot.Fetch(ctx)
+	err := vdSnapshot.Fetch(ctx)
 	if err != nil {
 		return reconcile.Result{}, err
 	}
 
-	if vdsnapshot.IsEmpty() {
+	if vdSnapshot.IsEmpty() {
 		return reconcile.Result{}, nil
 	}
 
@@ -72,18 +72,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 
 	for _, h := range r.handlers {
 		var res reconcile.Result
-		res, err = h.Handle(ctx, vdsnapshot.Changed())
+		res, err = h.Handle(ctx, vdSnapshot.Changed())
 		if err != nil {
-			log.Error("Failed to handle vdsnapshot", logger.SlogErr(err), logger.SlogHandler(reflect.TypeOf(h).Elem().Name()))
+			log.Error("Failed to handle vdSnapshot", logger.SlogErr(err), logger.SlogHandler(reflect.TypeOf(h).Elem().Name()))
 			handlerErrs = append(handlerErrs, err)
 		}
 
 		result = service.MergeResults(result, res)
 	}
 
-	vdsnapshot.Changed().Status.ObservedGeneration = vdsnapshot.Changed().Generation
+	vdSnapshot.Changed().Status.ObservedGeneration = vdSnapshot.Changed().Generation
 
-	err = vdsnapshot.Update(ctx)
+	err = vdSnapshot.Update(ctx)
 	if err != nil {
 		return reconcile.Result{}, err
 	}

diff --git a/images/virtualization-artifact/pkg/controller/vdsnapshot/vdsnapshot_webhook.go b/images/virtualization-artifact/pkg/controller/vdsnapshot/vdsnapshot_webhook.go
@@ -37,20 +37,9 @@ func NewValidator(logger *slog.Logger) *Validator {
 	}
 }
 
-func (v *Validator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) {
-	vds, ok := obj.(*virtv2.VirtualDiskSnapshot)
-	if !ok {
-		return nil, fmt.Errorf("expected a VirtualDiskSnapshot but got a %T", obj)
-	}
-
-	if vds.Spec.VirtualDiskName == "" {
-		return nil, fmt.Errorf("virtual disk name cannot be empty")
-	}
-
-	if vds.Spec.VolumeSnapshotClassName == "" {
-		return nil, fmt.Errorf("volume snapshot class name cannot be empty")
-	}
-
+func (v *Validator) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
+	err := fmt.Errorf("misconfigured webhook rules: delete operation not implemented")
+	v.logger.Error("Ensure the correctness of ValidatingWebhookConfiguration", "err", err)
 	return nil, nil
 }
 

diff --git a/templates/virtualization-controller/validation-webhook.yaml b/templates/virtualization-controller/validation-webhook.yaml
@@ -143,7 +143,7 @@ webhooks:
     rules:
       - apiGroups:   ["virtualization.deckhouse.io"]
         apiVersions: ["v1alpha2"]
-        operations:  ["CREATE", "UPDATE"]
+        operations:  ["UPDATE"]
         resources:   ["virtualdisksnapshots"]
         scope:       "Namespaced"
     clientConfig:
@@ -172,4 +172,4 @@ webhooks:
       caBundle: |
         {{ .Values.virtualization.internal.controller.cert.ca }}
     admissionReviewVersions: ["v1"]
-    sideEffects: None
+    sideEffects: None