Conversation
Write the process ID to a file on startup and remove it on clean exit. Enables reliable integration with process supervisors (systemd, monit), signal delivery, and scripting for multi-instance deployments.
3ba9e83 to
b228524
Compare
smalis-msft
left a comment
There was a problem hiding this comment.
This seems reasonable to me. I'm not sure if all the tests really provide value, but they seem cheap enough.
if thats not ok, just request a change. I am new to that repo, so I dont know the common vibes on that. |
|
I think a test is useful, as you need a full integration test to make sure it works. |
chris-oo
left a comment
There was a problem hiding this comment.
should we be gating support for this to just linux/macos? or is this useful on all platforms?
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(ref path) = opt.pidfile { |
There was a problem hiding this comment.
is this supported on windows/macos? or should we be limiting it to linux?
There was a problem hiding this comment.
I would say definitely on macos, on windows I dont know if it can be useful. I am more the linux guy.
But since we never know what users want to archieve, we can also have it on windows.
There can be a DOS issue, if the user passes a random (but important) existing filepath as --pidfile, that file will be overwritten. (i guess the same applies for openvmm logfile setting, but I did not check that).
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| //! Integration tests for OpenVMM's --pidfile option. |
There was a problem hiding this comment.
Four integration tests for a simple thing seems like overkill. Can we incorporate this into the existing ttrpc test?
Unit test for CLI argument parsing. Pidfile integration test added to the existing ttrpc test - verifies pidfile creation with correct PID content and cleanup on exit.
4221dd7 to
80ebe4a
Compare
|
Re: test consolidation (jstarks) Done - consolidated the four standalone pidfile tests into the existing ttrpc integration test. It now adds Re: platform gating (chris-oo) The implementation uses only Regarding the file overwrite concern I mentioned earlier - |
|
thank yall for the review and valuable input. |
|
Thank you for the contributions! |
Add `--pidfile <PATH>` CLI option that writes the process ID to a file on startup and removes it on clean exit
Summary
--pidfile <PATH>CLI option that writes the process ID to a file on startup and removes it on clean exitopenvmm_main()beforepal::process::terminate()to ensure removal even though destructors are skippedMotivation
Long-running openvmm instances need to be managed by external tooling - systemd units, orchestrators, health monitors, and shell scripts. Without a pidfile, callers must resort to fragile
pgrep/pidofheuristics that break when multiple VM instances run concurrently with similar command lines.A pidfile provides a reliable, race-free way to:
PIDFile=, monit, supervisord) that depend on pidfiles for lifecycle managementThis is a standard mechanism used by virtually all long-running Unix daemons (nginx, sshd, postgres, qemu) and its absence makes openvmm harder to integrate into production workflows.
Design notes
--write-saved-state-proto)Test plan
cargo build --release -p openvmmcompiles cleanlyopenvmm --pidfile /tmp/openvmm.pid ...creates file with correct PID