Add Topology for Snapshot #274
Conversation
csi.proto
Outdated
| // Given a volume or snapshot should be accessible from TWO zones | ||
| // (because an opaque parameter in CreateVolumeRequest, for example, | ||
| // specifies the volume is accessible from two zones, aka | ||
| // synchronously replicated), and |
There was a problem hiding this comment.
This comment says that specifying the volume is accessible from two zones is equivalent to synchronous replication. But per the discussion in the meeting, I don't believe that is necessarily true - replication behavior is out of scope, e.g. implementers may replicate to satisfy accessibility requirements but are not required to.
There was a problem hiding this comment.
I agree with this. It's important to state that while snapshot replication MAY take place in order to satisfy accessibility requrements, clients MUST NOT assume that accessibility has any relation to replication, and if replication is desired, it should be requested in a different manner. Today, that means using opaque parameters to request replication.
There was a problem hiding this comment.
Reworded the comments.
xing-yang
force-pushed
the
topology
branch
3 times, most recently
from
a8448c4
to
0dbe605
Compare
|
PSA: As a general rule, please don't squash commits once reviewing has started. Commits can always be squashed at the end. This way reviewers that are following along can more easily review changes made over time to the PR. Thanks! |
spec.md
Outdated
| // ACCESSIBILITY_CONSTRAINTS plugin capability, the SP MAY choose | ||
| // where the provisioned volume is accessible from. | ||
| // VOLUME_ACCESSIBILITY_CONSTRAINTS plugin capability, the SP MAY | ||
| // choose where the provisioned volume is accessible from. | ||
| TopologyRequirement accessibility_requirements = 7; |
There was a problem hiding this comment.
Since a volume MAY be created from a snapshot, perhaps with specific workload placement in mind, it seems that the interesting/valid set of topologies (for on-demand workload placement) MAY be the intersection of the topologies of the (a) candidate nodes; (b) snapshots, and; (c) volume accessibility requirements specified here. Is this actually the case, or am I over-thinking this?
If my thinking is in line with the spirit of what this PR is trying to accomplish, then it would be great to see a bit more description fleshed out in the spec somewhere about the interaction between these three object types and their topologies.
There was a problem hiding this comment.
In other words, simply adding "or snapshot" to the existing set of documentation (as has been done throughout the PR here) probably isn't sufficient.
There was a problem hiding this comment.
When creating a volume from snapshot, SP will look at accessibility_requirements in CreateVolumeRequest and accessible_topology in CreateSnapshotResponse, and find an intersection of the two. This will be reflected in accessible_topology of CreateVolumeResponse.
Regarding accessibility for a node, the existing comment about accessible_topology in NodeGetInfoResponse says “COs MAY use this information along with the topology information returned in CreateVolumeResponse to ensure that a given volume is accessible from a given node when scheduling workloads.” This comment is still true for creating volume from snapshot.
I can add this to the spec.
There was a problem hiding this comment.
Spec updated with this clarification.
spec.md
Outdated
| // COs MAY use this information along with the topology information | ||
| // returned in CreateVolumeResponse to ensure that a given volume is | ||
| // accessible from a given node when scheduling workloads. | ||
| // returned in CreateVolumeResponse CreateSnapshotResponse to ensure |
This PR adds topology requirements for snapshots.
This PR adds topology requirements for snapshots.
Closes #241