[Windows] Support for isolation mode on container spec#2342
[Windows] Support for isolation mode on container spec#2342nishanttotla merged 2 commits intomoby:masterfrom
Conversation
f60599b to
9fc4043
Compare
Codecov Report
@@ 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 |
eb8512f to
0d86037
Compare
|
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? |
|
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 |
28d2d26 to
95380b7
Compare
|
This has just been rebased. cc @aluzzardi |
0d86037 to
07d1018
Compare
|
ping @nishanttotla @dperny @stevvooe @johnstep PTAL |
api/specs.proto
Outdated
| bool init_value = 23; | ||
| } | ||
|
|
||
| // Isolation defines the isolation level for engines supporting multiple isolation modes |
There was a problem hiding this comment.
Looks like we need an enum here, according to swarmkit's style.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
| type_name: ".docker.swarmkit.v1.NodeRole" | ||
| json_name: "role" | ||
| } | ||
| field { |
There was a problem hiding this comment.
Looks like this was missed in another PR.
07d1018 to
554fefa
Compare
|
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 |
5d7e1e9 to
32e891d
Compare
|
I added a commit with an enum. @stevvooe, let me know if you prefer the code with or without it |
api/specs.proto
Outdated
| option (gogoproto.goproto_enum_prefix) = false; | ||
|
|
||
| // Default uses whatever default value from the container runtime | ||
| Default = 0 [(gogoproto.enumvalue_customname) = "ContainerIsolationDefault"]; |
There was a problem hiding this comment.
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.
|
@simonferquel Thanks! There is just one more naming tweak, which needs to be done because of the way protobuf enums are namespaced. |
2193829 to
6360312
Compare
|
Fixed and rebased. Not sure why CI failed though (seem unrelated) |
api/specs.proto
Outdated
| option (gogoproto.goproto_enum_prefix) = false; | ||
|
|
||
| // DEFAULT uses whatever default value from the container runtime | ||
| DEFAULT = 0 [(gogoproto.enumvalue_customname) = "ContainerIsolationDefault"]; |
There was a problem hiding this comment.
These need to be ISOLATION_DEFAULT, ISOLATION_PROCESS and ISOLATION_HYPERV.
There was a problem hiding this comment.
Sorry about that, just amended my last commit
6360312 to
6b6688f
Compare
|
LGTM |
anshulpundir
left a comment
There was a problem hiding this comment.
Looks fine to me but @nishanttotla should also take a look.
|
@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>
6b6688f to
7d10d60
Compare
|
@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 |
|
@simonferquel is there more you're planning to add to this PR, or can we merge it? |
|
@nishanttotla i am good with it. |
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.