Add `--read-only` for `service create` and `service update` by yongtang · Pull Request #30162 · moby/moby

yongtang · 2017-01-14T08:22:33Z

- What I did

This fix tries to address the issue raised in #29972 where it was not possible to specify --read-only for docker service create and docker service update, in order to have the container's root file system to be read only.

- How I did it

This fix adds --read-only and update the ReadonlyRootfs in HostConfig through service create and service update.

Related docs has been updated.

- How to verify it

Integration test has been added.

- Description for the changelog

Add --read-only for service create and service update

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #29972.

Related SwarmKit PR is moby/swarmkit#1872

Signed-off-by: Yong Tang [email protected]

thaJeztah

design looks good to me; left some general thoughts on the tests

thaJeztah · 2017-01-16T12:14:35Z

integration-cli/docker_cli_swarm_test.go

I don't think we need to test this here; this functionality is already tested in testReadOnlyFile()

Thanks @thaJeztah. That test has been removed.

thaJeztah · 2017-01-16T12:16:25Z

integration-cli/docker_cli_swarm_test.go

I think it would be good to test that running an docker service update without specifying the --read-only flag, does not change ReadonlyRootfs from true to false

Thanks. I added a unit test to cover this scenario.

thaJeztah · 2017-01-16T12:35:02Z

integration-cli/docker_cli_swarm_test.go

Looking at all these tests; I'm not sure how much of this is tested in SwarmKit, but possibly it could even be changed to a unit test (i.e., test that the client is setting / updating the ReadonlyRootfs in the TaskTemplate)

/cc @vdemeester perhaps you have some thoughts on that?

Thanks. Some of the test has been changed to unit tests.

thaJeztah · 2017-01-16T12:59:57Z

cli/command/service/opts.go

Oh, can you version this flag, so that it only shows if the daemon is docker 1.14 or above?

flags.SetAnnotation(flagReadonlyRootfs, "version", []string{"1.26"})

(hm, thinking of which, I think there's some other flags that we need to update as well)

LK4D4 · 2017-01-27T18:28:38Z

@yongtang there are some comments from @thaJeztah
Thanks!

yongtang · 2017-01-27T18:34:45Z

@LK4D4 Thanks for the reminder. moby/swarmkit#1872 was merged yesterday. I will work on updating this PR shortly.

yongtang · 2017-01-28T05:09:04Z

@thaJeztah Thanks for the review. The PR has been updated. And some of the tests have been changed to unit tests. Please take a look and let me know if there are any issues.

LK4D4 · 2017-01-30T17:22:09Z

@yongtang I'm not sure, but failures look real enough

yongtang · 2017-01-30T17:26:51Z

@LK4D4 Ah the failure is related to the changes in SwarmKit about explicitly disallow network pluginv1 creation in swarm mode (See discussion in moby/swarmkit#1899, moby/swarmkit#1894, and #30332)

The failure is being addressed in PR #30548 now. Please take a look.

LK4D4 · 2017-01-30T17:40:57Z

@yongtang cool, thanks!

LK4D4 · 2017-01-30T17:41:34Z

@yongtang just merged it.

yongtang · 2017-01-30T17:53:44Z

Thanks @LK4D4! This PR has been updated as well now.

thaJeztah

changes LGTM perhaps you can also update the completion scripts for bash and zsh as part of this PR? I think bash change should go here; https://github.com/docker/docker/blob/master/contrib/completion/bash/docker#L2833 (but double check 😄)

This fix tries to address the issue raised in 29972 where it was not possible to specify `--read-only` for `docker service create` and `docker service update`, in order to have the container's root file system to be read only. This fix adds `--read-only` and update the `ReadonlyRootfs` in `HostConfig` through `service create` and `service update`. Related docs has been updated. Integration test has been added. This fix fixes 29972. Signed-off-by: Yong Tang <[email protected]>

This commit updates bash and zsh completion for flag `--read-only` in `service create/update`. Signed-off-by: Yong Tang <[email protected]>

yongtang · 2017-01-30T21:23:27Z

Thanks @thaJeztah. The PR has been updated with related bash and zsh completion added.

thaJeztah

LGTM, thanks!

LK4D4 · 2017-01-31T21:19:57Z

LGTM

vieux · 2017-02-18T01:59:50Z

@yongtang can you please open a PR to update docs/api/version-history.md

yongtang · 2017-02-18T03:23:27Z

Thanks @vieux for the reminder. The PR #31145 has been opened.

Add `--read-only` for `service create` and `service update`

GordonTheTurtle added the status/0-triage label Jan 14, 2017

This was referenced Jan 14, 2017

Add ReadonlyRootfs in ContainerSpec for --read-only moby/swarmkit#1872

Merged

Add --read-only for Swarm Services #29972

Closed

cpuguy83 added status/1-design-review status/needs-vendoring and removed status/0-triage labels Jan 14, 2017

thaJeztah requested changes Jan 16, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Jan 16, 2017

thaJeztah requested changes Jan 16, 2017

View reviewed changes

yongtang force-pushed the 29972-service-read-only branch from 8a162eb to 6393cc0 Compare January 28, 2017 05:02

yongtang force-pushed the 29972-service-read-only branch from 6393cc0 to 4e61743 Compare January 28, 2017 05:09

AkihiroSuda added rebuild/experimental and removed status/needs-vendoring labels Jan 30, 2017

GordonTheTurtle removed rebuild/experimental labels Jan 30, 2017

yongtang force-pushed the 29972-service-read-only branch from 4e61743 to 2ede0eb Compare January 30, 2017 17:51

thaJeztah reviewed Jan 30, 2017

View reviewed changes

yongtang added 2 commits January 30, 2017 12:47

Update bash and zsh completion for service create/update --read-only

885e1f2

This commit updates bash and zsh completion for flag `--read-only` in `service create/update`. Signed-off-by: Yong Tang <[email protected]>

yongtang force-pushed the 29972-service-read-only branch from 2ede0eb to 885e1f2 Compare January 30, 2017 21:22

thaJeztah approved these changes Jan 30, 2017

View reviewed changes

LK4D4 merged commit 1d2f5de into moby:master Jan 31, 2017

GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017

yongtang deleted the 29972-service-read-only branch January 31, 2017 21:23

thaJeztah added impact/api impact/changelog impact/cli labels Jan 31, 2017

thaJeztah mentioned this pull request Feb 9, 2017

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

Open

AkihiroSuda mentioned this pull request Feb 15, 2017

can not find --read-only option for service create and service update #31036

Closed

yongtang mentioned this pull request Feb 18, 2017

Update version-history.md for ReadOnly parameter #31145

Merged

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30162 from yongtang/29972-service-read-only

7ed541d

Add `--read-only` for `service create` and `service update`

Conversation

yongtang commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 27, 2017

Uh oh!

yongtang commented Jan 27, 2017

Uh oh!

yongtang commented Jan 28, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

yongtang commented Jan 30, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

yongtang commented Jan 30, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang commented Jan 30, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 31, 2017

Uh oh!

vieux commented Feb 18, 2017

Uh oh!

yongtang commented Feb 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yongtang commented Jan 14, 2017 •

edited

Loading