add volumeGroup spec #54

matancarmeli7 · 2023-06-28T12:52:00Z

No description provided.

nixpanic

Looks pretty complete at a first glance. Make sure you use existing CSI messages/options where you can, it will prevent duplicate type definitions and special handling (like not logging secrets) in CSI-drivers that implement this feature.

identity/README.md

nixpanic · 2023-06-28T13:28:37Z

volumegroup/README.md

@@ -0,0 +1,455 @@
+# Container Storage Interface (CSI)


This is not CSI directly, call it CSI-Addons instead?

As this is heavlly based on CSI and Kubernetes proposals for VolumeGroups, it would be nice to mention that and maybe provide links to the abandoned/rejected PRs/issues.

volumegroup/README.md

nixpanic · 2023-06-30T10:21:10Z

volumegroup/README.md

+  // Indicates that this message is OPTIONAL and part of an experimental
+  // API that may be deprecated and eventually removed between minor
+  // releases.
+  bool vg_alpha_message = 1102;


The alpha notations are not needed, we don't use them for other operations/messages/.. in csi-addons either. If you want to use this, use the defined values that are in csi.proto and drop the definitions from this file.

Leaving them out has my preference, there is currently no plan to handle alpha annotation specially.

done removed method/message alpha

volumegroup/README.md

nixpanic · 2023-07-03T12:30:01Z

volumegroup/README.md

+
+## Container Storage Interface
+
+This section describes the interface between COs and Plugins.


A description of what VolumeGroups are, and how users can benefit from them would be good to have here. Some hints are in the CreateVolumeGroup section, but more practical examples should be added here.

volumegroup/README.md

Madhu-1 · 2023-07-07T08:00:13Z

identity/README.md

+      VOLUME_GROUP = 1;
+      // LIMIT_VOLUME_TO_ONE_VG indicates that the CSI-driver does not support
+      // that one volume will be in multiple VGs, so it can be in only one VG.
+      LIMIT_VOLUME_TO_ONE_VG = 2;


LIMIT_VOLUME_TO_ONE_VG to LIMIT_VOLUME_TO_ONE_VOLUME_GROUP

Expand VG in all other places as well

Madhu-1 · 2023-07-07T08:01:23Z

identity/README.md

+      // won't delete the volumes under it.
+      DO_NOT_ALLOW_VG_TO_DELETE_VOLUMES = 3;
+      // MODIFY_GROUP_MEMBERSHIP indicates that the CSI-driver
+      // supports that nodifying the VG.


nodifying to modifying

Madhu-1 · 2023-07-07T08:02:03Z

identity/README.md

+      DO_NOT_ALLOW_VG_TO_DELETE_VOLUMES = 3;
+      // MODIFY_GROUP_MEMBERSHIP indicates that the CSI-driver
+      // supports that nodifying the VG.
+      MODIFY_GROUP_MEMBERSHIP = 4;


it should be called MODIFY_VOLUME_GROUP why this is called MODIFY_GROUP_MEMBERSHIP?

because the RPC call that it will enable, is ModifyVolumeGroupMembershipResponse, what do you think better? MODIFY_VOLUME_GROUP or MODIFY_VOLUME_GROUP_Membership?

MODIFY_VOLUME_GROUP looks good to me.

Madhu-1 · 2023-07-07T08:03:39Z

volumegroup/volumegroup.proto

+
+option go_package = ".;volumegroup";
+
+// Controller holds the RPC methods for replication and all the methods it


this is not about replication, comment need to be updated

done, changed it

Madhu-1 · 2023-07-07T08:28:02Z

volumegroup/volumegroup.proto

+  // If no volume_ids are provided, all existing volumes will
+  // be removed from the group.
+  // This field is OPTIONAL.
+  repeated string volume_ids = 2;


Can you please explain why this is an optional, not required?

all existing volumes will be removed from the group.

What is the difference between this and Deleting a volumeGroup. Giving more flexibility to the users might create a problem.

Some storage systems can have empty VolumeGroups, or the VolumeGroup needs to be created before volumes can be added to it. Possibly VolumeGroups can not be created dynamically, only by storage admins (with super powers 🦸). For these cases, the GetVolumeGroup RPC can be used, after which volumes can be added.

agree with @nixpanic, and we need the volume_ids in the modify call, because the driver needs to know which volumes to add and which to remove. In delete call, we just deleting the VG we don't care about the volumes in it

sounds good 👍🏻

Madhu-1 · 2023-07-07T08:29:12Z

volumegroup/volumegroup.proto

+message ModifyVolumeGroupMembershipResponse {
+  // Contains all attributes of the modified volume group.
+  // This field is REQUIRED.
+  VolumeGroup volume_group = 1;


we we need to send the group detail in response?

yes, because we can know by this field which volumeID were changed/ which one are now in the volumeGroup in the storage

Pull request has been modified.

nixpanic · 2023-07-10T14:40:00Z

identity/identity.proto

+      // plugin can invoke RPCs that require access to the storage system,
+      // similar to the CSI Controller (provisioner).
+      VOLUME_GROUP = 1;
+      // LIMIT_VOLUME_TO_ONE_VOLUME_GROUP indicates that the CSI-driver does not support


CI fails on this line:

[identity/identity.proto:181:1] The line length is 88, but it must be shorter than 80

Madhu-1 · 2023-07-12T11:22:14Z

@Mergifyio rebase

Signed-off-by: matancarmeli7 <[email protected]>

mergify · 2023-07-12T11:22:19Z

rebase

✅ Branch has been successfully rebased

ShyamsundarR

Late, but as this is a design noting comments here for the future.

ShyamsundarR · 2023-07-12T12:14:58Z

volumegroup/volumegroup.proto

+  // ListVolumeGroups RPC call to list volume groups.
+  rpc ListVolumeGroups(ListVolumeGroupsRequest)
+      returns (ListVolumeGroupsResponse) {}
+  // CreateVolumeGroup RPC call to get a volume group.


(nit) comment needs update based on the function declaration below

ShyamsundarR · 2023-07-12T12:29:43Z

volumegroup/README.md

+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.
+
+At restore time, create a single volume from individual snapshot and then join an existing group. Create an empty group.


This is currently just a definition of the group CRUD APIs, so some doubt remains on how these other functionalities (like restore) would be provided in the future. Which I assume would come in later, but just noting it here.

mergify bot added the design Adds or updates an operation or service label Jun 28, 2023

nixpanic requested changes Jun 28, 2023

View reviewed changes

matancarmeli7 force-pushed the add_volume_group_spec branch 4 times, most recently from d0f8038 to 3d178bf Compare June 29, 2023 13:05

nixpanic reviewed Jun 30, 2023

View reviewed changes

matancarmeli7 force-pushed the add_volume_group_spec branch 2 times, most recently from 90455fb to bd35954 Compare July 3, 2023 06:37

nixpanic reviewed Jul 3, 2023

View reviewed changes

volumegroup/README.md Show resolved Hide resolved

matancarmeli7 force-pushed the add_volume_group_spec branch 2 times, most recently from 15d6023 to 9736cbe Compare July 6, 2023 14:21

nixpanic previously approved these changes Jul 6, 2023

View reviewed changes

Madhu-1 reviewed Jul 7, 2023

View reviewed changes

matancarmeli7 force-pushed the add_volume_group_spec branch from 931309d to 2c3666f Compare July 9, 2023 06:46

matancarmeli7 force-pushed the add_volume_group_spec branch from 532ff7b to c7e2717 Compare July 9, 2023 07:20

nixpanic reviewed Jul 10, 2023

View reviewed changes

matancarmeli7 force-pushed the add_volume_group_spec branch from 6f01613 to d8d7817 Compare July 11, 2023 06:21

nixpanic mentioned this pull request Jul 11, 2023

adding vg api csi-addons/kubernetes-csi-addons#402

Closed

nixpanic requested a review from Madhu-1 July 11, 2023 12:43

nixpanic approved these changes Jul 11, 2023

View reviewed changes

Madhu-1 approved these changes Jul 12, 2023

View reviewed changes

add volumeGroup spec

662a996

Signed-off-by: matancarmeli7 <[email protected]>

Madhu-1 force-pushed the add_volume_group_spec branch from d8d7817 to 662a996 Compare July 12, 2023 11:22

mergify bot merged commit cc24a2c into csi-addons:main Jul 12, 2023
6 checks passed

ShyamsundarR reviewed Jul 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add volumeGroup spec #54

add volumeGroup spec #54

matancarmeli7 commented Jun 28, 2023

nixpanic left a comment

nixpanic Jun 28, 2023

matancarmeli7 Jun 29, 2023

nixpanic Jun 30, 2023

matancarmeli7 Jul 3, 2023

nixpanic Jul 3, 2023

matancarmeli7 Jul 3, 2023

Madhu-1 Jul 7, 2023

Madhu-1 Jul 7, 2023

matancarmeli7 Jul 9, 2023

Madhu-1 Jul 7, 2023

matancarmeli7 Jul 9, 2023

Madhu-1 Jul 7, 2023

matancarmeli7 Jul 9, 2023

Madhu-1 Jul 10, 2023

matancarmeli7 Jul 11, 2023

Madhu-1 Jul 7, 2023

matancarmeli7 Jul 9, 2023

Madhu-1 Jul 7, 2023

nixpanic Jul 7, 2023

matancarmeli7 Jul 9, 2023

Madhu-1 Jul 10, 2023

Madhu-1 Jul 7, 2023

matancarmeli7 Jul 9, 2023

nixpanic Jul 10, 2023

matancarmeli7 Jul 11, 2023

Madhu-1 commented Jul 12, 2023

mergify bot commented Jul 12, 2023

ShyamsundarR left a comment

ShyamsundarR Jul 12, 2023

ShyamsundarR Jul 12, 2023


		## Container Storage Interface

		This section describes the interface between COs and Plugins.


		option go_package = ".;volumegroup";

		// Controller holds the RPC methods for replication and all the methods it

add volumeGroup spec #54

add volumeGroup spec #54

Conversation

matancarmeli7 commented Jun 28, 2023

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 12, 2023

mergify bot commented Jul 12, 2023

✅ Branch has been successfully rebased

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment