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

applehv: Rosetta support #21670

Merged
merged 1 commit into from
May 17, 2024
Merged

Conversation

tnk4on
Copy link
Contributor

@tnk4on tnk4on commented Feb 15, 2024

This PR adds Rosetta support to the AppleHV Podman machine(only v5).
Rosetta is only available on macOS with Apple Silicon.
With Rosetta, the execution performance is several times better than QEMU emulation.
https://developer.apple.com/documentation/virtualization/running_intel_binaries_in_linux_vms_with_rosetta

Todo

Note

[SELinux context] > resolved
Perhaps because of the use of vfkit, the SELinux context needs to be with *exec_t when mounting the Rosetta directory.
On lima and UTM which support Rosetta, it can be executed with nfs_t without error.

https://github.com/lima-vm/lima/blob/88c89165273d87b33753a593af867b20c1d1c67d/pkg/cidata/cidata.TEMPLATE.d/boot/05-rosetta-volume.sh#L22

Does this PR introduce a user-facing change?

Podman machine can now use Rosetta 2 (a.k.a Rosetta) on macOS with Apple Silicon. This is enabled by default. If you wish to change this option, you can set via containers.conf.

[no new tests needed]

@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2024

Why would a user ever disable this? Shouldn't this be on by default?

@tnk4on
Copy link
Contributor Author

tnk4on commented Feb 15, 2024

To activate Rosetta, you must mount the Rosetta directory on your Linux OS and add the magic code to binfmt_misc. Some users may want to remove this as an afterthought.
The default can be turned on (true).

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

cmd/podman/machine/init.go Outdated Show resolved Hide resolved
cmd/podman/machine/set.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2024

Man page needs to be updated as well.

@edsantiago are we verifying man pages for podman machine?

@edsantiago
Copy link
Member

edsantiago commented Feb 15, 2024

Yes, podman machine is checked. CI just didn't get to that check because of other earlier failures. If CI had made it past those, it would've failed with:

xref-helpmsgs-manpages: 'podman machine init --help' lists '--rosetta', which is not in docs/source/markdown/podman-machine-init.1.md
xref-helpmsgs-manpages: 'podman machine inspect --format <TAB>' lists '.Rosetta', which is not in docs/source/markdown/podman-machine-inspect.1.md
xref-helpmsgs-manpages: 'podman machine set --help' lists '--rosetta', which is not in docs/source/markdown/podman-machine-set.1.md

pkg/machine/shim/host.go Outdated Show resolved Hide resolved
@edsantiago
Copy link
Member

Hi, before you push again, please rebase on current main. This will be necessary to check man pages and to avoid breaking future main. Thank you!

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

A couple of points, AFAIK rossetta only emulates x86_64 on arm macs:

applehv is also support on intel macs so we cannot do the mounts/setup there at all. This code has to handle it.
Also do we really need to set option? Just keep things simple who has to realistically switch this?

Also I don't like that the flag is added on non apple platforms. It would make much more sense to hide this flag when machine is not running on macos.

Also @baude PTAL

pkg/machine/update.go Outdated Show resolved Hide resolved
pkg/machine/ignition/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition/ignition.go Outdated Show resolved Hide resolved
pkg/machine/vmconfigs/config.go Outdated Show resolved Hide resolved
cmd/podman/machine/init.go Outdated Show resolved Hide resolved
cmd/podman/machine/set.go Outdated Show resolved Hide resolved
pkg/machine/e2e/init_test.go Outdated Show resolved Hide resolved
@baude
Copy link
Member

baude commented Feb 18, 2024

@tnk4on i did an initial pass on your PR, but given this is a big change, i hit the big topics first ... thorough code reviews will need to be done once we decide how this fits in podman machine.

my biggest comment, which is a repeated here and there in the review, is that rosetta should be enabled via containers common. this keeps it out of podman docs proper, out of the cli, and structs that are supposed to be general in use.

However, i would like the rest of the maintainers of machine to comment: @Luap99 @mheon @ashley-cui @n1hility @rhatdan ptal

@cfergeau PTAL how are you thinking about enabling rosetta?

And finally, should this be held for after we branch?

@tnk4on
Copy link
Contributor Author

tnk4on commented Feb 19, 2024

@baude I appreciate your review and your comments !
The latest commit(0ca90ce) was written before I read your comment, but I pushed it forward to move this PR.
I will continually work on improving this PR.

@rhatdan
Copy link
Member

rhatdan commented Feb 19, 2024

I agree that containers.conf in the machine section is the proper place for this. I don't think the CLI option is necessary since I don't think people will be changing back and forth. I do believe most users will want Rosetta=true, since building x86 images on Mac Arm platforms is going to be very common.

@ashley-cui
Copy link
Member

Thanks for the contribution! This looks super promising! One small formatting nit: all files need to end with a newline.

pkg/machine/applehv/vfkit.go Outdated Show resolved Hide resolved
pkg/machine/e2e/config_init_test.go Outdated Show resolved Hide resolved
pkg/machine/ignition/ignition.go Outdated Show resolved Hide resolved
@tnk4on
Copy link
Contributor Author

tnk4on commented Feb 21, 2024

Thank you all for your comments and reviews.
The implementation of common (containers.conf) and the removal of the --rosetta option(init/set) is working well in my environment.

My understanding is that I need to send a PR to containers/common ,right?

github.com/containers/common/pkg/config/default.go

Rosetta: true,

github.com/containers/common/pkg/config/config.go

// Rosetta
Rosetta bool `toml: "rosetta,omitempty"`

@n1hility
Copy link
Member

Sorry for the late comment:

@rhatdan:I do believe most users will want Rosetta=true, since building x86 images on Mac Arm platforms is going to be very common.

I agree this should just always be on for apple silicon. The only reason to change this is in the rare case you want qemu-user-static to take precedence, which would be a fairly specialized situation. Only case I think of that happening is if there is some bug in rosetta, which breaks a container you need to run, so you need to temporarily apply a workaround.

@rhatdan: I agree that containers.conf in the machine section is the proper place for this. I don't think the CLI option is necessary since I don't think people will be changing back and forth.

IMO there are two issues with it being only specifiable in containers.conf:

  1. The machine section is a global, yet the setting scope is really per-machine. Adding it could be reasonable as a default, but if its something that is rare then it might not be necessary
  2. Introduces UX complexity, since to set it you first need to write a containers.conf file, and then run the CLI. If you need to change it you then need to set it, delete the machine and then recreate it. It also introduces a similar UX contract API interaction complexity with Podman Desktop, since it needs to incorporate containers.conf editing when this particular change is made.

I think it's inevitable that we will need provider specific settings, but agree it can create a problem with MxN parameter clutter across providers. I can think of two ways we could handle that

  1. We could use a generic option for provider settings using name/value arrays (e.g. --provider-setting-name="foo" --provider-setting-value="blah" --provider-setting-name="other" --provider-setting-value="thing"). These would be passed straight into the provider as a map
  2. We could have a phase were each provider gets a hook to register extra flags under an opaque type as part of the cobra init, and then perhaps we can add a common prefix convention to make the docs cleaner. Some made up hypothetical examples --applehv-rosetta=false, --applehv-disk-cache=automatic --hyperv-dynamic-memory=true --wsl-wayland=true

Although not suggesting we do this now. With Rosetta could just wait until someone actually needs it to be turned off.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024
@@ -62,7 +63,13 @@ var _ = BeforeSuite(func() {
if pullError != nil {
Fail(fmt.Sprintf("failed to pull wsl disk: %q", pullError))
}

if testProvider.VMType() == define.AppleHvVirt {
cmd := exec.Command("softwareupdate", "--install-rosetta", "--agree-to-license")
Copy link
Member

Choose a reason for hiding this comment

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

Does this download anything?
I rather have this not be part of the actual test code and part of the CI setup somewhere.
@cevich How easy is this do add to the macos runners by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, download a small file. This requires an Internet connection.
https://support.apple.com/en-us/102527

% du -sh /Library/Updates/Rosetta/SoftwareUpdate/RosettaUpdateAuto.pkg
380K	/Library/Updates/Rosetta/SoftwareUpdate/RosettaUpdateAuto.pkg

I agree with including it in the CI setup.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming it requires privileges to install, then it must be installed during the setup where sudo is available. Note: After making the addition there, it will take a few days before all the Macs in the testing pool have the change.

Though I'm afraid the softwareupdate command might fail, doesn't that require the machine to be "attached" (don't know the term) to the Apple mothership via some login?

In either case, somebody (probably me) with privileges to our EC2 dedicated hosts will be needed to test the change (assuming it's possible). Testing setup.sh changes cannot be automated in any trivial way, since there's a 3-hour turnaround for a fresh machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I have tried, install-rosetta does not require sudo.
I don't know how often Apple updates, but I am concerned about doing a CI setup for every update of the rosetta pkg.

Copy link
Member

Choose a reason for hiding this comment

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

Well I guess the answer is simple, the CI currently passes because that command works just fine as user. I assume you only added this because CI failed without it @tnk4on?

So yes in this case I think we really should add this this to the runner setup, because I Really do not want this in the test suite. This stuff can be run locally by users so we should never install dependencies without their knowledge.
I think we we can add it to contrib/cirrus/mac_runner.sh temporarily until we have it in the proper runner seup linked by @cevich

Copy link
Member

Choose a reason for hiding this comment

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

I think we we can add it to contrib/cirrus/mac_runner.sh

Keep in mind these Mac's are shared by multiple tasks and multiple PRs (not at the same time). If you add it into that script, maybe wrap it in a condition that checks if it's already installed first?

doing a CI setup for every update of the rosetta pkg.

The Macs servicing CI are recreated roughly every 24 hours. If/when it's installed in the big setup.sh script, any updates would be picked up over time. This isn't ideal, since multiple runs of a PR could net different results depending on which Mac you get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Luap99 @cevich
The other code fixes for this PR are complete, waiting for mac CI setup changes.
What will happen regarding CI setup changes?

Copy link
Member

Choose a reason for hiding this comment

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

I suggest you move this command into contrib/cirrus/mac_runner.sh for now. Possibly checking if it is already installed before calling this command.

I don't like this being in the test code.

Copy link
Member

Choose a reason for hiding this comment

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

We can work on a proper integration in the automation repo later but for now we should get this merged

Copy link
Member

Choose a reason for hiding this comment

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

I opened a Jira card to track this.

Comment on lines 252 to 255
{
Enabled: BoolToPtr(true),
Name: "unregister-handler.service",
},
Copy link
Member

Choose a reason for hiding this comment

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

just so we do not forget, if we go a ahead with containers/podman-machine-os#8 this must be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, and latest machine-mac CI passed !

Copy link

Cockpit tests failed for commit 02be9b4. @martinpitt, @jelly, @mvollmer please check.

@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

@Luap99 @baude @ashley-cui PTAL

@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

@tnk4on Any reason this has the Hold Label?

@tnk4on
Copy link
Contributor Author

tnk4on commented May 16, 2024

@rhatdan
My fix work on the latest commit is already done and awaiting review.

I think we are still not done deciding whether to pre-install rosetta on the mac CI mothership in this discussion(#21670 (comment)).
However, we can pass this PR through.

@rhatdan rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024
@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

Enough users hitting issues on qemu-user-static, so I would like to get this on by default and see if it works.

@Luap99
Copy link
Member

Luap99 commented May 17, 2024

@tnk4on Also if you don't mind I prefer the commits to be squashed because the later commits fixes code from the previous commit in this PR so it doesn't really make sense to have them separated.

@tnk4on tnk4on force-pushed the rosetta-support branch from 02be9b4 to fe7cc67 Compare May 17, 2024 08:56
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented May 17, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2024
Copy link
Contributor

openshift-ci bot commented May 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, tnk4on

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 34d2d20 into containers:main May 17, 2024
89 of 91 checks passed
@tnk4on tnk4on deleted the rosetta-support branch May 18, 2024 01:34
@mheon
Copy link
Member

mheon commented May 20, 2024

Let's get this in the next release
/cherry-pick v5.1

@openshift-cherrypick-robot
Copy link
Collaborator

@mheon: new pull request created: #22757

In response to this:

Let's get this in the next release
/cherry-pick v5.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 28, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
5.1 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. machine release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.