fix(vdsnapshot): resolve fsfreeze race condition

api/core/v1alpha2/vdscondition/condition.go

@@ -63,6 +63,8 @@ const (
 	VolumeSnapshotLost VirtualDiskSnapshotReadyReason = "Lost"
 	// FileSystemFreezing signifies that the `VirtualDiskSnapshot` resource is in the process of freezing the filesystem of the virtual machine associated with the source virtual disk.
 	FileSystemFreezing VirtualDiskSnapshotReadyReason = "FileSystemFreezing"
+	// FileSystemUnfreezing signifies that the `VirtualDiskSnapshot` resource is in the process of unfreezing the filesystem of the virtual machine associated with the source virtual disk.
+	FileSystemUnfreezing VirtualDiskSnapshotReadyReason = "FileSystemUnfreezing"
 	// Snapshotting signifies that the `VirtualDiskSnapshot` resource is in the process of taking a snapshot of the virtual disk.
 	Snapshotting VirtualDiskSnapshotReadyReason = "Snapshotting"
 	// VirtualDiskSnapshotReady signifies that the snapshot process is complete and the `VirtualDiskSnapshot` is ready for use.

api/core/v1alpha2/vmscondition/condition.go

@@ -49,6 +49,8 @@ const (
 	VirtualDiskSnapshotLost VirtualMachineSnapshotReadyReason = "VirtualDiskSnapshotLost"
 	// FileSystemFreezing signifies that the `VirtualMachineSnapshot` resource is in the process of freezing the filesystem of the virtual machine.
 	FileSystemFreezing VirtualMachineSnapshotReadyReason = "FileSystemFreezing"
+	// FileSystemUnfreezing signifies that the `VirtualMachineSnapshot` resource is in the process of unfreezing the filesystem of the virtual machine.
+	FileSystemUnfreezing VirtualMachineSnapshotReadyReason = "FileSystemUnfreezing"
 	// Snapshotting signifies that the `VirtualMachineSnapshot` resource is in the process of taking a snapshot of the virtual machine.
 	Snapshotting VirtualMachineSnapshotReadyReason = "Snapshotting"
 	// VirtualMachineSnapshotReady signifies that the snapshot process is complete and the `VirtualMachineSnapshot` is ready for use.

images/virtualization-artifact/pkg/common/annotations/annotations.go

@@ -187,6 +187,9 @@ const (
 	AnnVMOPUID = AnnAPIGroupV + "/vmop-uid"
 	// AnnVMOPSnapshotName is an annotation on vmop that represents name a snapshot created for VMOP.
 	AnnVMOPSnapshotName = AnnAPIGroupV + "/vmop-snapshot-name"
+
+	// AnnVMFilesystemRequest is an annotation on a virtual machine that indicates a request to freeze or unfreeze the filesystem has been sent.
+	AnnVMFilesystemRequest = AnnAPIGroupV + "/virtual-machine-filesystem-request"
 )
 
 // AddAnnotation adds an annotation to an object

images/virtualization-artifact/pkg/controller/service/snapshot_service.go

@@ -18,23 +18,34 @@ package service
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	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"
 
+	"github.com/deckhouse/virtualization-controller/pkg/common/annotations"
 	"github.com/deckhouse/virtualization-controller/pkg/common/object"
-	"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
 	"github.com/deckhouse/virtualization/api/client/kubeclient"
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
-	"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
 	subv1alpha2 "github.com/deckhouse/virtualization/api/subresources/v1alpha2"
 )
 
+const (
+	RequestFSFreeze   = "freeze"
+	RequestFSUnfreeze = "unfreeze"
+	FSFrozen          = "frozen"
+)
+
+var (
+	ErrUntrustedFilesystemFrozenCondition = errors.New("the filesystem status is not synced")
+	ErrUnexpectedFilesystemRequest        = errors.New("found unexpected filesystem request in the virtual machine instance annotations")
+)
+
 type SnapshotService struct {
 	virtClient kubeclient.Client
 	client     Client
@@ -49,37 +60,75 @@ func NewSnapshotService(virtClient kubeclient.Client, client Client, protection
 	}
 }
 
-func (s *SnapshotService) IsFrozen(vm *v1alpha2.VirtualMachine) bool {
-	if vm == nil {
-		return false
+// IsFrozen checks if a freeze or unfreeze request has been performed
+// and returns the "true" fsFreezeStatus if the internal virtual machine instance is "frozen",
+// and "false" otherwise.
+func (s *SnapshotService) IsFrozen(kvvmi *virtv1.VirtualMachineInstance) (bool, error) {
+	if kvvmi == nil {
+		return false, nil
 	}
 
-	filesystemFrozen, _ := conditions.GetCondition(vmcondition.TypeFilesystemFrozen, vm.Status.Conditions)
+	if request, ok := kvvmi.Annotations[annotations.AnnVMFilesystemRequest]; ok {
+		return false, fmt.Errorf("failed to check %s/%s fsFreezeStatus: request: %s: %w", kvvmi.Namespace, kvvmi.Name, request, ErrUntrustedFilesystemFrozenCondition)
+	}
 
-	return filesystemFrozen.Status == metav1.ConditionTrue && filesystemFrozen.Reason == vmcondition.ReasonFilesystemFrozen.String()
+	return kvvmi.Status.FSFreezeStatus == FSFrozen, nil
 }
 
-func (s *SnapshotService) CanFreeze(vm *v1alpha2.VirtualMachine) bool {
-	if vm == nil || vm.Status.Phase != v1alpha2.MachineRunning || s.IsFrozen(vm) {
-		return false
+func (s *SnapshotService) CanFreeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) (bool, error) {
+	if kvvmi == nil || kvvmi.Status.Phase != virtv1.Running {
+		return false, nil
+	}
+
+	isFrozen, err := s.IsFrozen(kvvmi)
+	if err != nil {
+		return false, err
+	}
+	if isFrozen {
+		return false, nil
 	}
 
-	agentReady, _ := conditions.GetCondition(vmcondition.TypeAgentReady, vm.Status.Conditions)
+	for _, c := range kvvmi.Status.Conditions {
+		if c.Type == virtv1.VirtualMachineInstanceAgentConnected {
+			return c.Status == corev1.ConditionTrue, nil
+		}
+	}
 
-	return agentReady.Status == metav1.ConditionTrue
+	return false, nil
 }
 
-func (s *SnapshotService) Freeze(ctx context.Context, name, namespace string) error {
-	err := s.virtClient.VirtualMachines(namespace).Freeze(ctx, name, subv1alpha2.VirtualMachineFreeze{})
+// TODO: The Freeze method should be atomic because there is a chance of encountering
+// a dead-end with the `ErrUntrustedFilesystemFrozenCondition` error in the SyncFSFreezeRequest method
+// when the API returns an error on an freeze request.
+func (s *SnapshotService) Freeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error {
+	if request, ok := kvvmi.Annotations[annotations.AnnVMFilesystemRequest]; ok {
+		return fmt.Errorf("failed to freeze %s/%s virtual machine filesystem: %w: request type: %s", kvvmi.Namespace, kvvmi.Name, ErrUnexpectedFilesystemRequest, request)
+	}
+
+	err := s.annotateWithFSFreezeRequest(ctx, RequestFSFreeze, kvvmi)
 	if err != nil {
-		return fmt.Errorf("failed to freeze virtual machine %s/%s: %w", namespace, name, err)
+		return fmt.Errorf("failed to annotate internal virtual machine instance with filesystem freeze request: %w", err)
+	}
+
+	err = s.virtClient.VirtualMachines(kvvmi.Namespace).Freeze(ctx, kvvmi.Name, subv1alpha2.VirtualMachineFreeze{})
+	if err != nil {
+		return fmt.Errorf("failed to freeze %s/%s virtual machine filesystem: %w", kvvmi.Namespace, kvvmi.Name, err)
 	}
 
 	return nil
 }
 
-func (s *SnapshotService) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *v1alpha2.VirtualMachine) (bool, error) {
-	if vm == nil || !s.IsFrozen(vm) {
+func (s *SnapshotService) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *v1alpha2.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) (bool, error) {
+	if vm == nil {
+		return false, nil
+	}
+
+	isFrozen, err := s.IsFrozen(kvvmi)
+	if err != nil {
+		return false, err
+	}
+
+	if !isFrozen {
 		return false, nil
 	}
 
@@ -91,7 +140,7 @@ func (s *SnapshotService) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context
 	}
 
 	var vdSnapshots v1alpha2.VirtualDiskSnapshotList
-	err := s.client.List(ctx, &vdSnapshots, &client.ListOptions{
+	err = s.client.List(ctx, &vdSnapshots, &client.ListOptions{
 		Namespace: vm.Namespace,
 	})
 	if err != nil {
@@ -126,8 +175,16 @@ func (s *SnapshotService) CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context
 	return true, nil
 }
 
-func (s *SnapshotService) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Context, vmSnapshotName string, vm *v1alpha2.VirtualMachine) (bool, error) {
-	if vm == nil || !s.IsFrozen(vm) {
+func (s *SnapshotService) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Context, vmSnapshotName string, vm *v1alpha2.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) (bool, error) {
+	if vm == nil {
+		return false, nil
+	}
+
+	isFrozen, err := s.IsFrozen(kvvmi)
+	if err != nil {
+		return false, err
+	}
+	if !isFrozen {
 		return false, nil
 	}
 
@@ -139,7 +196,7 @@ func (s *SnapshotService) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Cont
 	}
 
 	var vdSnapshots v1alpha2.VirtualDiskSnapshotList
-	err := s.client.List(ctx, &vdSnapshots, &client.ListOptions{
+	err = s.client.List(ctx, &vdSnapshots, &client.ListOptions{
 		Namespace: vm.Namespace,
 	})
 	if err != nil {
@@ -174,10 +231,22 @@ func (s *SnapshotService) CanUnfreezeWithVirtualMachineSnapshot(ctx context.Cont
 	return true, nil
 }
 
-func (s *SnapshotService) Unfreeze(ctx context.Context, name, namespace string) error {
-	err := s.virtClient.VirtualMachines(namespace).Unfreeze(ctx, name)
+// TODO: The Unfreeze method should be atomic because there is a chance of encountering
+// a dead-end with the `ErrUntrustedFilesystemFrozenCondition` error in the SyncFSFreezeRequest method
+// when the API returns an error on an unfreeze request.
+func (s *SnapshotService) Unfreeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error {
+	if request, ok := kvvmi.Annotations[annotations.AnnVMFilesystemRequest]; ok {
+		return fmt.Errorf("failed to unfreeze %s/%s virtual machine filesystem: %w: request type: %s", kvvmi.Namespace, kvvmi.Name, ErrUnexpectedFilesystemRequest, request)
+	}
+
+	err := s.annotateWithFSFreezeRequest(ctx, RequestFSUnfreeze, kvvmi)
 	if err != nil {
-		return fmt.Errorf("unfreeze virtual machine %s/%s: %w", namespace, name, err)
+		return fmt.Errorf("failed to annotate internal virtual machine instance with filesystem unfreeze request: %w", err)
+	}
+
+	err = s.virtClient.VirtualMachines(kvvmi.Namespace).Unfreeze(ctx, kvvmi.Name)
+	if err != nil {
+		return fmt.Errorf("unfreeze virtual machine %s/%s: %w", kvvmi.Namespace, kvvmi.Name, err)
 	}
 
 	return nil
@@ -243,3 +312,75 @@ func (s *SnapshotService) CreateVirtualDiskSnapshot(ctx context.Context, vdSnaps
 
 	return vdSnapshot, nil
 }
+
+func (s *SnapshotService) GetVirtualMachineInstance(ctx context.Context, vm *v1alpha2.VirtualMachine) (*virtv1.VirtualMachineInstance, error) {
+	if vm == nil {
+		return nil, nil
+	}
+	return object.FetchObject(ctx, client.ObjectKeyFromObject(vm), s.client, &virtv1.VirtualMachineInstance{})
+}
+
+func (s *SnapshotService) annotateWithFSFreezeRequest(ctx context.Context, requestType string, kvvmi *virtv1.VirtualMachineInstance) error {
+	if kvvmi == nil {
+		return fmt.Errorf("failed to annotate virtual machine instance; virtual machine instance cannot be nil")
+	}
+
+	if kvvmi.Annotations == nil {
+		kvvmi.Annotations = make(map[string]string)
+	}
+	kvvmi.Annotations[annotations.AnnVMFilesystemRequest] = requestType
+
+	err := s.client.Update(ctx, kvvmi)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (s *SnapshotService) removeAnnFSFreezeRequest(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error {
+	if kvvmi == nil {
+		return fmt.Errorf("failed to annotate virtual machine instance; virtual machine instance cannot be nil")
+	}
+
+	if kvvmi.Annotations == nil {
+		return nil
+	}
+
+	delete(kvvmi.Annotations, annotations.AnnVMFilesystemRequest)
+
+	err := s.client.Update(ctx, kvvmi)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func (s *SnapshotService) SyncFSFreezeRequest(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error {
+	if kvvmi == nil {
+		return nil
+	}
+
+	request, ok := kvvmi.Annotations[annotations.AnnVMFilesystemRequest]
+	if !ok {
+		return nil
+	}
+
+	switch {
+	case request == RequestFSFreeze && kvvmi.Status.FSFreezeStatus == FSFrozen:
+		err := s.removeAnnFSFreezeRequest(ctx, kvvmi)
+		if err != nil {
+			return fmt.Errorf("failed to sync kvvmi %s/%s fsFreezeStatus: request: %s: %w", kvvmi.Namespace, kvvmi.Name, request, err)
+		}
+		return nil
+	case request == RequestFSUnfreeze && kvvmi.Status.FSFreezeStatus != FSFrozen:
+		err := s.removeAnnFSFreezeRequest(ctx, kvvmi)
+		if err != nil {
+			return fmt.Errorf("failed to sync kvvmi %s/%s fsFreezeStatus: request: %s: %w", kvvmi.Namespace, kvvmi.Name, request, err)
+		}
+		return nil
+	default:
+		return fmt.Errorf("failed to sync kvvmi %s/%s fsFreezeStatus: request: %s: %w", kvvmi.Namespace, kvvmi.Name, request, ErrUntrustedFilesystemFrozenCondition)
+	}
+}

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

@@ -18,7 +18,10 @@ package internal
 
 import (
 	"context"
+	"errors"
+	"time"
 
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -61,6 +64,11 @@ func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *v1alpha2.Virtua
 			}
 		}
 
+		kvvmi, err := h.snapshotter.GetVirtualMachineInstance(ctx, vm)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+
 		if vs != nil {
 			err = h.snapshotter.DeleteVolumeSnapshot(ctx, vs)
 			if err != nil {
@@ -70,14 +78,20 @@ func (h DeletionHandler) Handle(ctx context.Context, vdSnapshot *v1alpha2.Virtua
 
 		if vm != nil {
 			var canUnfreeze bool
-			canUnfreeze, err = h.snapshotter.CanUnfreezeWithVirtualDiskSnapshot(ctx, vdSnapshot.Name, vm)
+			canUnfreeze, err = h.snapshotter.CanUnfreezeWithVirtualDiskSnapshot(ctx, vdSnapshot.Name, vm, kvvmi)
 			if err != nil {
+				if errors.Is(err, service.ErrUntrustedFilesystemFrozenCondition) {
+					return reconcile.Result{}, nil
+				}
 				return reconcile.Result{}, err
 			}
 
 			if canUnfreeze {
-				err = h.snapshotter.Unfreeze(ctx, vm.Name, vm.Namespace)
+				err = h.snapshotter.Unfreeze(ctx, kvvmi)
 				if err != nil {
+					if k8serrors.IsConflict(err) {
+						return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
+					}
 					return reconcile.Result{}, err
 				}
 			}

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

@@ -21,6 +21,7 @@ import (
 
 	vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
+	virtv1 "kubevirt.io/api/core/v1"
 
 	"github.com/deckhouse/virtualization/api/core/v1alpha2"
 )
@@ -32,14 +33,16 @@ type VirtualDiskReadySnapshotter interface {
 }
 
 type LifeCycleSnapshotter interface {
-	Freeze(ctx context.Context, name, namespace string) error
-	IsFrozen(vm *v1alpha2.VirtualMachine) bool
-	CanFreeze(vm *v1alpha2.VirtualMachine) bool
-	CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *v1alpha2.VirtualMachine) (bool, error)
-	Unfreeze(ctx context.Context, name, namespace string) error
+	Freeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error
+	IsFrozen(kvvmi *virtv1.VirtualMachineInstance) (bool, error)
+	CanFreeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) (bool, error)
+	CanUnfreezeWithVirtualDiskSnapshot(ctx context.Context, vdSnapshotName string, vm *v1alpha2.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) (bool, error)
+	Unfreeze(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) 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) (*v1alpha2.VirtualDisk, error)
 	GetVirtualMachine(ctx context.Context, name, namespace string) (*v1alpha2.VirtualMachine, error)
 	GetVolumeSnapshot(ctx context.Context, name, namespace string) (*vsv1.VolumeSnapshot, error)
+	SyncFSFreezeRequest(ctx context.Context, kvvmi *virtv1.VirtualMachineInstance) error
+	GetVirtualMachineInstance(ctx context.Context, vm *v1alpha2.VirtualMachine) (*virtv1.VirtualMachineInstance, error)
 }