Hostpath snapshot implementation #94

wackxu · 2018-06-11T06:19:57Z

I am working with @xing-yang about Move Snapshot APIs in-tree, and we have test the implementation of host path volume snapshot. so push this code.

wackxu · 2018-06-11T06:24:52Z

@lpabon

lpabon · 2018-06-19T16:45:46Z

Looks like two errors from the unit tests: 1. go fmt 2. Seems one of the API calls did not pass.

lpabon · 2018-06-19T16:47:29Z

pkg/hostpath/controllerserver.go

 	return &csi.ValidateVolumeCapabilitiesResponse{Supported: true, Message: ""}, nil
 }
+
+func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {


Could you please add a lot of comments to this function to explain what the algorithm is and what tools it needs from the container?

lpabon · 2018-06-19T16:50:03Z

pkg/hostpath/controllerserver.go

+	createAt := time.Now().UnixNano()
+	volPath := hostPathVolume.VolPath
+	file := snapshotRoot + snapshotID + ".tgz"
+	args := []string{"czf", file, "-C", volPath, "."}


lpabon · 2018-06-19T16:50:56Z

pkg/hostpath/controllerserver.go

+	}
+
+	glog.V(4).Infof("create volume snapshot %s", file)
+	hostPathSna := hostPathSnapshot{}


what is Sna? Is that Snapshot? Maybe call it just snap or snapshot since it is a local variable.

lpabon · 2018-06-19T16:52:19Z

pkg/hostpath/hostpath.go

 }

+type hostPathSnapshot struct {
+	SnaName  string                  `json:"snaName"`


You can remove all Sna prefixes since they are part of the hostPathSnapshot object. So, SnaID being just Id would be the id of the snapshot. and so one.

lpabon · 2018-06-19T16:53:08Z

pkg/hostpath/controllerserver.go

+	glog.V(4).Infof("deleting volume %s", snapshotID)
+	path := snapshotRoot + snapshotID + ".tgz"
+	os.RemoveAll(path)
+	delete(hostPathVolumeSnapshots, snapshotID)


We should check first if it is there. If it is not, then it should return that the snapshot id is not found.

I do not think so. from the spec, we can see this for DeleteSnapshot,

This operation MUST be idempotent. If a snapshot corresponding to the specified snapshot_id does not exist or the artifacts associated with the snapshot do not exist anymore, the Plugin MUST reply 0 OK.

I think we can just do the delete operation like deleteVolume and we do not need check whether the snapshot exists.

Ah, great point.

lpabon · 2018-06-19T16:54:47Z

pkg/hostpath/controllerserver.go

+
+func (cs *controllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) {
+	// Check arguments
+	if len(req.GetSnapshotId()) == 0 {


This is optional. It is only used to get the snapshot information about a single snapshot id. it should not be checked if it was passed.

Also, we should return a list of all the snapshots in the system that:

Match the source volume id

Match the snapshot id

Match Anything (meaning all of the snapshots)

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

@lpabon test has been fixed by kubernetes-csi/csi-test#81

lpabon · 2018-06-20T16:05:52Z

@wackxu Seems there is one more csi-sanity issue:

[Fail] ControllerGetCapabilities [Controller Server] [It] should return appropriate capabilities 
/home/travis/gopath/src/github.com/kubernetes-csi/csi-test/pkg/sanity/controller.go:100

I'll take a look to see if we are using the latest from csi-sanity

xing-yang · 2018-06-21T01:04:04Z

Can you fix the typo in the title s/Hostpaht/hostpath.

wackxu · 2018-06-21T01:29:24Z

@xing-yang Done

wackxu · 2018-06-21T01:39:13Z

@lpabon Yes, I see we are using the latest from csi-sanity, and there is a bug when csi-sanity switch to CSI 0.3.0. And we should first merge kubernetes-csi/csi-test#81. then test will pass.

# Get csi-sanity
if [ ! -x $GOPATH/bin/csi-sanity ] ; then
	go get -u github.com/kubernetes-csi/csi-test
	pushd $GOPATH/src/github.com/kubernetes-csi/csi-test/cmd/csi-sanity
	make all
	make install
	popd
#./hack/get-sanity.sh
fi

sarjeet2013 · 2018-08-07T22:01:24Z

pkg/hostpath/controllerserver.go

+				goto result
+			}
+		}
+	}


Shouldn't there be duplicate snapshot entries where both, snapshotId and sourceVolumeId are given?

Actually it won't as it'll goto result once process this block. You can skip above comment

sarjeet2013 · 2018-08-07T22:04:02Z

pkg/hostpath/controllerserver.go

+	}
+
+	rsp := &csi.ListSnapshotsResponse{
+		Entries: entries,


It shouldn't return more than MaxEntries entries.

sarjeet2013 · 2018-08-07T22:12:45Z

pkg/hostpath/controllerserver.go

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


Can it be consistent to be req.GetSourceVolumeId()?

xing-yang · 2018-08-20T02:44:21Z

pkg/hostpath/controllerserver.go

+					Id:             exSnap.Id,
+					SourceVolumeId: exSnap.VolID,
+					CreatedAt:      exSnap.CreateAt,
+					Status:         exSnap.Status,


Need to return SizeBytes.

xing-yang · 2018-08-20T02:47:05Z

pkg/hostpath/controllerserver.go

+			SourceVolumeId: snapshot.VolID,
+			CreatedAt:      snapshot.CreateAt,
+			Status:         snapshot.Status,
+		},


