Revendored Swarmkit #35326

RenaudWasTaken · 2017-10-28T15:10:48Z

includes moby/swarmkit#2419, which:

fixes #34825
fixes #34996
fixes #35046
fixes #35395

Hello!

- What I did

Revendored swarmkit to the latest commit to include the recent GenericResource change
Updated the Generic resource CLI to match it's new CLI format
Fixed the DockerSwarmSuite.TestSwarmVolumePlugin as the error message is now surfaced in a new field

- Description for the changelog
Revendored Swarmkit

@anshulpundir can you confirm that the revendoring does not require me to change moby/moby ?
Ping @thaJeztah

- Bumps

moby/swarmkit@28f91d8...de950a7

Delete node attachments when node is removed Delete node attachments when node is removed swarmkit#2409
add pid limits add pid limits swarmkit#2415 (related to Security Proposal: add "--pids-limit" to "docker service create/update" option list #28618)
Provide custom gRPC dialer to override default proxy dialer Provide custom gRPC dialer to override default proxy dialer swarmkit#2419
Generic resource cli Generic resource cli swarmkit#2347

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

yongtang · 2017-10-29T17:37:15Z

Looks like the following failure happens on all platform:

14:55:19 FAIL: docker_cli_swarm_unix_test.go:15: DockerSwarmSuite.TestSwarmVolumePlugin
14:55:19 
14:55:19 [d539f05da0b31] waiting for daemon to start
14:55:19 [d539f05da0b31] daemon started
14:55:19 
14:55:19 docker_cli_swarm_unix_test.go:22:
14:55:19     // Make sure task stays pending before plugin is available
14:55:19     waitAndAssert(c, defaultReconciliationTimeout, d.CheckServiceTasksInState("top", swarm.TaskStatePending, "missing plugin on 1 node"), checker.Equals, 1)
14:55:19 docker_utils_test.go:465:
14:55:19     c.Assert(v, checker, args...)
14:55:19 ... obtained int = 0
14:55:19 ... expected int = 1
14:55:19 
14:55:19 [d539f05da0b31] exiting daemon
14:55:20

GordonTheTurtle · 2017-10-29T21:11:35Z

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 "swarmkit-revendored" git@github.com:RenaudWasTaken/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354008608
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

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

RenaudWasTaken · 2017-10-29T21:12:56Z

After some work I found that the root cause of this bug was introduced by moby/swarmkit#2287

The error message is now surfaced in the Error field. I've added a commit to fix this,

thaJeztah · 2017-10-30T13:50:18Z

Thanks! There's another PR as well that bumps swarm kit (#34424), so one of both will have to rebase probably. I created a quick overview of changes included in this PR (currently);

moby/swarmkit@872861d...de950a7

From that list; included in this bump are:

api: Use TaskStatus Err field for non-terminal errors api: Use TaskStatus Err field for non-terminal errors swarmkit#2287
Support the init flag for container spec Support the init flag for container spec swarmkit#2386
[Windows] Support for isolation mode on container spec [Windows] Support for isolation mode on container spec swarmkit#2342 (related to Added support for swarm service isolation mode #34424)
Increase gRPC request timeout to 20 seconds for sending snapshots Increase gRPC request timeout to 20 seconds for sending snapshots swarmkit#2391
Delete node attachments when node is removed Delete node attachments when node is removed swarmkit#2409
add pid limits add pid limits swarmkit#2415 (related to Security Proposal: add "--pids-limit" to "docker service create/update" option list #28618)
Provide custom gRPC dialer to override default proxy dialer Provide custom gRPC dialer to override default proxy dialer swarmkit#2419
Generic resource cli Generic resource cli swarmkit#2347

thaJeztah · 2017-10-30T13:53:26Z

daemon/config/config.go

@@ -171,7 +171,8 @@ type CommonConfig struct {
 	Experimental bool `json:"experimental"` // Experimental indicates whether experimental features should be exposed or not

 	// Exposed node Generic Resources
-	NodeGenericResources string `json:"node-generic-resources,omitempty"`
+	// e.g: ["orange=red", "orange=green", "orange=blue", "apple=3"]
+	NodeGenericResources []string `json:"node-generic-resources,omitempty"`


Do we have a test to test loading this configuration from a daemon.json configuration file?

Just added one :)

anshulpundir

Should we also include moby/swarmkit@312be59? @thaJeztah

RenaudWasTaken · 2017-10-31T18:11:52Z

Thanks @thaJeztah for the list!
Including it and moby/swarmkit#2419 in the first post.

Can you point me to the mentioned PR revendoring swarmkit ?
EDIT: Found it :)

thaJeztah · 2017-11-03T14:23:00Z

@anshulpundir ah! yes, missed that one in my overview; I updated the list, and updated the list at the top as well

@RenaudWasTaken #34424 was merged, so this PR now needs a rebase 😅; probably just amending the first commit ("Revendored swarmkit "), and running vndr again should do

Signed-off-by: Renaud Gaubert <renaud.gaubert@gmail.com>

RenaudWasTaken · 2017-11-03T15:32:44Z

@thaJeztah Looks like it also fixed the integration test: https://github.com/moby/moby/pull/34424/files#diff-eb3f7d0d8dfa41106bb74d7e5dfe43ecR203

I've removed that commit.
Thanks for updating my first post!

RenaudWasTaken · 2017-11-06T18:38:54Z

@thaJeztah is this PR good to go ?
Hoping to revendor the CLI after this so we can get support of NVIDIA GPUs in swarm!

thaJeztah

Thanks! Just some minor nits, but looks good otherwise

thaJeztah · 2017-11-06T18:58:14Z

cmd/dockerd/config.go

@@ -65,7 +65,8 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) {

 	flags.StringVar(&conf.MetricsAddress, "metrics-addr", "", "Set default address and port to serve the metrics api on")

-	flags.StringVar(&conf.NodeGenericResources, "node-generic-resources", "", "user defined resources (e.g. fpga=2;gpu={UUID1,UUID2,UUID3})")
+	flags.Var(opts.NewListOptsRef(&conf.NodeGenericResources, opts.ValidateSingleGenericResource), "node-generic-resource", "Advertise user defined resource")


nit: can you change user defined to user-defined here?

thaJeztah · 2017-11-06T19:06:42Z

opts/opts.go

+// i.e 'GPU=UID1' is valid however 'GPU:UID1' or 'UID1' isn't
+func ValidateSingleGenericResource(val string) (string, error) {
+	if strings.Count(val, "=") < 1 {
+		return "", fmt.Errorf("bad attribute format `%s` expected argument of the for `name=value`", val)


This message looks incomplete ("argument of the for .. ")?

Looking at other messages, I think we use invalid ... for other similar messages (e.g.

moby/daemon/cluster/convert/container.go

Line 296 in d91c5f4

return nil, fmt.Errorf("invalid MountType: %q", m.Type)

)

Perhaps something like:

invalid node-generic-resource format: expected 'name=value':

@MistyHacks perhaps you have a better suggestion?

I think it's ok, if you also include a got portion to the error message. What is a "node-generic-resource"?

@MistyHacks the full design document is here.
The basic idea is that it's a way for a node to advertise user-defined resources

Signed-off-by: Renaud Gaubert <renaud.gaubert@gmail.com>

thaJeztah

LGTM, thanks!

vdemeester

LGTM 🐮

thaJeztah · 2017-11-07T14:59:05Z

Thanks @RenaudWasTaken - I think this needs an update to the documentation (as the format for the dockerd options changed); unfortunately those are not in this repository, but could you open a pull request for the dockerd command-line reference, and the man-page in the https://github.com/docker/cli repository?

RenaudWasTaken · 2017-11-08T13:40:54Z

@thaJeztah will be doing this as part of docker/cli/pull/429

thaJeztah · 2017-11-08T14:28:47Z

Perfect, thanks @RenaudWasTaken

RenaudWasTaken · 2017-11-08T14:49:50Z

Looks like I'll have to revendor ~100 commits of docker/docker though :(

thaJeztah · 2017-11-08T15:00:52Z

If you prefer, I can do a separate bump for all changes before yours

RenaudWasTaken · 2017-11-08T15:14:40Z

@thaJeztah It would make things a lot easier :) Thanks!

On the other hand it does look like I'll still have to revendor docker because of the way parsing is done.

Basically parsing in the daemon is through swarmkit/api/genericresource.Parse and then docker/daemon/cluster/convert.GenericResourcesFromGRPC

Meaning that if I want to do the same in the CLI I'll need to add a lot of new dependencies. Because that'll mean vendoring docker/daemon/cluster/convert which has a lot of dependencies.

Should I copy swarmkit/api/genericresource.Parse in docker/cli or add the 10+ dependencies to the vendor.conf ?

dnephin · 2017-11-08T15:26:50Z

The dependencies of swarmkit/api/genericresource/ are fine, it's just swarmkit/api which is already a dependency of docker/cli.

I would duplicate convert.GenericResourcesFromGRPC(). There's not much logic there, and daemon/cluster/convert is not an appropriate dependency for docker/cli.

GordonTheTurtle added the status/0-triage label Oct 28, 2017

fntlnz added the rebuild/* label Oct 29, 2017

GordonTheTurtle removed the rebuild/* label Oct 29, 2017

yongtang added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Oct 29, 2017

RenaudWasTaken force-pushed the swarmkit-revendored branch from 4b3625a to 8f7ac41 Compare October 29, 2017 21:11

RenaudWasTaken requested review from dnephin and vdemeester as code owners October 29, 2017 21:11

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 29, 2017

RenaudWasTaken force-pushed the swarmkit-revendored branch from 8f7ac41 to f091918 Compare October 29, 2017 21:13

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 29, 2017

RenaudWasTaken force-pushed the swarmkit-revendored branch from f091918 to 094102c Compare October 29, 2017 21:14

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 29, 2017

yongtang removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Oct 30, 2017

thaJeztah reviewed Oct 30, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 30, 2017

RenaudWasTaken force-pushed the swarmkit-revendored branch from 990a354 to 6828193 Compare October 30, 2017 22:24

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 30, 2017

anshulpundir reviewed Oct 30, 2017

View reviewed changes

thaJeztah mentioned this pull request Nov 3, 2017

Swarm worker cannot connect to master if proxy is configured #35395

Closed

Revendored swarmkit

b3960fc

Signed-off-by: Renaud Gaubert <renaud.gaubert@gmail.com>

RenaudWasTaken force-pushed the swarmkit-revendored branch from 6828193 to 216abca Compare November 3, 2017 15:04

thaJeztah mentioned this pull request Nov 6, 2017

Need documentation on new docker-compose feature "init" docker/docs#3149

Closed

thaJeztah requested changes Nov 6, 2017

View reviewed changes

RenaudWasTaken added 2 commits November 6, 2017 21:47

Updated GenericResource CLI

ebe1431

Signed-off-by: Renaud Gaubert <renaud.gaubert@gmail.com>

Added NodeGenericResource config tests

734346a

Signed-off-by: Renaud Gaubert <renaud.gaubert@gmail.com>

RenaudWasTaken force-pushed the swarmkit-revendored branch from 216abca to 734346a Compare November 6, 2017 20:47

thaJeztah approved these changes Nov 6, 2017

View reviewed changes

vdemeester approved these changes Nov 7, 2017

View reviewed changes

yongtang merged commit eec662b into moby:master Nov 7, 2017

thaJeztah added the impact/changelog label Nov 7, 2017

thaJeztah added the docs/revisit label Nov 7, 2017

This was referenced Nov 8, 2017

Integrated Generic resource in service create docker/cli#429

Merged

Add Support for Swarmkit Generic Resources #33439

Open

thaJeztah mentioned this pull request Nov 10, 2017

support of GPU schedule #35457

Closed

RenaudWasTaken deleted the swarmkit-revendored branch March 26, 2019 22:52

This was referenced Sep 9, 2019

[epic] add more options to service create / service update #25303

Open

Add API support for PidsLimit on services #39882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revendored Swarmkit #35326

Revendored Swarmkit #35326

RenaudWasTaken commented Oct 28, 2017 •

edited by thaJeztah

Loading

yongtang commented Oct 29, 2017

GordonTheTurtle commented Oct 29, 2017

RenaudWasTaken commented Oct 29, 2017

thaJeztah commented Oct 30, 2017 •

edited

Loading

thaJeztah Oct 30, 2017

RenaudWasTaken Oct 30, 2017

anshulpundir left a comment

RenaudWasTaken commented Oct 31, 2017 •

edited

Loading

thaJeztah commented Nov 3, 2017

RenaudWasTaken commented Nov 3, 2017 •

edited

Loading

RenaudWasTaken commented Nov 6, 2017 •

edited

Loading

thaJeztah left a comment

thaJeztah Nov 6, 2017

thaJeztah Nov 6, 2017

mdlinville Nov 6, 2017

RenaudWasTaken Nov 6, 2017

thaJeztah left a comment

vdemeester left a comment

thaJeztah commented Nov 7, 2017

RenaudWasTaken commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

RenaudWasTaken commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

RenaudWasTaken commented Nov 8, 2017

dnephin commented Nov 8, 2017

Revendored Swarmkit #35326

Revendored Swarmkit #35326

Conversation

RenaudWasTaken commented Oct 28, 2017 • edited by thaJeztah Loading

yongtang commented Oct 29, 2017

GordonTheTurtle commented Oct 29, 2017

RenaudWasTaken commented Oct 29, 2017

thaJeztah commented Oct 30, 2017 • edited Loading

thaJeztah Oct 30, 2017

Choose a reason for hiding this comment

RenaudWasTaken Oct 30, 2017

Choose a reason for hiding this comment

anshulpundir left a comment

Choose a reason for hiding this comment

RenaudWasTaken commented Oct 31, 2017 • edited Loading

thaJeztah commented Nov 3, 2017

RenaudWasTaken commented Nov 3, 2017 • edited Loading

RenaudWasTaken commented Nov 6, 2017 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Nov 6, 2017

Choose a reason for hiding this comment

thaJeztah Nov 6, 2017

Choose a reason for hiding this comment

mdlinville Nov 6, 2017

Choose a reason for hiding this comment

RenaudWasTaken Nov 6, 2017

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 7, 2017

RenaudWasTaken commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

RenaudWasTaken commented Nov 8, 2017

thaJeztah commented Nov 8, 2017

RenaudWasTaken commented Nov 8, 2017

dnephin commented Nov 8, 2017

RenaudWasTaken commented Oct 28, 2017 •

edited by thaJeztah

Loading

thaJeztah commented Oct 30, 2017 •

edited

Loading

RenaudWasTaken commented Oct 31, 2017 •

edited

Loading

RenaudWasTaken commented Nov 3, 2017 •

edited

Loading

RenaudWasTaken commented Nov 6, 2017 •

edited

Loading