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

engine: add recursive bind mounts #18669

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

dvdksn
Copy link
Collaborator

@dvdksn dvdksn commented Nov 14, 2023

Copy link

netlify bot commented Nov 14, 2023

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 2d55bac
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/655e03e5a54a7e0008c27f6b
😎 Deploy Preview https://deploy-preview-18669--docsdocker.netlify.app/storage/bind-mounts
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dvdksn dvdksn requested a review from thaJeztah November 14, 2023 13:49
@dvdksn dvdksn marked this pull request as ready for review November 14, 2023 13:50
@dvdksn dvdksn force-pushed the recursive-ro-bind-mounts branch from 8e78526 to 9f6852d Compare November 14, 2023 14:37
@dvdksn dvdksn force-pushed the recursive-ro-bind-mounts branch from 9f6852d to 53d7ffd Compare November 16, 2023 07:52
@dvdksn dvdksn requested a review from a team November 16, 2023 08:52
aevesdocker
aevesdocker previously approved these changes Nov 16, 2023
@dvdksn dvdksn added the status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. label Nov 16, 2023
@dvdksn dvdksn added this to the Engine v25.0.0 milestone Nov 16, 2023

`bind-recursive=readonly`
: Enables recursive bind-mounts.
Submounts are read-only.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should mention that this produces an error if the kernel or doesn't support this feature (kernel versions older than v5.12).

Comment on lines +277 to +279
read-only mounts. Recursive read-only mounts require Linux kernel version 5.12
or later. If you're running an older kernel version, submounts are
Copy link
Member

Choose a reason for hiding this comment

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

Actually wondering now; what do we do for native Windows containers, @AkihiroSuda ? Do we produce an error, or do we silently ignore the options (or a warning?)

Is there an equivalent of submounts on Windows? (and are they inherited?) 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Likely to be ignored

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; wondering if Windows in any form inherits nested mounts. I can try asking around for someone to try.

If it doesn't, then ignoring is probably fine (we could print a warning in some cases I guess, but more complicated on docker run, because we can't mix those with the container's own output)

content/storage/bind-mounts.md Outdated Show resolved Hide resolved
content/storage/bind-mounts.md Show resolved Hide resolved
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM! But leaving a "request changes" as we need to wait for v25.0 to be released before we publish 😅

@thaJeztah thaJeztah removed the status/do-not-merge Pull requests that are awaiting some event or decision before they can be merged. label Jan 19, 2024
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 8a7d8e3 into docker:main Jan 19, 2024
8 checks passed
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.

4 participants