Repository: dell/csi-baremetal
Pull request #432 (2021-06-17 14:20:39)

[ISSUE-429] Invoke Fake Attach logic on NodeStage request
## Purpose
### Issue #429 

Implementation of #425

- Update proposal
- Skip error on NodeStage if PVC annotated
- Mount tmpfs on NodePublish if fake-attach

## PR checklist
- [x] Add link to the issue
- [x] Choose PR label
- [x] New unit tests added
- [x] Modified code has meaningful comments
- [ ] All TODOs are linked with the issues
- [ ] All comments are resolved

## Testing



Patch:
diff --git a/docs/proposals/fake-attach.md b/docs/fake-attach.md
similarity index 67%
rename from docs/proposals/fake-attach.md
rename to docs/fake-attach.md
index cccea6a..027f5a9 100644
--- a/docs/proposals/fake-attach.md
+++ b/docs/fake-attach.md
@@ -27,17 +27,21 @@ failure.
 This feature will require application Operator to put specific annotation on PVC
 `pv.attach.kubernetes.io/ignore-if-inaccessible: yes`
 
+If fake-attach volume is successfully created, CSI Volume CR will be annotated with `fake-attach:yes` and `Operational Status` will equal to `MISSING`. The pod must be restarted after `Operational Status` returns to `OPERATIVE`.
+
 ## Implementation
 
-When `pv.attach.kubernetes.io/ignore-if-inaccessible: yes` annotation is set CSI must ignore NodeStageVolume errors and
-put `fake-attach: yes` annotation on CSI Volume CR. On NodePublishVolume CSI must check `fake-attach` annotation and mount
-tmpfs volume in read-only mode.
+When `pv.attach.kubernetes.io/ignore-if-inaccessible: yes` annotation is set CSI must ignore NodeStageVolume errors and put `fake-attach: yes` annotation on CSI Volume CR. 
 
-Command to mount tmpfs volume: `mount -t tmpfs -o size=1K -o ro tmpfs <destination folder>`
+On NodePublishVolume CSI must check `fake-attach` annotation and mount
+tmpfs volume in read-only mode.
+Command to mount tmpfs volume: `mount -t tmpfs -o size=1M,ro <volumeID> <destination folder>`
 
 CSI must generate event to notify user about this Fake-Attach Volume.
 
-On NodeUnstage tmpfs volume must be deleted.
+On NodeUnpublishVolume tmpfs volume must be unmounted usually.
+
+On NodeUnstageVolume `fake-attach` annotation must be deleted.
 
 ## Assumptions (if applicable)
 
diff --git a/pkg/mocks/provisioners/fs_operations.go b/pkg/mocks/provisioners/fs_operations.go
index 9b50f55..9649741 100644
--- a/pkg/mocks/provisioners/fs_operations.go
+++ b/pkg/mocks/provisioners/fs_operations.go
@@ -30,6 +30,13 @@ func (m *MockFsOpts) PrepareAndPerformMount(src, dst string, bindMount, dstIsDir
 	return args.Error(0)
 }
 
+// MountFakeTmpfs is a mock implementation
+func (m *MockFsOpts) MountFakeTmpfs(volumeID, path string) error {
+	args := m.Mock.Called(volumeID, path)
+
+	return args.Error(0)
+}
+
 // UnmountWithCheck is a mock implementation
 func (m *MockFsOpts) UnmountWithCheck(path string) error {
 	args := m.Mock.Called(path)
diff --git a/pkg/node/node.go b/pkg/node/node.go
index 19e0f0b..65a99d4 100644
--- a/pkg/node/node.go
+++ b/pkg/node/node.go
@@ -48,7 +48,12 @@ import (
 	csibmnodeconst "github.com/dell/csi-baremetal/pkg/crcontrollers/operator/common"
 )
 
-const stagingFileName = "dev"
+const (
+	stagingFileName = "dev"
+
+	fakeAttachVolumeAnnotation = "fake-attach"
+	fakeAttachVolumeKey        = "yes"
+)
 
 // CSINodeService is the implementation of NodeServer interface from GO CSI specification.
 // Contains VolumeManager in a such way that it is a single instance in the driver
@@ -161,7 +166,7 @@ func (s *CSINodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 		return nil, status.Error(codes.InvalidArgument, "Stage Path missing in request")
 	}
 
-	volumeID := req.VolumeId
+	volumeID := req.GetVolumeId()
 	volumeCR, err := s.crHelper.GetVolumeByID(volumeID)
 	if err != nil {
 		message := fmt.Sprintf("Unable to find volume with ID %s", volumeID)
@@ -170,39 +175,60 @@ func (s *CSINodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStage
 	}
 
 	currStatus := volumeCR.Spec.CSIStatus
-	// if currStatus not in [Created (first call), VolumeReady (retry), Published (multiple pods)]
-	if currStatus != apiV1.Created && currStatus != apiV1.VolumeReady && currStatus != apiV1.Published {
-		ll.Errorf("Current volume CR status - %s, expected to be in - [%s, %s, %s]",
-			currStatus, apiV1.Created, apiV1.VolumeReady, apiV1.Published)
-		return nil, fmt.Errorf("corresponding volume CR is in unexpected state - %s",
-			currStatus)
+	switch currStatus {
+	// expected currStatus in [Created (first call), VolumeReady (retry), Published (multiple pods)]
+	case apiV1.Created, apiV1.VolumeReady, apiV1.Published:
+		ll.Infof("Volume status: %s", currStatus)
+	// also need to retry on FAILED
+	case apiV1.Failed:
+		ll.Warningf("Volume status: %s. Need to retry.", currStatus)
+	default:
+		ll.Errorf("Unexpected volume status: %s", currStatus)
+		return nil, fmt.Errorf("corresponding volume is in unexpected state - %s", currStatus)
+	}
+
+	var (
+		resp        = &csi.NodeStageVolumeResponse{}
+		errToReturn error
+		newStatus   = apiV1.VolumeReady
+	)
+
+	if volumeCR.Annotations == nil {
+		volumeCR.Annotations = make(map[string]string)
 	}
 
 	targetPath := getStagingPath(ll, req.GetStagingTargetPath())
 
+	isFakeAttach := false
+	ignoreErrorIfFakeAttach := func(err error) {
+		if s.isPVCNeedFakeAttach(volumeID) {
+			isFakeAttach = true
+		} else {
+			newStatus = apiV1.Failed
+			resp, errToReturn = nil, status.Error(codes.Internal, fmt.Sprintf("failed to stage volume: %s", err.Error()))
+		}
+	}
+
 	partition, err := s.getProvisionerForVolume(&volumeCR.Spec).GetVolumePath(volumeCR.Spec)
 	if err != nil {
-		ll.Errorf("failed to get partition, for volume %v: %v", volumeCR.Spec, err)
-		ll.Infof("Volume %s can be ignored - %t. Mount fake path...", volumeID, s.isPVCNeedFakeAttach(volumeID))
-		return nil, status.Error(codes.Internal, "failed to stage volume: partition error")
+		ll.Errorf("failed to get partition for volume %v: %v", volumeCR.Spec, err)
+		ignoreErrorIfFakeAttach(err)
+	} else {
+		ll.Infof("Partition to stage: %s", partition)
+		if err := s.fsOps.PrepareAndPerformMount(partition, targetPath, true, false); err != nil {
+			ll.Errorf("Unable to stage volume: %v", err)
+			ignoreErrorIfFakeAttach(err)
+		}
 	}
-	ll.Infof("Work with partition %s", partition)
 
-	var (
-		resp        = &csi.NodeStageVolumeResponse{}
-		errToReturn error
-		newStatus   = apiV1.VolumeReady
-	)
-	if err := s.fsOps.PrepareAndPerformMount(partition, targetPath, true, false); err != nil {
-		ll.Errorf("Unable to prepare and mount: %v. Going to set volumes status to failed", err)
-		ll.Infof("Volume %s can be ignored - %t. Mount fake path...", volumeID, s.isPVCNeedFakeAttach(volumeID))
-		newStatus = apiV1.Failed
-		resp, errToReturn = nil, status.Error(codes.Internal, "failed to stage volume: mount error")
+	if isFakeAttach {
+		volumeCR.Annotations["fake-attach"] = "yes"
+		ll.Warningf("Adding fake-attach annotation to the volume with ID %s", volumeID)
 	}
 
-	if currStatus != apiV1.VolumeReady || newStatus == apiV1.Failed {
+	if currStatus != apiV1.VolumeReady {
 		volumeCR.Spec.CSIStatus = newStatus
-		if err := s.crHelper.UpdateVolumeCRSpec(volumeCR.Name, volumeCR.Namespace, volumeCR.Spec); err != nil {
+		if err := s.k8sClient.UpdateCR(ctx, volumeCR); err != nil {
 			ll.Errorf("Unable to set volume status to %s: %v", newStatus, err)
 			resp, errToReturn = nil, fmt.Errorf("failed to stage volume: update volume CR error")
 		}
@@ -240,9 +266,13 @@ func (s *CSINodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUns
 	if len(req.GetStagingTargetPath()) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "Stage Path missing in request")
 	}
-	volumeCR, err := s.crHelper.GetVolumeByID(req.GetVolumeId())
+
+	volumeID := req.GetVolumeId()
+	volumeCR, err := s.crHelper.GetVolumeByID(volumeID)
 	if err != nil {
-		return nil, status.Error(codes.NotFound, "Unable to find volume")
+		message := fmt.Sprintf("Unable to find volume with ID %s", volumeID)
+		ll.Error(message)
+		return nil, status.Error(codes.NotFound, message)
 	}
 
 	currStatus := volumeCR.Spec.CSIStatus
@@ -262,7 +292,11 @@ func (s *CSINodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUns
 		resp        = &csi.NodeUnstageVolumeResponse{}
 		errToReturn error
 	)
-	if errToReturn = s.fsOps.UnmountWithCheck(getStagingPath(ll, req.GetStagingTargetPath())); errToReturn != nil {
+
+	if volumeCR.Annotations[fakeAttachVolumeAnnotation] == fakeAttachVolumeKey {
+		ll.Warningf("Removing fake-attach annotation for volume %s", volumeID)
+		delete(volumeCR.Annotations, fakeAttachVolumeAnnotation)
+	} else if errToReturn = s.fsOps.UnmountWithCheck(getStagingPath(ll, req.GetStagingTargetPath())); errToReturn != nil {
 		volumeCR.Spec.CSIStatus = apiV1.Failed
 		resp = nil
 	}
@@ -367,11 +401,18 @@ func (s *CSINodeService) NodePublishVolume(ctx context.Context, req *csi.NodePub
 		errToReturn error
 	)
 
-	_, isBlock := req.GetVolumeCapability().GetAccessType().(*csi.VolumeCapability_Block)
-	if err := s.fsOps.PrepareAndPerformMount(srcPath, dstPath, isBlock, !isBlock); err != nil {
-		ll.Errorf("Unable to mount volume: %v", err)
-		newStatus = apiV1.Failed
-		resp, errToReturn = nil, fmt.Errorf("failed to publish volume: mount error")
+	if volumeCR.Annotations[fakeAttachVolumeAnnotation] == fakeAttachVolumeKey {
+		if err := s.fsOps.MountFakeTmpfs(volumeID, dstPath); err != nil {
+			newStatus = apiV1.Failed
+			resp, errToReturn = nil, fmt.Errorf("failed to publish volume: fake attach error %s", err.Error())
+		}
+	} else {
+		_, isBlock := req.GetVolumeCapability().GetAccessType().(*csi.VolumeCapability_Block)
+		if err := s.fsOps.PrepareAndPerformMount(srcPath, dstPath, isBlock, !isBlock); err != nil {
+			ll.Errorf("Unable to mount volume: %v", err)
+			newStatus = apiV1.Failed
+			resp, errToReturn = nil, fmt.Errorf("failed to publish volume: mount error %s", err.Error())
+		}
 	}
 
 	var podName string
diff --git a/pkg/node/provisioners/utilwrappers/fs_operations.go b/pkg/node/provisioners/utilwrappers/fs_operations.go
index 3133a65..68031ec 100644
--- a/pkg/node/provisioners/utilwrappers/fs_operations.go
+++ b/pkg/node/provisioners/utilwrappers/fs_operations.go
@@ -31,6 +31,8 @@ type FSOperations interface {
 	// PrepareAndPerformMount composite methods which is prepare source and destination directories
 	// and performs mount operation from src to dst
 	PrepareAndPerformMount(src, dst string, bindMount, dstIsDir bool) error
+	// MountFakeTmpfs does attach of a temporary folder on failure
+	MountFakeTmpfs(volumeID, dst string) error
 	// UnmountWithCheck unmount operation
 	UnmountWithCheck(path string) error
 	fs.WrapFS
@@ -147,3 +149,26 @@ func (fsOp *FSOperationsImpl) UnmountWithCheck(path string) error {
 
 	return fsOp.Unmount(path)
 }
+
+// MountFakeTmpfs does attach of temp folder in read only mode
+func (fsOp *FSOperationsImpl) MountFakeTmpfs(volumeID, dst string) error {
+	/*
+		CMD example:
+			mount -t tmpfs -o size=1M,ro <volumeID> <dst>
+	*/
+	ll := fsOp.log.WithFields(logrus.Fields{"method": "FakeAttach"})
+
+	ll.Warningf("Simulate attachment")
+	_, err := os.Stat(dst)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return err
+		}
+		createCMD := fsOp.MkDir
+		if err = createCMD(dst); err != nil {
+			return err
+		}
+	}
+
+	return fsOp.Mount(volumeID, dst, "-t tmpfs -o size=1M,ro")
+}


Test patch:
diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go
index 08818cd..373fb12 100644
--- a/pkg/node/node_test.go
+++ b/pkg/node/node_test.go
@@ -19,7 +19,6 @@ package node
 import (
 	"errors"
 	"fmt"
-
 	"path"
 	"testing"
 	"time"
@@ -32,6 +31,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/health/grpc_health_v1"
 	"google.golang.org/grpc/status"
+	corev1 "k8s.io/api/core/v1"
 
 	api "github.com/dell/csi-baremetal/api/generated/v1"
 	apiV1 "github.com/dell/csi-baremetal/api/v1"
@@ -280,7 +280,7 @@ var _ = Describe("CSINodeService NodeStage()", func() {
 			resp, err := node.NodeStageVolume(testCtx, req)
 			Expect(resp).To(BeNil())
 			Expect(err).NotTo(BeNil())
-			Expect(err.Error()).To(ContainSubstring("partition error"))
+			Expect(err.Error()).To(ContainSubstring("failed to stage volume"))
 			Expect(status.Code(err)).To(Equal(codes.Internal))
 		})
 		It("Failed because PrepareAndPerformMount had failed", func() {
@@ -313,17 +313,6 @@ var _ = Describe("CSINodeService NodeStage()", func() {
 			Expect(err).NotTo(BeNil())
 			Expect(err.Error()).To(ContainSubstring("mount error"))
 		})
-		It("Should fail, because Volume has failed status", func() {
-			req := getNodeStageRequest(testV1ID, *testVolumeCap)
-			vol1 := testVolumeCR1
-			vol1.Spec.CSIStatus = apiV1.Failed
-			err := node.k8sClient.UpdateCR(testCtx, &vol1)
-			Expect(err).To(BeNil())
-
-			resp, err := node.NodeStageVolume(testCtx, req)
-			Expect(resp).To(BeNil())
-			Expect(err).NotTo(BeNil())
-		})
 	})
 })
 
@@ -689,6 +678,95 @@ var _ = Describe("CSINodeService Probe()", func() {
 	})
 })
 
+var _ = Describe("CSINodeService Fake-Attach", func() {
+	BeforeEach(func() {
+		setVariables()
+	})
+
+	It("Should stage volume with fake-attach annotation", func() {
+		req := getNodeStageRequest(testVolume1.Id, *testVolumeCap)
+		vol1 := testVolumeCR1
+		vol1.Spec.CSIStatus = apiV1.Created
+		err := node.k8sClient.UpdateCR(testCtx, &vol1)
+		Expect(err).To(BeNil())
+
+		pvcName := "pvcName"
+		pvcNamespace := "pvcNamespace"
+
+		pv := &corev1.PersistentVolume{}
+		pv.Name = vol1.Name
+		pv.Spec.ClaimRef = &corev1.ObjectReference{}
+		pv.Spec.ClaimRef.Name = pvcName
+		pv.Spec.ClaimRef.Namespace = pvcNamespace
+		err = node.k8sClient.Create(testCtx, pv)
+		Expect(err).To(BeNil())
+
+		pvc := &corev1.PersistentVolumeClaim{}
+		pvc.Name = pvcName
+		pvc.Namespace = pvcNamespace
+		pvc.Annotations = map[string]string{fakeAttachAnnotation: fakeAttachAllowKey}
+		err = node.k8sClient.Create(testCtx, pvc)
+		Expect(err).To(BeNil())
+
+		partitionPath := "/partition/path/for/volume1"
+		prov.On("GetVolumePath", vol1.Spec).Return(partitionPath, nil)
+		fsOps.On("PrepareAndPerformMount",
+			partitionPath, path.Join(req.GetStagingTargetPath(), stagingFileName), true, false).
+			Return(errors.New("mount error"))
+
+		resp, err := node.NodeStageVolume(testCtx, req)
+		Expect(resp).NotTo(BeNil())
+		Expect(err).To(BeNil())
+
+		err = node.k8sClient.ReadCR(testCtx, testV1ID, "", &vol1)
+		Expect(err).To(BeNil())
+		Expect(vol1.Spec.CSIStatus).To(Equal(apiV1.VolumeReady))
+		Expect(vol1.Annotations[fakeAttachVolumeAnnotation]).To(Equal(fakeAttachVolumeKey))
+	})
+	It("Should publish volume with fake-attach annotation", func() {
+		req := getNodePublishRequest(testV1ID, targetPath, *testVolumeCap)
+		req.VolumeContext[util.PodNameKey] = testPodName
+
+		vol1 := testVolumeCR1
+		vol1.Annotations = map[string]string{fakeAttachVolumeAnnotation: fakeAttachVolumeKey}
+		err := node.k8sClient.UpdateCR(testCtx, &vol1)
+		Expect(err).To(BeNil())
+
+		fsOps.On("MountFakeTmpfs",
+			testV1ID, req.GetTargetPath()).
+			Return(nil)
+
+		resp, err := node.NodePublishVolume(testCtx, req)
+		Expect(resp).NotTo(BeNil())
+		Expect(err).To(BeNil())
+
+		// check owner appearance
+		volumeCR := &vcrd.Volume{}
+		err = node.k8sClient.ReadCR(testCtx, testV1ID, "", volumeCR)
+		Expect(err).To(BeNil())
+		Expect(volumeCR.Spec.Owners[0]).To(Equal(testPodName))
+	})
+	It("Should unstage volume with fake-attach", func() {
+		req := getNodeUnstageRequest(testV1ID, stagePath)
+
+		vol1 := testVolumeCR1
+		vol1.Annotations = map[string]string{fakeAttachVolumeAnnotation: fakeAttachVolumeKey}
+		err := node.k8sClient.UpdateCR(testCtx, &vol1)
+		Expect(err).To(BeNil())
+
+		resp, err := node.NodeUnstageVolume(testCtx, req)
+		Expect(resp).NotTo(BeNil())
+		Expect(err).To(BeNil())
+		// check owners and CSI status
+		volumeCR := &vcrd.Volume{}
+		err = node.k8sClient.ReadCR(testCtx, testV1ID, "", volumeCR)
+		Expect(err).To(BeNil())
+		Expect(volumeCR.Spec.CSIStatus).To(Equal(apiV1.Created))
+		_, ok := volumeCR.Annotations[fakeAttachVolumeAnnotation]
+		Expect(ok).To(Equal(false))
+	})
+})
+
 func getNodePublishRequest(volumeID, targetPath string, volumeCap csi.VolumeCapability) *csi.NodePublishVolumeRequest {
 	return &csi.NodePublishVolumeRequest{
 		VolumeId:          volumeID,