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

MacOS improvements #13409

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Conversation

baude
Copy link
Member

@baude baude commented Mar 3, 2022

  • Enable support of virtfs in Podman and darwin. At the time of this writing, it requires a special patch not yet included in upstream qemu.
  • Prefer to use a specially built qemu to support virtfs. The qemu is installed under libexec/podman.

Signed-off-by: brent baude [email protected]

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 3, 2022
pkg/machine/qemu/machine.go Outdated Show resolved Hide resolved
pkg/machine/qemu/options_darwin.go Outdated Show resolved Hide resolved
UID int
}

// GetUID is helper function to get the userid that should be
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a GetGID?

Copy link
Member Author

Choose a reason for hiding this comment

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

i dont think we need it r/n, but we can in the future

pkg/machine/config.go Outdated Show resolved Hide resolved
pkg/machine/qemu/options_darwin.go Outdated Show resolved Hide resolved
@willcohen
Copy link

Output when applying the PR commit to the nixpkgs derivation:

✗ ~/.nix-profile/bin/podman machine start                                  
Starting machine "podman-machine-default"
INFO[0000] waiting for clients...                       
INFO[0000] new connection from  to /var/folders/pg/wl09ypf93rq84r1gc2g8ytsw0000gn/T/podman/qemu_podman-machine-default.sock 
Waiting for VM ...
INFO[0020] Socket forward established: /Users/wcohen/.local/share/containers/podman/machine/podman-machine-default/podman.sock to /run/user/501/podman/podman.sock 
Mounting volume... /Users/wcohen:/mnt/wcohen
time="2022-03-03T17:30:38-05:00" level=error msg="cannot find UID/GID for user core: No subuid ranges found for user \"core\" in /etc/subuid - check rootless mode in man pages."
time="2022-03-03T17:30:38-05:00" level=warning msg="Using rootless single mapping into the namespace. This might break some images. Check /etc/subuid and /etc/subgid for adding sub*ids if not using a network user"

@baude is this kind of error related to upstream or a nix quirk? Does it mean that mounting volumes is going to need to be rootful going forward if the UID is being passed through this way?

@zowoq is there a reason that the GetUID stuff added in this PR would be failing for nix alone?

@zowoq
Copy link

zowoq commented Mar 4, 2022

is there a reason that the GetUID stuff added in this PR would be failing for nix alone?

I wouldn't have thought there would be any nix specific problems with that.

@baude baude force-pushed the virtfsdarwin branch 4 times, most recently from cfc2cb1 to ac5bbf1 Compare March 5, 2022 03:33
@baude
Copy link
Member Author

baude commented Mar 5, 2022

Output when applying the PR commit to the nixpkgs derivation:

✗ ~/.nix-profile/bin/podman machine start                                  
Starting machine "podman-machine-default"
INFO[0000] waiting for clients...                       
INFO[0000] new connection from  to /var/folders/pg/wl09ypf93rq84r1gc2g8ytsw0000gn/T/podman/qemu_podman-machine-default.sock 
Waiting for VM ...
INFO[0020] Socket forward established: /Users/wcohen/.local/share/containers/podman/machine/podman-machine-default/podman.sock to /run/user/501/podman/podman.sock 
Mounting volume... /Users/wcohen:/mnt/wcohen
time="2022-03-03T17:30:38-05:00" level=error msg="cannot find UID/GID for user core: No subuid ranges found for user \"core\" in /etc/subuid - check rootless mode in man pages."
time="2022-03-03T17:30:38-05:00" level=warning msg="Using rootless single mapping into the namespace. This might break some images. Check /etc/subuid and /etc/subgid for adding sub*ids if not using a network user"

@baude is this kind of error related to upstream or a nix quirk? Does it mean that mounting volumes is going to need to be rootful going forward if the UID is being passed through this way?

@zowoq is there a reason that the GetUID stuff added in this PR would be failing for nix alone?

@willcohen that is now fixed ... it took me a bit to try and find root cause plus implement a fix.

pkg/machine/ignition.go Outdated Show resolved Hide resolved
@tricktron
Copy link
Contributor

Very nice work. I already used this in combination with the patched QEMU from #8016 (comment) to run a container with volume mounts and it worked like a charm:+1:.

