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

Flags for specifying bind mount consistency #31047

Merged
merged 1 commit into from Mar 13, 2017

Conversation

@yallop
Contributor

yallop commented Feb 15, 2017

This PR introduces a small change to the Docker CLI as part of a larger project to address some longstanding file sharing performance on Docker for Mac ([1], [2]).

The poor performance of bind mounts in Docker for Mac for some applications arises from osxfs ensuring perfectly consistent views of bind mounts between container and host: reads, writes and events from within a container are propagated synchronously to the host, and vice versa.

Perfect consistency is the right default, but for some workloads where it's unnecessary it puts a low ceiling on performance. To improve performance for such workloads, we plan to allow the user to relax consistency guarantees for individual bind-mounted directories.

The full details of the proposal are available here. This patch includes one small piece of the work, namely additional bind options cached, delegated, consistent, for selecting consistency modes. Although the immediate aim is improving osxfs performance, these options are not macOS-specific; they give a general view of consistency that applies to many systems, both for bind mounts and volume plugins. For example,

  • essentially the same consistency taxonomy is used by Infinit
  • the same approach may be used in future to improve performance on Windows
  • on systems that directly expose the host file system to a container, such as Linux, ignoring the options is an acceptable implementation of all three modes. (Even on Linux a container runtime could in principle take advantage of the relaxed semantics associated with delegated and cached.)
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Feb 16, 2017

Member

Can we add these flags to --mount (opts/mount.go) as well?

Member

AkihiroSuda commented Feb 16, 2017

Can we add these flags to --mount (opts/mount.go) as well?

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop
Contributor

yallop commented Feb 16, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 16, 2017

Member

Design LGTM

Member

dnephin commented Feb 16, 2017

Design LGTM

Show outdated Hide outdated api/types/mount/mount.go Outdated
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor

While I definitely appreciate the goal of making osxfs faster (directly affects me!) this does not seem right.
This is adding an API only change and nothing in Docker would even be using it.

Additionally it seems like we're pushing osxfs performance issues to the user to deal with.
Suddenly we'll see compose files everywhere with cached or delegated added to them just so performance doesn't suck on OSX.

Contributor

cpuguy83 commented Feb 17, 2017

While I definitely appreciate the goal of making osxfs faster (directly affects me!) this does not seem right.
This is adding an API only change and nothing in Docker would even be using it.

Additionally it seems like we're pushing osxfs performance issues to the user to deal with.
Suddenly we'll see compose files everywhere with cached or delegated added to them just so performance doesn't suck on OSX.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Feb 17, 2017

Contributor

Additionally it seems like we're pushing osxfs performance issues to the user to deal with.

Improving performance on Docker for Mac is an immediate goal of this introducing this feature now, but there's nothing specific to osxfs in the design.

We're allowing the user to describe the consistency requirements of the mount, because that's information that only the user has, and because Docker can make use of that information to improve performance.

The interface currently has no way to enable caching because it's been designed with the Linux implementation in mind. If we want Docker to perform well on other systems then we need to adapt the interface so that it's less Linux-specific.

Suddenly we'll see compose files everywhere with cached or delegated added to them just so performance doesn't suck on OSX.

We can certainly hope that people start to use the feature. If we see cached or delegated in compose files it's because those are the consistency levels required by the application.

Contributor

yallop commented Feb 17, 2017

Additionally it seems like we're pushing osxfs performance issues to the user to deal with.

Improving performance on Docker for Mac is an immediate goal of this introducing this feature now, but there's nothing specific to osxfs in the design.

We're allowing the user to describe the consistency requirements of the mount, because that's information that only the user has, and because Docker can make use of that information to improve performance.

The interface currently has no way to enable caching because it's been designed with the Linux implementation in mind. If we want Docker to perform well on other systems then we need to adapt the interface so that it's less Linux-specific.

Suddenly we'll see compose files everywhere with cached or delegated added to them just so performance doesn't suck on OSX.

We can certainly hope that people start to use the feature. If we see cached or delegated in compose files it's because those are the consistency levels required by the application.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Feb 20, 2017

Contributor

@dnephin, did the label (rebuild/experimental) trigger an experimental rebuild? I'm not sure whether the test failure represents a genuine problem.

Contributor

yallop commented Feb 20, 2017

@dnephin, did the label (rebuild/experimental) trigger an experimental rebuild? I'm not sure whether the test failure represents a genuine problem.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Feb 20, 2017

Contributor

I just retriggered the experimental rebuild, we were having some issues, I don't think it was triggered as the label was not automatically removed.

Contributor

justincormack commented Feb 20, 2017

