Repository: topolvm/topolvm
Pull request #693 (2023-04-04 10:41:41)

Cleanup publish test
The test contains conformance tests, the validation test that can be done by unit test, and the mount option checking. I removed the first because we used csi-test, and converted the second as a unit test. The last only remains, so I changed the test file name to be a more appropriate one.


Patch:
diff --git a/driver/node.go b/driver/node.go
index 3976e524..61475ef7 100644
--- a/driver/node.go
+++ b/driver/node.go
@@ -180,6 +180,27 @@ func (s *nodeServerNoLocked) NodePublishVolume(ctx context.Context, req *csi.Nod
 	return &csi.NodePublishVolumeResponse{}, nil
 }
 
+func makeMountOptions(readOnly bool, mountOption *csi.VolumeCapability_MountVolume) ([]string, error) {
+	var mountOptions []string
+	if readOnly {
+		mountOptions = append(mountOptions, "ro")
+	}
+
+	for _, f := range mountOption.MountFlags {
+		if f == "rw" && readOnly {
+			return nil, status.Error(codes.InvalidArgument, "mount option \"rw\" is specified even though read only mode is specified")
+		}
+		mountOptions = append(mountOptions, f)
+	}
+
+	// avoid duplicate UUIDs
+	if mountOption.FsType == "xfs" {
+		mountOptions = append(mountOptions, "nouuid")
+	}
+
+	return mountOptions, nil
+}
+
 func (s *nodeServerNoLocked) nodePublishFilesystemVolume(req *csi.NodePublishVolumeRequest, lv *proto.LogicalVolume) error {
 	// Check request
 	mountOption := req.GetVolumeCapability().GetMount()
@@ -194,16 +215,9 @@ func (s *nodeServerNoLocked) nodePublishFilesystemVolume(req *csi.NodePublishVol
 		return err
 	}
 
-	var mountOptions []string
-	if req.GetReadonly() {
-		mountOptions = append(mountOptions, "ro")
-	}
-
-	for _, f := range mountOption.MountFlags {
-		if f == "rw" && req.GetReadonly() {
-			return status.Error(codes.InvalidArgument, "mount option \"rw\" is specified even though read only mode is specified")
-		}
-		mountOptions = append(mountOptions, f)
+	mountOptions, err := makeMountOptions(req.GetReadonly(), mountOption)
+	if err != nil {
+		return err
 	}
 
 	err = os.MkdirAll(req.GetTargetPath(), 0755)
@@ -220,11 +234,6 @@ func (s *nodeServerNoLocked) nodePublishFilesystemVolume(req *csi.NodePublishVol
 		return status.Errorf(codes.Internal, "target device is already formatted with different filesystem: volume=%s, current=%s, new:%s", req.GetVolumeId(), fsType, mountOption.FsType)
 	}
 
-	// avoid duplicate UUIDs
-	if mountOption.FsType == "xfs" {
-		mountOptions = append(mountOptions, "nouuid")
-	}
-
 	mounted, err := filesystem.IsMounted(device, req.GetTargetPath())
 	if err != nil {
 		return status.Errorf(codes.Internal, "mount check failed: target=%s, error=%v", req.GetTargetPath(), err)


Test patch:
diff --git a/driver/node_test.go b/driver/node_test.go
new file mode 100644
index 00000000..6fec5178
--- /dev/null
+++ b/driver/node_test.go
@@ -0,0 +1,16 @@
+package driver
+
+import (
+	"testing"
+
+	"github.com/container-storage-interface/spec/lib/go/csi"
+)
+
+func TestMakeMountOptions(t *testing.T) {
+	_, err := makeMountOptions(true, &csi.VolumeCapability_MountVolume{
+		MountFlags: []string{"rw"},
+	})
+	if err == nil {
+		t.Fatalf("err should happen")
+	}
+}
diff --git a/e2e/mount_option_test.go b/e2e/mount_option_test.go
new file mode 100644
index 00000000..0b2c5f7f
--- /dev/null
+++ b/e2e/mount_option_test.go
@@ -0,0 +1,80 @@
+package e2e
+
+import (
+	_ "embed"
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"strings"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+)
+
+//go:embed testdata/mount_option/pod-with-mount-option-pvc.yaml
+var podWithMountOptionPVCYAML []byte
+
+func testMountOption() {
+	var cc CleanupContext
+
+	BeforeEach(func() {
+		cc = commonBeforeEach()
+	})
+
+	AfterEach(func() {
+		commonAfterEach(cc)
+	})
+
+	It("should publish filesystem with mount option", func() {
+		By("creating a PVC and Pod")
+		_, err := kubectlWithInput(podWithMountOptionPVCYAML, "apply", "-f", "-")
+		Expect(err).ShouldNot(HaveOccurred())
+
+		Eventually(func() error {
+			var pod corev1.Pod
+			err := getObjects(&pod, "pod", "pause-mount-option")
+			if err != nil {
+				return fmt.Errorf("failed to get pod. err: %w", err)
+			}
+
+			if pod.Status.Phase != corev1.PodRunning {
+				return errors.New("Pod is not running")
+			}
+
+			return nil
+		}).Should(Succeed())
+
+		By("check mount option")
+		var pvc corev1.PersistentVolumeClaim
+		err = getObjects(&pvc, "pvc", "topo-pvc-mount-option")
+		Expect(err).ShouldNot(HaveOccurred())
+
+		f, err := os.Open("/proc/mounts")
+		Expect(err).ShouldNot(HaveOccurred())
+		defer f.Close()
+		mounts, err := io.ReadAll(f)
+		Expect(err).ShouldNot(HaveOccurred())
+
+		var isExistingOption bool
+		lines := strings.Split(string(mounts), "\n")
+		for _, line := range lines {
+			if strings.Contains(line, pvc.Spec.VolumeName) {
+				fields := strings.Split(line, " ")
+				Expect(len(fields)).To(Equal(6))
+				options := strings.Split(fields[3], ",")
+				for _, option := range options {
+					if option == "debug" {
+						isExistingOption = true
+					}
+				}
+			}
+		}
+		Expect(isExistingOption).Should(BeTrue())
+
+		By("cleaning pvc/pod")
+		_, err = kubectlWithInput(podWithMountOptionPVCYAML, "delete", "-f", "-")
+		Expect(err).ShouldNot(HaveOccurred())
+	})
+}
diff --git a/e2e/publish_test.go b/e2e/publish_test.go
deleted file mode 100644
index 1f104aba..00000000
--- a/e2e/publish_test.go
+++ /dev/null
@@ -1,394 +0,0 @@
-package e2e
-
-import (
-	"context"
-	_ "embed"
-	"errors"
-	"fmt"
-	"net"
-	"strings"
-
-	"github.com/container-storage-interface/spec/lib/go/csi"
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
-	"github.com/topolvm/topolvm"
-	topolvmv1 "github.com/topolvm/topolvm/api/v1"
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/credentials/insecure"
-	corev1 "k8s.io/api/core/v1"
-)
-
-//go:embed testdata/publish/lv-template.yaml
-var lvTemplateYAML string
-
-//go:embed testdata/publish/pod-with-mount-option-pvc.yaml
-var podWithMountOptionPVCYAML []byte
-
-type cleanup struct {
-	// key is volumeID, value is target path
-	volumes map[string]string
-}
-
-func (c *cleanup) register(volumeID, targetPath string) {
-	By("[cleanup] register")
-	if c.volumes == nil {
-		c.volumes = make(map[string]string)
-	}
-	c.volumes[volumeID] = targetPath
-}
-
-func (c *cleanup) unregister(volumeID, targetPath string) {
-	By("[cleanup] unregister")
-	if c.volumes != nil {
-		delete(c.volumes, volumeID)
-	}
-}
-
-func (c *cleanup) unpublishVolumes(nc csi.NodeClient) {
-	By("[cleanup] unpublishVolumes")
-	for volumeID, targetPath := range c.volumes {
-		req := &csi.NodeUnpublishVolumeRequest{
-			VolumeId:   volumeID,
-			TargetPath: targetPath,
-		}
-		_, err := nc.NodeUnpublishVolume(context.Background(), req)
-		if err != nil {
-			fmt.Printf("failed to unpublish volume: %v", req)
-		}
-	}
-	c.volumes = nil
-}
-
-func testPublishVolume() {
-	cl := &cleanup{}
-	var (
-		nc   csi.NodeClient
-		conn *grpc.ClientConn
-	)
-
-	nodeSocket := "/tmp/topolvm/worker1/plugins/" + topolvm.GetPluginName() + "/node/csi-topolvm.sock"
-	if isDaemonsetLvmdEnvSet() {
-		nodeSocket = "/var/lib/kubelet/plugins/" + topolvm.GetPluginName() + "/node/csi-topolvm.sock"
-	}
-
-	var cc CleanupContext
-
-	BeforeEach(func() {
-		cc = commonBeforeEach()
-
-		dialer := &net.Dialer{}
-		dialFunc := func(ctx context.Context, a string) (net.Conn, error) {
-			return dialer.DialContext(ctx, "unix", a)
-		}
-		var err error
-		conn, err = grpc.Dial(nodeSocket, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(dialFunc))
-		Expect(err).ShouldNot(HaveOccurred())
-
-		nc = csi.NewNodeClient(conn)
-	})
-
-	AfterEach(func() {
-		cl.unpublishVolumes(nc)
-		if conn != nil {
-			conn.Close()
-			conn = nil
-		}
-
-		commonAfterEach(cc)
-	})
-
-	It("should publish filesystem", func() {
-		mountTargetPath := "/mnt/csi-node-test"
-
-		nodeName := "topolvm-e2e-worker"
-		if isDaemonsetLvmdEnvSet() {
-			nodeName = getDaemonsetLvmdNodeName()
-		}
-
-		By("creating a logical volume resource")
-		name := "csi-node-test-fs"
-		lvYaml := []byte(fmt.Sprintf(lvTemplateYAML, topolvm.GetPluginName(), name, name, nodeName))
-		_, err := kubectlWithInput(lvYaml, "apply", "-f", "-")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		var volumeID string
-		Eventually(func() error {
-			var lv topolvmv1.LogicalVolume
-			err := getObjects(&lv, "logicalvolumes", name)
-			if err != nil {
-				return fmt.Errorf("failed to get logical volume. err: %w", err)
-			}
-
-			if len(lv.Status.VolumeID) == 0 {
-				return errors.New("VolumeID is not set")
-			}
-
-			if lv.Labels == nil {
-				return errors.New("logical volume label is nil")
-			}
-
-			if lv.Labels[topolvm.CreatedbyLabelKey] != topolvm.CreatedbyLabelValue {
-				return fmt.Errorf("logical volume label %q is not eqaual %q", topolvm.CreatedbyLabelKey, topolvm.CreatedbyLabelValue)
-			}
-
-			volumeID = lv.Status.VolumeID
-			return nil
-		}).Should(Succeed())
-
-		cl.register(volumeID, mountTargetPath)
-
-		By("creating Filesystem volume")
-		mountVolCap := &csi.VolumeCapability{
-			AccessType: &csi.VolumeCapability_Mount{
-				Mount: &csi.VolumeCapability_MountVolume{FsType: "xfs"},
-			},
-			AccessMode: &csi.VolumeCapability_AccessMode{
-				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
-			},
-		}
-
-		req := &csi.NodePublishVolumeRequest{
-			PublishContext:   map[string]string{},
-			TargetPath:       mountTargetPath,
-			VolumeCapability: mountVolCap,
-			VolumeId:         volumeID,
-		}
-		resp, err := nc.NodePublishVolume(context.Background(), req)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(resp).ShouldNot(BeNil())
-
-		By("publishing Filesystem volume again to check idempotency")
-		resp, err = nc.NodePublishVolume(context.Background(), req)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(resp).ShouldNot(BeNil())
-
-		By("publishing volume on same target path, but requested volume and existing one are different")
-		_, err = nc.NodePublishVolume(context.Background(), &csi.NodePublishVolumeRequest{
-			PublishContext: map[string]string{},
-			TargetPath:     mountTargetPath,
-			VolumeCapability: &csi.VolumeCapability{
-				AccessType: &csi.VolumeCapability_Mount{
-					Mount: &csi.VolumeCapability_MountVolume{FsType: "ext4"},
-				},
-				AccessMode: &csi.VolumeCapability_AccessMode{
-					Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
-				},
-			},
-			VolumeId: volumeID,
-		})
-		Expect(err).Should(HaveOccurred())
-
-		By("unpublishing the volume")
-		unpubReq := csi.NodeUnpublishVolumeRequest{
-			VolumeId:   volumeID,
-			TargetPath: mountTargetPath,
-		}
-		unpubResp, err := nc.NodeUnpublishVolume(context.Background(), &unpubReq)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(unpubResp).ShouldNot(BeNil())
-
-		By("unpublishing the volume again to check idempotency")
-		unpubResp, err = nc.NodeUnpublishVolume(context.Background(), &unpubReq)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(unpubResp).ShouldNot(BeNil())
-
-		cl.unregister(volumeID, mountTargetPath)
-
-		By("cleaning logicalvolume")
-		_, err = kubectl("delete", "logicalvolumes", "csi-node-test-fs")
-		Expect(err).ShouldNot(HaveOccurred())
-	})
-
-	It("should be worked NodePublishVolume successfully to create a block device", func() {
-		deviceTargetPath := "/dev/csi-node-test"
-
-		By("creating a logical volume resource")
-		nodeName := "topolvm-e2e-worker"
-		if isDaemonsetLvmdEnvSet() {
-			nodeName = getDaemonsetLvmdNodeName()
-		}
-
-		name := "csi-node-test-block"
-		lvYaml := []byte(fmt.Sprintf(lvTemplateYAML, topolvm.GetPluginName(), name, name, nodeName))
-		_, err := kubectlWithInput(lvYaml, "apply", "-f", "-")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		var volumeID string
-		Eventually(func() error {
-			var lv topolvmv1.LogicalVolume
-			err = getObjects(&lv, "logicalvolumes", name)
-			if err != nil {
-				return fmt.Errorf("failed to get logical volume. err: %w", err)
-			}
-
-			if len(lv.Status.VolumeID) == 0 {
-				return errors.New("VolumeID is not set")
-			}
-			volumeID = lv.Status.VolumeID
-			return nil
-		}).Should(Succeed())
-
-		cl.register(volumeID, deviceTargetPath)
-
-		By("creating raw block volume")
-		blockVolCap := &csi.VolumeCapability{
-			AccessType: &csi.VolumeCapability_Block{
-				Block: &csi.VolumeCapability_BlockVolume{},
-			},
-			AccessMode: &csi.VolumeCapability_AccessMode{
-				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
-			},
-		}
-
-		req := &csi.NodePublishVolumeRequest{
-			PublishContext:   map[string]string{},
-			TargetPath:       deviceTargetPath,
-			VolumeCapability: blockVolCap,
-			VolumeId:         volumeID,
-		}
-		resp, err := nc.NodePublishVolume(context.Background(), req)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(resp).ShouldNot(BeNil())
-
-		By("creating raw block volume again to check idempotency")
-		resp, err = nc.NodePublishVolume(context.Background(), req)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(resp).ShouldNot(BeNil())
-
-		By("creating volume on the same target path, but requested volume and existing one are different")
-		_, err = nc.NodePublishVolume(context.Background(), &csi.NodePublishVolumeRequest{
-			PublishContext:   map[string]string{},
-			TargetPath:       deviceTargetPath,
-			VolumeCapability: blockVolCap,
-			VolumeId:         volumeID + "-invalid",
-		})
-		Expect(err).Should(HaveOccurred())
-
-		By("unpublishing the volume")
-		unpubReq := csi.NodeUnpublishVolumeRequest{
-			VolumeId:   volumeID,
-			TargetPath: deviceTargetPath,
-		}
-		unpubResp, err := nc.NodeUnpublishVolume(context.Background(), &unpubReq)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(unpubResp).ShouldNot(BeNil())
-
-		By("deleting the volume again to check idempotency")
-		unpubResp, err = nc.NodeUnpublishVolume(context.Background(), &unpubReq)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(unpubResp).ShouldNot(BeNil())
-
-		cl.unregister(volumeID, deviceTargetPath)
-
-		By("cleaning logicalvolume")
-		_, err = kubectl("delete", "logicalvolumes", "csi-node-test-block")
-		Expect(err).ShouldNot(HaveOccurred())
-	})
-
-	It("should publish filesystem with mount option", func() {
-		By("creating a PVC and Pod")
-		_, err := kubectlWithInput(podWithMountOptionPVCYAML, "apply", "-f", "-")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		Eventually(func() error {
-			var pod corev1.Pod
-			err := getObjects(&pod, "pod", "pause-mount-option")
-			if err != nil {
-				return fmt.Errorf("failed to get pod. err: %w", err)
-			}
-
-			if pod.Status.Phase != corev1.PodRunning {
-				return errors.New("Pod is not running")
-			}
-
-			return nil
-		}).Should(Succeed())
-
-		By("check mount option")
-		var pvc corev1.PersistentVolumeClaim
-		err = getObjects(&pvc, "pvc", "topo-pvc-mount-option")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		stdout, err := execAtLocal("cat", nil, "/proc/mounts")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		var isExistingOption bool
-		lines := strings.Split(string(stdout), "\n")
-		for _, line := range lines {
-			if strings.Contains(line, pvc.Spec.VolumeName) {
-				fields := strings.Split(line, " ")
-				Expect(len(fields)).To(Equal(6))
-				options := strings.Split(fields[3], ",")
-				for _, option := range options {
-					if option == "debug" {
-						isExistingOption = true
-					}
-				}
-			}
-		}
-		Expect(isExistingOption).Should(BeTrue())
-
-		By("cleaning pvc/pod")
-		_, err = kubectlWithInput(podWithMountOptionPVCYAML, "delete", "-f", "-")
-		Expect(err).ShouldNot(HaveOccurred())
-	})
-
-	It("validate mount options", func() {
-		mountTargetPath := "/mnt/csi-node-test"
-
-		By("creating a logical volume resource")
-		nodeName := "topolvm-e2e-worker"
-		if isDaemonsetLvmdEnvSet() {
-			nodeName = getDaemonsetLvmdNodeName()
-		}
-
-		name := "csi-node-test-fs"
-		lvYaml := []byte(fmt.Sprintf(lvTemplateYAML, topolvm.GetPluginName(), name, name, nodeName))
-		_, err := kubectlWithInput(lvYaml, "apply", "-f", "-")
-		Expect(err).ShouldNot(HaveOccurred())
-
-		var volumeID string
-		Eventually(func() error {
-			var lv topolvmv1.LogicalVolume
-			err := getObjects(&lv, "logicalvolumes", name)
-			if err != nil {
-				return fmt.Errorf("failed to get logical volume. err: %w", err)
-			}
-
-			if len(lv.Status.VolumeID) == 0 {
-				return errors.New("VolumeID is not set")
-			}
-			volumeID = lv.Status.VolumeID
-			return nil
-		}).Should(Succeed())
-
-		cl.register(volumeID, mountTargetPath)
-
-		By("mount option \"rw\" is specified even though read only mode is specified")
-		mountVolCap := &csi.VolumeCapability{
-			AccessType: &csi.VolumeCapability_Mount{
-				Mount: &csi.VolumeCapability_MountVolume{
-					FsType:     "xfs",
-					MountFlags: []string{"rw"},
-				},
-			},
-			AccessMode: &csi.VolumeCapability_AccessMode{
-				Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
-			},
-		}
-
-		req := &csi.NodePublishVolumeRequest{
-			PublishContext:   map[string]string{},
-			TargetPath:       mountTargetPath,
-			VolumeCapability: mountVolCap,
-			VolumeId:         volumeID,
-			Readonly:         true,
-		}
-		_, err = nc.NodePublishVolume(context.Background(), req)
-		Expect(err).Should(HaveOccurred())
-
-		By("cleaning logicalvolume")
-		_, err = kubectl("delete", "logicalvolumes", "csi-node-test-fs")
-		Expect(err).ShouldNot(HaveOccurred())
-	})
-}
diff --git a/e2e/suite_test.go b/e2e/suite_test.go
index 2ecee3cb..d97b6661 100644
--- a/e2e/suite_test.go
+++ b/e2e/suite_test.go
@@ -121,7 +121,7 @@ var _ = Describe("TopoLVM", func() {
 		Context("topolvm-controller", testController)
 		Context("scheduling", testScheduling)
 		Context("metrics", testMetrics)
-		Context("publish", testPublishVolume)
+		Context("mount option", testMountOption)
 		Context("ReadWriteOncePod", testReadWriteOncePod)
 		Context("e2e", testE2E)
 		Context("multiple-vg", testMultipleVolumeGroups)
diff --git a/e2e/testdata/publish/pod-with-mount-option-pvc.yaml b/e2e/testdata/mount_option/pod-with-mount-option-pvc.yaml
similarity index 100%
rename from e2e/testdata/publish/pod-with-mount-option-pvc.yaml
rename to e2e/testdata/mount_option/pod-with-mount-option-pvc.yaml
diff --git a/e2e/testdata/publish/lv-template.yaml b/e2e/testdata/publish/lv-template.yaml
deleted file mode 100644
index 47ef820a..00000000
--- a/e2e/testdata/publish/lv-template.yaml
+++ /dev/null
@@ -1,9 +0,0 @@
-apiVersion: %s/v1
-kind: LogicalVolume
-metadata:
-  name: %s
-spec:
-  deviceClass: ssd
-  name: %s
-  nodeName: %s
-  size: 1Gi