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

revert commits link to mount API over bind changes #12141

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

glours
Copy link
Contributor

@glours glours commented Sep 20, 2024

What I did
I reverted the following commits:

Related issue

fixes #12139 #12140

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

@glours glours self-assigned this Sep 20, 2024
Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

👍
This experiment demonstrate Mount and Bind API are not equivalent, as mount require client to know about engine constraints (typically "must use either propagation mode "rslave" or "rshared" when mount source is within the daemon root").
compse-go makes the assumption a bind can be safely expanded in to a mount, so the "long syntax", but actually this is wrong. We will have to manage this in compose-go in a better way, to really distinguish user intent to use mount-specific options. Maybe we should introduce a mount section and deprecate use of advanced volume attributes (like subpath) which are only supported by the mount API, to fully align with the docker CLI

@ndeloof ndeloof merged commit 9c60fe6 into docker:main Sep 20, 2024
28 checks passed
@thaJeztah
Copy link
Member

@ndeloof IIUC this worked on a bare Linux machine, but failed with desktop? Do we know what Desktop did that made it fail?

@prashant-shahi
Copy link

The previous release failed in my Linux machine.

@thaJeztah
Copy link
Member

@prashant-shahi Yes; the long story was that;

  1. f9c7a0c switched to using a different API (to replace a legacy API)
  2. This caused an issue with Docker Desktop, which had some code-path related to mapping host <--> VM mounts that got triggered if no bind-propagation was set cb00aaa
  3. Which caused an issue for native Windows containers, because windows has no concept of "bind propagation"; so it was made conditional c16df17

Step 2. chose to use rprivate, as it's the default that would be set by the daemon, so the assumption was that setting the same default on the client-side (compose) would be OK, but that default was not always applicable; a special exception existed on the daemon side to use rshared as a default when bind-mounting Docker's own storage location (/var/lib/docker by default). That special case is NOT handled when the user ("compose" in this case) explicitly sets a propagation mode. It's also quite likely that exception was never documented (other than "in code"), and bind-mounting /var/lib/docker is generally not recommended and more a niche use-case (at least compose did not have tests for this). But for those that did use this, an error was now produced by the daemon, which has a safeguard in place if an incorrect propagation is set;

Error response from daemon: invalid mount config: must use either propagation mode "rslave" or "rshared" when mount source is within the daemon root, daemon root: "/var/lib/docker", bind mount source: "/", propagation: "rprivate"

This PR reverts all of the above, pending changes in Docker Desktop to allow this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to bind mount when mount source is within the daemon root
4 participants