add snapshot create delete capability #111
Conversation
…dd-snapshot-create-delete-capability
mweibel
left a comment
mweibel
left a comment
There was a problem hiding this comment.
looks good so far. I have a few questions and mostly nits.
For reviewers sake: it would be great to have an example in the examples folder ready to use for testing this. I currently didn't test it on a cluster although I did install the version to see if it starts and if we have any immediate error logs (we don't).
Co-authored-by: Michael Weibel <307427+mweibel@users.noreply.github.com>
mweibel
left a comment
mweibel
left a comment
There was a problem hiding this comment.
overall this looks very good. Most of the things I commented are not super critical things. Good work 👏
| - apiGroups: ["snapshot.storage.k8s.io"] | ||
| resources: ["volumesnapshots"] | ||
| verbs: [ "get", "list", "watch", "update" ] | ||
| - apiGroups: ["snapshot.storage.k8s.io"] | ||
| resources: ["volumesnapshotcontents"] | ||
| verbs: ["get", "list"] |
There was a problem hiding this comment.
maybe we misunderstood each other in the first review.
I'd keep these in, because external-provisioner has them as well.
In the end it doesn't really matter because snapshotter-role also maps those to the same SA but I think updating later to newer provisioner versions with potentially updated role definitions makes the update easier.
There was a problem hiding this comment.
what's the reason for this to not be in the chart?
driver/controller.go
Outdated
| AccessibleTopology: []*csi.Topology{ | ||
| { | ||
| Segments: map[string]string{ | ||
| topologyZonePrefix: d.zone, |
There was a problem hiding this comment.
| topologyZonePrefix: d.zone, | |
| topologyZonePrefix: snapshot.Zone.Slug, |
shouldn't we use snapshot.Zone.Slug instead?
There was a problem hiding this comment.
The d.zone is not entirely incorrect. Since the driver does not currently work across zones, it gets the zone from the metadata of the node on which it is running. The same zone logic is happening during the normal volume creation.
I think we will move to multi-zone support at some point, for which we will need to implement proper topology support. So i decided to refactor this whole section a bit. We now use the actual data from the volume to create the csiVolume.
Co-authored-by: Michael Weibel <307427+mweibel@users.noreply.github.com>
Co-authored-by: Michael Weibel <307427+mweibel@users.noreply.github.com>
Co-authored-by: Michael Weibel <307427+mweibel@users.noreply.github.com>
….com:disperate/csi-cloudscale into julian/add-snapshot-create-delete-capability
… TestPod_Create_Volume_From_Snapshot, remove unused client param
2 participants
Adds support for
ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT.