Proposed Cluster (CSI) Volume Command (rebase) by thaJeztah · Pull Request #3606 · docker/cli

thaJeztah · 2022-05-13T14:42:38Z

rebase of Proposed Cluster (CSI) Volume Command #3129
cli-side changes for Add Swarm Cluster Volume support moby/moby#41982
relates to Proposal: Container Storage Interface (CSI) proposed standard moby/moby#31923
relates to [Draft Proposal] Swarm Cluster Volume Support with CSI Plugins moby/moby#39624

This proposal involves merging the cluster volume specific options and commands with the existing volume subcommand. There are a handful of new create flags which are only applicable to cluster volumes. There is also a new flag for list, which lists only cluster volumes and formats the table differently.

Examples of the output can be found in the new .golden files.

thaJeztah · 2022-05-13T15:45:41Z

looks like most of the intermediate commits no longer built (due to the vendoring changes), so I squashed them

- Write test for cluster volumes - Add inspect test, add update command - Add cluster volume opts to create - Add requisite and preferred topology flags - volume: move cluster bool in opts Signed-off-by: Drew Erny <derny@mirantis.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

codecov-commenter · 2022-05-13T18:15:12Z

Codecov Report

Merging #3606 (c994476) into master (d0df532) will increase coverage by 0.05%.
The diff coverage is 69.61%.

❗ Current head c994476 differs from pull request most recent head 2a77228. Consider uploading reports for the commit 2a77228 to get more accurate results

@@            Coverage Diff             @@
##           master    #3606      +/-   ##
==========================================
+ Coverage   58.96%   59.02%   +0.05%     
==========================================
  Files         288      289       +1     
  Lines       24419    24588     +169     
==========================================
+ Hits        14399    14513     +114     
- Misses       9148     9202      +54     
- Partials      872      873       +1

thaJeztah · 2022-05-13T18:17:28Z

Made some changes;

Annotated docker volume update and the cluster-volume flags on docker volume create both for documentation, and to hide them if swarm is not enabled (or the daemon is too old); for example;

docker volume update
docker volume update requires API version 1.42, but the Docker daemon API version is 1.41

And I removed the -g shorthand for --group (for now); we're trying to be conservative on shorthand flags, as they easily "conflict" with other options, and to reserve them for options that are very frequently used. It's easier to add later, than to remove/deprecated.

Worth noting that docker stack deploy does not yet support this feature, and may need changes in the compose-spec (?); we probably need a follow-up after this to add support for it.

And of course, we need to do a full review of the UX before GA (are we ok with all the flags? should some be "combined"?)

thaJeztah · 2022-05-13T18:21:41Z

cli/command/volume/list.go

+	flags.BoolVar(&options.cluster, "cluster", false, "Display only cluster volumes, and use cluster volume list formatting")



Let me annotate this one as well

Perhaps we should have a --filter for this, but we can discuss that further

This hides the flags when connecting to an older engine, or if swarm is not enabled, and is also used to add badges in the documentation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-05-13T18:57:46Z

@dperny @corhere @tianon @cpuguy83 PTAL

corhere · 2022-05-16T19:44:06Z

cli/command/volume/create.go

+	return flags.Changed("group") || flags.Changed("scope") ||
+		flags.Changed("sharing") || flags.Changed("availability") ||
+		flags.Changed("type") || flags.Changed("secrets") ||
+		flags.Changed("limit-bytes") || flags.Changed("required-bytes")


Would it be feasible to derive the set of flags to check from the annotations so that it couldn't get out of sync with the flag definitions?

Yes, there's some things to clean up in this area. I had a variant where I defined a slice of flag-names that are related, and used the slice for both setting the annotations and for this code, but in the end decided to keep my changes on top of @dperny's PR minimal

My goal with this rebase was to have the PR in a "mergeable" state, so that I could do a test-build/test-release of 22.06 with at least something that exposes the new feature. So I only fixed the type renames from upstream, and added the second commit to have the correct annotations on the flags.

corhere · 2022-05-16T20:23:31Z

cli/command/volume/create.go

 				}
 				options.name = args[0]
 			}
+			options.cluster = hasClusterVolumeOptionSet(cmd.Flags())