The only unresolved issue is that I can't run containers built on X86 without first manually installing qemu-static as described in #12144 (comment).

@baude
Copy link
Member Author

baude commented Mar 5, 2022

for those playing along ... my testing environment is based on ...

$ brew uninstall podman
$ brew uninstall qemu
$ brew tap baude/podman
$ brew install baude/podman/podman

@baude baude force-pushed the virtfsdarwin branch 2 times, most recently from f88fd7e to 5520dce Compare March 5, 2022 22:02
@baude baude changed the title [WIP]MacOS improvements MacOS improvements Mar 5, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2022
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
@tricktron
Copy link
Contributor

tricktron commented Mar 6, 2022

Right now I mount my user dir the following way: podman machine init -v /Users:/mnt/Users:ro which works. But that has the disadvantage that I need to prefix all mount paths, e.g. in my docker-compose/podman-compose files with /mnt. E.g. I can't use the following in a docker compose:

volumes:
    - ./app:/app

because it is missing the /mnt prefix.

I tried to solve that by mounting my user dir at the root dir with podman machine init -v /Users:/Users:ro. However, that only works for the first time.
If I do the following:

podman machine init -v /Users:/Users:ro
podman machine start
# Now it works
podman machine stop
podman machine start
# fails with mkdir /Users Operation not permitted

It fails here:
https://github.com/containers/podman/blob/5520dce939c2c0f9af70cb64a93b058634970725/pkg/machine/qemu/machine.go#L478

What is the reason for this? Is the first connection always done using root and afterwards with the core user?
Does it even make sense to mount at the root dir? Could we alternatively create a symlink /Users -> /mnt/Users to solve that issue?

In the end, that would allow me to use one docker-compose file with docker-desktop, colima and podman.

@baude
Copy link
Member Author

baude commented Mar 6, 2022

Right now I mount my user dir the following way: podman machine init -v /Users:/mnt/Users:ro which works. But that has the disadvantage that I need to prefix all mount paths, e.g. in my docker-compose/podman-compose files with /mnt. E.g. I can't use the following in a docker compose:

volumes:
    - ./app:/app

because it is missing the /mnt prefix.

I tried to solve that by mounting my user dir at the root dir with podman machine init -v /Users:/Users:ro. However, that only works for the first time. If I do the following:

podman machine init -v /Users:/Users:ro
podman machine start
# Now it works
podman machine stop
podman machine start
# fails with mkdir /Users Operation not permitted

It fails here:

https://github.com/containers/podman/blob/5520dce939c2c0f9af70cb64a93b058634970725/pkg/machine/qemu/machine.go#L478

What is the reason for this? Is the first connection always done using root and afterwards with the core user? Does it even make sense to mount at the root dir? Could we alternatively create a symlink /Users -> /mnt/Users to solve that issue?

In the end, that would allow me to use one docker-compose file with docker-desktop, colima and podman.

Yes, this is something we need to discuss as a team tomorrow. While I work on the weekend, there is no expectation that the rest of the team do so. We have a couple of behaviors that we need to iron out. Moreover, we have only so much wiggle room in 4.0 because it has already been released. I expect this to be smoothed out by 4.1, but figured people would be happier if they had something that mounted.

@tricktron
Copy link
Contributor

@baude Thanks for the quick answer.

Just as an additional input: I just checked my colima installation and it seems like they mount /Users:/Users as a default when running colima start.

@baude
Copy link
Member Author

baude commented Mar 6, 2022

how does /home/core/host strike you?

@tricktron
Copy link
Contributor

@baude I like that. Does that solve the prefix issue though?

@LionsAd
Copy link

LionsAd commented Mar 6, 2022

Why not just check if the directory exists before doing the mkdir -p?

Or does that mean it's not successfully unmounted on stop?

@baude
Copy link
Member Author

baude commented Mar 7, 2022

Why not just check if the directory exists before doing the mkdir -p?

Or does that mean it's not successfully unmounted on stop?

@LionsAd I did not write that code. It was contributed by a wonderful member of the community. As Podman Architect, I need to understand and value the intent of the submission as well as keep into account what the Podman team wants to do ... and don't forget balancing also what the community wants. Like I have said earlier, we have very little wiggle room on how much we can do here in this version. We can do a lot more for Podman 4.1.