I just retriggered the experimental rebuild, we were having some issues, I don't think it was triggered as the label was not automatically removed.

@thaJeztah thaJeztah added this to backlog in maintainers-session Feb 21, 2017

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Feb 22, 2017

Contributor

There's not a lot of activity on this issue, perhaps because it's not clear to the casual reader how critical this patch is to the success of Docker on macOS.

Here's an account posted on Reddit yesterday of how one company gave up on Docker:

This is our primary blocker for not using docker in our infrastructure right now. [...]
Docker is basically a no go at our company because of this issue. (Entire office uses MBP's)
There are some workarounds using rsync or something but it just seems like too much complexity.
I actually ended up spending a ton of time dockerizing our stack only to run into this brick wall. It was devistating.

This PR allows us to address the company's major concern. And this is far from an isolated case; it would be easy to give many more such examples.

Contributor

yallop commented Feb 22, 2017

There's not a lot of activity on this issue, perhaps because it's not clear to the casual reader how critical this patch is to the success of Docker on macOS.

Here's an account posted on Reddit yesterday of how one company gave up on Docker:

This is our primary blocker for not using docker in our infrastructure right now. [...]
Docker is basically a no go at our company because of this issue. (Entire office uses MBP's)
There are some workarounds using rsync or something but it just seems like too much complexity.
I actually ended up spending a ton of time dockerizing our stack only to run into this brick wall. It was devistating.

This PR allows us to address the company's major concern. And this is far from an isolated case; it would be easy to give many more such examples.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 22, 2017

Contributor

@yallop I get it, I really do... but again this is not doing anything but changing the API. Docker is not consuming it any way. Major red flag here.

Why not at least provide a switch to change the default behavior in d4mac preferences? This could be done either globally or per share.
If there's still an issue after such an option is available it can be addressed accordingly.

Contributor

cpuguy83 commented Feb 22, 2017

@yallop I get it, I really do... but again this is not doing anything but changing the API. Docker is not consuming it any way. Major red flag here.

Why not at least provide a switch to change the default behavior in d4mac preferences? This could be done either globally or per share.
If there's still an issue after such an option is available it can be addressed accordingly.

@nathanleclaire

This comment has been minimized.

Show comment
Hide comment
@nathanleclaire

nathanleclaire Feb 23, 2017

Contributor

I applaud the general effort to improve shared folder performance in Docker for Mac but I share @cpuguy83's concerns that this is adding something to the Docker API which does not get used by the Docker daemon at all.

Why would this not be added as a configurable setting in Docker for Mac? It seems to be very Docker for Mac specific. The Docker daemon is designed to run on Linux and Windows, but this adds a parameter which will be ignored by both.

Contributor

nathanleclaire commented Feb 23, 2017

I applaud the general effort to improve shared folder performance in Docker for Mac but I share @cpuguy83's concerns that this is adding something to the Docker API which does not get used by the Docker daemon at all.

Why would this not be added as a configurable setting in Docker for Mac? It seems to be very Docker for Mac specific. The Docker daemon is designed to run on Linux and Windows, but this adds a parameter which will be ignored by both.

@dsheets

This comment has been minimized.

Show comment
Hide comment
@dsheets

dsheets Feb 23, 2017

Contributor

@nathanleclaire while Docker for Mac is the first use case for these semantics, they are applicable to Infinit, Docker for Windows, volume plugins, and potentially Docker on Linux with a future extension point (delegated can effectively disable disk flushes during container run-time).

Contributor

dsheets commented Feb 23, 2017

@nathanleclaire while Docker for Mac is the first use case for these semantics, they are applicable to Infinit, Docker for Windows, volume plugins, and potentially Docker on Linux with a future extension point (delegated can effectively disable disk flushes during container run-time).

Show outdated Hide outdated api/types/mount/mount.go Outdated
Show outdated Hide outdated api/types/mount/mount.go Outdated
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 23, 2017

Contributor

Just for the record, I was wooed and am ok with this change.

Contributor

cpuguy83 commented Feb 23, 2017

Just for the record, I was wooed and am ok with this change.

@dsheets

This comment has been minimized.

Show comment
Hide comment
@dsheets

dsheets Feb 24, 2017

Contributor

@cpuguy83 we're open to changing the name of the field and the names of the modes. We initially selected state and [consistent,cached,delegated,default] by evaluating three criteria:

  1. -v comprehensibility
  2. --mount comprehensibility
  3. verbalization

The cases considered look like:

-v /a:/a:consistent pronounced "the mount is consistent"
-v /a:/a:cached pronounced "the mount is cached"
-v /a:/a:delegated pronounced "the mount is delegated"
--mount type=bind,src=/a,dst=/a,state=consistent pronounced "the mount's state is consistent"
--mount type=bind,src=/a,dst=/a,state=default pronounced "the mount's state is default"
--mount type=bind,src=/a,dst=/a,state=cached pronounced "the mount's state is cached"
--mount type=bind,src=/a,dst=/a,state=delegated pronounced "the mount's state is delegated"

I think you are proposing to change consistent to synchronous or full and state to consistency? I understand that state seems somewhat ambiguous about whether it is referring to the state of the mount concept or the underlying file system. I think it is unlikely to collide with future changes that do not interact with this specification as state is too general for any Docker mount concept and only makes sense in reference to the state of the file system itself.

I don't think full will work due to the case -v /a:/a:full which is not explanatory.

-v /a:/a:synchronous makes more sense but is longer, contains non-phonetic letters, and describes the implementation rather than the user's obervable experience. We are fine with naming the mode this if these trade-offs are considered and deemed acceptable. sync wouldn't have as many of these downsides but still describes implementation rather than behavior.

Renaming state to consistency would result in:

--mount type=bind,src=/a,dst=/a,consistency=consistent pronounced "the mount's consistency is consistent"
--mount type=bind,src=/a,dst=/a,consistency=default pronounced "the mount's consistency is default"
--mount type=bind,src=/a,dst=/a,consistency=cached pronounced "the mount's consistency is cached"
--mount type=bind,src=/a,dst=/a,consistency=delegated pronounced "the mount's consistency is delegated"

consistency is significantly longer than state, though arguably more descriptive. I don't think the verbalizations read as smoothly as state but they are still understandable.

We also believe that the terms used should be the same between -v and --mount and any future uses. In discussion with Ian Campbell, he suggested that state is ambiguous with some existing flags like ro which was undesirable. I know that the design of ro and rw are themselves in question as they are anonymous booleans. I think it could make sense to unify them with the state field to produce arguments like

--mount type=bind,src=/a,dst=/a,state=cached+ro pronounced "the mount's state is cached and read-only"

Ultimately, this all comes down to various comprehensibility and ease-of-use trade-offs in the design space. We think that the current names are the best of those we considered during design (e.g. state was initially semantics!) but we're happy to explore other options in a framework that takes the whole UX into account.

What issues do you foresee with the current set of names?

Contributor

dsheets commented Feb 24, 2017

@cpuguy83 we're open to changing the name of the field and the names of the modes. We initially selected state and [consistent,cached,delegated,default] by evaluating three criteria:

  1. -v comprehensibility
  2. --mount comprehensibility
  3. verbalization

The cases considered look like:

-v /a:/a:consistent pronounced "the mount is consistent"
-v /a:/a:cached pronounced "the mount is cached"
-v /a:/a:delegated pronounced "the mount is delegated"
--mount type=bind,src=/a,dst=/a,state=consistent pronounced "the mount's state is consistent"
--mount type=bind,src=/a,dst=/a,state=default pronounced "the mount's state is default"
--mount type=bind,src=/a,dst=/a,state=cached pronounced "the mount's state is cached"
--mount type=bind,src=/a,dst=/a,state=delegated pronounced "the mount's state is delegated"

I think you are proposing to change consistent to synchronous or full and state to consistency? I understand that state seems somewhat ambiguous about whether it is referring to the state of the mount concept or the underlying file system. I think it is unlikely to collide with future changes that do not interact with this specification as state is too general for any Docker mount concept and only makes sense in reference to the state of the file system itself.

I don't think full will work due to the case -v /a:/a:full which is not explanatory.

-v /a:/a:synchronous makes more sense but is longer, contains non-phonetic letters, and describes the implementation rather than the user's obervable experience. We are fine with naming the mode this if these trade-offs are considered and deemed acceptable. sync wouldn't have as many of these downsides but still describes implementation rather than behavior.

Renaming state to consistency would result in:

--mount type=bind,src=/a,dst=/a,consistency=consistent pronounced "the mount's consistency is consistent"
--mount type=bind,src=/a,dst=/a,consistency=default pronounced "the mount's consistency is default"
--mount type=bind,src=/a,dst=/a,consistency=cached pronounced "the mount's consistency is cached"
--mount type=bind,src=/a,dst=/a,consistency=delegated pronounced "the mount's consistency is delegated"

consistency is significantly longer than state, though arguably more descriptive. I don't think the verbalizations read as smoothly as state but they are still understandable.

We also believe that the terms used should be the same between -v and --mount and any future uses. In discussion with Ian Campbell, he suggested that state is ambiguous with some existing flags like ro which was undesirable. I know that the design of ro and rw are themselves in question as they are anonymous booleans. I think it could make sense to unify them with the state field to produce arguments like

--mount type=bind,src=/a,dst=/a,state=cached+ro pronounced "the mount's state is cached and read-only"

Ultimately, this all comes down to various comprehensibility and ease-of-use trade-offs in the design space. We think that the current names are the best of those we considered during design (e.g. state was initially semantics!) but we're happy to explore other options in a framework that takes the whole UX into account.

What issues do you foresee with the current set of names?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Feb 24, 2017

Member

I think state is too generic. consistency makes the setting more clear.

Member

dnephin commented Feb 24, 2017

I think state is too generic. consistency makes the setting more clear.

@dnephin

LGTM

Show outdated Hide outdated volume/volume_unix.go Outdated
@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Mar 11, 2017

Contributor
Contributor

justincormack commented Mar 11, 2017

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Mar 11, 2017

Contributor

I've pushed docs to #31749.

Contributor

yallop commented Mar 11, 2017

I've pushed docs to #31749.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 13, 2017

Member

Thanks for doing the docs PR; let's go ahead and merge this

Member

thaJeztah commented Mar 13, 2017

Thanks for doing the docs PR; let's go ahead and merge this

@thaJeztah thaJeztah merged commit 7f6e708 into moby:master Mar 13, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31213 has succeeded
Details
janky Jenkins build Docker-PRs 39829 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10892 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Mar 13, 2017

@yallop yallop deleted the yallop:cli-v-state branch Mar 13, 2017

@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks

mistyhacks Mar 17, 2017

Contributor

This seems to be missing CLI docs. @thaJeztah PTAL

Contributor

mistyhacks commented Mar 17, 2017

This seems to be missing CLI docs. @thaJeztah PTAL

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 17, 2017

Member

@mstanleyjones see #31749

Member

thaJeztah commented Mar 17, 2017

@mstanleyjones see #31749

@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks

mistyhacks Mar 17, 2017

Contributor

Wow! Thanks.

Contributor

mistyhacks commented Mar 17, 2017

Wow! Thanks.

vdemeester added a commit that referenced this pull request Mar 22, 2017

Merge pull request #31749 from yallop/docs-31047
Add documentation for bind mount consistency flags (#31047).

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Apr 3, 2017

Merge pull request moby#31749 from yallop/docs-31047
Add documentation for bind mount consistency flags (moby#31047).
(cherry picked from commit 9439402)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@andrerom andrerom referenced this pull request Apr 18, 2017

Closed

[WIP] Update to use Docker 17.04 for improved performance #173

1 of 4 tasks complete

@yallop yallop referenced this pull request Apr 18, 2017

Merged

Osxfs caching updates #2883

@masterful masterful referenced this pull request May 3, 2017

Closed

Plan 0.3.6 #329

@dsheets dsheets referenced this pull request May 5, 2017

Open

Mount Point Plugins #33048

@zikphil

This comment has been minimized.

Show comment
Hide comment
@zikphil

zikphil Jun 8, 2017

Just wanted to thank the Moby team for integrating those changes. 👍

zikphil commented Jun 8, 2017

Just wanted to thank the Moby team for integrating those changes. 👍

Source string `json:",omitempty"`
Target string `json:",omitempty"`
ReadOnly bool `json:",omitempty"`
Consistency Consistency `json:",omitempty"`

This comment has been minimized.

@stevvooe

stevvooe Jul 7, 2017

Contributor

Any reason this didn't land on BindOptions? This seems to only apply to bind mounts.

@stevvooe

stevvooe Jul 7, 2017

Contributor

Any reason this didn't land on BindOptions? This seems to only apply to bind mounts.

mattolenik pushed a commit to mattolenik/vagrant that referenced this pull request May 9, 2018

Matthew Olenik
Support Docker volume consistency for synced folders
Adds the `docker_consistency` option, which sets the Docker volume
consistency level. This can be used to greatly improved synced folder
performance, especially on macOS.

See for details: moby/moby#31047

briancain added a commit to briancain/vagrant that referenced this pull request Jul 27, 2018

Support Docker volume consistency for synced folders
Adds the `docker_consistency` option, which sets the Docker volume
consistency level. This can be used to greatly improved synced folder
performance, especially on macOS.

See for details: moby/moby#31047

briancain added a commit to briancain/vagrant that referenced this pull request Jul 27, 2018

Support Docker volume consistency for synced folders
Adds the `docker_consistency` option, which sets the Docker volume
consistency level. This can be used to greatly improved synced folder
performance, especially on macOS.

See for details: moby/moby#31047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment