-
Notifications
You must be signed in to change notification settings - Fork 18.9k
Add exec option to tmpfs #36720
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
Add exec option to tmpfs #36720
Conversation
|
I think the integration test in I can add a commit to use assertions for all the tests in |
|
needs a rebase |
|
Is changing tmpfs to use noexec by default breaking? |
c77ba50 to
5349017
Compare
5349017 to
4af5dc6
Compare
|
I concur @thaJeztah idea to have a more generic way to pass options to tmpfs rather than implementing a specific option for exec. The options specified should be validated against a whitelist. We have to figure which options to allow but let's at least include Line 112 in 8bb5a28
We can add Options such as Options @adshmh would you wish to work on that? |
|
@kolyshkin Sure, I can update the PR. Is the following correct regarding the requirements?
|
From a CLI /UX perspective, this could use the same as
Yes; we discussed this, and generally it's easier to add more options later than removing an option (as that would be a breaking change), so for that reason, we prefer a whitelist.
I'm not against adding them, just wondering how common they are (i.e. should we add those now, or add them once we see a need); @kolyshkin ? |
4af5dc6 to
b73cea5
Compare
Codecov Report
@@ Coverage Diff @@
## master #36720 +/- ##
=========================================
Coverage ? 36.61%
=========================================
Files ? 608
Lines ? 45055
Branches ? 0
=========================================
Hits ? 16496
Misses ? 26279
Partials ? 2280 |
b73cea5 to
fec9954
Compare
api/types/mount/mount.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should store/pass these options as a plain string, or if we should split and use a map or array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just name this Options, this is in-line with the language used around mounts?
|
Discussing in the maintainers meeting with @kolyshkin @cpuguy83, and having these options as a string should be ok for now; no reason to make it more complicated. Moving this to code review |
|
This also needs a PR in swarmkit to make the required changes there |
|
Thank you for the review. I will submit PRs on swarmkit and docker/cli. |
fec9954 to
3c88f43
Compare
|
@adshmh this one needs to be rebased |
|
@olljanat thanks for the reminder. I will rebase the PR. |
4065413 to
59c69f7
Compare
|
reserved for my derek commands) |
api/types/mount/mount.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to the Swagger.yml as well, and to the API history https://github.com/moby/moby/blob/b4842cfe88b39af42416429396aee10d1fe7c3d6/docs/api/version-history.md#v140-api-changes?
Lines 297 to 307 in b4842cf
| TmpfsOptions: | |
| description: "Optional configuration for the `tmpfs` type." | |
| type: "object" | |
| properties: | |
| SizeBytes: | |
| description: "The size for the tmpfs mount in bytes." | |
| type: "integer" | |
| format: "int64" | |
| Mode: | |
| description: "The permission mode for the tmpfs mount in an integer." | |
| type: "integer" |
@kolyshkin did we want this to be a []string or map[string]string ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I updated the swagger.yaml and the version-history.md files, assuming string for the Options field as I recall from previous discussions. If a map[string]string is to be used instead, please let me know.
59c69f7 to
403613a
Compare
|
What's the status on this? |
|
Using docker for CI/CD that requires lots of unpacking, build, throw-away is about 10 times faster with proper use of tmpfs, and this almost always requires a tmpfs mounted with -o exec. What is needed to get this thing out of the door? I mean, it has been what, two years that this is languishing ? |
|
At least this needs to be rebased |
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
403613a to
ccc9577
Compare
|
Needs another rebase. |
|
@adshmh Any updates? |
Rebase #36720 "Add exec option to tmpfs"
This PR adds the ability to handle exec option on tmpfs mounts.
--mounttmpfs withexecsupport for vscode devcontainer #45385Notes:
This is the first draft. As discussed here: #32131 (comment), I have included the changes on
docker/swarmkittypes that will need vendoring. I will submit PR(s) ondocker/swarmkitanddocker/clionce if this PR is approved.- What I did
Feature: allow specifying exec option for tmpfs mounts
- How I did it
Added the required code and tests to pkg/mount, volume, api/types, and daemon.
- How to verify it
I have added an integration test for verification.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)