diff --git a/cmd/csi-sanity/sanity_test.go b/cmd/csi-sanity/sanity_test.go index 032b791a..078f7a6b 100644 --- a/cmd/csi-sanity/sanity_test.go +++ b/cmd/csi-sanity/sanity_test.go @@ -38,8 +38,8 @@ func init() { flag.StringVar(&config.Address, prefix+"endpoint", "", "CSI endpoint") flag.StringVar(&config.ControllerAddress, prefix+"controllerendpoint", "", "CSI controller endpoint") flag.BoolVar(&version, prefix+"version", false, "Version of this program") - flag.StringVar(&config.TargetPath, prefix+"mountdir", os.TempDir()+"/csi", "Mount point for NodePublish") - flag.StringVar(&config.StagingPath, prefix+"stagingdir", os.TempDir()+"/csi", "Mount point for NodeStage if staging is supported") + flag.StringVar(&config.TargetPath, prefix+"mountdir", os.TempDir()+"/csi-mount", "Mount point for NodePublish") + flag.StringVar(&config.StagingPath, prefix+"stagingdir", os.TempDir()+"/csi-staging", "Mount point for NodeStage if staging is supported") flag.StringVar(&config.CreateTargetPathCmd, prefix+"createmountpathcmd", "", "Command to run for target path creation") flag.StringVar(&config.CreateStagingPathCmd, prefix+"createstagingpathcmd", "", "Command to run for staging path creation") flag.IntVar(&config.CreatePathCmdTimeout, prefix+"createpathcmdtimeout", 10, "Timeout for the commands to create target and staging paths, in seconds") diff --git a/hack/_apitest/api_test.go b/hack/_apitest/api_test.go index 07c1aeff..6971a374 100644 --- a/hack/_apitest/api_test.go +++ b/hack/_apitest/api_test.go @@ -9,8 +9,8 @@ import ( func TestMyDriver(t *testing.T) { config := &sanity.Config{ - TargetPath: os.TempDir() + "/csi", - StagingPath: os.TempDir() + "/csi", + TargetPath: os.TempDir() + "/csi-target", + StagingPath: os.TempDir() + "/csi-staging", Address: "/tmp/e2e-csi-sanity.sock", TestNodeVolumeAttachLimit: true, } diff --git a/hack/_embedded/embedded_test.go b/hack/_embedded/embedded_test.go index b745e570..eced87a0 100644 --- a/hack/_embedded/embedded_test.go +++ b/hack/_embedded/embedded_test.go @@ -26,8 +26,8 @@ var _ = AfterSuite(func() {}) var _ = Describe("MyCSIDriver", func() { Context("Config A", func() { config := &sanity.Config{ - TargetPath: os.TempDir() + "/csi", - StagingPath: os.TempDir() + "/csi", + TargetPath: os.TempDir() + "/csi-target", + StagingPath: os.TempDir() + "/csi-staging", Address: "/tmp/e2e-csi-sanity.sock", TestNodeVolumeAttachLimit: true, } diff --git a/mock/service/node.go b/mock/service/node.go index 51eebf7f..5b3073a9 100644 --- a/mock/service/node.go +++ b/mock/service/node.go @@ -136,7 +136,7 @@ func (s *service) NodePublishVolume( return nil, status.Error(codes.InvalidArgument, "Volume Capability cannot be empty") } - if err := checkTargetExists(req.TargetPath); err != nil { + if err := checkTargetNotExists(req.TargetPath); err != nil { return nil, status.Error(codes.Internal, err.Error()) } @@ -353,8 +353,24 @@ func (s *service) NodeGetVolumeStats(ctx context.Context, // does not exists. func checkTargetExists(targetPath string) error { _, err := os.Stat(targetPath) - if err != nil && os.IsNotExist(err) { + if err == nil { + return nil + } + if os.IsNotExist(err) { return status.Errorf(codes.Internal, "target path %s does not exists", targetPath) } - return nil + return status.Errorf(codes.Internal, "stat target path %s: %s", targetPath, err) +} + +// checkTargetNotExists checks if a given path does not exist and returns error if the path +// does exist. +func checkTargetNotExists(targetPath string) error { + _, err := os.Stat(targetPath) + if err == nil { + return status.Errorf(codes.Internal, "target path %s does exist", targetPath) + } + if os.IsNotExist(err) { + return nil + } + return status.Errorf(codes.Internal, "stat target path %s: %s", targetPath, err) } diff --git a/pkg/sanity/node.go b/pkg/sanity/node.go index 62339525..c7818ab3 100644 --- a/pkg/sanity/node.go +++ b/pkg/sanity/node.go @@ -186,7 +186,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodePublishVolumeRequest{ VolumeId: "id", - TargetPath: sc.targetPath, + TargetPath: sc.targetPath + "/target", Secrets: sc.Secrets.NodePublishVolumeSecret, }, ) @@ -534,7 +534,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodePublishVolumeRequest{ VolumeId: vol.GetVolume().GetVolumeId(), - TargetPath: sc.targetPath, + TargetPath: sc.targetPath + "/target", StagingTargetPath: stagingPath, VolumeCapability: &csi.VolumeCapability{ AccessType: &csi.VolumeCapability_Mount{ @@ -573,7 +573,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodeUnpublishVolumeRequest{ VolumeId: vol.GetVolume().GetVolumeId(), - TargetPath: sc.targetPath, + TargetPath: sc.targetPath + "/target", }) Expect(err).NotTo(HaveOccurred()) Expect(nodeunpubvol).NotTo(BeNil()) @@ -718,7 +718,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodePublishVolumeRequest{ VolumeId: vol.GetVolume().GetVolumeId(), - TargetPath: sc.targetPath, + TargetPath: sc.targetPath + "/target", StagingTargetPath: stagingPath, VolumeCapability: &csi.VolumeCapability{ AccessType: &csi.VolumeCapability_Mount{ @@ -743,7 +743,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodeGetVolumeStatsRequest{ VolumeId: vol.GetVolume().GetVolumeId(), - VolumePath: sc.targetPath, + VolumePath: sc.targetPath + "/target", }, ) Expect(err).ToNot(HaveOccurred()) @@ -756,7 +756,7 @@ var _ = DescribeSanity("Node Service", func(sc *SanityContext) { context.Background(), &csi.NodeUnpublishVolumeRequest{ VolumeId: vol.GetVolume().GetVolumeId(), - TargetPath: sc.targetPath, + TargetPath: sc.targetPath + "/target", }) Expect(err).NotTo(HaveOccurred()) Expect(nodeunpubvol).NotTo(BeNil()) diff --git a/pkg/sanity/sanity.go b/pkg/sanity/sanity.go index 42cc0d05..d08b1ab8 100644 --- a/pkg/sanity/sanity.go +++ b/pkg/sanity/sanity.go @@ -53,8 +53,14 @@ type CSISecrets struct { // Config provides the configuration for the sanity tests. It // needs to be initialized by the user of the sanity package. type Config struct { - TargetPath string - StagingPath string + // TargetPath is the *parent* directory for NodePublishVolumeRequest.target_path. + // It gets created and removed by csi-sanity. + TargetPath string + + // StagingPath is the NodeStageVolumeRequest.staging_target_path. + // It gets created and removed by csi-sanity. + StagingPath string + Address string ControllerAddress string SecretsFile string @@ -70,6 +76,17 @@ type Config struct { // directories. Returns the new paths for mount and staging. // If not defined, directories are created in the default way at TargetPath // and StagingPath on the host. + // + // Both functions can replace the suggested path. What the test then uses + // is the path returned by them. + // + // Note that target and staging directory have different + // semantics in the CSI spec: for NodeStateVolume, + // CreateTargetDir must create the directory and return the + // full path to it. For NodePublishVolume, CreateStagingDir + // must create the *parent* directory of `path` (or some other + // directory) and return the full path for an entry inside + // that created directory. CreateTargetDir func(path string) (string, error) CreateStagingDir func(path string) (string, error) @@ -77,6 +94,12 @@ type Config struct { // directories. // If not defined, directories are removed in the default way at TargetPath // and StagingPath on the host. + // + // Both functions are passed the actual paths as used during the test. + // + // Note that RemoveTargetPath only needs to remove the *parent* of the + // given path. The CSI driver should have removed the entry at that path + // already. RemoveTargetPath func(path string) error RemoveStagingPath func(path string) error @@ -253,20 +276,11 @@ func createMountTargetLocation(targetPath string, createPathCmd string, customCr } newTargetPath = newpath } else { - // Create the target path using mkdir. - fileInfo, err := os.Stat(targetPath) - if err != nil { - if !os.IsNotExist(err) { - return "", err - } - if err := os.MkdirAll(targetPath, 0755); err != nil { - return "", err - } - return targetPath, nil - } - - if !fileInfo.IsDir() { - return "", fmt.Errorf("Target location %s is not a directory", targetPath) + // Create the target path. Only the directory itself + // and not its parents get created, and it is an error + // if the directory already exists. + if err := os.Mkdir(targetPath, 0755); err != nil { + return "", err } newTargetPath = targetPath } @@ -297,7 +311,8 @@ func removeMountTargetLocation(targetPath string, removePathCmd string, customRe return err } } else { - return os.RemoveAll(targetPath) + // It's an error if the directory is not empty by now. + return os.Remove(targetPath) } return nil }