Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for swarm service isolation mode #34424

Merged
merged 1 commit into from Nov 1, 2017

Conversation

@simonferquel
Copy link
Contributor

@simonferquel simonferquel commented Aug 7, 2017

- 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:

docker/swarmkit@872861d...28f91d8

From that list; included in this bump are included:

@friism
Copy link
Contributor

@friism friism commented Aug 7, 2017

Fixes #31616

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 7, 2017

@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 simonferquel force-pushed the simonferquel:swarm-service-isolation branch 2 times, most recently from a8cac79 to 94613bf Aug 8, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Aug 8, 2017

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.

@@ -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:

This comment has been minimized.

@thaJeztah

thaJeztah Aug 8, 2017
Member

Shouldn't this go into TaskTemplate.ContainerSpec?

This comment has been minimized.

@simonferquel

simonferquel Aug 8, 2017
Author Contributor

It is the case :)

This comment has been minimized.

@thaJeztah

thaJeztah Aug 8, 2017
Member

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

Copy link
Member

@thaJeztah thaJeztah left a comment

left some notes


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

This comment has been minimized.

@thaJeztah

thaJeztah Aug 8, 2017
Member

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

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

This comment has been minimized.

@simonferquel

simonferquel Aug 8, 2017
Author Contributor

"" 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.

@@ -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..."

This comment has been minimized.

@thaJeztah

thaJeztah Aug 8, 2017
Member

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.

  <p><br /></p>

  > **Note**: Container isolation is only used for Windows containers.
  > This field is ignored for Linux containers.

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

@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch 2 times, most recently from 01ca146 to 967fe0c Aug 8, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Aug 8, 2017

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

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 8, 2017

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

@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch 2 times, most recently from f56ac1c to 016b4c5 Aug 9, 2017
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 17, 2017

@simonferquel this needs a rebase

@coolljt0725
Copy link
Contributor

@coolljt0725 coolljt0725 commented Aug 22, 2017

ping @simonferquel needs refactor to fix the conflict

@simonferquel simonferquel requested review from dnephin and vdemeester as code owners Aug 28, 2017
@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch 2 times, most recently from 0b709b3 to 3787b2e Aug 28, 2017
@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch from 3787b2e to 548ff8a Sep 6, 2017
@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch from 29c91b0 to 688cf6e Oct 12, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 12, 2017

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 simonferquel force-pushed the simonferquel:swarm-service-isolation branch 2 times, most recently from 63372c4 to ad51f45 Oct 12, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 12, 2017

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 simonferquel force-pushed the simonferquel:swarm-service-isolation branch from ad51f45 to efceb52 Oct 20, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 20, 2017

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
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 24, 2017

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

@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch from efceb52 to b3ace3b Oct 24, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 24, 2017

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 simonferquel force-pushed the simonferquel:swarm-service-isolation branch from b3ace3b to ea63d05 Oct 24, 2017
@simonferquel
Copy link
Contributor Author

@simonferquel simonferquel commented Oct 24, 2017

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

Copy link
Member

@thaJeztah thaJeztah left a comment

LGTM, but needs a minor rebase

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 30, 2017

ping @cpuguy83 @dnephin PTAL

@thaJeztah thaJeztah mentioned this pull request Oct 30, 2017
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
@simonferquel simonferquel force-pushed the simonferquel:swarm-service-isolation branch from ea63d05 to f28cb42 Oct 30, 2017
@vieux
Copy link
Contributor

@vieux vieux commented Nov 1, 2017

LGTM ping @cpuguy83 @dnephin

Copy link
Contributor

@cpuguy83 cpuguy83 left a comment

LGTM

@cpuguy83 cpuguy83 merged commit d91c5f4 into moby:master Nov 1, 2017
7 checks passed
7 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37562 has succeeded
Details
janky Jenkins build Docker-PRs 46259 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6674 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3879 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17836 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6471 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.