Add VolumeSnapshotDelta CSI RPCs #522
Conversation
Signed-off-by: Ivan Sim <[email protected]>
There was a problem hiding this comment.
Please provide more context in the description of this RPC. There's no need to regurgitate the KEP in its entirety, but it was difficult for me to understand the usefulness of this w/o reading through parts of the related KEP.
For example (and I'm struggling w/ how to phrase this next part, so please bear w/ me)..
Dataplane is out of scope of CSI - and at the same time, CSI has "publish" calls that feel very close to dataplane territory (maybe since mount operations can involve a bit of (meta)data xfer). This proposal is purely controlplane and folks w/o CBT experience may be left wondering how in the world a CO is supposed to make use of this metadata since there are no other CSI calls that point to the dataplane at all here. Its only half of the overall useful API surface area. Without going into k8s details, it may be useful for such readers if the docs here mentioned something about the dataplane API aspect (block transfer w/ respect to backups) and how it's the CO's job to facilitate access to the raw data.
spec.md
Outdated
| option (alpha_message) = true; | ||
|
|
||
| // The block logical offset on the volume. This field is REQUIRED. | ||
| uint64 offset = 1; |
There was a problem hiding this comment.
Not necessarily. E.g., the AWS EBS Direct API uses its own logical offset scheme.
| CO MUST implement the specified error recovery behavior when it encounters the | ||
| gRPC error code. | ||
|
|
||
| | Condition | gRPC Code | Description | Recovery Behavior | |
There was a problem hiding this comment.
what if from/to snapshot id are swapped? are SPs required to accept a newer id in "from" and an older it in "to"? or is that considered a bad/invalid request?
There was a problem hiding this comment.
The intent is to let the SP do the error handling, and CSI just relays the response back to the end users. Different SP might handle this differently.
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
|
|
||
| | Condition | gRPC Code | Description | Recovery Behavior | | ||
| |-----------|-----------|-------------|-------------------| | ||
| | Target volume snapshot does not exist | 5 NOT_FOUND | Indicates that the target volume snapshot corresponding to the specified `to_snapshot_id` does not exist. | Caller MUST verify that the `to_snapshot_id` is correct and that the volume snapshot is accessible and has not been deleted before retrying with exponential back off. | |
There was a problem hiding this comment.
The base snapshot may not exist as well?
There was a problem hiding this comment.
In that case, the delta between the first snapshot since volume creation and the target snapshot will be retrieved.
|
This file is missing: lib/go/csi/csi.pb.go |
| // in the subsequent `ListSnapshotDeltas` call. This field is | ||
| // OPTIONAL. If not specified (zero value), it means there is no | ||
| // restriction on the number of entries that can be returned. | ||
| // The value of this field MUST NOT be negative. |
There was a problem hiding this comment.
if this value MUST NOT be negative, why not use an unsigned int?
There was a problem hiding this comment.
This the convention used for other definitions of max_entries in the spec.
| // in the subsequent `ListSnapshotDeltas` call. This field is | ||
| // OPTIONAL. If not specified (zero value), it means there is no | ||
| // restriction on the number of entries that can be returned. | ||
| // The value of this field MUST NOT be negative. |
There was a problem hiding this comment.
if this value MUST NOT be negative, why not use an unsigned int?
|
|
||
| // The ID of the base snapshot handle to use for comparison. If | ||
| // not specified, return all changed blocks up to the target | ||
| // specified by snapshot_target. This field is OPTIONAL. |
There was a problem hiding this comment.
snapshot_target is not part of this message, I think you mean to_snapshot_id.
| response to ensure data plane operations remain outside the scope of CSI. The | ||
| response payload MUST contain enough block location metadata for the end users | ||
| to retrieve the raw data blocks. The end users are responsbile for devising | ||
| their own transport and retrieval mechanism to access the raw data blocks. |
There was a problem hiding this comment.
Without an option to transport changed blocks, I am not sure how useful this is. Users would still need to talk to the specific SP directly, and that is exactly what CSI tries to prevent.
There was a problem hiding this comment.
At this point, data path remains outside the scope of CSI. The data protection white paper describes a SP-agnostic data movement mechanism here. Our observation is that different providers have different data movement implementation, which makes coming up with generic APIs challenging.
|
Superseded by #551. |
KEP: kubernetes/enhancements#4082