Skip to content

/ gvisor

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

Jump to bottom

OCI spec: mount type is optional for bind mounts #2330

Open
fvoznika opened this issue Apr 2, 2020 · 5 comments
Open

OCI spec: mount type is optional for bind mounts #2330

fvoznika opened this issue Apr 2, 2020 · 5 comments
Labels
area: docker area: integration good first issue priority: p3 type: bug

Comments

@fvoznika
Copy link
Member

@fvoznika fvoznika commented Apr 2, 2020

the rule for type field in mount is strange. If option is bind, then type is optional. In runsc today type is required.

type (string, OPTIONAL) The type of the filesystem to be mounted.
Linux: filesystem types supported by the kernel as listed in /proc/filesystems (e.g., "minix", "ext2", "ext3", "jfs", "xfs", "reiserfs", "msdos", "proc", "nfs", "iso9660"). For bind mounts (when options include either bind or rbind), the type is a dummy, often "none" (not listed in /proc/filesystems).
@ianlewis
Copy link
Contributor

@ianlewis ianlewis commented Apr 3, 2020

Does this cause problems when using bind mounts with Docker? or is this issue just for spec completeness?
@fvoznika
Copy link
Member Author

@fvoznika fvoznika commented Apr 3, 2020

Just completeness. Someone bumped into this crafting OCI spec by hand.
@moricho moricho mentioned this issue Apr 20, 2020
Merged
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@moricho @gvisor-bot
Make mount type optional for bind mounts
Loading status checks…
9368249 
## Description
This fixes behavior of `getMountNameAndOptions` func based on OCI runtime spec(https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-mounts).
In detail, if the mount type is equal to "none" or other dummy string and the mount option includes either "bind" or  "rbind",  `getMountNameAndOptions` behave as the mount type is equal to "bind".

## Relevant issue
Fix: #2330

Signed-off-by: moricho <ikeda.morito@gmail.com>
FUTURE_COPYBARA_INTEGRATE_REVIEW=#2487 from moricho:fix/bindmount fc53d64
PiperOrigin-RevId: 308637079
@copybara-service copybara-service bot mentioned this issue Apr 29, 2020
Merged
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@moricho @gvisor-bot
Make mount type optional for bind mounts
Loading status checks…
3b23d16 
## Description
This fixes behavior of `getMountNameAndOptions` func based on OCI runtime spec(https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-mounts).
In detail, if the mount type is equal to "none" or other dummy string and the mount option includes either "bind" or  "rbind",  `getMountNameAndOptions` behave as the mount type is equal to "bind".

## Relevant issue
Fix: #2330

Signed-off-by: moricho <ikeda.morito@gmail.com>
FUTURE_COPYBARA_INTEGRATE_REVIEW=#2487 from moricho:fix/bindmount fc53d64
PiperOrigin-RevId: 308637079
copybara-service bot pushed a commit that referenced this issue Apr 29, 2020
@moricho @gvisor-bot
Make mount type optional for bind mounts
Loading status checks…
80ff579 
## Description
This fixes behavior of `getMountNameAndOptions` func based on OCI runtime spec(https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-mounts).
In detail, if the mount type is equal to "none" or other dummy string and the mount option includes either "bind" or  "rbind",  `getMountNameAndOptions` behave as the mount type is equal to "bind".

## Relevant issue
Fix: #2330

Signed-off-by: moricho <ikeda.morito@gmail.com>
FUTURE_COPYBARA_INTEGRATE_REVIEW=#2487 from moricho:fix/bindmount fc53d64
PiperOrigin-RevId: 308637079
@fvoznika
Copy link
Member Author

@fvoznika fvoznika commented May 29, 2020

The fix had to be rolled back because it broken the handling for EmptyDir in Kubernetes. It preserves bind and rbind options when the mount is redirected to tmpfs. The fix made tmpfs be "wrongly" ignored in this case.
@fvoznika fvoznika reopened this May 29, 2020
@moricho
Copy link
Contributor

@moricho moricho commented May 29, 2020

@fvoznika

It preserves bind and rbind options when the mount is redirected to tmpfs. The fix made tmpfs be "wrongly" ignored in this case.

Does that means bind and rbind options should be ignored when mount type is equal to tmpfs? (not to be redirected to bind type)
@fvoznika
Copy link
Member Author

@fvoznika fvoznika commented May 29, 2020

It's better to fix emptydir handling in gvisor-containerd-shim to remove [r]bind option when converting the mounting to internal tmpfs. This way we don't need to special case tmpfs here.

google/gvisor-containerd-shim#61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: docker area: integration good first issue priority: p3 type: bug
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Make mount type optional for bind mounts
Make mount type optional for bind mounts
3 participants
@ianlewis @fvoznika @moricho
You can’t perform that action at this time.