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

vmspawn: open dbus session in VM #32482

Conversation

sam-leonard-ct
Copy link
Contributor

@sam-leonard-ct sam-leonard-ct commented Apr 25, 2024

In order to support rebooting / shutting down the VM nicely we need to be able to talk to D-BUS inside the VM.
This is enabled by constructing a remote D-BUS address manually as the existing sd_bus_set_address_system_remote function does not parse the address correctly.

This PR is based on #32208 the new commits on top of it are the latest 3 commits.

These changes allow vmspawn to support machinectl kill and machinectl reboot without any changes to machinectl.

@sam-leonard-ct sam-leonard-ct marked this pull request as ready for review April 30, 2024 08:39
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Apr 30, 2024
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved

(void) sd_event_add_signal(event, NULL, SIGINT | SD_EVENT_SIGNAL_PROCMASK, request_reboot, &ssh_info);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems wrong to handle SIGINT as reboot in vmspawn itself. machined is documented to send SIGINT to the container's init process, but that is very different from the container manager. I don't think handling SIGINT in vmspawn as a workaround is going to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is possible using GetUnitByPID(1) then calling KillUnit(<unit of pid 1>, "leader", )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I have a pushed a new version which behaves like this, I couldn't get mkosi to build an image in time though so please test this before merging.

src/vmspawn/vmspawn.c Show resolved Hide resolved
",argv4=-o,argv5=IdentityFile=", private_key_path,
",argv6=-p,argv7=", port_str,
",argv8=--",
",argv9=root@vsock/", cid_str,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should unconditionally connect as root 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.

What would you suggest instead?

My rationale for connecting as root was that we need to get a privileged connection to the system bus so root was required in order to get a privileged connection?

This commit adds the new varlink interface io.systemd.Machine at
/run/systemd/machine/io.systemd.Machine with a single method Register

It supports all combinations of RegisterMachine[WithSSH,WithNetwork] all
under the same method.
Also adds three properties:
- VsockCid: the VSOCK CID of the VM
- SshAddress: the address of the VM in a format SSH can connect to
- SshPrivateKeyPath: the path to the SSH private key to use to connect
  to the VM.

GetMachineSSHInfo is essentially a convenience method to query both the
SshAddress and SshPrivateKeyPath properties at once.
@sam-leonard-ct sam-leonard-ct force-pushed the vmspawn/open-machine-dbus-session branch from 701929e to bbd1de4 Compare May 8, 2024 11:40
Copy link

github-actions bot commented May 8, 2024

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@bluca
Copy link
Member

bluca commented May 8, 2024

Continuing on #32701

@bluca bluca closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants