Skip to content
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.

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

Inconsistent --tmpfs behavior #22438

Merged
merged 1 commit into from May 27, 2016

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented May 1, 2016

- What I did

This fix tries to address the issue raised in #22420. When --tmpfs is specified with /tmp, the default value is rw,nosuid,nodev,noexec,relatime,size=65536k. When --tmpfs is specified with /tmp:rw, then the value changed to rw,nosuid,nodev,noexec,relatime.

The reason for such an inconsistency is because docker tries to add size=65536k option only when user provides no option.

- How I did it

This fix tries to address this issue by always pre-probating size=65536k along with rw,nosuid,nodev,noexec,relatime. If user provides a different value (e.g., size=8192k), it will override the size=65536k anyway.

- How to verify it

Additional test cases have been added to cover the changes in this fix.

- Description for the changelog

Address the inconsistent --tmpfs behavior by so that size=65536k will always be the default value.

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

This fix fixes #22420.

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

@@ -463,11 +463,9 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c
}

if m.Source == "tmpfs" {
opt := []string{"noexec", "nosuid", "nodev", volume.DefaultPropagationMode}
opt := []string{"noexec", "nosuid", "nodev", volume.DefaultPropagationMode, "size=65536k"}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test for specifying a custom propagation mode as well?
Also wondering how deterministic the behavior of having duplicate keys is.

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 @cpuguy83 I just added a test for custom propagation mode. Since busybox does not have the findmnt I used debian:jessie so that

 findmnt -o TARGET,PROPAGATION /tmp

could be specified.

I also take a look at the mount command:
http://man7.org/linux/man-pages/man8/mount.8.html

The usual behavior is that the last option wins if there are conflicting ones.

So I think it should be ok to having duplicate options as long as the last one is the one we want.

Copy link
Member

Choose a reason for hiding this comment

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

The "usual" part bothers me a bit, it sounds like something that may be different between distros. So perhaps we should explicitly merge user options with the defaults?

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 I just updated the PR to merge the duplicate options. Please let me know if there are any issues.

@yongtang yongtang force-pushed the 22420-inconsistent-tmpfs-behavior branch 2 times, most recently from 14f936d to 068b0c3 Compare May 3, 2016 15:39
@yongtang
Copy link
Member Author

@thaJeztah @cpuguy83 The removal of duplicate options was handled by MergeTmpfsOptions() in pkg/mount/flags.go of the updated PR. Please take a look and let me know if there are any issues.

@thaJeztah
Copy link
Member

Thanks @yongtang, I tested this, and it works correctly. I was a bit unsure if we needed the level of validation that we have now in MergeTmpfsOptions (or delegate to Linux and capture errors in case invalid options are specified, if possible), but perhaps it's good to do.

LGTM

ping @cpuguy83 ptal

@@ -463,13 +463,17 @@ func setMounts(daemon *Daemon, s *specs.Spec, c *container.Container, mounts []c
}

if m.Source == "tmpfs" {
opt := []string{"noexec", "nosuid", "nodev", volume.DefaultPropagationMode}
options := "noexec,nosuid,nodev," + volume.DefaultPropagationMode + ",size=65536k"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand point of using string here if you're immediately splitting it in MergeTmpfsOptions

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 @LK4D4. The pull request has been updated to use []string directly.

@yongtang yongtang force-pushed the 22420-inconsistent-tmpfs-behavior branch 2 times, most recently from d10ceed to 6f5e6ba Compare May 24, 2016 02:24
@yongtang
Copy link
Member Author

Thanks for the review @LK4D4. The pull request has been updated to cover the raised issues. Please let me know if there are any other issues.

if f, ok := flags[option]; ok && f.flag != 0 {
// There is only one propagation mode
key := f.flag
if _, ok := propagationFlags[option]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

if propagationFlags[option]

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR. Thanks @LK4D4.

@yongtang yongtang force-pushed the 22420-inconsistent-tmpfs-behavior branch from 6f5e6ba to a96188a Compare May 26, 2016 14:11
@yongtang
Copy link
Member Author

Thanks @LK4D4 for the review. I updated the pull request and covered most of the issues.

For the defaults case, I noticed that in Docker 1.11.1 this option is treated as an error case.

Let me know if we want to make the change for defaults (as the behavior will be slightly different) and I will update the pull request accordingly.

}
continue
}
opt := strings.SplitN(option, "=", 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a comment here about "defaults", it has f.flag == 0, so this func will return error.

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 @LK4D4. I had some confusion initially. The pull request has been updated.

This fix tries to address the issue raised in moby#22420. When
`--tmpfs` is specified with `/tmp`, the default value is
`rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs`
is specified with `/tmp:rw`, then the value changed to
`rw,nosuid,nodev,noexec,relatime`.

The reason for such an inconsistency is because docker tries
to add `size=65536k` option only when user provides no option.

This fix tries to address this issue by always pre-progating
`size=65536k` along with `rw,nosuid,nodev,noexec,relatime`.
If user provides a different value (e.g., `size=8192k`), it
will override the `size=65536k` anyway since the combined
options will be parsed and merged to remove any duplicates.

Additional test cases have been added to cover the changes
in this fix.

This fix fixes moby#22420.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 22420-inconsistent-tmpfs-behavior branch from a96188a to 397a6fe Compare May 27, 2016 02:28
@yongtang
Copy link
Member Author

Thanks @LK4D4 the pull request has been updated. Please let me know if there are any other issues.

@LK4D4
Copy link
Contributor

LK4D4 commented May 27, 2016

LGTM
janky failure is unrelated
Thanks @yongtang !

@cpuguy83
Copy link
Member

LGTM

@cpuguy83 cpuguy83 merged commit ec3ccde into moby:master May 27, 2016
@yongtang yongtang deleted the 22420-inconsistent-tmpfs-behavior branch May 27, 2016 19:57
@thaJeztah
Copy link
Member

🎉 thank you so much @yongtang!

@thaJeztah thaJeztah added this to the 1.12.0 milestone May 30, 2016
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
This fix tries to address the issue raised in moby#22420. When
`--tmpfs` is specified with `/tmp`, the default value is
`rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs`
is specified with `/tmp:rw`, then the value changed to
`rw,nosuid,nodev,noexec,relatime`.

The reason for such an inconsistency is because docker tries
to add `size=65536k` option only when user provides no option.

This fix tries to address this issue by always pre-progating
`size=65536k` along with `rw,nosuid,nodev,noexec,relatime`.
If user provides a different value (e.g., `size=8192k`), it
will override the `size=65536k` anyway since the combined
options will be parsed and merged to remove any duplicates.

Additional test cases have been added to cover the changes
in this fix.

This fix fixes moby#22420.

Link: moby#22438

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: zhaolongquan <zhaolongquan@huawei.com>

Conflicts:
	daemon/oci_linux.go
	integration-cli/docker_cli_run_unix_test.go
liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017
Inconsistent --tmpfs behavior

This fix tries to address the issue raised in moby#22420. When
`--tmpfs` is specified with `/tmp`, the default value is
`rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs`
is specified with `/tmp:rw`, then the value changed to
`rw,nosuid,nodev,noexec,relatime`.

The reason for such an inconsistency is because docker tries
to add `size=65536k` option only when user provides no option.

This fix tries to address this issue by always pre-progating
`size=65536k` along with `rw,nosuid,nodev,noexec,relatime`.
If user provides a different value (e.g., `size=8192k`), it
will override the `size=65536k` anyway since the combined
options will be parsed and merged to remove any duplicates.

Additional test cases have been added to cover the changes
in this fix.

This fix fixes moby#22420.

Link: moby#22438

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: zhaolongquan <zhaolongquan@huawei.com>

Conflicts:
	daemon/oci_linux.go
	integration-cli/docker_cli_run_unix_test.go



See merge request docker/docker!643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent --tmpfs behavior
5 participants