[Windows] Support for isolation mode on container spec #2342

simonferquel · 2017-08-07T13:44:55Z

This adds an "isolation" field to the container spec, so that isolation mode (process, hyperv) can be decided per service instead of globally for a node.

codecov · 2017-08-08T09:12:20Z

Codecov Report

Merging #2342 into master will decrease coverage by 0.01%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #2342      +/-   ##
==========================================
- Coverage   60.62%   60.61%   -0.02%     
==========================================
  Files         128      128              
  Lines       26309    26319      +10     
==========================================
+ Hits        15951    15953       +2     
- Misses       8954     8965      +11     
+ Partials     1404     1401       -3

aluzzardi · 2017-08-15T23:13:14Z

Hey @simonferquel - why do we need to change the import path for logrus?

And if we do need to change it - why update all the dependencies in vendor.conf rather than just logrus?

simonferquel · 2017-08-16T07:22:31Z

The problem is that moby/moby has already updated logrus import path and it is forbidden to import 2 different packages that only have casing differences in their import path... so we need to update it in swarmkit (to be able to revendor it in moby), and we need to depend on components that are their logrus import path updated... same thing apply to cli, notary, ucp, swarm classic...

Sent from my Samsung SM-G935F using FastHub

simonferquel · 2017-08-28T11:43:19Z

This has just been rebased. cc @aluzzardi

thaJeztah · 2017-09-20T13:23:36Z

ping @nishanttotla @dperny @stevvooe @johnstep PTAL

stevvooe · 2017-09-22T01:33:48Z

api/specs.proto

@@ -298,6 +298,11 @@ message ContainerSpec {
 	oneof init {
 		bool init_value = 23;
 	}
+
+	// Isolation defines the isolation level for engines supporting multiple isolation modes


Looks like we need an enum here, according to swarmkit's style.

I considered that, but after a bit of thinking, it felt that valid values for isolationMode might ultimately depend on the underlying container runtime. For now only dockerd on Windows support different values, but it might change, and we might even require a non fixed set of values.
For exemple, I foresee that with lcow, we might have at some point values like: hyperv{<something identifying a specific linuxkit/Ubuntu/whatever version>}

Additionaly, isolationMode is a config setting specific to the container runtime, and has no effect on swarm scheduling. So I don't think we should put validation logic at the swarmkit level here

@simonferquel The validation consideration here is a valid point and perhaps we could have made this more flexible, but we found that much of the input on the docker side was weakly validated and lead to challenging errors to diagnose in an orchestration system. By ensuring that input is correct, we can eliminate these errors, at the cost of being very literal.

stevvooe · 2017-09-22T01:52:17Z

api/api.pb.txt

@@ -5413,6 +5434,14 @@ file {
      type_name: ".docker.swarmkit.v1.NodeRole"
      json_name: "role"
    }
+    field {


Looks like this was missed in another PR.

simonferquel · 2017-09-22T10:05:02Z

well it is specific to the runtime adapter, not the runtime itself. But one thing still remains: for example containerd on windows allows to specify a different hypervUtilityVMPath than default. The containerd adapter could eventually parse the isolation mode field to override the utility VMPath

simonferquel · 2017-09-22T13:44:09Z

I added a commit with an enum. @stevvooe, let me know if you prefer the code with or without it

stevvooe · 2017-09-22T17:56:19Z

api/specs.proto

+		option (gogoproto.goproto_enum_prefix) = false;
+
+		// Default uses whatever default value from the container runtime
+		Default = 0 [(gogoproto.enumvalue_customname) = "ContainerIsolationDefault"];


Please use all caps for enums. See the TaskState for an example of enum style. Remember that these are scoped to the package from protobufs perspective, so they should be prefixed:

ISOLATION_DEFAULT
ISOLATION_PROCESS

The Go names you have in place are perfect.

stevvooe · 2017-09-22T17:58:26Z

@simonferquel Thanks!

There is just one more naming tweak, which needs to be done because of the way protobuf enums are namespaced.

simonferquel · 2017-09-27T15:37:36Z

Fixed and rebased. Not sure why CI failed though (seem unrelated)
cc @stevvooe

stevvooe · 2017-09-27T17:03:00Z

api/specs.proto

+		option (gogoproto.goproto_enum_prefix) = false;
+
+		// DEFAULT uses whatever default value from the container runtime
+		DEFAULT = 0 [(gogoproto.enumvalue_customname) = "ContainerIsolationDefault"];


These need to be ISOLATION_DEFAULT, ISOLATION_PROCESS and ISOLATION_HYPERV.

Sorry about that, just amended my last commit

stevvooe · 2017-10-02T21:57:00Z

LGTM

anshulpundir

Looks fine to me but @nishanttotla should also take a look.

thaJeztah · 2017-10-05T05:58:53Z

@simonferquel can you rebase (again)?

For executors supporting multiple isolation levels (ie: Docker API on Windows), added an "isolation" field in the container spec, so that we can schedule services with different isolation modes than the system defaults Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

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

simonferquel · 2017-10-05T10:16:26Z

@thaJeztah just rebased / push

I'll have to work a bit on the moby / cli PRs once it is merged, to adapt to the move to enums

nishanttotla

LGTM

nishanttotla · 2017-10-05T21:14:56Z

@simonferquel is there more you're planning to add to this PR, or can we merge it?

simonferquel · 2017-10-06T06:51:20Z

@nishanttotla i am good with it.

simonferquel force-pushed the swarm-service-isolation branch from f60599b to 9fc4043 Compare August 7, 2017 15:32

simonferquel force-pushed the swarm-service-isolation branch from eb8512f to 0d86037 Compare August 8, 2017 09:19

simonferquel mentioned this pull request Aug 8, 2017

Added support for swarm service isolation mode moby/moby#34424

Merged

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 28d2d26 to 95380b7 Compare August 28, 2017 10:04

simonferquel force-pushed the swarm-service-isolation branch 3 times, most recently from 0d86037 to 07d1018 Compare September 18, 2017 09:51

simonferquel mentioned this pull request Sep 19, 2017

--isolation for setting swarm service isolation mode docker/cli#426

Merged

stevvooe reviewed Sep 22, 2017

View reviewed changes

simonferquel force-pushed the swarm-service-isolation branch from 07d1018 to 554fefa Compare September 22, 2017 09:46

simonferquel force-pushed the swarm-service-isolation branch from 5d7e1e9 to 32e891d Compare September 22, 2017 13:43

stevvooe reviewed Sep 22, 2017

View reviewed changes

thaJeztah mentioned this pull request Sep 27, 2017

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

Open

simonferquel force-pushed the swarm-service-isolation branch 2 times, most recently from 2193829 to 6360312 Compare September 27, 2017 15:27

stevvooe reviewed Sep 27, 2017

View reviewed changes

simonferquel force-pushed the swarm-service-isolation branch from 6360312 to 6b6688f Compare October 2, 2017 08:30

anshulpundir reviewed Oct 3, 2017

View reviewed changes

simonferquel added 2 commits October 5, 2017 12:12

Use an enum for isolation

7d10d60

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

simonferquel force-pushed the swarm-service-isolation branch from 6b6688f to 7d10d60 Compare October 5, 2017 10:13

nishanttotla approved these changes Oct 5, 2017

View reviewed changes

nishanttotla merged commit a05f661 into moby:master Oct 6, 2017

thaJeztah mentioned this pull request Oct 30, 2017

Revendored Swarmkit moby/moby#35326

Merged

thaJeztah mentioned this pull request Nov 6, 2017

Add isolation option to docker service moby/moby#31616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Support for isolation mode on container spec #2342

[Windows] Support for isolation mode on container spec #2342

simonferquel commented Aug 7, 2017

codecov bot commented Aug 8, 2017 •

edited

Loading

aluzzardi commented Aug 15, 2017

simonferquel commented Aug 16, 2017

simonferquel commented Aug 28, 2017

thaJeztah commented Sep 20, 2017

stevvooe Sep 22, 2017

simonferquel Sep 22, 2017

stevvooe Sep 22, 2017

stevvooe Sep 22, 2017

simonferquel commented Sep 22, 2017

simonferquel commented Sep 22, 2017

stevvooe Sep 22, 2017

stevvooe commented Sep 22, 2017 •

edited

Loading

simonferquel commented Sep 27, 2017

stevvooe Sep 27, 2017

simonferquel Oct 2, 2017

stevvooe commented Oct 2, 2017

anshulpundir left a comment

thaJeztah commented Oct 5, 2017

simonferquel commented Oct 5, 2017

nishanttotla left a comment

nishanttotla commented Oct 5, 2017

simonferquel commented Oct 6, 2017

[Windows] Support for isolation mode on container spec #2342

[Windows] Support for isolation mode on container spec #2342

Conversation

simonferquel commented Aug 7, 2017

codecov bot commented Aug 8, 2017 • edited Loading

Codecov Report

aluzzardi commented Aug 15, 2017

simonferquel commented Aug 16, 2017

simonferquel commented Aug 28, 2017

thaJeztah commented Sep 20, 2017

stevvooe Sep 22, 2017

Choose a reason for hiding this comment

simonferquel Sep 22, 2017

Choose a reason for hiding this comment

stevvooe Sep 22, 2017

Choose a reason for hiding this comment

stevvooe Sep 22, 2017

Choose a reason for hiding this comment

simonferquel commented Sep 22, 2017

simonferquel commented Sep 22, 2017

stevvooe Sep 22, 2017

Choose a reason for hiding this comment

stevvooe commented Sep 22, 2017 • edited Loading

simonferquel commented Sep 27, 2017

stevvooe Sep 27, 2017

Choose a reason for hiding this comment

simonferquel Oct 2, 2017

Choose a reason for hiding this comment

stevvooe commented Oct 2, 2017

anshulpundir left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 5, 2017

simonferquel commented Oct 5, 2017

nishanttotla left a comment

Choose a reason for hiding this comment

nishanttotla commented Oct 5, 2017

simonferquel commented Oct 6, 2017

codecov bot commented Aug 8, 2017 •

edited

Loading

stevvooe commented Sep 22, 2017 •

edited

Loading