Skip to content

Conversation

@adshmh
Copy link
Contributor

@adshmh adshmh commented Mar 28, 2018

This PR adds the ability to handle exec option on tmpfs mounts.

Notes:

This is the first draft. As discussed here: #32131 (comment), I have included the changes on docker/swarmkit types that will need vendoring. I will submit PR(s) on docker/swarmkit and docker/cli once 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)

@adshmh
Copy link
Contributor Author

adshmh commented Mar 28, 2018

I think the integration test in integration/container/mounts_linux_test.go would read better using assertions, but followed the existing style on the file.

I can add a commit to use assertions for all the tests in integration/container/mounts_linux_test.go if that is a better option.

@justincormack
Copy link
Contributor

needs a rebase

@cpuguy83
Copy link
Member

Is changing tmpfs to use noexec by default breaking?

@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from c77ba50 to 5349017 Compare April 11, 2018 20:52
@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from 5349017 to 4af5dc6 Compare April 12, 2018 13:01
@kolyshkin
Copy link
Contributor

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 exec and noexec, but maybe others that are valid for tmpfs (see man 8 mount and the comment in

// TODO(stevvooe): There are several more tmpfs flags, specified in the
).

We can add nr_inodes and mpol to the whitelist.

Options such as uid and gid might not work well in a cluster environment so they should probably not be added at this time.

Options nr_blocks and size should not be added, as they are already handled.

@adshmh would you wish to work on that?

@adshmh
Copy link
Contributor Author

adshmh commented Apr 19, 2018

@kolyshkin Sure, I can update the PR.

Is the following correct regarding the requirements?

  • Allow a more generic way of passing options to tmpfs (perhaps a string)
  • A white list for the allowed options
  • Currently allow: exec, noexec, nr_inodes and mpol

@thaJeztah
Copy link
Member

Allow a more generic way of passing options to tmpfs (perhaps a string)

From a CLI /UX perspective, this could use the same as docker volume create --opt foo=bar --opt bar=baz

A white list for the allowed options

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.

nr_inodes and mpol

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 ?

@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from 4af5dc6 to b73cea5 Compare May 14, 2018 21:25
@codecov
Copy link

codecov bot commented May 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@5c152ea). Click here to learn what that means.
The diff coverage is 86.66%.

@@            Coverage Diff            @@
##             master   #36720   +/-   ##
=========================================
  Coverage          ?   36.61%           
=========================================
  Files             ?      608           
  Lines             ?    45055           
  Branches          ?        0           
=========================================
  Hits              ?    16496           
  Misses            ?    26279           
  Partials          ?     2280

@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from b73cea5 to fec9954 Compare May 15, 2018 01:58
Copy link
Member

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

Copy link
Contributor

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?

@thaJeztah
Copy link
Member

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

@thaJeztah
Copy link
Member

This also needs a PR in swarmkit to make the required changes there

@adshmh
Copy link
Contributor Author

adshmh commented May 17, 2018

Thank you for the review. I will submit PRs on swarmkit and docker/cli.

@olljanat
Copy link
Contributor

@adshmh this one needs to be rebased

@derek derek bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018
@adshmh
Copy link
Contributor Author

adshmh commented Dec 28, 2018

@olljanat thanks for the reminder. I will rebase the PR.

@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from 4065413 to 59c69f7 Compare December 29, 2018 03:53
@derek derek bot added rebuild/z and removed status/failing-ci Indicates that the PR in its current state fails the test suite rebuild/z status/needs-vendoring labels Jan 1, 2019
@olljanat
Copy link
Contributor

olljanat commented Jan 12, 2019

reserved for my derek commands)

Copy link
Member

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?

moby/api/swagger.yaml

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 ?

Copy link
Contributor Author

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.

@adshmh adshmh force-pushed the add-exec-option-to-tmpfs branch from 59c69f7 to 403613a Compare February 28, 2019 21:26
@cpuguy83
Copy link
Member

cpuguy83 commented Mar 3, 2020

What's the status on this?

@hmh
Copy link

hmh commented Aug 18, 2020

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 ?

@AkihiroSuda
Copy link
Member

At least this needs to be rebased

adshmh added 2 commits March 4, 2021 05:36
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
@AkihiroSuda
Copy link
Member

Needs another rebase.
LGTM if CI green

@brunoais
Copy link

@adshmh Any updates?

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.

[feat] Add --mount tmpfs with exec support for vscode devcontainer Swarm mode, mount tmpfs, how to remove noexec?