I think creating a cluster volume should be explicitly set by the user, rather than implied by setting any of the cluster-volume options so that users don't unintentionally create cluster volumes by setting a generically-named flag. Strawman: the --group flag pulls double-duty as the "create a cluster volume" flag by renaming it to ‑‑cluster‑volume GROUPNAME and making it an error to set any of the other cluster-only options unless that flag is also set. If a user wanted for whatever reason to create a cluster volume in the empty-string group, they could do so explicitly: docker volume create ‑‑cluster‑volume ""

I was considering that we could use the --scope flag to match docker network create. However docker network create accepts --scope=swarm for "swarm managed / scoped networks" and --scope=local for node-local networks, so we may have to do a follow-up to make that work as an option.

We should finalise that discussion before "GA" of 22.06, but I'd be ok with shipping what we have now in the first builds, and do follow-ups on that (as long as changes are done before 22.06 GA)

rumpl

Let's get this one in, we can fix the UX/flags/todos in a followup

dperny · 2022-05-17T15:46:28Z

This looks fine to me for now. I'm more than happy to make any UI changes desired. UI is the easy/fun part.

thaJeztah · 2022-05-17T16:43:12Z

Okay; let's get this in for now

olljanat · 2023-02-05T10:48:54Z

FYI. I have build some example scripts and guidance to https://github.com/olljanat/csi-plugins-for-docker-swarm about how to make existing CSI plugins working with Swarm which those who following this discussion might find useful.

See docker/cli#3606

thaJeztah force-pushed the carry_csi_volumes branch from 20be9c0 to 29f12f4 Compare May 13, 2022 15:45

thaJeztah force-pushed the carry_csi_volumes branch 2 times, most recently from 81840bc to af1306c Compare May 13, 2022 16:03

thaJeztah added impact/changelog kind/feature status/2-code-review area/swarm area/volumes labels May 13, 2022

thaJeztah added this to the 22.06.0 milestone May 13, 2022

thaJeztah force-pushed the carry_csi_volumes branch 2 times, most recently from a8bcd82 to c994476 Compare May 13, 2022 16:22

thaJeztah mentioned this pull request May 13, 2022

Proposed Cluster (CSI) Volume Command #3129

Closed

thaJeztah force-pushed the carry_csi_volumes branch from c994476 to 2a77228 Compare May 13, 2022 18:12

thaJeztah marked this pull request as ready for review May 13, 2022 18:12

thaJeztah commented May 13, 2022

View reviewed changes

volume: annotate cluster-volumes flags for API version and swarm

0fab8ec

This hides the flags when connecting to an older engine, or if swarm is not enabled, and is also used to add badges in the documentation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the carry_csi_volumes branch from 2a77228 to 0fab8ec Compare May 13, 2022 18:22

corhere reviewed May 16, 2022

View reviewed changes

rumpl approved these changes May 17, 2022

View reviewed changes

thaJeztah merged commit 7f073ab into docker:master May 17, 2022

thaJeztah deleted the carry_csi_volumes branch May 17, 2022 16:43

This was referenced May 28, 2022

vendor: github.com/docker/docker cb01202de88e20d5699c76efe948dfb065da873f #3635

Merged

Flaky test: TestVolumeCreateClusterOpts #3636

Closed

pedroetb mentioned this pull request Feb 3, 2023

add docker volume driver seaweedfs/seaweedfs#891

Open

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023

CLI 23.0.0: update volume

1a58c2a

See docker/cli#3606

matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023

CLI 23.0.0: update volume

68f36a8

See docker/cli#3606

ieugen mentioned this pull request Feb 16, 2023

Does it work with docker 23.x with CSI support? kubernetes-sigs/aws-ebs-csi-driver#1513

Closed

		flags.BoolVar(&options.cluster, "cluster", false, "Display only cluster volumes, and use cluster volume list formatting")

Conversation

thaJeztah commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented May 13, 2022

Uh oh!

codecov-commenter commented May 13, 2022

Codecov Report

Uh oh!

thaJeztah commented May 13, 2022

Uh oh!

thaJeztah May 13, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 13, 2022

Uh oh!

corhere May 16, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 17, 2022

Choose a reason for hiding this comment

Uh oh!

corhere May 16, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 17, 2022

Choose a reason for hiding this comment

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

dperny commented May 17, 2022

Uh oh!

thaJeztah commented May 17, 2022

Uh oh!

olljanat commented Feb 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaJeztah commented May 13, 2022 •

edited

Loading