add test for volume snapshot #83

wackxu · 2018-06-20T12:02:03Z

darkowlzz

The changes look good.
Could you also implement snapshots in the mock driver so that we could test the changes and fix any bugs that are not obvious?

darkowlzz · 2018-06-21T09:57:11Z

pkg/sanity/controller.go

+
+	It("should fail when no name is provided", func() {
+
+		req := &csi.CreateSnapshotRequest{}


I think it would be better to add the other attributes here and just exclude name.

darkowlzz · 2018-06-21T09:57:19Z

pkg/sanity/controller.go

+	BeforeEach(func() {
+		c = csi.NewControllerClient(conn)
+
+		if !isControllerCapabilitySupported(c, csi.ControllerServiceCapability_RPC_LIST_VOLUMES) {


This should be ControllerServiceCapability_RPC_LIST_SNAPSHOTS. Looks like this is causing the test to fail.

wackxu · 2018-06-28T12:57:46Z

@darkowlzz Thanks for your review, will update it.

davidz627 · 2018-06-29T22:17:26Z

/cc @jingxu97 @xing-yang

darkowlzz

Thanks for adding the mock driver implementation.
I've suggested some improvements.

darkowlzz · 2018-07-01T11:12:09Z

mock/service/controller.go

-	return nil, status.Error(codes.InvalidArgument, "Not Implemented")
+	// Check arguments
+	if len(req.GetName()) == 0 {
+		return nil, status.Error(codes.InvalidArgument, "Name missing in request")


"Snapshot Name cannot be empty"? Maybe it's better to be consistent with CreateVolume error messages.

darkowlzz · 2018-07-01T11:16:07Z

mock/service/controller.go

+		return nil, status.Error(codes.InvalidArgument, "Name missing in request")
+	}
+	if len(req.GetSourceVolumeId()) == 0 {
+		return nil, status.Error(codes.InvalidArgument, "SourceVolumeId missing in request")


"Snapshot SourceVolumeId cannot be empty"? Maybe it's better to be consistent with CreateVolume error messages.

darkowlzz · 2018-07-01T11:27:50Z

mock/service/controller.go

+		// Requested snapshot name already exists
+		if v.GetSourceVolumeId() == req.SourceVolumeId {
+			return nil, status.Error(codes.AlreadyExists,
+				fmt.Sprintf("Snapshot with name %s already exists", req.GetName()))


If the snapshot exists with the same source volume ID, I think it should return OK.
And there should be a check for parameters as well.
If the source volume ID and parameters are equal, return OK, else return ALREADY_EXISTS error.

+1. If source_volume_id, create_snapshot_secrets, and parameters are the same for the same snapshot name, it should return OK for idempotency.

Updated for check parameters also, I do not think we should check create_snapshot_secrets that may change every request.

darkowlzz · 2018-07-01T12:02:17Z

mock/service/service.go

+	ctx context.Context, name string) (int, csi.Snapshot) {
+
+	return s.findSnapshot("name", name)
+}


Let's add a newline at the end.

darkowlzz · 2018-07-01T12:08:02Z

pkg/sanity/controller.go

@@ -48,6 +48,11 @@ func verifyVolumeInfo(v *csi.Volume) {
 	Expect(v.GetId()).NotTo(BeEmpty())
 }

+func verifySnapshotInfo(snapshot *csi.Snapshot) {
+	Expect(snapshot).NotTo(BeNil())
+	Expect(snapshot.GetId()).NotTo(BeEmpty())


I think we can add some more verification here.

SourceVolumeID is REQUIRED.

Snapshot definition talks about SizeBytes.

The size of the volume MUST NOT be less than the size of the source snapshot.

Let's add those.

Updated for check attributes that is REQUIRED. As for SizeBytes,

The purpose of this field is to give CO guidance on how much space is needed to
// create a volume from this snapshot. The size of the volume MUST NOT
// be less than the size of the source snapshot.

I think it is used to CO when restore volume from a snapshot to verify there is enough space. So we do not need verify here.

darkowlzz · 2018-07-01T12:35:44Z

mock/service/controller.go

+	}
+
+	// case 2: SourceVolumeId is not empty, return snapshots that match the source volume id.
+	if len(req.SourceVolumeId) != 0 {


Would recommend using req.GetSourceVolumeId().

darkowlzz · 2018-07-01T12:38:56Z

mock/service/controller.go

+				},
+			},
+		}, nil
+	}


I think we need to consider both req.GetSnapshotId() and req.GetSourceVolumeID when they aren't empty. If the volume received from ID has a different source volume ID, do not include them in the snapshot list.

darkowlzz · 2018-07-01T12:39:30Z

mock/service/controller.go

+
+	// case 2: SourceVolumeId is not empty, return snapshots that match the source volume id.
+	if len(req.SourceVolumeId) != 0 {
+		i, snapshot := s.findSnapshotNoLock("id", req.SourceVolumeId)


"id" -> "sourceVolumeId"

Thanks, done

darkowlzz · 2018-07-01T12:56:06Z

.idea/workspace.xml

+      </provider>
+    </entry>
+  </component>
+</project>


I don't think we need this file :)

Correct, please remove this file.

darkowlzz · 2018-07-01T12:58:09Z

mock/service/controller.go

@@ -271,7 +271,7 @@ func (s *service) ListVolumes(
 		i       int
 		j       = startingToken
 		entries = make(
-			[]*csi.ListVolumesResponse_Entry,
+		[]*csi.ListVolumesResponse_Entry,


Looked better before :)

lpabon · 2018-07-02T16:17:00Z

Seems that there are two CI errors: Go fmt errors and some failed csi-sanity errors

wackxu · 2018-07-06T06:55:23Z

@lpabon @darkowlzz updated all comments, PTAL

lpabon · 2018-07-10T17:45:50Z

mock/service/controller.go

+	// themselves have fields that are.
+	copy(s.snapshots[i:], s.snapshots[i+1:])
+	s.snapshots[len(s.snapshots)-1] = Snapshot{}
+	s.snapshots = s.snapshots[:len(s.snapshots)-1]


line 440 removes the last element which was just initialized in line 439. It looks to me that line 439 is not needed, right?

Just imitate the code about volume deletion, make sense, will delete it.

lpabon · 2018-07-10T17:48:07Z

mock/service/controller.go

+	// locking the service's snapshot slice for the duration of the
+	// ListSnapshots RPC.
+	var snapshots []csi.Snapshot
+	func() {


I've never seen an anonymous function without a go statement or assigned to a variable. Just curious, but why use this style?

FYI, when I review, I'm not only looking for bugs, but I am looking at maintenance. If it is hard to maintain, even if it works, it should be redone.

lpabon · 2018-07-10T17:51:19Z

mock/service/controller.go

+	// ListSnapshots RPC.
+	var snapshots []csi.Snapshot
+	func() {
+		s.snapshotsRWL.RLock()


Have to be very careful creating a deadlock situation sometime in the future. I would instead place these locks as close to the object as possible. I would suggest creating a new data structure object that handles the the work with the list. It supports Insert, Delete, List, Find, etc. and it is the one managing the lock

Just imitate the code about volume deletion, make sense, I will update the code and commit a separate pr to change the volume part.

lpabon · 2018-07-10T17:52:04Z

mock/service/controller.go

 }

 func (s *service) ListSnapshots(ctx context.Context,
 	req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) {
-	return nil, status.Error(codes.InvalidArgument, "Not Implemented")
+
+	// case 1: SnapshotId is not empty, return snapshots that match the snapshot id.


This function is too large, do you mind breaking it up by the cases you have?

Make sense, will update.

add test for volume snapshot

darkowlzz reviewed Jun 27, 2018

View reviewed changes

wackxu force-pushed the addtest branch 3 times, most recently from ff727c2 to a709649 Compare June 28, 2018 14:09

darkowlzz reviewed Jul 1, 2018

View reviewed changes

wackxu force-pushed the addtest branch 7 times, most recently from 0888172 to 8ab5016 Compare July 6, 2018 06:53

lpabon requested changes Jul 10, 2018

View reviewed changes

pohly mentioned this pull request Jul 11, 2018

test suite reorganization #93

Merged

wackxu force-pushed the addtest branch 2 times, most recently from 0a5929c to 626cfbb Compare July 13, 2018 09:34

add test for volume snapshot

5c842fd

wackxu force-pushed the addtest branch from d8526d5 to 5c842fd Compare July 13, 2018 10:08

lpabon merged commit 718c954 into kubernetes-csi:master Jul 18, 2018

wackxu deleted the addtest branch July 19, 2018 02:51

wackxu mentioned this pull request Sep 13, 2018

remove test case for starting_token when ListSnapshots #114

Closed

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

Merge pull request kubernetes-csi#83 from wackxu/addtest

9523eaf

add test for volume snapshot


		It("should fail when no name is provided", func() {

		req := &csi.CreateSnapshotRequest{}

add test for volume snapshot #83

add test for volume snapshot #83

Conversation

wackxu commented Jun 20, 2018 • edited Loading

darkowlzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackxu commented Jun 28, 2018

davidz627 commented Jun 29, 2018

darkowlzz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpabon commented Jul 2, 2018

wackxu commented Jul 6, 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

wackxu commented Jun 20, 2018 •

edited

Loading

lpabon Jul 2, 2018 •

edited

Loading