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: implement machinectl shell for vm class machines and send termination signal via D-Bus-over-SSH #32701

Merged
merged 3 commits into from
May 9, 2024

Conversation

bluca
Copy link
Member

@bluca bluca commented May 7, 2024

Cannot push to org, so continuing here from #32208

@evverx
Copy link
Member

evverx commented May 7, 2024

Can I ask why instead of stabilizing the release and addressing known regressions like #30421 introduced by sloppy merges new features with no tests are being pushed in a hurry?

@YHNdnzj
Copy link
Member

YHNdnzj commented May 8, 2024

Yeah, since we've already taken over the PR, there's genuinely no need to rush it in. I'll move the milestone to v257.

@YHNdnzj YHNdnzj modified the milestones: v256, v257 May 8, 2024
@bluca
Copy link
Member Author

bluca commented May 8, 2024

Yeah, since we've already taken over the PR, there's genuinely no need to rush it in. I'll move the milestone to v257.

See yesterday's chat

@bluca bluca modified the milestones: v257, v256 May 8, 2024
@YHNdnzj
Copy link
Member

YHNdnzj commented May 8, 2024

Yeah, since we've already taken over the PR, there's genuinely no need to rush it in. I'll move the milestone to v257.

See yesterday's chat

I've read that. But there's no clear indication that we need to get this in for v256, and when you asked "anyone disagrees" I wasn't there...

@DaanDeMeyer
Copy link
Contributor

This breaks registering vmspawn vms when using newer vmspawn with older machined, which is pretty much every vmspawn invocation right now. The varlink method will not be available and registering will fail.

src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/shared/json.c Outdated Show resolved Hide resolved
@bluca
Copy link
Member Author

bluca commented May 8, 2024

Rebased on top of #32709

@evverx
Copy link
Member

evverx commented May 8, 2024

See yesterday's chat

Can anyone copy-paste that here for mere mortals to see?

@evverx
Copy link
Member

evverx commented May 8, 2024

Either way I guess the question isn't going to be answered. It answers my question too in a different way.

@systemd systemd deleted a comment from YHNdnzj May 8, 2024
@evverx
Copy link
Member

evverx commented May 8, 2024

@bluca can you please stop deleting comments? That's not how open source projects are supposed to operate.

I've found the feedback form on the STF site and I'll contact them. I was going to do that anyway in the context of their bug resilience program. Thanks to the recent improvements (which I have no problem with) and miscommunications (which are problematic) I pulled out my downstream fuzzing infrastructure (even though it isn't broken yet) and they'd probably be interested to see the spreadsheets showing how much they should invest in the infrastructure alone to even cover the basics. Just to be clear I don't think it's STF fault. They were probably be misled into thinking that there is some actual upstream infrastructure where use-after-frees appear out of thin air.

@bluca bluca force-pushed the vmspawn/machinectl-shell branch 2 times, most recently from 3e5a8ed to 06a5706 Compare May 8, 2024 12:41
@bluca
Copy link
Member Author

bluca commented May 8, 2024

Rebased and picked changes from #32482

@bluca bluca changed the title machinectl/vmspawn: implement machinectl shell for vm class machines vmspawn: implement machinectl shell for vm class machines and open dbus session in VM May 8, 2024
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

Since we're supposed to merge this after -rc1, the commits must include a solid explanation what is being done and why and what the implications are.

src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn-register.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn.c Show resolved Hide resolved
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
src/vmspawn/vmspawn.c Outdated Show resolved Hide resolved
When available, use varlink to register the VM, so that we can talk
over SSH to the guest. Enables 'machinectl shell' for vmspawn guests.
Allow to gracefully shutdown by initiating the operation from
the guest
@bluca bluca force-pushed the vmspawn/machinectl-shell branch from 06a5706 to 521e928 Compare May 8, 2024 22:25
@bluca bluca changed the title vmspawn: implement machinectl shell for vm class machines and open dbus session in VM vmspawn: implement machinectl shell for vm class machines and send termination signal via D-Bus-over-SSH May 8, 2024
@keszybz keszybz removed the please-review PR is ready for (re-)review by a maintainer label May 9, 2024
@keszybz
Copy link
Member

keszybz commented May 9, 2024

Code LGTM.

I think it's fine to merge this now. The first commit is a fix, and the other two are relatively simple plumbing changes that will make machinectl shell much more useful. The work of vmspawn and related parts is a big part of this release, so we want to make it work as well as we can. We're after -rc1, but I think this a change of this magnitude is reasonable if it complements some changes that were done earlier in the cycle.

@keszybz
Copy link
Member

keszybz commented May 9, 2024

Ubunutautopkgtests are busted.

@keszybz keszybz merged commit 2019f68 into systemd:main May 9, 2024
44 of 49 checks passed
@bluca bluca deleted the vmspawn/machinectl-shell branch May 9, 2024 08:35
@evverx
Copy link
Member

evverx commented May 9, 2024

Code LGTM.

There are still no tests though as far as I can see.

the other two are relatively simple plumbing changes that will make machinectl shell much more useful

The commit where ssh was chainloaded was removed as far as I can tell so machinectl shell doesn't use that. There were a few other PRs in the meantime apparently where it was probably implemented (or not).

@evverx
Copy link
Member

evverx commented May 10, 2024

Looking at

ssh: Could not resolve hostname vsock/2747562528: Name or service not known
Failed to get init process of VM: Transport endpoint is not connected

I'm not sure stuff should get stuck like that for several hours but OK.

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

6 participants