-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[QEMU] Fixing an unintentional variable shadowing #3798
[QEMU] Fixing an unintentional variable shadowing #3798
Conversation
merging from upstream
Signed-off-by: Simarpreet Singh <simar@linux.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
client/driver/qemu.go
Outdated
@@ -464,6 +464,7 @@ func (h *qemuHandle) Kill() error { | |||
// If Nomad did not send a graceful shutdown signal, issue an interrupt to | |||
// the qemu process as a last resort | |||
if gracefulShutdownSent == false { | |||
h.logger.Printf("[DEBUG] driver.qemu: graceful shutdown is not enabled, sending an interrupt signal to QEMU") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to include something to indicate what task log lines refer to. Perhaps something like:
[DEBUG] driver.qemu: graceful shutdown disabled, sending interrupt signal to pid %d", h.userPid
Signed-off-by: Simarpreet Singh <simar@linux.com>
@schmichael do we know if the tests are broken on master? The changes I've made don't seem to be related to the changes in the PR. |
@simar7 Can we improve the test to check that the vm shutdown post signaling? The test should fail before this change and pass after. |
Chiming in, as I investigated validation of VM shutdown as part of PR# 3411: the minimal Linux image in linux-0.2.img doesn't appear to respond to an ACPI shutdown signal passed via the QEMU monitor, which makes a full test more difficult. (This can surely be fixed, but I haven't dug any deeper.) |
@cheeseprocedure @simar7 I tried adding an Alpine Linux image in #3819 to test graceful shutdowns and have had no luck. Not sure what I'm doing wrong. (I pushed it to my own fork to keep the 40MB image out of the main Nomad repo if this effort doesn't work out.) |
@schmichael it looks like I'm not aware of a sensible way to verify the state of the guest in a black-box fashion before triggering a shutdown. Perhaps we could call |
@cheeseprocedure Thanks for the tip! The Alpine image has sshd installed, so I just poll until sshd starts and then know it's safe to issue the shutdown command. Pushed updates to #3819. Looks like everything is working! |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR fixes the shadowing of the
monitorPath
variablewithin the QEMU driver for Nomad. With this variable being shadowed
within the scope, all graceful shutdowns are effectively disabled
as
monitorPath
will not be set.Also adds a helpful debug statement for the operator to verify.
Signed-off-by: Simarpreet Singh simar@linux.com