Added support for swarm service isolation mode #34424

simonferquel · 2017-08-07T14:33:06Z

- What I did
Added isolation field on swarm service creation / updates to enable setting hyperv/process isolation per service on Windows
- How I did it
Updated swarmkit (PR with isolation in containerspec), added the field in swarm.ContainerSpec and in convert logic, use it in the executor
- How to verify it
Missing a test for now (incoming)
- Description for the changelog
Isolation mode (default, process, hyperv) can be set on swarm services to bypass host node defaults

~~Depends on #34745, otherwise breaks CLI~~

SwarmKit changes:

moby/swarmkit@872861d...28f91d8

From that list; included in this bump are included:

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
Increase gRPC request timeout to 20 seconds for sending snapshots Increase gRPC request timeout to 20 seconds for sending snapshots swarmkit#2391

friism · 2017-08-07T14:49:12Z

Fixes #31616

thaJeztah · 2017-08-07T21:24:34Z

@simonferquel looks like you vendored using an older version of vndr; can you update to the current version, and run vndr again?

14:45:45 The result of vndr differs
14:45:45 
14:45:45 ?? vendor/github.com/docker/distribution/vendor.conf
14:45:45 ?? vendor/github.com/docker/libnetwork/vendor.conf
14:45:45 ?? vendor/github.com/docker/swarmkit/vendor.conf
14:45:45 ?? vendor/github.com/moby/buildkit/vendor.conf
14:45:45 ?? vendor/github.com/opencontainers/runc/vendor.conf
14:45:45 
14:45:45 Please vendor your package with github.com/LK4D4/vndr.
14:45:45

Also, this needs an update of the swagger.yml, and the API version history; https://github.com/moby/moby/blob/master/docs/api/version-history.md

simonferquel · 2017-08-08T12:51:41Z

Swarm test suite is not executed on Windows. Can't write a usefull test on it. I'll just make a test putting an explicit "default" isolation mode, and will inspect service and container to check if it is not empty.

thaJeztah · 2017-08-08T13:17:05Z

api/swagger.yaml

@@ -2260,7 +2260,9 @@ definitions:
                    ConfigName is the name of the config that this references, but this is just provided for
                    lookup/display purposes. The config in the reference will be identified by its ID.
                  type: "string"
-
+          Isolation:


Shouldn't this go into TaskTemplate.ContainerSpec?

It is the case :)

oh boy, you're right; It looked as if it was at the wrong indentation level, but you're right. Sorry, my mistake ha!

thaJeztah

left some notes

thaJeztah · 2017-08-08T13:20:19Z

api/swagger.yaml

-
+          Isolation:
+            description: "Isolation mode of the containers running the service (default, process, hyperv..."
+            type: "string"


Perhaps add an enum - what is the behaviour if "" is passed? Is that the same as default?

enum: - "default" - "process" - "hyperv"

"" or null have the same behavior as default (except service inspect will report with an explicit "default" value for isolation).
I am not completely certain about making it an enum btw, as underlying executor (hcs for windows) might introduce new isolation mode. I'll check on container/create messages and harmonize that.

thaJeztah · 2017-08-08T13:24:14Z

api/swagger.yaml

@@ -2260,7 +2260,9 @@ definitions:
                    ConfigName is the name of the config that this references, but this is just provided for
                    lookup/display purposes. The config in the reference will be identified by its ID.
                  type: "string"
-
+          Isolation:
+            description: "Isolation mode of the containers running the service (default, process, hyperv..."


Perhaps the same terminology as is used on docker run, and add a note that it's only used for Windows containers;

description: | Container isolation technology to use for the container. > **Note**: Container isolation is only used for Windows containers. > This field is ignored for Linux containers.

