From 843b4a93fe13589bbff4c8b99c4d7cbc688ae7b5 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 5 Jul 2016 14:43:28 -0400 Subject: [PATCH] Use newer default values for mounts CLI In the API: `Writable` changed to `ReadOnly` `Populate` changed to `NoCopy` Corresponding CLI options updated to: `volume-writable` changed to `volume-readonly` `volume-populate` changed to `volume-nocopy` Signed-off-by: Brian Goff (cherry picked from commit 56f3422468a0b43da7bae7a01762ce4f0a92d9ff) Signed-off-by: Tibor Vass --- api/client/service/opts.go | 32 +++++++++--- api/client/service/opts_test.go | 50 ++++++++++++++++++- daemon/cluster/convert/container.go | 12 ++--- .../cluster/executor/container/container.go | 16 +++--- docs/reference/api/docker_remote_api_v1.24.md | 9 ++-- .../docker_cli_service_create_hack_test.go | 2 +- pkg/testutil/assert/assert.go | 18 +++++-- 7 files changed, 109 insertions(+), 30 deletions(-) diff --git a/api/client/service/opts.go b/api/client/service/opts.go index dda2290729..2b963d4fc1 100644 --- a/api/client/service/opts.go +++ b/api/client/service/opts.go @@ -176,10 +176,16 @@ func (m *MountOpt) Set(value string) error { } } + // Set writable as the default for _, field := range fields { parts := strings.SplitN(field, "=", 2) - if len(parts) == 1 && strings.ToLower(parts[0]) == "writable" { - mount.Writable = true + if len(parts) == 1 && strings.ToLower(parts[0]) == "readonly" { + mount.ReadOnly = true + continue + } + + if len(parts) == 1 && strings.ToLower(parts[0]) == "volume-nocopy" { + volumeOptions().NoCopy = true continue } @@ -195,15 +201,16 @@ func (m *MountOpt) Set(value string) error { mount.Source = value case "target": mount.Target = value - case "writable": - mount.Writable, err = strconv.ParseBool(value) + case "readonly": + ro, err := strconv.ParseBool(value) if err != nil { - return fmt.Errorf("invalid value for writable: %s", value) + return fmt.Errorf("invalid value for readonly: %s", value) } + mount.ReadOnly = ro case "bind-propagation": bindOptions().Propagation = swarm.MountPropagation(strings.ToUpper(value)) - case "volume-populate": - volumeOptions().Populate, err = strconv.ParseBool(value) + case "volume-nocopy": + volumeOptions().NoCopy, err = strconv.ParseBool(value) if err != nil { return fmt.Errorf("invalid value for populate: %s", value) } @@ -229,6 +236,17 @@ func (m *MountOpt) Set(value string) error { return fmt.Errorf("target is required") } + if mount.VolumeOptions != nil && mount.Source == "" { + return fmt.Errorf("source is required when specifying volume-* options") + } + + if mount.Type == swarm.MountType("BIND") && mount.VolumeOptions != nil { + return fmt.Errorf("cannot mix 'volume-*' options with mount type '%s'", swarm.MountTypeBind) + } + if mount.Type == swarm.MountType("VOLUME") && mount.BindOptions != nil { + return fmt.Errorf("cannot mix 'bind-*' options with mount type '%s'", swarm.MountTypeVolume) + } + m.values = append(m.values, mount) return nil } diff --git a/api/client/service/opts_test.go b/api/client/service/opts_test.go index 808ce236ef..c2a17115f5 100644 --- a/api/client/service/opts_test.go +++ b/api/client/service/opts_test.go @@ -111,5 +111,53 @@ func TestMountOptSetErrorInvalidField(t *testing.T) { func TestMountOptSetErrorInvalidWritable(t *testing.T) { var mount MountOpt - assert.Error(t, mount.Set("type=VOLUME,writable=yes"), "invalid value for writable: yes") + assert.Error(t, mount.Set("type=VOLUME,readonly=no"), "invalid value for readonly: no") +} + +func TestMountOptDefaultEnableWritable(t *testing.T) { + var m MountOpt + assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo")) + assert.Equal(t, m.values[0].ReadOnly, false) + + m = MountOpt{} + assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly")) + assert.Equal(t, m.values[0].ReadOnly, true) + + m = MountOpt{} + assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly=1")) + assert.Equal(t, m.values[0].ReadOnly, true) + + m = MountOpt{} + assert.NilError(t, m.Set("type=bind,target=/foo,source=/foo,readonly=0")) + assert.Equal(t, m.values[0].ReadOnly, false) +} + +func TestMountOptVolumeNoCopy(t *testing.T) { + var m MountOpt + assert.Error(t, m.Set("type=volume,target=/foo,volume-nocopy"), "source is required") + + m = MountOpt{} + assert.NilError(t, m.Set("type=volume,target=/foo,source=foo")) + assert.Equal(t, m.values[0].VolumeOptions == nil, true) + + m = MountOpt{} + assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy=true")) + assert.Equal(t, m.values[0].VolumeOptions != nil, true) + assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true) + + m = MountOpt{} + assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy")) + assert.Equal(t, m.values[0].VolumeOptions != nil, true) + assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true) + + m = MountOpt{} + assert.NilError(t, m.Set("type=volume,target=/foo,source=foo,volume-nocopy=1")) + assert.Equal(t, m.values[0].VolumeOptions != nil, true) + assert.Equal(t, m.values[0].VolumeOptions.NoCopy, true) +} + +func TestMountOptTypeConflict(t *testing.T) { + var m MountOpt + assert.Error(t, m.Set("type=bind,target=/foo,source=/foo,volume-nocopy=true"), "cannot mix") + assert.Error(t, m.Set("type=volume,target=/foo,source=/foo,bind-propagation=rprivate"), "cannot mix") } diff --git a/daemon/cluster/convert/container.go b/daemon/cluster/convert/container.go index c943537ad4..83cc5342bc 100644 --- a/daemon/cluster/convert/container.go +++ b/daemon/cluster/convert/container.go @@ -26,7 +26,7 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec { Target: m.Target, Source: m.Source, Type: types.MountType(strings.ToLower(swarmapi.Mount_MountType_name[int32(m.Type)])), - Writable: m.Writable, + ReadOnly: m.ReadOnly, } if m.BindOptions != nil { @@ -37,8 +37,8 @@ func containerSpecFromGRPC(c *swarmapi.ContainerSpec) types.ContainerSpec { if m.VolumeOptions != nil { mount.VolumeOptions = &types.VolumeOptions{ - Populate: m.VolumeOptions.Populate, - Labels: m.VolumeOptions.Labels, + NoCopy: m.VolumeOptions.NoCopy, + Labels: m.VolumeOptions.Labels, } if m.VolumeOptions.DriverConfig != nil { mount.VolumeOptions.DriverConfig = &types.Driver{ @@ -77,7 +77,7 @@ func containerToGRPC(c types.ContainerSpec) (*swarmapi.ContainerSpec, error) { mount := swarmapi.Mount{ Target: m.Target, Source: m.Source, - Writable: m.Writable, + ReadOnly: m.ReadOnly, } if mountType, ok := swarmapi.Mount_MountType_value[strings.ToUpper(string(m.Type))]; ok { @@ -98,8 +98,8 @@ func containerToGRPC(c types.ContainerSpec) (*swarmapi.ContainerSpec, error) { if m.VolumeOptions != nil { mount.VolumeOptions = &swarmapi.Mount_VolumeOptions{ - Populate: m.VolumeOptions.Populate, - Labels: m.VolumeOptions.Labels, + NoCopy: m.VolumeOptions.NoCopy, + Labels: m.VolumeOptions.Labels, } if m.VolumeOptions.DriverConfig != nil { mount.VolumeOptions.DriverConfig = &swarmapi.Driver{ diff --git a/daemon/cluster/executor/container/container.go b/daemon/cluster/executor/container/container.go index ece2ec3743..4a42ef91e1 100644 --- a/daemon/cluster/executor/container/container.go +++ b/daemon/cluster/executor/container/container.go @@ -163,9 +163,13 @@ func (c *containerConfig) bindMounts() []string { var r []string for _, val := range c.spec().Mounts { - mask := getMountMask(&val) if val.Type == api.MountTypeBind || (val.Type == api.MountTypeVolume && val.Source != "") { - r = append(r, fmt.Sprintf("%s:%s:%s", val.Source, val.Target, mask)) + mask := getMountMask(&val) + spec := fmt.Sprintf("%s:%s", val.Source, val.Target) + if mask != "" { + spec = fmt.Sprintf("%s:%s", spec, mask) + } + r = append(r, spec) } } @@ -173,9 +177,9 @@ func (c *containerConfig) bindMounts() []string { } func getMountMask(m *api.Mount) string { - maskOpts := []string{"ro"} - if m.Writable { - maskOpts[0] = "rw" + var maskOpts []string + if m.ReadOnly { + maskOpts = append(maskOpts, "ro") } if m.BindOptions != nil { @@ -196,7 +200,7 @@ func getMountMask(m *api.Mount) string { } if m.VolumeOptions != nil { - if !m.VolumeOptions.Populate { + if m.VolumeOptions.NoCopy { maskOpts = append(maskOpts, "nocopy") } } diff --git a/docs/reference/api/docker_remote_api_v1.24.md b/docs/reference/api/docker_remote_api_v1.24.md index 0f12218553..42c0154263 100644 --- a/docs/reference/api/docker_remote_api_v1.24.md +++ b/docs/reference/api/docker_remote_api_v1.24.md @@ -3986,11 +3986,11 @@ JSON Parameters: - **Target** – Container path. - **Source** – Mount source (e.g. a volume name, a host path). - **Type** – The mount type (`bind`, or `volume`). - - **Writable** – A boolean indicating whether the mount should be writable. + - **ReadOnly** – A boolean indicating whether the mount should be read-only. - **BindOptions** - Optional configuration for the `bind` type. - **Propagation** – A propagation mode with the value `[r]private`, `[r]shared`, or `[r]slave`. - **VolumeOptions** – Optional configuration for the `volume` type. - - **Populate** – A boolean indicating if volume should be + - **NoCopy** – A boolean indicating if volume should be populated with the data from the target. (Default false) - **Labels** – User-defined name and labels for the volume. - **DriverConfig** – Map of driver-specific options. @@ -4204,11 +4204,12 @@ Update the service `id`. - **Target** – Container path. - **Source** – Mount source (e.g. a volume name, a host path). - **Type** – The mount type (`bind`, or `volume`). - - **Writable** – A boolean indicating whether the mount should be writable. + - **ReadOnly** – A boolean indicating whether the mount should be read-only. - **BindOptions** - Optional configuration for the `bind` type - **Propagation** – A propagation mode with the value `[r]private`, `[r]shared`, or `[r]slave`. - **VolumeOptions** – Optional configuration for the `volume` type. - - **Populate** – A boolean indicating if volume should be populated with the data from the target. (Default false) + - **NoCopy** – A boolean indicating if volume should be + populated with the data from the target. (Default false) - **Labels** – User-defined name and labels for the volume. - **DriverConfig** – Map of driver-specific options. - **Name** - Name of the driver to use to create the volume diff --git a/integration-cli/docker_cli_service_create_hack_test.go b/integration-cli/docker_cli_service_create_hack_test.go index abffa977ab..4814ddee59 100644 --- a/integration-cli/docker_cli_service_create_hack_test.go +++ b/integration-cli/docker_cli_service_create_hack_test.go @@ -41,5 +41,5 @@ func (s *DockerSwarmSuite) TestServiceCreateMountVolume(c *check.C) { c.Assert(mounts[0].Name, checker.Equals, "foo") c.Assert(mounts[0].Destination, checker.Equals, "/foo") - c.Assert(mounts[0].RW, checker.Equals, false) + c.Assert(mounts[0].RW, checker.Equals, true) } diff --git a/pkg/testutil/assert/assert.go b/pkg/testutil/assert/assert.go index a36b58bae4..5b0dcce67a 100644 --- a/pkg/testutil/assert/assert.go +++ b/pkg/testutil/assert/assert.go @@ -2,6 +2,9 @@ package assert import ( + "fmt" + "path/filepath" + "runtime" "strings" ) @@ -15,7 +18,7 @@ type TestingT interface { // they are not equal. func Equal(t TestingT, actual, expected interface{}) { if expected != actual { - t.Fatalf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual) + fatal(t, fmt.Sprintf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)) } } @@ -37,7 +40,7 @@ func EqualStringSlice(t TestingT, actual, expected []string) { // NilError asserts that the error is nil, otherwise it fails the test. func NilError(t TestingT, err error) { if err != nil { - t.Fatalf("Expected no error, got: %s", err.Error()) + fatal(t, fmt.Sprintf("Expected no error, got: %s", err.Error())) } } @@ -45,11 +48,11 @@ func NilError(t TestingT, err error) { // otherwise it fails the test. func Error(t TestingT, err error, contains string) { if err == nil { - t.Fatalf("Expected an error, but error was nil") + fatal(t, "Expected an error, but error was nil") } if !strings.Contains(err.Error(), contains) { - t.Fatalf("Expected error to contain '%s', got '%s'", contains, err.Error()) + fatal(t, fmt.Sprintf("Expected error to contain '%s', got '%s'", contains, err.Error())) } } @@ -57,6 +60,11 @@ func Error(t TestingT, err error, contains string) { // test. func Contains(t TestingT, actual, contains string) { if !strings.Contains(actual, contains) { - t.Fatalf("Expected '%s' to contain '%s'", actual, contains) + fatal(t, fmt.Sprintf("Expected '%s' to contain '%s'", actual, contains)) } } + +func fatal(t TestingT, msg string) { + _, file, line, _ := runtime.Caller(2) + t.Fatalf("%s:%d: %s", filepath.Base(file), line, msg) +}