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).
| image: "{{ .Values.snapshotter.image.registry }}/{{ .Values.snapshotter.image.repository }}:{{ .Values.snapshotter.image.tag }}" | ||
| args: | ||
| - "--csi-address=$(CSI_ENDPOINT)" | ||
| - "--leader-election=true" |
There was a problem hiding this comment.
not related to this PR in particular, but setting the leader-election flag reminds me that we should check if we should enable this for other sidecars too or why it hasn't been enabled until now and whether we should synchronize this. Maybe @alakae knows more about the history in this regard.
Fact is, we use a StatefulSet for the controller deployment with 1 replica. This usually means that it's not possible for two controllers to run at the same time and we could therefore disable leader election.
driver/controller.go
Outdated
| // Verify snapshot exists and get its properties, must return NotFound when snapshot does not exist. | ||
| snapshot, err := d.cloudscaleClient.VolumeSnapshots.Get(ctx, sourceSnapshotID) | ||
| if err != nil { | ||
| errorResponse, ok := err.(*cloudscale.ErrorResponse) |
There was a problem hiding this comment.
type hinting on errors is possible but can be wrong if an error wraps another. This is most likely not the case here, but generally we should avoid using type hinting and instead use errors.Is or errors.As - see an introduction and documentation on how to use it in the go doc for the errors package.
There was a problem hiding this comment.
Updated to the errors.As() pattern. See 73a757f
| }, | ||
| } | ||
|
|
||
| f.snapshots[id] = snap |
There was a problem hiding this comment.
I know we don't have this in other tests here too, but I suspect we might run into data races if we run tests in parallel.
We should obtain a mutex before reading/writing to f.snapshots. See fakeMounter on how this can be done. Let me know if you need support implementing a mutex.
| kubectl apply -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v8.4.0/client/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml | ||
| kubectl apply -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v8.4.0/client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml | ||
| kubectl apply -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v8.4.0/client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml | ||
| # Install snapshot controller with RBAC | ||
| kubectl apply -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v8.4.0/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml | ||
| kubectl apply -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v8.4.0/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml | ||
| ``` |
There was a problem hiding this comment.
is there a reason we recommend to do this manually instead of using the recommended kustomize apply from the snapshot usage section?
makes the instructions a bit more brittle, should they ever rename the files.
Co-authored-by: Michael Weibel <307427+mweibel@users.noreply.github.com>
2 participants
Adds support for
ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT.