8000 Add `--read-only` for `service create` and `service update` by yongtang · Pull Request #30162 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 31, 2017
Merged

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

merged 2 commits into from
Jan 31, 2017
+72 −4

Conversation

yongtang
Copy link
Member
@yongtang yongtang commented Jan 14, 2017 •
edited
Loading

- 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)

baby-animals-cat-cats-cute-kitten-kittens-desktop-image

This fix fixes #29972.

Related SwarmKit PR is moby/swarmkit#1872

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This was referenced Jan 14, 2017
Merged
Closed
thaJeztah
thaJeztah requested changes Jan 16, 2017
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

integration-cli/docker_cli_swarm_test.go
out, err = d.Cmd("inspect", "--type", "container", "--format", "{{.HostConfig.ReadonlyRootfs}}", containers[0])
c.Assert(err, checker.IsNil, check.Commentf(out))

expected := "touch: /file: Read-only file system"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thaJeztah. That test has been removed.

integration-cli/docker_cli_swarm_test.go
c.Assert(err, checker.NotNil, check.Commentf(out))
c.Assert(out, checker.Contains, expected)

out, err = d.Cmd("service", "update", "--read- "top")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

integration-cli/docker_cli_swarm_test.go
out, err = d.Cmd("service", "update", "--read- "top")
c.Assert(err, checker.IsNil, check.Commentf(out))

out, err = d.Cmd("service", "inspect", "--format", "{{ .Spec.TaskTemplate.ContainerSpec.ReadonlyRootfs }}", "top")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

thaJeztah
thaJeztah requested changes Jan 16, 2017
View reviewed changes
cli/command/service/opts.go
@@ -479,6 +481,7 @@ func addServiceFlags(cmd *cobra.Command, opts *serviceOptions) {
flags.BoolVar(&opts.healthcheck.noHealthcheck, flagNoHealthcheck, false, "Disable any container-specified HEALTHCHECK")

flags.BoolVarP(&opts.tty, flagTTY, "t", false, "Allocate a pseudo-TTY")
flags.BoolVar(&opts.readonlyRootfs, flagReadonlyRootfs, false, "Mount the container's root filesystem as read only")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@LK4D4
Copy link
Contributor
LK4D4 commented Jan 27, 2017

@yongtang there are some comments from @thaJeztah
Thanks!

@yongtang
Copy link
Member Author

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

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

@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.

@yongtang yongtang force-pushed the 29972-service-read-only branch from 6393cc0 to 4e61743 Compare January 28, 2017 05:09
@LK4D4
Copy link
Contributor
LK4D4 commented Jan 30, 2017

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

@yongtang
Copy link
Member Author

@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
Copy link
Contributor
LK4D4 commented Jan 30, 2017

@yongtang cool, thanks!

@LK4D4
Copy link
Contributor
LK4D4 commented Jan 30, 2017

@yongtang just merged it.

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

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

thaJeztah
thaJeztah reviewed Jan 30, 2017
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄)

@yongtang
Add --read-only for service create and service update
499a0dd 
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 <yong.tang.github@outlook.com>
@yongtang
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 <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 29972-service-read-only branch from 2ede0eb to 885e1f2 Compare January 30, 2017 21:22
@yongtang
Copy link
Member Author

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

thaJeztah
thaJeztah approved these changes Jan 30, 2017
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@LK4D4
Copy link
Contributor
LK4D4 commented Jan 31, 2017

LGTM

@LK4D4 LK4D4 merged commit 1d2f5de into moby:master Jan 31, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017
@yongtang yongtang deleted the 29972-service-read-only branch January 31, 2017 21:23
@thaJeztah thaJeztah mentioned this pull request Feb 9, 2017
Open
@AkihiroSuda AkihiroSuda mentioned this pull request Feb 15, 2017
Closed
@vieux
Copy link
Contributor
vieux commented Feb 18, 2017

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

@yongtang yongtang mentioned this pull request Feb 18, 2017
Merged
@yongtang
Copy link
Member Author

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

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@LK4D4
Merge pull request moby#30162 from yongtang/29972-service-read-only
7ed541d 
Add `--read-only` for `service create` and `service update`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
impact/api impact/changelog impact/cli status/2-code-review
Projects
None yet
Milestone
17.04.0
Development

Successfully merging this pull request may close these issues.

Add --read-only for Swarm Services
7 participants
@yongtang @LK4D4 @vieux @thaJeztah @cpuguy83 @AkihiroSuda @GordonTheTurtle
0