Global Default Address Pool feature support #1233

selansen · 2018-07-25T17:02:22Z

This feature brings new attribute/option for swarm init command.
default-addr-pool will take string input which can be in below format.
"CIDR,CIDR,CIDR...:SUBNET-SIZE".

Signed-off-by: selansen elango.siva@docker.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cpuguy83 · 2018-07-25T17:23:10Z

Do you have links to upstream PRs for this?

thaJeztah · 2018-07-25T18:43:38Z

cli/command/swarm/init.go

@@ -44,6 +45,7 @@ func newInitCommand(dockerCli command.Cli) *cobra.Command {
 	flags.BoolVar(&opts.forceNewCluster, "force-new-cluster", false, "Force create a new cluster from current state")
 	flags.BoolVar(&opts.autolock, flagAutolock, false, "Enable manager autolocking (requiring an unlock key to start a stopped manager)")
 	flags.StringVar(&opts.availability, flagAvailability, "active", `Availability of the node ("active"|"pause"|"drain")`)
+	flags.StringVar(&opts.defaultAddrPool, flagDefaultAddrPool, "", "List of default subnet addresses followed by subnet size (format: <subnet,subnet,..:subnet-size)")


This flag likely needs an API version annotation, so that it's hidden on daemons not having this feature

A bit unsure about the format used for this option as well (combination of comma-separated and colon-separated options); also wondering; must all address pools have the same subnet size? I can foresee situations where sizes can differ for each (could use input on that one)

We(our networking team) had gone through multiple rounds of discussion as part of design and decided to support single subnet size for all subnets. Main reason is , currently there is no CLI option to explicitly specify what kind of subnet (ex /24 or /20) user want when they create network.

@thaJeztah I didn't understand how I should address below comment. I will sync with you offline
"This flag likely needs an API version annotation, so that it's hidden on daemons not having this feature"

for the annotation, see for example

cli/cli/command/stack/deploy.go

Line 62 in 48fbb12

flags.SetAnnotation("compose-file", "version", []string{"1.25"})

the API version in that annotation will enable the flag if the daemon supports that version of the API; if not (if the daemon is older), the flag is disabled and hidden

local scope still works(Daemon option). This is for global scope and work (swarm init option). So its kind of two different feature now.

We have design document which I can share it with you. Pls let me know.

I thought I responded to this question. May be in libnetwork PR. Local scope configuration works in Daemon config and global scope configuration works in swarm Init . Now its kind of two separate feature.

@thaJeztah added API version info

It might be cleaner to have a separate default-addr-pool-size parameter, which could default to 24. : is the group separator for IPv6 literals, so it is awkward also to use it to separate fields in a parameter.

I'm a little late to the party here but the long syntax for swarm port publishing may be a good example to follow:
--publish published=8080,target=80,mode=host

We could do something like:
--default-addr-pool pools=<subnet>,<subnet>,<subnet>,mask=20

If the mask is not included it defaults to 24.

thaJeztah · 2018-07-25T18:48:26Z

cli/command/swarm/init.go

@@ -56,6 +58,7 @@ func runInit(dockerCli command.Cli, flags *pflag.FlagSet, opts initOptions) erro
 		ListenAddr:       opts.listenAddr.String(),
 		AdvertiseAddr:    opts.advertiseAddr,
 		DataPathAddr:     opts.dataPathAddr,
+		DefaultAddrPool:  opts.defaultAddrPool,


Haven't seen the related dockerd PR yet, but from an API perspective, we should probably parse the flag and send the information more structured over the wire (i.e., not use the string format)

Validation is taken care in DockerD/swarmkit.

selansen · 2018-07-25T21:54:05Z

@cpuguy83 , I have just published Libnetwork, swarmkit and CLI PRs. Once CI passed for all, then I will create PR for upstream linking all of of the PR.

thaJeztah · 2018-07-25T23:08:11Z

docs/reference/commandline/swarm_init.md

@@ -26,6 +26,7 @@ Options:
      --availability string             Availability of the node ("active"|"pause"|"drain") (default "active")
      --cert-expiry duration            Validity period for node certificates (ns|us|ms|s|m|h) (default 2160h0m0s)
      --data-path-addr string           Address or interface to use for data path traffic (format: <ip|interface>)
+      --default-addr-pool string	List of default subnet addresses followed by subnet size (format: <subnet,subnet,..:subnet-size)


Should this option also be on swarm update? (i.e., is it possible to change default address pools without having to tear down the cluster?)

that is P1. once I finish all P0 tasks, I will start working on P1.

nit: no closing '>' Actually, maybe the format should be cidr[,cidr]*:masklen.

In general, a problem I'm having is that we refer to that last parameter as the "subnet size". But that's actually misleading. It's the length of the mask that accordingly defines the subnet size. This applies to parts of the code as well.

Maybe clearer terminology would be

"address block" or "address range" -- contiguous address space that will be carved up into allocatable subnets

"subnet mask length" or just "mask length" -- the mask length for all subnets in the global address pool

These are a bit more verbose. I dunno. Maybe the existing terminology is more clear to others. Would welcome comment.

I changed the format . WRT subnet size, the same name has been used in existing code and changing it will again mislead having two different name which points to same attribute.

albers

Thanks for also considering the completions.
I found one problem, please fix.

albers · 2018-07-26T07:29:03Z

contrib/completion/bash/docker

+		--default-addr-pool)
+			COMPREPLY=( $( compgen -W "list of subnet addresses followed by subnet size" -- "$cur" ) )
+			return
+			;;


This is not correct. The argument to -W is the list of possible completions.
Furthermore, I cannot think of a useful completion, so it's best not to complete anything here, allowing any string to be entered.

You can acchieve this by removing this block and adding --default-addr-pool to the option list in https://github.com/selansen/cli/blob/14dc79f6d3604f474efc500f665574aaac9d3808/contrib/completion/bash/docker#L3664.

codecov-io · 2018-07-28T20:11:38Z

Codecov Report

Merging #1233 into master will decrease coverage by <.01%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
- Coverage   54.77%   54.77%   -0.01%     
==========================================
  Files         292      292              
  Lines       19260    19275      +15     
==========================================
+ Hits        10549    10557       +8     
- Misses       8056     8061       +5     
- Partials      655      657       +2

GordonTheTurtle · 2018-07-29T02:13:12Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "default_addr_pool" git@github.com:selansen/cli.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

thaJeztah · 2018-08-21T10:38:10Z

vendor/github.com/docker/docker/vendor.conf

@@ -26,7 +26,7 @@ github.com/imdario/mergo v0.3.6
 golang.org/x/sync 1d60e4601c6fd243af51cc01ddf169918a5407ca

 # buildkit
-github.com/moby/buildkit e57eed420c7573ae44875be98fa877175b4677a1
+github.com/moby/buildkit 46f9075ab68a07df2c40ae6e240ce4f9392b3a66 git://github.com/tiborvass/buildkit.git


~~I ignored this bump, because it's a temporary fork, and this didn't bring in changes to vendored files.~~

whoops, commented on the wrong PR 😅

thaJeztah · 2018-08-21T10:50:31Z

vendor/github.com/docker/docker/vendor.conf

@@ -125,7 +125,7 @@ github.com/containerd/ttrpc 94dde388801693c54f88a6596f713b51a8b30b2d
 github.com/gogo/googleapis 08a7655d27152912db7aaf4f983275eaf8d128ef

 # cluster
-github.com/docker/swarmkit 8852e8840e30d69db0b39a4a3d6447362e17c64f
+github.com/docker/swarmkit cfa742c8abe6f8e922f6e4e920153c408e7d9c3b


Not critical, but SwarmKit should also be bumped in this PR, as the protobufs are used in the CLI (to show default values). I have another PR open that also brings in these changes, so if that's merged first, it's already taken care of (see #1225)

Then Ideally I should go back to Moby update swarmkit vndr there and then update moby vndr here. I cant directly update swarmkit vndr here, right ?

Oh; sorry. perhaps my comment was confusing. When I update the (e.g.) github.com/docker/docker in /vendor.conf, I also check if the new version of github.com/docker/docker is expecting more recent versions of other dependencies. I do so by checking the nested vendor.conf of the github.com/docker/docker dependency (this file 😄).

I then check any dependency that was changed there and that's also used by docker/cli, and check if it's more recent than the one currently used; if so, I consider updating the dependency in docker/cli as well.

will update now. In that way one PR will have all related information.

thaJeztah · 2018-08-21T13:26:32Z

I still don't like the custom format for this flag for various reasons (some also mentioned above);

We need to maintain custom parsing code to handle this format
The format uses positional arguments (1..n CIDR's, optionally followed by a separator, and an optional size). Because of this, the format is not easily extensible; adding another option (for whatever reason) would mean that new option is also positional, and with size being optional, this means including empty values in the format (10.10.0.0/16,20.20.0.0/16::someoption:::otheroption; :: meaning; an optional argument was left blank).
Using : as a separator is troublesome; even though we currently don't support it, using : as a separator means we will never be able to specify IPv6 pools.
This option will only be set once; when initializing a new swarm cluster. Because of that, there's no reason at all to define a shorthand syntax. We should limit shorthand syntaxes to options that are frequently used (e.g. the shorthand -p 1234:80 notation to publish ports). For options that are rarely used, we should prefer verbose over short.

So, what options do we have?

1. (rejected) CSV notation; repeat flag for each pool.

This would've been my preferred option; use the same format as is used on the daemon;

docker swarm init \
  --advertise-addr 127.0.0.1:2377 \
  --default-addr-pool base=10.10.0.0/16,size=24 \
  --default-addr-pool base=20.20.0.0/16,size=24 \

We discussed that option, but having independent sizes per pool would complicate things, and (though this could be enforced in validation), preference was to have a single size for all pools, and to reflect this in the API and CLI

2. CSV notation; allow multiple pools to be in a single flag;

As suggested by @mark-church #1233 (comment)

docker swarm init \
  --advertise-addr 127.0.0.1:2377 \
  --default-addr-pool pools=10.10.0.0/16,10.10.0.0/16,10.10.0.0/16,mask=20

Some initial thoughts;

we may need special parsing for that format, due to a comma being both used as separator for different options in the CSV format, but also as separator between pools
pools feels a bit redundant; should we use base? cidr? Something else?
interestingly, mask is suggested; there was discussion around this on the SwarmKit PR, because size was not the best name for that option. In that PR it was decided to use size because that's what's already used on the daemon.
- I'm still on the fence for that one; if the daemon.json option is the _only_place where we currently use this naming, we can still deprecate that option (keep it as an alias for backward compatibiity), and change to a better name.

There is one more caveat with this format: if we (again, for whatever reason) need to add an additional configuation per pool, things will become complicated quickly. The flag now describes multiple pools, and a single mask (size); if we want to add an option per-pool, that option would (probably) need to be a comma-separated field, and options in that field will be positional.

Perhaps better described with an example;

docker swarm init \
  --default-addr-pool pools=<cidr-1>,<cidr-2>(......),<cidr-5>,mask=20,<option-for-cidr-1>,,,,<option-for-cidr-5>

To correlate "option-for-cidr-N" with "cidr-N", those options would have to be specifiedin the same order, and at the same position; easy to make mistakes there, and difficult to read.

3. CSV: one flag per pool, separate flags for CIDR and Size ("mask"? "mask-length"?)

Use CSV notation to allow adding options in future, but keep size separate (because it's shared between all pools);

docker swarm init \
  --advertise-addr 127.0.0.1:2377 \
  --default-addr-pool cidr=10.10.0.0/16 \
  --default-addr-pool cidr=20.20.0.0/16 \
  --default-addr-pool cidr=30.30.0.0/16 \
  --default-addr-pool-size 20

4. No CSV; one flag per pool, separate flags for CIDR and Size ("mask"? "mask-length"?)

The CSV format in 3. is currently "overkill", given that there's only one option (cidr). We can use a plain cidr format;

docker swarm init \
  --advertise-addr 127.0.0.1:2377 \
  --default-addr-pool 10.10.0.0/16 \
  --default-addr-pool 20.20.0.0/16 \
  --default-addr-pool 30.30.0.0/16 \
  --default-addr-pool-size 20

If, in future, we do need additional options per-pool, we can add support for the CSV format; this can be done while remaining backward-compatible. Similar to -p accepting both a shorthand and CSV notation;

# no comma or "=": parse as plain CIDR
--default-addr-pool 10.10.0.0/16

# contains "=": parse as CSV
--default-addr-pool cidr=10.10.0.0/16

With the current status, my preference would be to use option 4;

It's verbose, but not overly lengthy
It uses well-defined, well-known formats for the values to specify
It's easy to validate (Golang standard library should provide this)
No custom format, so no custom parsing is needed

vdemeester · 2018-08-21T13:50:51Z

Same as @thaJeztah, I would definitely prefer the option 4. :angel:

selansen · 2018-08-21T14:03:31Z

@mark-church pls let me know if you are ok with @thaJeztah's option 4. I will start working on the PR.

thaJeztah · 2018-08-21T14:57:00Z

cli/command/system/info.go

+			fmt.Fprintln(dockerCli.Out(), " Default Address Pool:", strAddrPool.String())
+			fmt.Fprintln(dockerCli.Out(), " SubnetSize:", info.Swarm.Cluster.SubnetSize)
+		} else {
+			fmt.Fprintln(dockerCli.Out(), " Default Address Pool:", "10.0.0.0/8")


Just noticed this, but wondering if this is correct; Is this the only pool that swarmkit uses?

Also; if this default is known daemon-side, it should be returned by the daemon (i.e., we should not hard-code the value here)

Yes. This is the only pool we have hardcoded in libnetwork. As this information is not available in swarmkit we dont store it in cluster object currently. I can set it in moby instead of CLI here.
Either I can remove when I create new moby PR and update it or I can remove it here now and later update moby code with new PR. let me know how do you prefer. I can fix it.

It's better to remove it here, and only print the information we actually get from the API.

This is the only pool we have hardcoded in libnetwork. As this information is not available in swarmkit we dont store it in cluster object currently.

Perhaps it's worth considering to define a default either in moby, or in swarmkit, so that the pool is always initialised (also from a swarmkit perspective); we can discuss, and change that after this PR is merged though

I initially thought of displaying only if the user configure it. But later I thought people will complain if they dont see output in in built case. will remove from here and update it on Moby

thaJeztah · 2018-08-21T14:58:30Z

cli/command/swarm/init.go

+	if len(result) > 2 {
+		return nil, 0, fmt.Errorf("Invalid default address pool format. Expected format CIDR[,CIDR]*:SUBNET-SIZE")
+	}
+	// if size is not specified default size is 24


We should make sure that a default is set daemon-side; at the API level, defaults should be useful, so sending 0 should make the daemon pick the default value.

This check will go away with new CLI syntax . But will add extra check and setting default value in moby along with above change you suggested

thaJeztah · 2018-08-21T15:29:24Z

@selansen I just recalled we were discussing to use an IPNet-flag for this at the time; spf13 didn't have a flag for an IPNet slice, but there's an open PR for that; I did a quick try to update this PR with option 3, and using a version of spf13 that does have the IPNet slice flag (forked that repo and pulled in the changes from that PR); with that the code needed for this feature is really minimal; master...thaJeztah:default_addr_pool_update

selansen · 2018-08-21T15:39:11Z

Yes. that will save few validation code I have just finished now.
is it ok to use temp commit id on cli vendor.conf ?

thaJeztah · 2018-08-21T15:47:22Z

cli/command/system/info.go

+			for _, p := range info.Swarm.Cluster.DefaultAddrPool {
+				strAddrPool.WriteString(p + "  ")
+			}
+			fmt.Fprintln(dockerCli.Out(), " Default Address Pool:", strAddrPool.String())


oh, sorry, missed that: this should be " Default Address Pools:" (plural) here; because we're showing all address pools

we had debate on that a while back. pool itself means group of address. so we decided to keep it like that. I can change it if you want .

thaJeztah · 2018-08-21T15:50:20Z

cli/command/system/info.go

+		var strAddrPool strings.Builder
+		if info.Swarm.Cluster.DefaultAddrPool != nil {
+			for _, p := range info.Swarm.Cluster.DefaultAddrPool {
+				strAddrPool.WriteString(p + "  ")


Perhaps we should use the same formatting as is used for manager IP-addresses here (one pool per line);

cli/cli/command/system/info.go

Lines 300 to 304 in fd060d2

sort.Strings(managers)

fmt.Fprintln(dockerCli.Out(), " Manager Addresses:")

for _, entry := range managers {

fmt.Fprintf(dockerCli.Out(), " %s\n", entry)

}

there were other existing examples had similar format. Hence I followed the same. if you want I can change that too.
Docker info
Network: bridge host macvlan null overlay
Log: awslogs fluentd gcplogs gelf journald json-file logentries splunk syslog

It's not a blocker, we can discuss later, and do a follow up if we think the other format is better

selansen · 2018-08-21T17:30:57Z

@thaJeztah @vdemeester modified code with option 4. PTAL

thaJeztah

LGTM, thanks!!

I think it would be good to have an integration test for this (as a follow-up). We also discussed

additional validation on the daemon for some corner-cases (mask length bigger than size in CIDR)
discuss putting the default pool information in the daemon, so that they are known to swarmkit (instead of only known to libnetwork)

thaJeztah · 2018-08-21T18:11:26Z

cli/command/swarm/init.go

 	addSwarmFlags(flags, &opts.swarmOptions)
 	return cmd
 }

 func runInit(dockerCli command.Cli, flags *pflag.FlagSet, opts initOptions) error {
+	var (


nit: this is now a single variable declaration, so can be;

var defaultAddrPool []string

I already added integration test in Moby. Here I added unit tests but after our code change , those tests were not required. Hence I removed them in last commit

I will start email thread with you and @anshulpundir and cover all points we discussed.

thaJeztah · 2018-08-21T18:14:12Z

cli/command/system/info.go

+		var strAddrPool strings.Builder
+		if info.Swarm.Cluster.DefaultAddrPool != nil {
+			for _, p := range info.Swarm.Cluster.DefaultAddrPool {
+				strAddrPool.WriteString(p + "  ")


It's not a blocker, we can discuss later, and do a follow up if we think the other format is better

thaJeztah · 2018-08-21T18:15:20Z

docs/reference/commandline/swarm_init.md

@@ -26,6 +26,8 @@ Options:
      --availability string             Availability of the node ("active"|"pause"|"drain") (default "active")
      --cert-expiry duration            Validity period for node certificates (ns|us|ms|s|m|h) (default 2160h0m0s)
      --data-path-addr string           Address or interface to use for data path traffic (format: <ip|interface>)
+      --default-addr-pool IPnet     	List of default address pool (format: <cidr>)


Indentation looks off here

thaJeztah · 2018-08-21T18:16:29Z

docs/reference/commandline/swarm_init.md

@@ -128,6 +130,14 @@ management traffic of the cluster.
 If unspecified, Docker will use the same IP address or interface that is used for the
 advertise address.

+### `--default-addr-pool`
+This flag specifies default subnet pools for global scope networks.
+Format example is `--default-addr-pool 30.30.0.0/16 --default-addr-pool 40.40.0.0/16`


I'll add a "docs-revisit" label; we'll probably want some more information about this feature (more details how it will be used by SwarmKit etc)

thaJeztah · 2018-08-21T18:17:27Z

vendor.conf

@@ -72,7 +72,8 @@ github.com/satori/go.uuid d41af8bb6a7704f00bc3b7cba9355ae6a5a80048
 github.com/shurcooL/sanitized_anchor_name 10ef21a441db47d8b13ebcc5fd2310f636973c77
 github.com/sirupsen/logrus v1.0.6
 github.com/spf13/cobra v0.0.3
-github.com/spf13/pflag v1.0.1
+# temporary fork with https://github.com/spf13/pflag/pull/170 applied, which isn't merged yet upstream
+github.com/spf13/pflag 4cb166e4f25ac4e8016a3595bbf7ea2e9aa85a2c https://github.com/thaJeztah/pflag.git


I'll open a tracking-issue for this; let's try to get the PR merged upstream 👍

^^ opened #1330

This feature brings new attribute/option for swarm init command. default-addr-pool will take string input which can be in below format. "CIDR,CIDR,CIDR...:SUBNET-SIZE". Signed-off-by: selansen <elango.siva@docker.com>

crosbymichael · 2018-08-21T18:55:19Z

LGTM

selansen · 2018-08-21T18:59:05Z

Thanks @thaJeztah @crosbymichael

ushuz · 2018-11-12T04:20:39Z

As of today, docs still not updated to reflect these changes.
https://docs.docker.com/engine/reference/commandline/swarm_init/

selansen requested review from albers, thaJeztah and vdemeester as code owners July 25, 2018 17:02

GordonTheTurtle added the status/0-triage label Jul 25, 2018

thaJeztah added the status/needs-vendoring label Jul 25, 2018

thaJeztah reviewed Jul 25, 2018

View reviewed changes

selansen force-pushed the default_addr_pool branch from e0e150b to 14dc79f Compare July 25, 2018 22:09

thaJeztah reviewed Jul 25, 2018

View reviewed changes

albers requested changes Jul 26, 2018

View reviewed changes

thaJeztah mentioned this pull request Jul 27, 2018

Global Default Address Pool feature support moby/swarmkit#2714

Merged

selansen force-pushed the default_addr_pool branch from 14dc79f to 1f43dc4 Compare July 28, 2018 20:11

selansen force-pushed the default_addr_pool branch 3 times, most recently from fcbd903 to 27fa79f Compare July 29, 2018 02:13

GordonTheTurtle added the dco/no label Jul 29, 2018

selansen force-pushed the default_addr_pool branch from 27fa79f to ce858bd Compare July 29, 2018 02:37

GordonTheTurtle removed the dco/no label Jul 29, 2018

selansen force-pushed the default_addr_pool branch 6 times, most recently from 0f82b83 to 95bd1e3 Compare July 30, 2018 03:20

vdemeester added status/1-design-review and removed status/0-triage labels Jul 30, 2018

selansen force-pushed the default_addr_pool branch 2 times, most recently from 53af407 to 80eb174 Compare August 21, 2018 03:00

thaJeztah reviewed Aug 21, 2018

View reviewed changes

thaJeztah mentioned this pull request Aug 21, 2018

Use warnings provided by daemon #1225

Merged

thaJeztah reviewed Aug 21, 2018

View reviewed changes

selansen force-pushed the default_addr_pool branch from 80eb174 to a8002b4 Compare August 21, 2018 13:12

thaJeztah reviewed Aug 21, 2018

View reviewed changes

thaJeztah requested changes Aug 21, 2018

View reviewed changes

selansen force-pushed the default_addr_pool branch from a8002b4 to 17fda92 Compare August 21, 2018 17:23

thaJeztah approved these changes Aug 21, 2018

View reviewed changes

selansen force-pushed the default_addr_pool branch 4 times, most recently from 897cf10 to 3c05cf1 Compare August 21, 2018 18:33

Global Default Address Pool feature support

587a94c

This feature brings new attribute/option for swarm init command. default-addr-pool will take string input which can be in below format. "CIDR,CIDR,CIDR...:SUBNET-SIZE". Signed-off-by: selansen <elango.siva@docker.com>

selansen force-pushed the default_addr_pool branch from 3c05cf1 to 587a94c Compare August 21, 2018 18:34

crosbymichael merged commit e97d004 into docker:master Aug 21, 2018

GordonTheTurtle added this to the 18.09.0 milestone Aug 21, 2018

selansen deleted the default_addr_pool branch August 21, 2018 18:59

thaJeztah mentioned this pull request Aug 31, 2018

un-fork github.com/spf13/pflag #1330

Open

	sort.Strings(managers)
	fmt.Fprintln(dockerCli.Out(), " Manager Addresses:")
	for _, entry := range managers {
	fmt.Fprintf(dockerCli.Out(), " %s\n", entry)
	}

Global Default Address Pool feature support #1233

Global Default Address Pool feature support #1233

Conversation

selansen commented Jul 25, 2018

cpuguy83 commented Jul 25, 2018

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

euanh Aug 15, 2018 • edited Loading

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

selansen commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selansen Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

albers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 28, 2018 • edited Loading

Codecov Report

GordonTheTurtle commented Jul 29, 2018

thaJeztah Aug 21, 2018 • edited Loading

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

thaJeztah commented Aug 21, 2018

So, what options do we have?

1. (rejected) CSV notation; repeat flag for each pool.

2. CSV notation; allow multiple pools to be in a single flag;

3. CSV: one flag per pool, separate flags for CIDR and Size ("mask"? "mask-length"?)

4. No CSV; one flag per pool, separate flags for CIDR and Size ("mask"? "mask-length"?)

vdemeester commented Aug 21, 2018

selansen commented Aug 21, 2018

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

thaJeztah commented Aug 21, 2018

selansen commented Aug 21, 2018

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

selansen commented Aug 21, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selansen Aug 21, 2018 • edited Loading

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

crosbymichael commented Aug 21, 2018

selansen commented Aug 21, 2018

ushuz commented Nov 12, 2018

euanh Aug 15, 2018 •

edited

Loading

selansen Aug 12, 2018 •

edited

Loading

codecov-io commented Jul 28, 2018 •

edited

Loading

thaJeztah Aug 21, 2018 •

edited

Loading

selansen Aug 21, 2018 •

edited

Loading