SizeBytes is missing.

k8s-ci-robot · 2018-09-03T11:45:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wackxu
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: sbezverk

If they are not already assigned, you can assign the PR to them by writing /assign @sbezverk in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wackxu · 2018-09-03T13:04:14Z

@lpabon @sarjeet2013 @xing-yang Sorry for the delay, update, PTAL

wackxu · 2018-09-04T06:30:52Z

@lpabon @xing-yang I think we should merge this asap, since external-snapshotter is merged and it is easy for us to test the external-snapshotter if we merge this.

xing-yang · 2018-09-05T03:16:33Z

pkg/hostpath/controllerserver.go

+	snapshot.VolID = volumeID
+	snapshot.Path = file
+	snapshot.CreateAt = createAt
+	snapshot.SizeBytes = fileInfo.Size()


This should be the size of the original volume, not how much space is used.
For example, "hostPathVol.VolSize = capacity" in CreateVolume. We should do the same thing here.

Thanks for clarify this, Updated

k8s-ci-robot · 2018-09-05T14:32:13Z

@xing-yang: changing LGTM is restricted to assignees, and only kubernetes-csi/drivers repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

xing-yang

LGTM

wackxu · 2018-09-07T01:24:09Z

kindly ping @lpabon for approval

sarjeet2013 · 2018-09-07T20:50:09Z

pkg/hostpath/controllerserver.go

+	}, nil
+}
+
+func covertSnapshot(snap hostPathSnapshot) *csi.ListSnapshotsResponse {


Did you mean * convertSnapshot*?

sarjeet2013 · 2018-09-07T20:50:33Z

pkg/hostpath/controllerserver.go

+	}
+
+	var nextToken string
+	if n := startingToken + int32(i); n < ulenSnapshots {


Why not directly use j as next token?

sarjeet2013 · 2018-09-07T21:01:16Z

LGTM. few minor comments but not a blocker to merge.

wackxu · 2018-09-11T02:06:13Z

@sarjeet2013 Thanks for your review, Updated

lpabon suggested changes Jun 19, 2018

View reviewed changes

wackxu force-pushed the volsnap branch 2 times, most recently from 8080b3e to 07864ee Compare June 20, 2018 09:20

wackxu mentioned this pull request Jun 20, 2018

add test for volume snapshot kubernetes-csi/csi-test#83

Merged

wackxu changed the title ~~Hostpaht snapshot implementation~~ Hostpath snapshot implementation Jun 21, 2018

wackxu force-pushed the volsnap branch from 07864ee to 18a80f6 Compare June 28, 2018 12:09

wackxu mentioned this pull request Jun 28, 2018

fix ControllerGetCapabilities test kubernetes-csi/csi-test#81

Merged

wackxu force-pushed the volsnap branch from 4101a5f to c494345 Compare July 19, 2018 09:27

sarjeet2013 reviewed Aug 7, 2018

View reviewed changes

xing-yang reviewed Aug 20, 2018

View reviewed changes

wackxu force-pushed the volsnap branch from c494345 to 8f396fb Compare September 3, 2018 11:45

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 3, 2018

wackxu force-pushed the volsnap branch 2 times, most recently from 10a8abd to ce10084 Compare September 3, 2018 12:56

wackxu mentioned this pull request Sep 4, 2018

add restore volume from snapshot for hostpath #108

Merged

xing-yang reviewed Sep 5, 2018

View reviewed changes

sarjeet2013 reviewed Sep 7, 2018

View reviewed changes

wackxu force-pushed the volsnap branch from 4dbd697 to b7169dd Compare September 11, 2018 02:05

hostpath snapshot implemention

0910c66

wackxu force-pushed the volsnap branch from b7169dd to 0910c66 Compare September 11, 2018 11:43

k8s-ci-robot merged commit 0910c66 into kubernetes-retired:master Sep 11, 2018

wackxu deleted the volsnap branch September 12, 2018 01:12

Hostpath snapshot implementation #94

Hostpath snapshot implementation #94

Uh oh!

Conversation

wackxu commented Jun 11, 2018

Uh oh!

wackxu commented Jun 11, 2018

Uh oh!

lpabon commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wackxu commented Jun 20, 2018

Uh oh!

lpabon commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xing-yang commented Jun 21, 2018

Uh oh!

wackxu commented Jun 21, 2018

Uh oh!

wackxu commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarjeet2013 Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 3, 2018

Uh oh!

wackxu commented Sep 3, 2018

Uh oh!

wackxu commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 5, 2018

Uh oh!

xing-yang left a comment

Choose a reason for hiding this comment

Uh oh!

wackxu commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarjeet2013 commented Sep 7, 2018

Uh oh!

wackxu commented Sep 11, 2018

Uh oh!

Reviewers

lpabon commented Jun 20, 2018 •

edited

Loading

wackxu commented Jun 21, 2018 •

edited

Loading

sarjeet2013 Aug 7, 2018 •

edited

Loading