(the   is a bit hacky, but I haven't found another way to add spacing yet 😄)

simonferquel · 2017-08-08T13:47:10Z

@thaJeztah fixed the doc (used the same exact description and enum as on HostConfig)
We'll need to wait for moby/swarmkit#2342 to be merged, update once again the vendoring before merging this one

thaJeztah · 2017-08-08T14:19:25Z

@simonferquel you forgot this one; 967fe0c#r131908016 😇

thaJeztah · 2017-08-17T14:55:25Z

@simonferquel this needs a rebase

coolljt0725 · 2017-08-22T06:04:15Z

ping @simonferquel needs refactor to fix the conflict

simonferquel · 2017-10-12T09:03:22Z

Since the PR Swarmkit side has been merged, I rebased and refreshed this PR.
However, Isolation field on swarm side has moved from a free form string to an enum, with an explicit "Default" value, so I can't use the fact that I could have both and "default" valid strings to make a Linux integration kit.

What I want to do then, is to introduce more unit testing, that would hopefully cover all aspects of the PR

simonferquel · 2017-10-12T10:07:52Z

Done (made a bit of cleanup for reusing Isolation type instead of a free string, and added tests to make sure conversions and executors take the change into account)

simonferquel · 2017-10-20T13:45:03Z

I just rebased it to see if the plugin tests still fail. Overwise I think the needs/vendoring flag can be removed (the pr now references swarmkit master)

simonferquel · 2017-10-24T08:45:08Z

Hmm swarmkit vendoring update seem to have broken DockerSwarmSuite.TestSwarmVolumePlugin somehow. Not sure why, but I could use some help here...

simonferquel · 2017-10-24T10:30:15Z

Ok, found the reason of the test regression: error details are reported in a separate task status field now. Updated the test, amended my commit, waiting for test results.

simonferquel · 2017-10-24T14:31:35Z

@thaJeztah could you update your review ? Want to make sure I did update the right swagger stuff.

thaJeztah

LGTM, but needs a minor rebase

thaJeztah · 2017-10-30T13:42:20Z

ping @cpuguy83 @dnephin PTAL

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

vieux · 2017-11-01T18:13:26Z

LGTM ping @cpuguy83 @dnephin

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Aug 7, 2017

thaJeztah added status/2-code-review area/swarm impact/api impact/changelog impact/documentation platform/windows and removed status/0-triage labels Aug 7, 2017

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from a8cac79 to 94613bf Compare August 8, 2017 11:29

thaJeztah reviewed Aug 8, 2017

View reviewed changes

thaJeztah requested changes Aug 8, 2017

View reviewed changes

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 01ca146 to 967fe0c Compare August 8, 2017 13:44

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from f56ac1c to 016b4c5 Compare August 10, 2017 09:30

thaJeztah added the status/needs-vendoring label Aug 15, 2017

GordonTheTurtle assigned coolljt0725 Aug 22, 2017

simonferquel requested review from dnephin and vdemeester as code owners August 28, 2017 12:14

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 0b709b3 to 3787b2e Compare August 29, 2017 14:37

simonferquel force-pushed the swarm-service-isolation branch from 3787b2e to 548ff8a Compare September 6, 2017 08:59

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 29c91b0 to 688cf6e Compare October 12, 2017 08:58

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 63372c4 to ad51f45 Compare October 12, 2017 10:05

simonferquel force-pushed the swarm-service-isolation branch from ad51f45 to efceb52 Compare October 20, 2017 13:42

dnephin removed the status/needs-vendoring label Oct 20, 2017

simonferquel force-pushed the swarm-service-isolation branch from efceb52 to b3ace3b Compare October 24, 2017 10:28

simonferquel force-pushed the swarm-service-isolation branch from b3ace3b to ea63d05 Compare October 24, 2017 11:35

thaJeztah approved these changes Oct 30, 2017

View reviewed changes

thaJeztah mentioned this pull request Oct 30, 2017

Revendored Swarmkit #35326

Merged

Added support for swarm service isolation mode

f28cb42

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel force-pushed the swarm-service-isolation branch from ea63d05 to f28cb42 Compare October 30, 2017 15:16

thaJeztah added rebuild/powerpc labels Oct 30, 2017

GordonTheTurtle removed rebuild/powerpc labels Oct 30, 2017

cpuguy83 approved these changes Nov 1, 2017

View reviewed changes

cpuguy83 merged commit d91c5f4 into moby:master Nov 1, 2017

This was referenced Nov 1, 2017

Move Isolation API changes to the correct version #35369

Merged

Add isolation option to docker service #31616

Closed

simonferquel mentioned this pull request Nov 14, 2017

Added validation of isolation settings on daemon.verifyContainerSettings #35495

Merged

mdlinville mentioned this pull request Dec 18, 2017

Review swarm service isolation mode for docs impact docker/docs#5557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for swarm service isolation mode #34424

Added support for swarm service isolation mode #34424

simonferquel commented Aug 7, 2017 •

edited by thaJeztah

Loading

friism commented Aug 7, 2017

thaJeztah commented Aug 7, 2017

simonferquel commented Aug 8, 2017

thaJeztah Aug 8, 2017 •

edited

Loading

simonferquel Aug 8, 2017

thaJeztah Aug 8, 2017

thaJeztah left a comment

thaJeztah Aug 8, 2017

simonferquel Aug 8, 2017

thaJeztah Aug 8, 2017

simonferquel commented Aug 8, 2017

thaJeztah commented Aug 8, 2017

thaJeztah commented Aug 17, 2017

coolljt0725 commented Aug 22, 2017 •

edited

Loading

simonferquel commented Oct 12, 2017

simonferquel commented Oct 12, 2017

simonferquel commented Oct 20, 2017

simonferquel commented Oct 24, 2017

simonferquel commented Oct 24, 2017

simonferquel commented Oct 24, 2017

thaJeztah left a comment

thaJeztah commented Oct 30, 2017

vieux commented Nov 1, 2017

cpuguy83 left a comment

Added support for swarm service isolation mode #34424

Added support for swarm service isolation mode #34424

Conversation

simonferquel commented Aug 7, 2017 • edited by thaJeztah Loading

SwarmKit changes:

friism commented Aug 7, 2017

thaJeztah commented Aug 7, 2017

simonferquel commented Aug 8, 2017

thaJeztah Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

simonferquel Aug 8, 2017

Choose a reason for hiding this comment

thaJeztah Aug 8, 2017

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Aug 8, 2017

Choose a reason for hiding this comment

simonferquel Aug 8, 2017

Choose a reason for hiding this comment

thaJeztah Aug 8, 2017

Choose a reason for hiding this comment

simonferquel commented Aug 8, 2017

thaJeztah commented Aug 8, 2017

thaJeztah commented Aug 17, 2017

coolljt0725 commented Aug 22, 2017 • edited Loading

simonferquel commented Oct 12, 2017

simonferquel commented Oct 12, 2017

simonferquel commented Oct 20, 2017

simonferquel commented Oct 24, 2017

simonferquel commented Oct 24, 2017

simonferquel commented Oct 24, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 30, 2017

vieux commented Nov 1, 2017

cpuguy83 left a comment

Choose a reason for hiding this comment

simonferquel commented Aug 7, 2017 •

edited by thaJeztah

Loading

thaJeztah Aug 8, 2017 •

edited

Loading

coolljt0725 commented Aug 22, 2017 •

edited

Loading