volumegroup: add volumeID's to createGroup #68
Conversation
Madhu-1
force-pushed
the
add-volume-id-to-grp
branch
from
51f6fb2 to
6456a12
Compare
|
hmm, I think it would be better to remove volumgroup support and have CSI-drivers handle internally how/if they need volumegroups. kubernetes-csi-addons will just pass listst of volumes to CSI-Addons procedures, just like VolumeGroupSnapshot does in its related CSI procedures. |
@nixpanic This is a complete change in plan for the implementation the plan was to reuse the volumegroup concept under the volumegroupreplication concept where the user will not see the volumegroup but we will make use of the volumegroup concept. any specific reason for this suggestion. |
CSI-drivers can use a volumegroup structure internally if they need one. I don't think it should be part of an API for consumers (like a CSI-Addons API). |
what is the use of it? the earlier discussion was to separate the replication and grouping into two different parts but now the suggestion is to make it a single API for both operations, am I assuming it is correct or you are suggesting something else? |
|
@nixpanic PTAL based on the discussion as we agreed to keep the current flow |
volumegroup/README.md
Outdated
| @@ -47,7 +47,7 @@ If a volume group corresponding to the specified volume group name already exist | |||
| parameters in the CreateVolumeGroupRequest, the Plugin MUST reply 0 OK with the corresponding CreateVolumeGroupResponse. | |||
| CSI Plugins MAY create the following types of volume groups: | |||
|
|
|||
| Create a new empty volume group. Note that N volumes with some backend label Y could be considered to be in "group Y" | |||
| Create a new empty volume group or a group with specific volumes. Note that N volumes with some backend label Y could be considered to be in "group Y" | |||
| which might not be a physical group on the storage backend. In this case, an empty group can still be created by the CO | |||
| to hold volumes. After the empty group is created, create a new volume, specifying the group name in the volume. | |||
There was a problem hiding this comment.
This isn't an option, maybe remove it?
create a new volume, specifying the group name in the volume
Or explain that ModifyVolumeGroupMembership needs to be called to add the volume afterwards.
There was a problem hiding this comment.
update to have ModifyVolumeGroupMembership call by CO
|
|
||
| // Specify volume_ids that will be added to the volume group. | ||
| // This field is OPTIONAL. | ||
| repeated string volume_ids = 4; |
There was a problem hiding this comment.
there should be an error code and description that explains an incompatible volume_ids list is passed
There was a problem hiding this comment.
I was expecting an INVALID_ARGUMENT error or something new... If the volume can only be part of a single group, returning ALREADY_EXISTS and asking to use a different name is not a solution.
There was a problem hiding this comment.
i referred to https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolumegroupsnapshot-errors for it, i can add invalid if you prefer but single volume is a limitation from the SP, let me know if you still suggest to have invalid error
There was a problem hiding this comment.
I prefer FAILED_PRECONDITION for volumes that can not be grouped together, for whatever reason (already part of a group, different features, ...). The ALREADY_EXISTS can then be a clear error that a volumegroup with the same name already exists (and the caller can then check if the volumegroup actually has the expected volumes).
Madhu-1
force-pushed
the
add-volume-id-to-grp
branch
from
6456a12 to
296c3b8
Compare
volumegroup/README.md
Outdated
| which might not be a physical group on the storage backend. In this case, an empty group can still be created by the CO | ||
| to hold volumes. After the empty group is created, create a new volume, specifying the group name in the volume. | ||
| to hold volumes. After the empty group is created, create a new volume, | ||
| specifying the group name in the volume. CO may call ModifyVolumeGroupMembership to add new volumes to the group. |
There was a problem hiding this comment.
This is not an option, a CSI Volume does not know about VolumeGroups:
specifying the group name in the volume
Please drop that part.
volumegroup/README.md
Outdated
| which might not be a physical group on the storage backend. In this case, an empty group can still be created by the CO | ||
| to hold volumes. After the empty group is created, create a new volume, specifying the group name in the volume. | ||
| to hold volumes. After the empty group is created, create a new volume, | ||
| specifying the group name in the volume. CO may call ModifyVolumeGroupMembership to add new volumes to the group. | ||
|
|
||
| At restore time, create a single volume from individual snapshot and then join an existing group. Create an empty group. | ||
| Create a volume from snapshot, specifying the group name in the volume. |
There was a problem hiding this comment.
Same bit is mentioned here too.
|
|
||
| // Specify volume_ids that will be added to the volume group. | ||
| // This field is OPTIONAL. | ||
| repeated string volume_ids = 4; |
There was a problem hiding this comment.
I was expecting an INVALID_ARGUMENT error or something new... If the volume can only be part of a single group, returning ALREADY_EXISTS and asking to use a different name is not a solution.
Madhu-1
force-pushed
the
add-volume-id-to-grp
branch
from
296c3b8 to
35f6b1e
Compare
add an optional volumeID's to the CreateVolumeGroupRequest where some storage providers might not support creating a empty group and expect some volume's to exists to create a group. and also helps to pre identify the group details for which volume already exists where a volume can be part of only one group and cannot be part of multiple groups. Signed-off-by: Madhu Rajanna <[email protected]>
Madhu-1
force-pushed
the
add-volume-id-to-grp
branch
from
35f6b1e to
8f93846
Compare
add an optional volumeID's to the CreateVolumeGroupRequest where some storage providers might not support creating a empty group and expect some volumes to exist to create a group. and also helps to pre-identify the group details for which volume already exists where a volume can be part of only one group and cannot be part of multiple groups.