add restore volume from snapshot for hostpath #108
Conversation
k8s-ci-robot
added
cncf-cla: yes
wackxu
force-pushed
the
restorevolume
branch
from
7eba5c3 to
059a528
Compare
|
/assign @lpabon @xing-yang |
|
@wackxu: GitHub didn't allow me to assign the following users: xing-yang. Note that only kubernetes-csi members and repo collaborators can be assigned. DetailsIn response to this:
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. |
lpabon
left a comment
lpabon
left a comment
There was a problem hiding this comment.
Looks great, just a few changes
| contentSource := req.GetVolumeContentSource() | ||
| if contentSource.GetSnapshot() != nil { | ||
| snapshotId := contentSource.GetSnapshot().GetId() | ||
| snapshot, ok := hostPathVolumeSnapshots[snapshotId] |
There was a problem hiding this comment.
We keep them in memory? So if we restart the driver they get lost, right? if this is is true, we should document this somewhere. I am ok with having it memory and documenting that this is a test-only driver.
There was a problem hiding this comment.
Yes, now, we store both hostpath volume and snapshot info in memory, agree with that it need document this since this a test-only driver.
| return nil, status.Errorf(codes.NotFound, "cannot find snapshot %v", snapshotId) | ||
| } | ||
| snapshotPath := snapshot.Path | ||
| args := []string{"zxvf", snapshotPath, "-C", path} |
| snapshotID := req.GetSnapshotId() | ||
| glog.V(4).Infof("deleting volume %s", snapshotID) | ||
| path := snapshotRoot + snapshotID + ".tgz" | ||
| os.RemoveAll(path) |
There was a problem hiding this comment.
Could log if the file not file for some reason. May be a good idea to return error if not found, which would be good for e2e tests.
There was a problem hiding this comment.
we already have a talk about this, #94 (comment)
|
Could you also add an issue to add unit tests to hostpath driver? We need to start adding tests. |
| if !ok { | ||
| return nil, status.Errorf(codes.NotFound, "cannot find snapshot %v", snapshotId) | ||
| } | ||
| snapshotPath := snapshot.Path |
There was a problem hiding this comment.
Do we also check the snapshot status before using it? This won't matter here, but a good practice if this example is being picked for other places.
There was a problem hiding this comment.
Thanks for your review, updated
|
LGTM |
wackxu
force-pushed
the
restorevolume
branch
from
059a528 to
26506de
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lpabon, wackxu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
based on #94