sanity: Add tests for NodeStageVolume/NodeUnstageVolume #66

darkowlzz · 2018-04-16T14:14:14Z

Fixes #46

lpabon · 2018-04-18T15:12:27Z

pkg/sanity/node.go

+		if nodeStageSupported {
+			err := createMountTargetLocation(config.StagingPath)
+			Expect(err).NotTo(HaveOccurred())
+		}


If STAGE_UNSTAGE_VOLUME is not supported it should skip this test.

See https://github.com/kubernetes-csi/csi-test/blob/master/pkg/sanity/controller.go#L105 as an example.

lpabon · 2018-04-18T15:13:57Z

mock/service/node.go

@@ -16,15 +16,84 @@ func (s *service) NodeStageVolume(
 	req *csi.NodeStageVolumeRequest) (
 	*csi.NodeStageVolumeResponse, error) {

-	return nil, status.Error(codes.Unimplemented, "")
+	device, ok := req.PublishInfo["device"]


To support this, STAGE_UNSTAGE_VOLUME needs to be supported in https://github.com/kubernetes-csi/csi-test/blob/master/mock/service/node.go#L119 and the test should skip if not supported.

lpabon · 2018-04-18T15:21:59Z

mock/service/node.go

+	nodeStgPathKey := path.Join(s.nodeID, req.StagingTargetPath)
+
+	// Check to see if the volume has already been unstaged.
+	if v.Attributes[nodeStgPathKey] == "" {


This assumes that the key is there. It could panic. There needs to be a check

Verified that if a key doesn't exists in a map, it doesn't panic. The value would just be empty.

lpabon · 2018-04-18T15:22:46Z

mock/service/node.go

+	nodeStgPathKey := path.Join(s.nodeID, req.StagingTargetPath)
+
+	// Check to see if the volume has already been staged.
+	if v.Attributes[nodeStgPathKey] != "" {


This assumes the key is in the map and could panic. It needs to get the key by also checking to see if it is there.

Just verified that if a key doesn't exists in a map, it doesn't panic. The value would just be empty. And this is done in the same way for NodePublishVolume as well. So, it should be fine.

You are right! That must be new since I started using golang (1.2): https://play.golang.org/p/OMC7Taio5h1

davidz627

Thanks for the PR! Really appreciate the work put into this 👍 Review is mostly just some nit's

davidz627 · 2018-04-18T18:37:12Z

mock/service/node.go

+
+	// Check to see if the volume has already been staged.
+	if v.Attributes[nodeStgPathKey] != "" {
+		return &csi.NodeStageVolumeResponse{}, nil


Should check whether the volume staged is compatible. See error "ALREADY_EXISTS" in spec.

I tried adding this, but this would require changes in service.go, in the Volume struct. I think it would be better to add separately, capability check for Controller functions as well. I'll add a TODO for now here.

davidz627 · 2018-04-18T18:39:25Z

mock/service/node.go

+	delete(v.Attributes, nodeStgPathKey)
+	s.vols[i] = v
+
+	return &csi.NodeUnstageVolumeResponse{}, nil
 }

 func (s *service) NodePublishVolume(


Need to modify NodePublishVolume to do the right thing when a StagingTargetPath is provided.

davidz627 · 2018-04-18T18:40:28Z

pkg/sanity/node.go

+
+		_, err := c.NodeStageVolume(
+			context.Background(),
+			&csi.NodeStageVolumeRequest{})


provide the rest of the required fields to isolate the error as just a volume id error

davidz627 · 2018-04-18T18:40:48Z

pkg/sanity/node.go

+		_, err := c.NodeStageVolume(
+			context.Background(),
+			&csi.NodeStageVolumeRequest{
+				VolumeId: "id",


provide the rest of the required fields.

davidz627 · 2018-04-18T18:43:01Z

pkg/sanity/node.go

+		Expect(serverError.Code()).To(Equal(codes.InvalidArgument))
+	})
+
+	It("should return appropriate values (no optional values added)", func() {


Is this similar enough to testFullWorkflowSuccess that we could reuse that - or make some minimal tweaks to that function and reuse it here?

davidz627 · 2018-04-18T18:43:27Z

pkg/sanity/node.go

+		Expect(serverError.Code()).To(Equal(codes.InvalidArgument))
+	})
+
+	It("should return appropriate values (no optional values added)", func() {


Is this similar enough to testFullWorkflowSuccess that we could reuse that - or make some minimal tweaks to that function and reuse it here?

https://github.com/kubernetes-csi/csi-test/blob/master/pkg/sanity/node.go#L224

Yes, it's completely same. I failed to see that :)

davidz627 · 2018-04-18T18:43:55Z

pkg/sanity/node.go

+		controllerPublishSupported = isControllerCapabilitySupported(
+			s,
+			csi.ControllerServiceCapability_RPC_PUBLISH_UNPUBLISH_VOLUME)
+		nodeStageSupported = isNodeCapabilitySupported(c, csi.NodeServiceCapability_RPC_STAGE_UNSTAGE_VOLUME)


skip test if nodeStage not supported

davidz627 · 2018-04-18T18:44:10Z

pkg/sanity/node.go

+
+		_, err := c.NodeUnstageVolume(
+			context.Background(),
+			&csi.NodeUnstageVolumeRequest{})


ditto: provide rest of the required args

davidz627 · 2018-04-18T18:44:14Z

pkg/sanity/node.go

+		_, err := c.NodeUnstageVolume(
+			context.Background(),
+			&csi.NodeUnstageVolumeRequest{
+				VolumeId: "id",


ditto: provide rest of the required args

darkowlzz · 2018-04-29T19:53:04Z

mock/service/node.go

@@ -64,7 +140,7 @@ func (s *service) NodePublishVolume(
 	}

 	// Publish the volume.
-	v.Attributes[nodeMntPathKey] = device
+	v.Attributes[nodeMntPathKey] = req.GetStagingTargetPath()


Changed this from device to the staging target path. I think staging path is bind mounted to the target path. Correct me if I'm wrong about that.

I'm not sure. I'll ask.

this should only be the staging target path if it is defined - if not, it would be device.

Right. Thanks!

Adds the missing volume ID, target path and capabilities validations as well.

darkowlzz · 2018-05-06T09:41:51Z

Made the changes. Hope it's ready.

davidz627 · 2018-05-07T17:25:29Z

/lgtm

lpabon · 2018-05-09T16:30:50Z

mock/service/node.go

+	nodeStgPathKey := path.Join(s.nodeID, req.StagingTargetPath)
+
+	// Check to see if the volume has already been staged.
+	if v.Attributes[nodeStgPathKey] != "" {


You are right! That must be new since I started using golang (1.2): https://play.golang.org/p/OMC7Taio5h1

lpabon · 2018-05-09T16:32:51Z

Looks great

sanity: Add tests for NodeStageVolume/NodeUnstageVolume

lpabon requested a review from sbezverk April 17, 2018 14:54

lpabon requested changes Apr 18, 2018

View reviewed changes

davidz627 suggested changes Apr 18, 2018

View reviewed changes

darkowlzz force-pushed the 46-nodestagetest branch 2 times, most recently from dce0b55 to 2624233 Compare April 29, 2018 19:51

darkowlzz commented Apr 29, 2018

View reviewed changes

darkowlzz force-pushed the 46-nodestagetest branch from 2624233 to f6f4d37 Compare May 6, 2018 09:25

darkowlzz added 4 commits May 6, 2018 15:03

sanity: Add tests for NodeStageVolume/NodeUnstageVolume

0e89bd0

Implement mock NodeStageVolume/NodeUnstageVolume

b43de19

Add STAGE_UNSTAGE in Node Capabilities & reuse testFullWorkflow

7349b5a

Use StageTargetPath in NodePublishVolume

0590ad0

Adds the missing volume ID, target path and capabilities validations as well.

darkowlzz force-pushed the 46-nodestagetest branch from f6f4d37 to 0590ad0 Compare May 6, 2018 09:34

lpabon approved these changes May 9, 2018

View reviewed changes

lpabon merged commit ed74243 into kubernetes-csi:master May 9, 2018

suneeth51 pushed a commit to suneeth51/csi-test that referenced this pull request Sep 11, 2019

Merge pull request kubernetes-csi#66 from darkowlzz/46-nodestagetest

070f826

sanity: Add tests for NodeStageVolume/NodeUnstageVolume

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sanity: Add tests for NodeStageVolume/NodeUnstageVolume #66

sanity: Add tests for NodeStageVolume/NodeUnstageVolume #66

darkowlzz commented Apr 16, 2018

lpabon Apr 18, 2018

lpabon Apr 18, 2018

lpabon Apr 18, 2018

lpabon Apr 18, 2018

darkowlzz Apr 29, 2018

lpabon Apr 18, 2018

darkowlzz Apr 29, 2018

lpabon May 9, 2018

davidz627 left a comment

davidz627 Apr 18, 2018

darkowlzz Apr 29, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

darkowlzz Apr 29, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

davidz627 Apr 18, 2018

darkowlzz Apr 29, 2018

lpabon May 4, 2018

davidz627 May 4, 2018

darkowlzz May 6, 2018

darkowlzz commented May 6, 2018

davidz627 commented May 7, 2018

lpabon May 9, 2018

lpabon commented May 9, 2018

sanity: Add tests for NodeStageVolume/NodeUnstageVolume #66

sanity: Add tests for NodeStageVolume/NodeUnstageVolume #66

Conversation

darkowlzz commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented May 6, 2018

davidz627 commented May 7, 2018

Choose a reason for hiding this comment

lpabon commented May 9, 2018