bhyvectl: support querying VM pid
Needs ReviewPublic
Actions

Authored by novel on Sat, Aug 22, 10:37 AM.

Details

Reviewers

andrew
manu

Group Reviewers

bhyve

Summary

Extend bhyvectl(8) to support querying VM pid using the
--get-vm-pid flag.

This is useful in the monitor mode when the VM pid differs
from the main bhyve(8) process run by the user. Knowing the VM pid
is necessary, for example, to query process resource usage or
trigger ACPI shutdown with SIGTERM.

Of course, it could be obtained by matching the monitor process
children by the process title, but it's a little more complex
and fragile than it could be.

Implement that by introducing the VM_GET_PID ioctl to vmm(8),
exposing that through libvmmapi, and using it to implement
bhyvectl --get-vm-pid.

When the VM pid is not known, ESRCH is returned.

This information is now also included in the bhyvectl --get-all
output. The --get-vm-pid fails when vm_get_pid() returns ESRCH,
and --get-all just skips it in this case.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76704
Build 73587: arc lint + arc unit

Event Timeline

novel created this revision.Sat, Aug 22, 10:37 AM

Herald added a reviewer: andrew. · View Herald TranscriptSat, Aug 22, 10:37 AM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added a reviewer: bhyve. · View Herald Transcript

Herald added subscribers: riscv, bcran, rgrimes and 2 others. · View Herald Transcript

novel requested review of this revision.Sat, Aug 22, 10:37 AM

Harbormaster completed remote builds in B75963: Diff 184734.Sat, Aug 22, 10:37 AM

This is a good feature, but I don't think that this is the correct approach design-wise. Adding an ioctl handler to just to fetch a VM's PID is overkill IMO, new ioctl commands should only be added when there's no other way to obtain the needed information (e.g., fetching memsegs and other metadata that is stored in the vmm module).

I strongly suggest that you implement this functionality using bhyve's IPC facilities. These were only reserved for the snapshot subsystem up until very recently, but I had to split these out into a separate subsystem as a part of my hotplugging work.
This should make the implementation way simpler since you just need to register an IPC handler instead of adding a new ioctl.
I just committed the refactor so you can build on top of it.

In D59108#1355951, @bnovkov wrote:

This is a good feature, but I don't think that this is the correct approach design-wise. Adding an ioctl handler to just to fetch a VM's PID is overkill IMO, new ioctl commands should only be added when there's no other way to obtain the needed information (e.g., fetching memsegs and other metadata that is stored in the vmm module).

I strongly suggest that you implement this functionality using bhyve's IPC facilities. These were only reserved for the snapshot subsystem up until very recently, but I had to split these out into a separate subsystem as a part of my hotplugging work.
This should make the implementation way simpler since you just need to register an IPC handler instead of adding a new ioctl.
I just committed the refactor so you can build on top of it.

Thanks, I'll take a look at the new IPC code.

Re-implement using IPC.

Harbormaster completed remote builds in B76158: Diff 185089.Tue, Aug 25, 4:49 PM

bnovkov added inline comments.Mon, Aug 31, 7:12 PM

usr.sbin/bhyvectl/bhyvectl.c
270	Are we allowing the error to be suppressed so we can handle `get_all` gracefully?
313	If we pass a non-NULL `replyp` we'll end up passing NULL to this `nvlist_destroy` call. We should only be destroying it if the caller isn't interested in the value.
577–581	Why not just handle the error in `vm_pid_request`?

Do not call nvlist_destroy() on NULL.
Update vm_pid_request() to properly set report_error depending on whether it's --get-all or --get-vm-pid option.
bhyvectl.8: rundir: s/checkpoint socket/bhyve IPC socket/.

Herald added a subscriber: ziaee. · View Herald TranscriptWed, Sep 2, 4:37 PM

Harbormaster completed remote builds in B76407: Diff 185635.Wed, Sep 2, 4:37 PM

novel marked 2 inline comments as not done.Wed, Sep 2, 4:41 PM

novel added inline comments.

usr.sbin/bhyvectl/bhyvectl.c
270	Correct. As described in summary, my idea is that `--get-all` should print the PID if it knows it, but do not fail if it does not (and should not print errors coming from attempting to get it). And when explicitly requested with `--get-vm-pid`, it should fail if it cannot get it, and report errors related to that.
313	`nvlist_destroy()` allows passing NULL and does nothing in this case, so it should be safe. I'll update the code to be more explicit though.
577–581	As mentioned in the other comment, I'd like treat errors differently based on the mode we're currently in (`--get-all` or `--get-vm-pid`). I noticed though that I don't set `report_error` to `true` for the `--get-vm-pid`, so I'll update `vm_pid_request()`. Anyway, it feels like this error handling logic fits here better than in the `vm_pid_request()`.

bnovkov added inline comments.Wed, Sep 9, 1:35 PM

usr.sbin/bhyvectl/bhyvectl.c
270	hm, I am a bit conflicted about having `--get-vm-pid` included in `--get-all` since it is implicitly dependent on the `rundir` option. I think that we shouldn't mix ioctl and IPC operations in `--get-all`, so I'd personally leave this out of `--get-all` . I'm not sure what the best solution here would be, so I won't insist.

novel added inline comments.Wed, Sep 9, 4:11 PM

usr.sbin/bhyvectl/bhyvectl.c
270	I don't have practical reasons to keep it in `--get-all`. The only motivation was consistency in sense that "vm pid" falls under "all". On the other hand, the `--get-all` seems to be a sort of a service feature that we do not document in the manual page and do not promise anything about it . I personally do not use `--get-all`, at least not in the libvirt driver. I think I'll remove this part from the patch for now, it seems to create too many complications for a feature that probably nobody needs.