QEMU: Correct graceful_shutdown logic

[olljanat]

Description

There seems to be several bugs in QEMU driver's graceful_shutdown feature which most likely has been there already as part of #4800 (at least description says that it was not tested). Those bugs are:

• Use of -monitor instead of -qmp which is read-only.
• Missing of Capabilities Negotiation.
• Commands are not send as JSON which QMP protocol expects.
• Not reading responses from QEMU.
• Not waiting VM shutdown before sending kill_signal for it.

Testing & Reproduction steps

If you check job error log, it contains message like qemu-system-x86_64: terminating on signal 2 from pid 63315 (/usr/bin/nomad) after you stop job, even when graceful_shutdown is configured.

That why you need use kill_signal = "SIGUSR1" to really see that guest shutdown does not work and VM process gets killed by Nomad.

You can also try Python script like this to see that socket created by -monitor flag is read only.

import json, socket
sock_path = "/path/to/socket"
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
s.connect(sock_path)
s.sendall(json.dumps({"execute":"qmp_capabilities"}).encode())
s.sendall(json.dumps({"execute":"system_powerdown"}).encode())
s.close()

and that it only works after you enable another with parameters like:

 "-mon", "chardev=mon1,mode=control,pretty=on",
 "-qmp", "unix:/tmp/qmp-sock,server,nowait"

Here is also working Golang client:

package main

import (
	"fmt"
	"net"
)

const (
	qemuGracefulShutdownMsg = `{"execute": "system_powerdown"}`
	qemuQmpCapabilitiesMsg  = `{"execute":"qmp_capabilities"}`
)

func main() {
	monitorPath := "/tmp/qmp-sock"
	monitorSocket, err := net.Dial("unix", monitorPath)
	if err != nil {
		fmt.Println("could not connect to qemu monitor", "monitorPath", monitorPath, "error", err)
		return
	}
	defer monitorSocket.Close()

	buf := make([]byte, 512)
	monitorSocket.Read([]byte(buf))
	fmt.Println("sending qmp_capabilities command to qemu monitor socket", "monitor_path", monitorPath)
	_, err = monitorSocket.Write([]byte(qemuQmpCapabilitiesMsg))
	if err != nil {
		fmt.Println("failed to send qmp_capabilities", "qmp_capabilities", qemuQmpCapabilitiesMsg, "monitorPath", monitorPath, "error", err)
		return
	}
	monitorSocket.Read([]byte(buf))

	fmt.Println("sending graceful shutdown command to qemu monitor socket", "monitor_path", monitorPath)
	_, err = monitorSocket.Write([]byte(qemuGracefulShutdownMsg))
	if err != nil {
		fmt.Println("failed to send shutdown message", "shutdown message", qemuGracefulShutdownMsg, "monitorPath", monitorPath, "error", err)
		return
	}
	monitorSocket.Read([]byte(buf))
}

but unlike with Python, it reading QEMU responses seems to be needed, other why commands are not effective.

Contributor Checklist

• Changelog Entry If this PR changes user-facing behavior, please generate and add a
  changelog entry using the make cl command.
• Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
  ensure regressions will be caught.
• Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
  and job configuration, please update the Nomad product documentation, which is stored in the
  web-unified-docs repo. Refer to the web-unified-docs contributor guide for docs guidelines.
  Please also consider whether the change requires notes within the upgrade
  guide. If you would like help with the docs, tag the nomad-docs team in this PR.

Reviewer Checklist

• Backport Labels Please add the correct backport labels as described by the internal
  backporting document.
• Commit Type Ensure the correct merge method is selected which should be "squash and merge"
  in the majority of situations. The main exceptions are long-lived feature branches or merges where
  history should be preserved.
• Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
  within the public repository.

• If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

[mismithhisler]

Hi @olljanat! Thanks for this contribution. I'll take a swing at reproducing this today.

[mismithhisler]

Thanks for the great contribution! I left a couple comments, let me know what you think.

[mismithhisler]

Last thing, do you mind adding a changelog via make cl with a description of something like qemu: fixes graceful_shotdown to wait kill_timeout before signalling process?

[olljanat]

Sure, changelog is included now

[mismithhisler]

Can we remove this line because we already have the task handle from line 718?

[olljanat]

ah, yes if that is actual handle where status updates without need to fetch it again. Updated (but not tested).

[mismithhisler]

LGTM. Thank you for this contribution!