Add volume snapshots feature #331
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Sorry for the failed tests. Will fix that. |
|
@tsmetana given how excessive/annoying golint can be, I am going to look into copying upstreams |
|
OK. Thank you. I will fix the easy ones (gofmt) and the "errors" ignoring the "warnings" for the moment. |
|
i've opened kubernetes/repo-infra#43, if nobody responds today I'll just patch our repo-infra myself |
|
okay, patched |
tsmetana
force-pushed
the
snapshot-merge-pr
branch
from
0c2b15a to
cf93ab6
Compare
|
OK... I definitely must not have done that correctly. |
|
no, it's working, golint isn't running on snapshots anymore. But there's |
|
I think upstream only runs govet, of the four gometalinters https://github.com/kubernetes-incubator/external-storage/blob/master/repo-infra/verify/go-tools/verify-gometalinter.sh have to wonder why repo-infra imposes stricter requirements than upstream but oh well. I guess there needs to be a way to skip not just golint but the other ones too, I will work on it. |
|
k, #342 is merged. next tests should pass... |
| } | ||
| prId := string(uuid.NewUUID()) | ||
| if *id != "" { | ||
| prId = *id |
There was a problem hiding this comment.
We should default to provisionerName instead of generating every start up like https://github.com/kubernetes-incubator/external-storage/blob/master/ceph/rbd/cmd/rbd-provisioner/main.go#L67
this means Delete will succeed from any instance of the provisioner with same provisionername, I assume that is safe.
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("Snapshot %s is NOT completed successfully.", snapshotName) |
There was a problem hiding this comment.
CI govet says unreachable code, we will never break out of the above loop
There was a problem hiding this comment.
It's intentional. The offending method is being called from a goroutine that actually is meant to either succeed or fail... And wait forever when none of that happens.
There was a problem hiding this comment.
okay, can safely remove this line then
Copied from master branch of github.com/rootfs/snapshot
Re-application of 27b8d9d by Humble
This is a cumulative patch changing controller to be conformant with the proposal mainly with regards to status conditions. Most of the changes were made by Xing Yang with contributions by Huamin Chen and Jing Xu.
tsmetana
force-pushed
the
snapshot-merge-pr
branch
from
d353e6f to
a090778
Compare
|
need boilerplate :( can run Will merge asap afterwards :) |
|
merging, pls consider #331 (comment) at a later date, a new identity every time means if the snapshot provisioner restarts it will avoid deleting old snapshots which is maybe overly cautious |
|
Thanks. I'll take a look at the provisioner name. |
This patch set adds code for the volume snapshots support. The code comes from github.com/rootfs/snapshot master branch. In order to make the new code compile there are updated vendor dependencies. I have also modified the Makefile by adding new target for snapshots and also traditional "all" target to compile everything in the tree.
cc: @rootfs