Skip to content

Commit 487defd

Browse files
committed
not support accessibility_requirements for now
1 parent b433a3f commit 487defd

File tree

1 file changed

+28
-20
lines changed
  • keps/sig-storage/5381-mutable-pv-affinity

1 file changed

+28
-20
lines changed

keps/sig-storage/5381-mutable-pv-affinity/README.md

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,8 @@ SIG Architecture for cross-cutting KEPs).
9191
- [Implementation History](#implementation-history)
9292
- [Drawbacks](#drawbacks)
9393
- [Alternatives](#alternatives)
94+
- [New GRPC](#new-grpc)
95+
- [User Specified Topology Requirement](#user-specified-topology-requirement)
9496
- [Infrastructure Needed (Optional)](#infrastructure-needed-optional)
9597
<!-- /toc -->
9698

@@ -368,22 +370,6 @@ message ControllerModifyVolumeRequest {
368370
// Indicates whether the CO allows the SP to update the topology
369371
// as a part of the modification.
370372
bool allow_topology_updates = 4;
371-
372-
// Specifies where (regions, zones, racks, etc.) the
373-
// volume MUST be accessible from after modification.
374-
// COs SHALL only specify topological accessibility
375-
// information supported by the SP.
376-
// This field is OPTIONAL.
377-
// This field SHALL NOT be specified unless the SP has the
378-
// VOLUME_ACCESSIBILITY_CONSTRAINTS plugin capability, the
379-
// MODIFY_VOLUME_TOPOLOGY controller capability and
380-
// allow_topology_updates is set to true.
381-
// If this field is not specified and the SP has the
382-
// VOLUME_ACCESSIBILITY_CONSTRAINTS plugin capability, the
383-
// MODIFY_VOLUME_TOPOLOGY controller capability and
384-
// allow_topology_updates is set to true, the SP MAY
385-
// modify the volume topology according to the mutable_parameters.
386-
TopologyRequirement accessibility_requirements = 5;
387373
}
388374
389375
message ControllerModifyVolumeResponse {
@@ -997,7 +983,10 @@ Why should this KEP _not_ be implemented?
997983
-->
998984

999985
## Alternatives
1000-
Instead of adding new fields to CSI GRPC `ControllerModifyVolume`, we could add a new GRPC `ControllerModifyVolumeTopology`:
986+
987+
### New GRPC
988+
989+
Instead of adding new fields to CSI GRPC `ControllerModifyVolume`, we could add a new GRPC `ControllerModifyVolumeTopology` (Other candidate names: `ControllerMigrateVolume`):
1001990

1002991
```protobuf
1003992
rpc ControllerModifyVolumeTopology (ControllerModifyVolumeTopologyRequest)
@@ -1010,7 +999,6 @@ message ControllerModifyVolumeTopologyRequest {
1010999
string volume_id = 1;
10111000
map<string, string> secrest = 2 [(csi_secret) = true];
10121001
map<string, string> mutable_parameters = 3;
1013-
TopologyRequirement accessibility_requirements = 4;
10141002
}
10151003
10161004
message ControllerModifyVolumeTopologyResponse {
@@ -1023,15 +1011,35 @@ message ControllerModifyVolumeTopologyResponse {
10231011
The workflow of this new GRPC is essentially the same as the current `ControllerModifyVolume` GRPC, but it allows SPs to mutate the accessible
10241012
topologies of volumes by default.
10251013

1026-
SPs with the `MODIFY_VOLUME_TOPOLOGY` controller capability should use this new GRPC instead of `ControllerModifyVolume` when modifying volumes.
1014+
SPs with the `MODIFY_VOLUME_TOPOLOGY` controller capability should implement both this new GRPC and `ControllerModifyVolume`.
1015+
New COs that support modify volume topology (i.e. external-resizer) should only call the new GRPC when modifying volumes.
1016+
Old COs can continue to call `ControllerModifyVolume`. SPs should reject such requests if topology will be changed.
10271017

10281018
Comparison between these two approaches:
1029-
| Criteria | PR 592 (Extended GRPC) | PR 593 (New GRPC) |
1019+
| Criteria | [PR 592](https://github.com/container-storage-interface/spec/pull/592) (Extended GRPC) | [PR 593](https://github.com/container-storage-interface/spec/pull/593) (New GRPC) |
10301020
| -------- | ---------------------- | ----------------- |
10311021
| Maintenance Difficulty | ✅ Low | ⚠️ High, need to also modify ControllerModifyVolumeTopology when making changes to ControllerModifyVolume |
10321022
| Implementation Complexity | ✅ Low | ⚠️ High, SPs will have to implement a new GRPC if they want to support topology modification even if they have implemented ControllerModifyVolume |
10331023
| Side Effects | ⚠️ Will impede the GA process of K8s VAC | ✅ No influence on other features |
10341024

1025+
### User Specified Topology Requirement
1026+
1027+
Currently we don't support user specified topology requirement.
1028+
We've considered a design:
1029+
* Add `accessibility_requirements` in `ModifyVolumeRequest`, like that in `CreateVolumeRequest`
1030+
* Add `allowedTopologies` in `VolumeAttributeClass`, like that in `StorageClass`
1031+
1032+
But facing a lot of unresolved questions:
1033+
* How to merge `allowedTopologies` from `VolumeAttributeClass`, `StorageClass`?
1034+
* Should we use `allowedTopologies` from `StorageClass` if it is not specified in `VolumeAttributeClass`?
1035+
* Should we consider the topology of the currently attached nodes?
1036+
* Should we consider the topology of all the nodes in the cluster?
1037+
1038+
In most cases, SP can determine the desired topology of a volume from the `mutable_parameters`, or from the currently attached nodes.
1039+
An exception could be: modifying a volume from regional to zonal, and it is not attached to any node.
1040+
In this case, SP will need more information from the CO to determine the desired zone.
1041+
But we don't have such use case now, we decided leave it as a future work.
1042+
10351043
<!--
10361044
What other approaches did you consider, and why did you rule them out? These do
10371045
not need to be as detailed as the proposal, but should include enough

0 commit comments

Comments
 (0)