From b6b9122eb57a77734b556da4cb07fb46804ea0cc Mon Sep 17 00:00:00 2001
From: Marcell Sevcsik <31651557+0sewa0@users.noreply.github.com>
Date: Mon, 15 Jul 2024 13:15:44 +0200
Subject: [PATCH] Handle the force-delete scenario in the OS volume publisher
 (#3453) (#3460)

---
 .../csi/driver/volumes/host/publisher.go      | 41 ++++++++++++-------
 .../csi/driver/volumes/host/publisher_test.go | 25 +++++++++++
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/pkg/controllers/csi/driver/volumes/host/publisher.go b/pkg/controllers/csi/driver/volumes/host/publisher.go
index 45a7e5bd4e..77629bec0d 100644
--- a/pkg/controllers/csi/driver/volumes/host/publisher.go
+++ b/pkg/controllers/csi/driver/volumes/host/publisher.go
@@ -37,18 +37,23 @@ const failedToGetOsAgentVolumePrefix = "failed to get OSMount from database: "
 
 func NewHostVolumePublisher(fs afero.Afero, mounter mount.Interface, db metadata.Access, path metadata.PathResolver) csivolumes.Publisher {
 	return &HostVolumePublisher{
-		fs:      fs,
-		mounter: mounter,
-		db:      db,
-		path:    path,
+		fs:           fs,
+		mounter:      mounter,
+		db:           db,
+		path:         path,
+		isNotMounted: mount.IsNotMountPoint,
 	}
 }
 
+// necessary for mocking, as the MounterMock will use the os package
+type mountChecker func(mounter mount.Interface, file string) (bool, error)
+
 type HostVolumePublisher struct {
-	fs      afero.Afero
-	mounter mount.Interface
-	db      metadata.Access
-	path    metadata.PathResolver
+	fs           afero.Afero
+	mounter      mount.Interface
+	db           metadata.Access
+	isNotMounted mountChecker
+	path         metadata.PathResolver
 }
 
 func (publisher *HostVolumePublisher) PublishVolume(ctx context.Context, volumeCfg csivolumes.VolumeConfig) (*csi.NodePublishVolumeResponse, error) {
@@ -62,7 +67,19 @@ func (publisher *HostVolumePublisher) PublishVolume(ctx context.Context, volumeC
 		return nil, status.Error(codes.Internal, failedToGetOsAgentVolumePrefix+err.Error())
 	}
 
-	if osMount != nil && osMount.DeletedAt.Valid {
+	if osMount != nil {
+		if !osMount.DeletedAt.Valid {
+			// If the OSAgents were removed forcefully, we might not get the unmount request, so we can't fully relay on the database, and have to directly check if its mounted or not
+			isNotMounted, err := publisher.isNotMounted(publisher.mounter, osMount.Location)
+			if err != nil {
+				return nil, err
+			}
+
+			if !isNotMounted {
+				return &csi.NodePublishVolumeResponse{}, goerrors.New("previous OSMount is yet to be unmounted, there can be only 1 OSMount per tenant per node, blocking until unmount") // don't want to have the stacktrace here, it just pollutes the logs
+			}
+		}
+
 		osMount.VolumeMeta = metadata.VolumeMeta{
 			ID:      volumeCfg.VolumeID,
 			PodName: volumeCfg.PodName,
@@ -79,9 +96,7 @@ func (publisher *HostVolumePublisher) PublishVolume(ctx context.Context, volumeC
 		}
 
 		return &csi.NodePublishVolumeResponse{}, nil
-	}
-
-	if osMount == nil && errors.Is(err, gorm.ErrRecordNotFound) {
+	} else {
 		osMount := metadata.OSMount{
 			VolumeMeta:    metadata.VolumeMeta{ID: volumeCfg.VolumeID, PodName: volumeCfg.PodName},
 			VolumeMetaID:  volumeCfg.VolumeID,
@@ -98,8 +113,6 @@ func (publisher *HostVolumePublisher) PublishVolume(ctx context.Context, volumeC
 		if err := publisher.db.CreateOSMount(&osMount); err != nil {
 			return nil, status.Error(codes.Internal, fmt.Sprintf("failed to insert OSMount to database. info: %v err: %s", osMount, err.Error()))
 		}
-	} else {
-		return &csi.NodePublishVolumeResponse{}, goerrors.New("previous OSMount is yet to be unmounted, there can be only 1 OSMount per tenant per node, blocking until unmount") // don't want to have the stacktrace here, it just pollutes the logs
 	}
 
 	return &csi.NodePublishVolumeResponse{}, nil
diff --git a/pkg/controllers/csi/driver/volumes/host/publisher_test.go b/pkg/controllers/csi/driver/volumes/host/publisher_test.go
index 08d46d2829..c5b8b84e87 100644
--- a/pkg/controllers/csi/driver/volumes/host/publisher_test.go
+++ b/pkg/controllers/csi/driver/volumes/host/publisher_test.go
@@ -68,6 +68,28 @@ func TestPublishVolume(t *testing.T) {
 		require.Error(t, err)
 		assert.NotNil(t, response)
 	})
+
+	t.Run("publish volume when previous OSMount force deleted => do mount", func(t *testing.T) {
+		mounter := mount.NewFakeMounter([]mount.MountPoint{})
+		publisher := newPublisherForTesting(mounter)
+		mockDynakube(t, &publisher)
+
+		response, err := publisher.PublishVolume(ctx, createTestVolumeConfig())
+
+		require.NoError(t, err)
+		assert.NotNil(t, response)
+		assert.NotEmpty(t, mounter.MountPoints)
+		assertReferencesForPublishedVolume(t, &publisher, mounter)
+
+		publisher.isNotMounted = func(mounter mount.Interface, file string) (bool, error) {
+			return true, nil
+		}
+		response, err = publisher.PublishVolume(ctx, createTestVolumeConfig())
+		require.NoError(t, err)
+		assert.NotNil(t, response)
+		assert.NotEmpty(t, mounter.MountPoints)
+		assertReferencesForPublishedVolume(t, &publisher, mounter)
+	})
 }
 
 func TestUnpublishVolume(t *testing.T) {
@@ -134,6 +156,9 @@ func newPublisherForTesting(mounter *mount.FakeMounter) HostVolumePublisher {
 		mounter: mounter,
 		db:      metadata.FakeMemoryDB(),
 		path:    metadata.PathResolver{RootDir: csiOptions.RootDir},
+		isNotMounted: func(mounter mount.Interface, file string) (bool, error) {
+			return false, nil
+		},
 	}
 }