Added support for swarm service isolation mode#34424
Conversation
|
Fixes #31616 |
|
@simonferquel looks like you vendored using an older version of 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 |
a8cac79 to
94613bf
Compare
|
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. |
api/swagger.yaml
Outdated
There was a problem hiding this comment.
Shouldn't this go into TaskTemplate.ContainerSpec?
There was a problem hiding this comment.
It is the case :)
There was a problem hiding this comment.
oh boy, you're right; It looked as if it was at the wrong indentation level, but you're right. Sorry, my mistake ha!
api/swagger.yaml
Outdated
There was a problem hiding this comment.
Perhaps add an enum - what is the behaviour if "" is passed? Is that the same as default?
enum:
- "default"
- "process"
- "hyperv"
There was a problem hiding this comment.
"" 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.
api/swagger.yaml
Outdated
There was a problem hiding this comment.
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 😄)
01ca146 to
967fe0c
Compare
|
@thaJeztah fixed the doc (used the same exact description and enum as on HostConfig) |
|
@simonferquel you forgot this one; 967fe0c#r131908016 😇 |
f56ac1c to
016b4c5
Compare
|
@simonferquel this needs a rebase |
|
ping @simonferquel needs refactor to fix the conflict |
0b709b3 to
3787b2e
Compare
3787b2e to
548ff8a
Compare
29c91b0 to
688cf6e
Compare
|
Since the PR Swarmkit side has been merged, I rebased and refreshed this PR. What I want to do then, is to introduce more unit testing, that would hopefully cover all aspects of the PR |
63372c4 to
ad51f45
Compare
|
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) |
ad51f45 to
efceb52
Compare
|
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) |
|
Hmm swarmkit vendoring update seem to have broken |
efceb52 to
b3ace3b
Compare
|
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. |
b3ace3b to
ea63d05
Compare
|
@thaJeztah could you update your review ? Want to make sure I did update the right swagger stuff. |
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, but needs a minor rebase
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
ea63d05 to
f28cb42
Compare
- 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 CLISwarmKit changes:
moby/swarmkit@872861d...28f91d8
From that list; included in this bump are included: