Change defaults for mount writable and volume copying #24053

cpuguy83 · 2016-06-28T18:45:15Z

Changes the writable CLI flag to readonly instead. This way CLI users
must opt-in to readonly as is expected from docker run.
The API is unchanged and defaults to to not writable.

Fixes #23915

thaJeztah · 2016-06-30T08:09:45Z

~~LGTM~~ see below

ping @stevvooe @tiborvass

vdemeester · 2016-06-30T08:28:37Z

The API is unchanged and defaults to to not writable.

Doesn't this make the API less consistent with the /container/… one then ? 👼

thaJeztah · 2016-06-30T22:58:59Z

I actually agree with @vdemeester, IMO we should have the same defaults on the client and API. Sending an "empty" request through the API should pick the defaults we decide on. If we think "RW" is the right default for volumes, the API should use that as a default.

stevvooe · 2016-06-30T23:03:53Z

@vdemeester @thaJeztah We can change it, but it is an API change during the RC period. I've been advocating this change, but there seems to be push back.

thaJeztah · 2016-06-30T23:15:48Z

Changing after GA will be even harder, I think...

On 30 Jun 2016, at 16:04, Stephen Day notifications@github.com wrote:

@vdemeester @thaJeztah We can change it, but it is an API change during the RC period. I've been advocating this change, but there seems to be push back.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

To ensure that the zero-value reflects the defaults for mounts, change the `Writable` to `ReadOnly`. Adds a breaking api change to address a point of confusion. If this PR is not merged by the 1.12 release, this can never be merged, so just close and move on. Related to moby/moby#24053. Signed-off-by: Stephen J Day <stephen.day@docker.com>

stevvooe · 2016-07-01T01:57:46Z

@thaJeztah @cpuguy83 @aaronlehmann I've submitted docker/engine-api#298. If we decide this is what we want, as this has come up a few times, merge that and I'll take care of the deltas.

cpuguy83 · 2016-07-01T02:00:22Z

This is fine, the big thing is going to be changing it in swarmkit.

To ensure that the zero-value reflects the defaults for mounts, change the `Writable` to `ReadOnly`. Adds a breaking api change to address a point of confusion. If this PR is not merged by the 1.12 release, this can never be merged, so just close and move on. Related to moby/moby#24053. Signed-off-by: Stephen J Day <stephen.day@docker.com>

stevvooe · 2016-07-01T02:23:32Z

@cpuguy83 I've submitted a fix in swarmkit and engine-api. Let's update the vendoring here to use this PR as a gate for the feature.

nishanttotla · 2016-07-01T19:37:07Z

@cpuguy83 if you update engine-api to the latest commit, you'll also need these changes: nishanttotla@9d62e6c

In case you don't wish to mix them in this PR, don't vendor in changes introduced by docker/engine-api#297, and I'll open a PR with my branch after this one is merged.

dnephin · 2016-07-03T16:34:55Z

pkg/testutil/assert/assert.go

+func fatal(t TestingT, msg string) {
+	_, file, line, _ := runtime.Caller(2)
+	t.Fatalf("%s:%d: %s", filepath.Base(file), line, msg)
+}


What's up with this fatal()? isn't this already what t.Fatalf() prints by default?

Oh nevermind , I missed this was in the assert package. I have a PR somewhere that adds this as well. I'll resolve the conflict if this merges first/

cpuguy83 · 2016-07-05T15:20:01Z

Thanks @nishanttotla! Extremely helpful.

cpuguy83 · 2016-07-05T18:51:48Z

Ok, this is updated.
Also changed the Populate field to NoCopy (which matches the -v syntax) here as well.

engine-api and swarmkit changes are vendored in.

stevvooe · 2016-07-05T18:56:43Z

LGTM

thaJeztah · 2016-07-06T20:54:40Z

moved to docs review; just the API docs, then we should be able to merge

cpuguy83 · 2016-07-06T21:05:40Z

Docs updated.

thaJeztah · 2016-07-06T21:10:48Z

docs LGTM

vdemeester · 2016-07-06T21:12:07Z

docs LGTM 🐯

thaJeztah · 2016-07-06T22:48:49Z

hmf, is this a known issue in CI? https://jenkins.dockerproject.org/job/Docker-PRs/29631/console

21:45:12 ----------------------------------------------------------------------
21:45:12 FAIL: docker_cli_service_create_hack_test.go:15: DockerSwarmSuite.TestServiceCreateMountVolume
21:45:12 
21:45:12 [d73632282] waiting for daemon to start
21:45:12 [d73632282] daemon started
21:45:12 docker_cli_service_create_hack_test.go:33:
21:45:12     waitAndAssert(c, defaultReconciliationTimeout, func(c *check.C) (interface{}, check.CommentInterface) {
21:45:12         if task.NodeID == "" || task.Status.ContainerStatus.ContainerID == "" {
21:45:12             task = d.getTask(c, task.ID)
21:45:12         }
21:45:12         return task.NodeID != "" && task.Status.ContainerStatus.ContainerID != "", nil
21:45:12     }, checker.Equals, true)
21:45:12 github.com/docker/docker/integration-cli/_test/_obj_test/docker_utils.go:2011:
21:45:12     ...open github.com/docker/docker/integration-cli/_test/_obj_test/docker_utils.go: no such file or directory
21:45:12 ... obtained bool = false
21:45:12 ... expected bool = true
21:45:12 
21:45:12 [d73632282] exiting daemon
21:45:29

thaJeztah · 2016-07-06T22:50:47Z

could be related?

cpuguy83 · 2016-07-06T23:14:26Z

It's a new test, I will have to update it before we merge.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

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 <cpuguy83@gmail.com>

cpuguy83 · 2016-07-07T02:35:20Z

Whew, that's a good test. Issue with rebase had some lines mixed up.
Pushed up fixes.

Seems CI took a vacation.

thaJeztah · 2016-07-07T07:37:27Z

retest this please

GordonTheTurtle added the status/0-triage label Jun 28, 2016

cpuguy83 added status/1-design-review and removed status/0-triage labels Jun 28, 2016

cpuguy83 added this to the 1.12.0 milestone Jun 28, 2016

cpuguy83 mentioned this pull request Jun 28, 2016

Service mounts default to read-only #23915

Closed

cpuguy83 force-pushed the mounts_default_writable branch from 509b884 to a72ab39 Compare June 28, 2016 19:09

thaJeztah added status/2-code-review process/cherry-pick and removed status/1-design-review labels Jun 30, 2016

stevvooe mentioned this pull request Jul 1, 2016

types/swarm: make mounts read write by default docker/engine-api#298

Merged

stevvooe mentioned this pull request Jul 1, 2016

api: use ReadOnly rather than Writable moby/swarmkit#1110

Merged

dnephin reviewed Jul 3, 2016
View reviewed changes

dnephin mentioned this pull request Jul 5, 2016

Improve docker service --mount syntax #24334

Closed

cpuguy83 force-pushed the mounts_default_writable branch from a72ab39 to bdba60a Compare July 5, 2016 18:48

cpuguy83 changed the title ~~Set service mount CLI to default to writable~~ Change defaults for mount writable and volume copying Jul 5, 2016

cpuguy83 force-pushed the mounts_default_writable branch from bdba60a to daeaadc Compare July 5, 2016 18:53

thaJeztah added status/3-docs-review and removed status/2-code-review labels Jul 6, 2016

cpuguy83 force-pushed the mounts_default_writable branch from daeaadc to 04881f0 Compare July 6, 2016 21:00

vdemeester mentioned this pull request Jul 6, 2016

Use "on-failure" for both containers and services #24369

Merged

vdemeester added status/4-merge and removed status/3-docs-review labels Jul 6, 2016

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 6, 2016

tiborvass added status/2-code-review and removed status/4-merge labels Jul 7, 2016

Brian Goff added 3 commits July 6, 2016 21:14

re-vendor engine-api

140ec59

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

re-vendor swarmkit

db271b8

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the mounts_default_writable branch from 04881f0 to 56f3422 Compare July 7, 2016 02:34

vdemeester removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 7, 2016

vdemeester merged commit 50674ec into moby:master Jul 7, 2016

tiborvass mentioned this pull request Jul 8, 2016

[WIP] Cherry picks 1.12.0 rc4 #24467

Merged

tiborvass added process/cherry-picked and removed process/cherry-pick labels Jul 12, 2016

iamluc mentioned this pull request Jul 31, 2016

WIP: Swarm services ehazlett/interlock#186

Open

thaJeztah mentioned this pull request Aug 16, 2016

service create --mount doesn't allow options for "ephemeral" volumes #25768

Closed

cpuguy83 deleted the mounts_default_writable branch September 20, 2017 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change defaults for mount writable and volume copying #24053

Change defaults for mount writable and volume copying #24053

cpuguy83 commented Jun 28, 2016

thaJeztah commented Jun 30, 2016 •

edited

vdemeester commented Jun 30, 2016 •

edited

thaJeztah commented Jun 30, 2016

stevvooe commented Jun 30, 2016

thaJeztah commented Jun 30, 2016

stevvooe commented Jul 1, 2016

cpuguy83 commented Jul 1, 2016

stevvooe commented Jul 1, 2016

nishanttotla commented Jul 1, 2016

dnephin Jul 3, 2016

dnephin Jul 3, 2016

cpuguy83 commented Jul 5, 2016

cpuguy83 commented Jul 5, 2016

stevvooe commented Jul 5, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

vdemeester commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 6, 2016

cpuguy83 commented Jul 7, 2016

thaJeztah commented Jul 7, 2016

Change defaults for mount writable and volume copying #24053

Change defaults for mount writable and volume copying #24053

Conversation

cpuguy83 commented Jun 28, 2016

thaJeztah commented Jun 30, 2016 • edited

vdemeester commented Jun 30, 2016 • edited

thaJeztah commented Jun 30, 2016

stevvooe commented Jun 30, 2016

thaJeztah commented Jun 30, 2016

stevvooe commented Jul 1, 2016

cpuguy83 commented Jul 1, 2016

stevvooe commented Jul 1, 2016

nishanttotla commented Jul 1, 2016

dnephin Jul 3, 2016

Choose a reason for hiding this comment

dnephin Jul 3, 2016

Choose a reason for hiding this comment

cpuguy83 commented Jul 5, 2016

cpuguy83 commented Jul 5, 2016

stevvooe commented Jul 5, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

vdemeester commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

thaJeztah commented Jul 6, 2016

cpuguy83 commented Jul 6, 2016

cpuguy83 commented Jul 7, 2016

thaJeztah commented Jul 7, 2016

thaJeztah commented Jun 30, 2016 •

edited

vdemeester commented Jun 30, 2016 •

edited