@rhatdan
Copy link
Member

rhatdan commented Mar 7, 2022

/lgtm
/hold
We can continue working on this for 4.1 but lets get this released ASAP.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022
@rhatdan
Copy link
Member

rhatdan commented Mar 7, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022
@baude
Copy link
Member Author

baude commented Mar 7, 2022

hold off on merging this ... need to see if i can squeeze in a fix for subsequent mounts.

* Enable support of virtfs in Podman and darwin.  At the time of this writing, it requires a special patch not yet included in upstream qemu.
* Prefer to use a specially built qemu to support virtfs.  The qemu is installed under libexec/podman.

[NO NEW TESTS NEEDED]
Signed-off-by: Brent Baude <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022
@tricktron
Copy link
Contributor

@baude what was the problem with subsequent mounts?

@baude
Copy link
Member Author

baude commented Mar 7, 2022

@baude what was the problem with subsequent mounts?

in fcos, / is immutable. so on reboots, they get removed. this will be something we improve in 4.1

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, rhatdan, willcohen

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

The pull request process is described 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

@willcohen
Copy link

I should note that I still get the following error when using the latest version of this PR, but the directory does in fact mount, so I'm not totally sure that there's actually still an issue, and it seems like the functionality is there. The core user does have the same UID as my macOS user when checking with podman machine ssh. This is running as non-root:

$ podman machine start
Starting machine "podman-machine-default"
INFO[0000] waiting for clients...                       
INFO[0000] new connection from  to /var/folders/pg/wl09ypf93rq84r1gc2g8ytsw0000gn/T/podman/qemu_podman-machine-default.sock 
Waiting for VM ...
INFO[0019] Socket forward established: /Users/wcohen/.local/share/containers/podman/machine/podman-machine-default/podman.sock to /run/user/501/podman/podman.sock 
Mounting volume... /Users/wcohen:/mnt/wcohen
time="2022-03-07T16:28:30-05:00" level=error msg="cannot find UID/GID for user core: No subuid ranges found for user \"core\" in /etc/subuid - check rootless mode in man pages."
time="2022-03-07T16:28:30-05:00" level=warning msg="Using rootless single mapping into the namespace. This might break some images. Check /etc/subuid and /etc/subgid for adding sub*ids if not using a network user"

This machine is currently configured in rootless mode. If your containers
require root permissions (e.g. ports < 1024), or if you run into compatibility
issues with non-podman clients, you can switch using the following command: 

	podman machine set --rootful

API forwarding listening on: /Users/wcohen/.local/share/containers/podman/machine/podman-machine-default/podman.sock

The system helper service is not installed; the default Docker API socket
address can't be used by podman. If you would like to install it run the
following command:

	sudo /nix/store/6wp5s55lhvvwzmp91qpafxr1f3c7qx62-podman-4.0.2/bin/podman-mac-helper install

You can still connect Docker API clients by setting DOCKER_HOST using the
following command in your terminal session:

	export DOCKER_HOST='unix:///Users/wcohen/.local/share/containers/podman/machine/podman-machine-default/podman.sock'

Machine "podman-machine-default" started successfully

@rhatdan
Copy link
Member

rhatdan commented Mar 7, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022
@rhatdan
Copy link
Member

rhatdan commented Mar 7, 2022

I tried this out on my M1 mac with
podman machine init -v $USER:$USER
podman machine start
podman run ./test:/mnt fedora ls /mnt

And I saw the content of test directory, very cool.
I believe we should make -v $HOME:$HOME the default in 4.1.

@rhatdan
Copy link
Member

rhatdan commented Mar 7, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4a242b1 into containers:main Mar 7, 2022
@typekpb
Copy link

typekpb commented Mar 21, 2022

hey, can we expect this in the 4.0.x versions or only in 4.1 ones? As volume mounts functionality on mac is the last pain point in the docker migration for me

@rhatdan
Copy link
Member

rhatdan commented Mar 23, 2022

4.0.2 should have volume support for MAC.
4.1 will default to volume mounting -v $HOME:$HOME, but you can do this now yourself.

@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
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.

9 participants