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

[WIP] Volume mount paths compatibility on darwin #13453

Conversation

tricktron
Copy link
Contributor

CoreOS disallows mounting at /root because it is immutable.

As an alternative, it is recommended to mount at /mnt, e.g: podman machine init -v /Users:/mnt/Users on darwin. This, however, breaks compatibility with other tools, e.g. docker desktop, colima when specifying mounts:

\# docker-compose.yml
...
volumes:
    - ./app:/app
  • Works in colima and docker desktop.
  • Fails with podman run because it is missing the /mnt prefix.

As a result, I can't use podman and docker desktop interchangeably
with one single docker-compose.yml file.

This WIP tries to solve that with the following convention:

  • podman machine: every target mount path into coreOS is internally prefixed with
    /mnt so that it can be mounted.
  • podman run: every source mount path is internally prefixed with /mnt if you
    are on darwin.

Now the last sentence if youare on darwin may break a lot stuff, e.g what happens when using the podman remote client on linux with server vs. when using it with a linux vm. Since I am brand new to this project, it may very well be that this approach is heading into the wrong direction. But it at least works on my m1 Mac and may serve as a good starting point for a discussion for v4.1.

There are multiple approaches to solving this issue. I can currently think of 4:

  • Removing the immutability of / in the coreOS image. This allows mounting on root. Allow creation of immutable directories below root folder coreos/fedora-coreos-tracker#270
  • Workaround the immutability by first disabling it, mounting the volume and then enabling it again. Currently in master:
    // create mountpoint directory if it doesn't exist
    // because / is immutable, we have to monkey around with permissions
    // if we dont mount in /home or /mnt
    args := []string{"-q", "--"}
    if !strings.HasPrefix(mount.Target, "/home") || !strings.HasPrefix(mount.Target, "/mnt") {
    args = append(args, "sudo", "chattr", "-i", "/", ";")
    }
    args = append(args, "sudo", "mkdir", "-p", mount.Target)
    if !strings.HasPrefix(mount.Target, "/home") || !strings.HasPrefix(mount.Target, "/mnt") {
    args = append(args, ";", "sudo", "chattr", "+i", "/", ";")
    }
    err = v.SSH(name, machine.SSHOptions{Args: args})
  • Mounting under /mnt, then disabling the immutability using the above trick by @baude, then creating a symlink in root, then enabling immutability again.
  • This pr: using a convention.
  • ...

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 8, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tricktron
To complete the pull request process, please assign mtrmac after the PR has been reviewed.
You can assign the PR to them by writing /assign @mtrmac in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tricktron tricktron changed the title Volume mount paths compatibility on darwin [WIP] Volume mount paths compatibility on darwin Mar 8, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2022
@Luap99
Copy link
Member

Luap99 commented Mar 8, 2022

The remote client is generic and not just limited to podman machine. You cannot just change the path for all darwin installs. This will break a lot of things.
If the code from @baude works I see no reason why we would need this.

@tricktron tricktron force-pushed the volume-mount-paths-compatibility-on-darwin branch from 863fe99 to 3ad9e0a Compare March 8, 2022 13:11
@tricktron
Copy link
Contributor Author

@Luap99 I was developing that in parallel to @baude's solution and since it worked well enough I wanted it to share it here. According to the tests, it doesn't even break that much.

I lack the broader background to judge which solution is better.

But in my opinion it doesn't make sense to use an immutable / only to disable it for mounting and then reenabling it again.

But this is up to you and the community to decide, this are just my 2 cents.

@baude
Copy link
Member

baude commented Mar 8, 2022

we are hoping to work with the fcos community to provide a better long-term solution. we kicked around some ideas the other day but never really came to a conclusion. @dustymabe @cgwalters here is a pr that cleans up some of what I did but nevertheless shows we need to figure this out ... i dont think i knew the other day that docker compose defaults to using / !!

@Luap99
Copy link
Member

Luap99 commented Mar 10, 2022

According to the tests, it doesn't even break that much.

Well only because macos/windows and podman machine are completely untested!

@tricktron
Copy link
Contributor Author

Well only because macos/windows and podman machine are completely untested!

Do you know a way to test this manually?

CoreOS disallows mounting at `/`root because it is
[immutable](https://docs.fedoraproject.org/en-US/fedora-coreos/storage/#_immutable_read_only_usr).

As an alternative, it is recommended to mount at `/mnt`, e.g:
`podman machine init -v /Users:/mnt/Users` on darwin. This, however,
breaks compatibility with other tools, e.g. docker desktop, colima when
specifying mounts:
```config
\# docker-compose.yml
...
volumes:
    - ./app:/app
```
- Works in colima and docker desktop.
- Fails with `podman run` because it is missing the `/mnt` prefix.

As a result, I can't use podman and docker desktop interchangeably
with one single `docker-compose.yml` file.

This WIP tries to solve that with the following convention:
- `podman machine:` every target mount path into coreOS is internally prefixed with
`/mnt` so that it can be mounted.
- `podman run`: every source mount path is internally prefixed with `/mnt` if you
are on darwin.

[NO NEW TESTS NEEDED]

Signed-off-by: Thibault Gagnaux <[email protected]>
@tricktron tricktron force-pushed the volume-mount-paths-compatibility-on-darwin branch from 3ad9e0a to 54d7552 Compare March 19, 2022 09:42
@tricktron
Copy link
Contributor Author

tricktron commented Mar 19, 2022

we are hoping to work with the fcos community to provide a better long-term solution. we kicked around some ideas the other day but never really came to a conclusion. @dustymabe @cgwalters here is a pr that cleans up some of what I did but nevertheless shows we need to figure this out ... i dont think i knew the other day that docker compose defaults to using / !!

@baude Any updates?

@baude
Copy link
Member

baude commented Mar 22, 2022

We have no updates to provide on that matter. We have a temporary fix in 4.0.2 for the Mac brew client. We have also decided to mount -v $HOME:$HOME as a default in the next release. It can be overriden, changed, and also you can opt out of any mounting.

@tricktron
Copy link
Contributor Author

@baude Ok, thanks for the info. Should I close this pr or keep it open?

@rhatdan
Copy link
Member

rhatdan commented Mar 23, 2022

I would say close this, then open a PR to add fields to containers/common, for default paths to volume mount into machines

machine_volumes=[ "$HOME:$HOME", "/tmp:/tmp:ro" ]

Supports both environment variables and paths.

@rhatdan rhatdan closed this Mar 23, 2022
@tricktron
Copy link
Contributor Author

tricktron commented Mar 23, 2022

@rhatdan This pr was an alternative solution to allow compatibility with docker-compose files without mounting at / because the immutability of it discourages it.
@baude already fixed that by just disabling the immutability when trying to mount under / then enabling it again. In my opinion, this is a more of a dirty hack so I presented my alternative solution here. Since his workaround is already in master and does not break things and my solution may break things according to @Luap99, I think we just close this here and I won't open a new pr.

@rhatdan
Copy link
Member

rhatdan commented Mar 23, 2022

We can move this to a discussion.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants