--isolation for setting swarm service isolation mode #426

simonferquel · 2017-08-07T14:37:31Z

fixes #414

- What I did
Added --isolation to service create/update and to compose files, to bypass default isolation mode on the host.
- How I did it
Updated dependencies to moby/moby and swarmkit (also had to depend on the logrus mega PR), added the flag in the service update/create cmd, passing the value accordingly
Updated the compose service struct and conversion to ServiceSpec accordingly
- How to verify it
Tests incoming
- Description for the changelog

Service creation and update supports --isolation flag (default, process, hyperv) for bypassing default isolation mode on the host
Isolation can also be set in compose files using the "isolation" property

~~Depends on #424~~

codecov-io · 2017-08-08T13:33:44Z

Codecov Report

Merging #426 into master will increase coverage by 0.53%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   50.75%   51.28%   +0.53%     
==========================================
  Files         216      216              
  Lines       17730    17743      +13     
==========================================
+ Hits         8998     9099     +101     
+ Misses       8276     8175     -101     
- Partials      456      469      +13

vdemeester · 2017-08-22T13:21:49Z

@simonferquel can you link PRs in swarmkit and moby related to this feature ? 👼

simonferquel · 2017-09-19T09:57:36Z

PR on Swarmkit: moby/swarmkit#2342
PR on Moby: moby/moby#34424

thaJeztah · 2017-11-01T20:45:56Z

moby/moby#34424 was merged, so this PR can be worked on again; @simonferquel can you do a rebase/revendor?

simonferquel · 2017-11-02T14:49:48Z

Requires moby/moby#35382. in the mean time i'll add tests to improve coverage (mainly on update)

dnephin · 2017-11-02T16:32:58Z

cli/command/service/opts_test.go

@@ -4,6 +4,8 @@ import (
 	"testing"
 	"time"

+	"github.com/stretchr/testify/require"
+


nit: This extra newline can be removed

dnephin · 2017-11-02T16:35:09Z

cli/command/service/update.go

+			err := o.Set(val)
+			if err != nil {
+				return err
+			}


You don't need to create a new isolationOpts struct here. Value is that struct, you just have to cast it (like the functions above).

dnephin · 2017-11-02T16:36:41Z

cli/command/service/update_test.go

+	assert.Equal(t, container.IsolationProcess, spec.TaskTemplate.ContainerSpec.Isolation)
+}
+
+func TestUpdateIsolationInvalid(t *testing.T) {


I think this test needs to call updateService() otherwise it's only testing isolationOpt

Ok, makes sense, I'll remove the test as opts is already tested and updateService won't be called if flags parsing fails

dnephin · 2017-11-02T16:37:17Z

cli/compose/convert/service.go

@@ -122,6 +122,11 @@ func Service(
 		}
 	}

+	isolation := container.Isolation(service.Isolation)
+	if !isolation.IsDefault() && !isolation.IsHyperV() && !isolation.IsProcess() {
+		return swarm.ServiceSpec{}, fmt.Errorf("Unsupported isolation %s. Valid values are %s, %s, %s", isolation, container.IsolationDefault, container.IsolationProcess, container.IsolationHyperV)


This is already validated by the isolationOpt we shouldn't need to do it again here, right?

The validation should be handled by the flags/value types so that any value stored in the opt is valid to send to the API.

In this case, the source is not an isolationOpt flag, but a compose-file field (which is of type string)

Oh, I missed that! For the compose file we do the validation as part of the types/loading. Could this validation be moved there?

Did not see that, thank! I changed validation by typing the isolation field in service type to container.Isolation, and introduced a transformValidation function at loading time (that does the validation). Also added a parsing test for invalid isolation.

dnephin · 2017-11-02T16:38:29Z

cli/compose/convert/service_test.go

@@ -372,3 +372,14 @@ func TestConvertUpdateConfigOrder(t *testing.T) {
 	})
 	assert.Equal(t, updateConfig.Order, "stop-first")
 }
+
+func TestIsolation(t *testing.T) {


s/TestIsolation/TestServiceConvertsIsolation/

dnephin · 2017-11-02T16:39:28Z

cli/compose/convert/service_test.go

+	result, err := Service("1.32", Namespace{name: "foo"}, src, nil, nil, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}


require.NoError(t, err) ?

thaJeztah · 2017-11-06T14:55:00Z

cli/command/service/opts.go

+		o.value = &tv
+		return nil
+	}
+	return fmt.Errorf("Unknown isolation mode %s. Valid values are %v, %v, %v", value, container.IsolationDefault, container.IsolationProcess, container.IsolationHyperV)


Ideally, this would be checked by the daemon, instead of the client. Do we have a similar check already daemon-side?

Trying to delegate validation to the daemon as much as possible, because (supported) options can differ, depending on the version of the daemon, and we should not have to keep track of that in the client.

Good point. Actually it is already validated daemon-side with different validation logic on Linux (which only supports empty or default isolation) and windows (that also supports process and hyperv). I'll remove that client-side validation alltogether, and I'll make sure daemon-side errors are surfaced in a good way to the user

thaJeztah · 2017-11-06T15:10:07Z

This will also need;

some changes to the command-line reference docs (documenting the new options for docker service create and docker service update)
updates in the bash/zsh completion scripts
updates to the docker-compose file reference (https://github.com/docker/docker.github.io/blob/master/compose/compose-file/index.md)

thaJeztah · 2017-11-08T16:54:01Z

Also opened a moby bump in #679, which should bring in the required changes

simonferquel · 2017-11-14T10:13:07Z

@thaJeztah sorry for the delay, it has been quite a busy time lately...
I will implement your feedback today.

thaJeztah · 2017-11-14T12:45:37Z

Thanks!

thaJeztah · 2017-11-15T17:36:33Z

cli/command/service/opts.go

+	o.source = value
+	tv := container.Isolation(value)
+	o.value = &tv
+	return nil


This never returns an error; is the error to satisfy an interface?

Yes, it is the flag.Value interface.

thaJeztah

one question, but otherwise I think this is ready to go after squashing

simonferquel · 2017-11-16T09:43:20Z

Squashed & rebased

thaJeztah · 2017-11-16T12:13:16Z

cli/compose/loader/loader.go

+	if !ok {
+		return value, errors.Errorf("invalid type %T for isolation", value)
+	}
+	return container.Isolation(strings.ToLower(str)), nil


Missed this in my previous review; can you remove the strings.ToLower() here as well?

thaJeztah

LGTM, thanks!

thaJeztah · 2017-11-16T13:36:51Z

ping @vdemeester @dnephin PTAL

dnephin

Just one small issue with the Compose support, otherwise looks good

dnephin · 2017-11-16T16:49:40Z

cli/command/service/opts.go

+}
+
+func (o *isolationOpts) Type() string {
+	return "isolation"


This type is what gets reported in the help text. I wonder if string would be more helpful to users.

dnephin · 2017-11-16T16:55:09Z

cli/compose/types/types.go

@@ -125,7 +127,8 @@ type ServiceConfig struct {
 	Ulimits         map[string]*UlimitsConfig
 	User            string
 	Volumes         []ServiceVolumeConfig
-	WorkingDir      string `mapstructure:"working_dir"`
+	WorkingDir      string              `mapstructure:"working_dir"`
+	Isolation       container.Isolation `mapstructure:"isolation"`


I think this should be string. Everywhere else we accept plain types and do the convert to swarm/api types in convert. This helps keep the compose config packages decoupled from the api.

I believe this change will remove the need for the custom transform in loader.go as well.

I guess this was here previously because of validation. I should have looked closer when I was commenting the first time. Sorry about that.

simonferquel · 2017-11-17T08:56:49Z

@dnephin I just reverted service options to use plain string type for isolation, and modified compose types to remove all traces of validation (removing transformIsolation). Should be ok now.

thaJeztah · 2017-11-17T09:41:59Z

docs/reference/commandline/service_create.md

@@ -42,6 +42,7 @@ Options:
      --help                               Print usage
      --host list                          Set one or more custom host-to-IP mappings (host:ip)
      --hostname string                    Container hostname
+      --isolation isolation                Service container isolation mode


this should be "string" now then? (same for the other usage output)

simonferquel · 2017-11-17T10:02:41Z

Good catch @thaJeztah , fixed :)

thaJeztah

still LGTM

thaJeztah · 2017-11-17T11:34:38Z

Oh, actually; dang, test is failing:

--- FAIL: TestLoadV35 (0.00s)
	Error Trace:	loader_test.go:1473
	Error:      	Not equal: 
	            	expected: container.Isolation("process")
	            	received: string("process")
=== RUN   TestLoadV35InvalidIsolation
--- FAIL: TestLoadV35InvalidIsolation (0.00s)
	Error Trace:	loader_test.go:1490
	Error:      	Not equal: 
	            	expected: container.Isolation("invalid")
	            	received: string("invalid")

thaJeztah · 2017-11-17T11:38:18Z

cli/compose/schema/data/config_schema_v3.5.json

@@ -0,0 +1,544 @@
+{


Wondering what we did previous times when adding a new version; perhaps it's cleaner if there were two commits; one that adds a "pristine" compose 3.5 (exact copy of 3.4), and one commit that adds your changes

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

vdemeester

LGTM 🐮

thaJeztah · 2017-11-17T14:40:03Z

🍪

simonferquel requested review from dnephin and vdemeester as code owners August 7, 2017 14:37

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

vdemeester added status/2-code-review and removed status/0-triage labels Aug 7, 2017

simonferquel force-pushed the service-isolation branch 2 times, most recently from a81a3fe to 9843598 Compare August 9, 2017 09:40

simonferquel force-pushed the service-isolation branch 2 times, most recently from 537ba2f to 83faf7d Compare September 6, 2017 16:12

simonferquel force-pushed the service-isolation branch 2 times, most recently from 4aa7e53 to 27b818a Compare September 19, 2017 09:43

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 service-isolation branch 2 times, most recently from 92b49da to ee86d46 Compare November 2, 2017 14:43

simonferquel force-pushed the service-isolation branch from a85b1a4 to 958ab3b Compare November 2, 2017 15:52

dnephin reviewed Nov 2, 2017

View reviewed changes

thaJeztah requested changes Nov 6, 2017

View reviewed changes

thaJeztah mentioned this pull request Nov 6, 2017

Add isolation option to docker service moby/moby#31616

Closed

thaJeztah added impact/changelog impact/compose-file impact/documentation labels Nov 6, 2017

thaJeztah reviewed Nov 15, 2017

View reviewed changes

mdlinville approved these changes Nov 15, 2017

View reviewed changes

thaJeztah mentioned this pull request Nov 15, 2017

Integrated Generic resource in service create #429

Merged

simonferquel force-pushed the service-isolation branch 2 times, most recently from 46246fb to 5a009e8 Compare November 16, 2017 09:43

thaJeztah reviewed Nov 16, 2017

View reviewed changes

simonferquel force-pushed the service-isolation branch from 5a009e8 to 735ca51 Compare November 16, 2017 13:18

thaJeztah approved these changes Nov 16, 2017

View reviewed changes

dnephin reviewed Nov 16, 2017

View reviewed changes

simonferquel force-pushed the service-isolation branch from 735ca51 to 2386b2e Compare November 17, 2017 08:55

thaJeztah reviewed Nov 17, 2017

View reviewed changes

simonferquel force-pushed the service-isolation branch from 2386b2e to e4d3b1d Compare November 17, 2017 10:01

thaJeztah approved these changes Nov 17, 2017

View reviewed changes

thaJeztah reviewed Nov 17, 2017

View reviewed changes

simonferquel force-pushed the service-isolation branch from e4d3b1d to e8215fb Compare November 17, 2017 14:25

simonferquel added 2 commits November 17, 2017 15:30

Preparing for compose schema v3.5

787e30d

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

Add isolation mode on service update/create and compose files

47cf2ea

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

simonferquel force-pushed the service-isolation branch from e8215fb to 47cf2ea Compare November 17, 2017 14:31

vdemeester approved these changes Nov 17, 2017

View reviewed changes

vdemeester merged commit 5e2be65 into docker:master Nov 17, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 17, 2017

shin- mentioned this pull request Feb 23, 2018

Isolation parameter does not work in compose files with versions >2.3 docker/compose#5069

Closed

thaJeztah mentioned this pull request Jan 9, 2019

Add maximum replicas per node support to stack version 3.8 #1410

Merged

2 tasks

--isolation for setting swarm service isolation mode #426

--isolation for setting swarm service isolation mode #426

Conversation

simonferquel commented Aug 7, 2017 • edited by thaJeztah Loading

codecov-io commented Aug 8, 2017 • edited Loading

Codecov Report

vdemeester commented Aug 22, 2017

simonferquel commented Sep 19, 2017

thaJeztah commented Nov 1, 2017

simonferquel commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Nov 6, 2017

thaJeztah commented Nov 8, 2017

simonferquel commented Nov 14, 2017

thaJeztah commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

simonferquel commented Nov 16, 2017

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 16, 2017

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonferquel commented Nov 17, 2017

Choose a reason for hiding this comment

simonferquel commented Nov 17, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 17, 2017

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 17, 2017

simonferquel commented Aug 7, 2017 •

edited by thaJeztah

Loading

codecov-io commented Aug 8, 2017 •

edited

Loading