Qemu driver: graceful shutdown feature #3411

cheeseprocedure · 2017-10-18T19:56:00Z

This PR enables the Qemu driver to attempt a graceful shutdown of guest VMs by passing an ACPI-shutdown via the Qemu monitor.

This is a best-effort mechanism for notifying guests of their impending termination. The driver will still forcibly terminate the process if the kill timeout expires.

I think the integration tests can be improved; any suggestions in that area are especially appreciated.

Thank you!

schmichael

Looks great! Thanks for submitting this! The only blocker is around the Kill() method logic.. I'm not sure it will block up to kill_timeout waiting for the VM to actually shutdown. See relevant comments on that code and the docs.

schmichael · 2017-10-27T16:56:04Z

client/driver/qemu.go

@@ -239,7 +288,7 @@ func (d *QemuDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse
 		)
 	}

-	d.logger.Printf("[DEBUG] Starting QemuVM command: %q", strings.Join(args, " "))
+	d.logger.Printf("[DEBUG] driver.qemu - starting QemuVM command: %q", strings.Join(args, " "))


Can you standardize on driver.qemu: as you logging prefix? That's the convention we use elsewhere.

schmichael · 2017-10-27T16:57:09Z

client/driver/qemu.go

@@ -317,7 +367,7 @@ func (d *QemuDriver) Open(ctx *ExecContext, handleID string) (DriverHandle, erro

 	exec, pluginClient, err := createExecutorWithConfig(pluginConfig, d.config.LogOutput)
 	if err != nil {
-		d.logger.Println("[ERR] driver.qemu: error connecting to plugin so destroying plugin pid and user pid")
+		d.logger.Println("[ERR] driver.qemu - error connecting to plugin so destroying plugin pid and user pid")


While you're changing this could you switch to Printf and include at least the pids. There's not really enough information in this log line to be useful.

schmichael · 2017-10-27T17:14:08Z

client/driver/qemu.go

+			defer monitorSocket.Close()
+			h.logger.Printf("[DEBUG] driver.qemu - sending graceful shutdown command to qemu monitor socket at %s", h.monitorPath)
+			_, err = monitorSocket.Write([]byte(qemuGracefulShutdownMsg))
+			if err == nil {


Is there any way to block until either the vm exits or the kill timeout is reached and make sure to always call h.executor.ShutDown() to kill Nomad's sidecar process?

I'm afraid this could leak VMs and Nomad executor processes.

schmichael · 2017-10-27T17:15:05Z

client/driver/qemu.go

+			if err == nil {
+				return nil
+			}
+			h.logger.Printf("[WARN] driver.qemu - failed to send '%s' to monitor socket '%s': %s", qemuGracefulShutdownMsg, h.monitorPath, err)


Using %q instead of '%s' will properly escape and quote strings for what it's worth. Not a big deal to change as we're not consistent either, but %q is pretty handy.

schmichael · 2017-10-27T17:16:13Z

client/driver/qemu.go

@@ -395,13 +462,13 @@ func (h *qemuHandle) Kill() error {
 	case <-h.doneCh:
 		return nil
 	case <-time.After(h.killTimeout):
+		h.logger.Printf("[DEBUG] driver.qemu - kill timeout exceeded")


Can you include monitorPath and/or userPid in log messages so users have some way of connectin gthem to a particular VM?

schmichael · 2017-10-27T17:17:26Z

client/driver/qemu.go

@@ -414,7 +481,7 @@ func (h *qemuHandle) run() {
 	ps, werr := h.executor.Wait()
 	if ps.ExitCode == 0 && werr != nil {
 		if e := killProcess(h.userPid); e != nil {
-			h.logger.Printf("[ERR] driver.qemu: error killing user process: %v", e)
+			h.logger.Printf("[ERR] driver.qemu - error killing user process: %v", e)


Include userPID

schmichael · 2017-10-27T17:19:45Z

client/driver/qemu_test.go

@@ -14,6 +14,14 @@ import (
 	ctestutils "github.com/hashicorp/nomad/client/testutil"
 )

+func generateString(length int) string {


Replace with strings.Repeat("x", n)

schmichael · 2017-10-27T17:21:57Z

client/driver/qemu_test.go

+			Config: map[string]interface{}{
+				"image_path":        "linux-0.2.img",
+				"accelerator":       "tcg",
+				"graceful_shutdown": false,


Adding a graceful_shutdown test would be nice. Maybe copy StartOpen_Wait, but don't worry about the Open part (that's the restore path)?

(Done! Thanks!)

schmichael · 2017-10-27T17:23:25Z

website/source/docs/drivers/qemu.html.md

@@ -47,6 +48,8 @@ The `qemu` driver supports the following configuration in the job spec:
  If the host machine has `qemu` installed with KVM support, users can specify
  `kvm` for the `accelerator`. Default is `tcg`.

+* `graceful_shutdown` `(bool: false)` - Using the [qemu monitor](https://en.wikibooks.org/wiki/QEMU/Monitor), send an ACPI shutdown signal to virtual machines rather than simply terminating them. This emulates a physical power button press, and gives instances a chance to shut down cleanly. If the VM is still running after ``kill_timeout``, it will be forcefully terminated. (Note that [prior to qemu 2.10.1](https://github.com/qemu/qemu/commit/ad9579aaa16d5b385922d49edac2c96c79bcfb6), the monitor socket path is limited to 108 characters. Graceful shutdown will be disabled if qemu is < 2.10.1 and the generated monitor path exceeds this length. You may encounter this issue if you set long [data_dir](https://www.nomadproject.io/docs/agent/configuration/index.html#data_dir) or [alloc_dir](https://www.nomadproject.io/docs/agent/configuration/client.html#alloc_dir) paths.)


Wrap at 80 characters and indent lines with 2 spaces (we're considering loosening this style, but now we're trying to stay consistent).

If the VM is still running after kill_timeout, it will be forcefully terminated.

Huh, I thought if you wrote the shutdown message to the monitor's socket properly the Kill() method returned and Nomad would consider the VM dead even if it wasn't? Perhaps my comment on that code above is wrong?

cheeseprocedure · 2017-11-01T22:38:34Z

Hi @schmichael - thank you for all the feedback!

I've made a number of improvements in response, including more actionable logging, fixes to Kill() to ensure we don't leak processes, and additional tests of graceful shutdown logic.

Please let me know if I can perform any additional changes and/or cleanup.

schmichael

Looks great! Thanks for all of the hard work @cheeseprocedure.

I left a few more comments, but I don't think any are blockers. I'll merge soon unless someone else spots something.

schmichael · 2017-11-01T22:59:11Z

client/driver/qemu.go

+	var err error
+	if monitorPath == "" {
+		logger.Printf("[DEBUG] driver.qemu: monitorPath not set; will not attempt graceful shutdown for user process pid %d", userPid)
+		err = errors.New("monitorPath not set")


You could return the error here and skip the else block and var err error declaration.

Not a blocker.

schmichael · 2017-11-01T23:02:35Z

client/driver/qemu_test.go

+	monitorPathExists := false
+	for i := 0; i < 5; i++ {
+		if _, err := os.Stat(monitorPath); !os.IsNotExist(err) {
+			fmt.Printf("Monitor socket exists at %q\n", monitorPath)


d.logger.Printf (or a testLogger()) is preferred but not a blocker.

schmichael · 2017-11-01T23:04:18Z

client/driver/qemu_test.go

+			monitorPathExists = true
+			break
+		}
+		time.Sleep(1 * time.Second)


We test on fast developer machines and very slow VMs, so testing more often (eg every 200ms) and more times (eg 50 times) would be preferred.

schmichael · 2017-11-01T23:04:47Z

client/driver/qemu_test.go

+	}
+
+	// Clean up
+	if err := resp.Handle.Kill(); err != nil {


Put this in a defer

schmichael · 2017-11-01T23:05:07Z

client/driver/qemu_test.go

+
+	// Clean up
+	if err := resp.Handle.Kill(); err != nil {
+		fmt.Printf("\nError killing Qemu test: %s", err)


Same as above (use a testLogger)

cheeseprocedure · 2017-11-01T23:56:35Z

Thanks for the additional feedback! I've made some small changes in response.

schmichael · 2017-11-02T00:13:58Z

GNUmakefile

 	@echo "==> Spell checking website..."
-	@misspell -error -source=text website/source/
+	# @misspell -error -source=text website/source/


Ha, I don't think you meant to commit this. :)

D'oh! Fixed.

schmichael · 2017-11-02T00:15:41Z

client/driver/qemu_test.go

+	}
+
+	// Clean up
+	defer func() {


Oh sorry, meant to do this block in a defer right after you check the error from d.Start. That way you cleanup even if a fatal error is hit.

Double d'oh. Fixed!

dadgar · 2017-11-02T17:02:08Z

client/driver/qemu.go

+	// The key populated in Node Attributes to indicate presence of the Qemu driver
+	qemuDriverAttr                = "driver.qemu"
+	qemuDriverVersionAttr         = "driver.qemu.version"
+	qemuDriverLongMonitorPathAttr = "driver.qemu.longsocketpaths"


I am not a fan of this being an attribute. I think we can just check in the start method

For scheduling purposes, constraining on this can be replicated by: https://www.nomadproject.io/docs/job-specification/constraint.html#quot-version-quot-

Thanks - I've removed this attribute as part of the changes to getMonitorPath!

dadgar · 2017-11-02T17:02:52Z

client/driver/qemu.go

+	// Relevant fix is here:
+	// https://github.com/qemu/qemu/commit/ad9579aaa16d5b385922d49edac2c96c79bcfb6
+	currentQemuSemver := semver.New(currentQemuVersion)
+	fixedSocketPathLenVer := semver.New("2.10.1")


Can you make this a variable with a comment. Can be a singleton.

dadgar · 2017-11-02T17:06:53Z

client/driver/qemu.go

@@ -64,6 +75,13 @@ type qemuHandle struct {
 	doneCh         chan struct{}
 }

+func getMonitorPath(dir string, longPathSupport string) (string, error) {


Comment on this method.

dadgar · 2017-11-02T17:08:02Z

client/driver/qemu.go

@@ -64,6 +75,13 @@ type qemuHandle struct {
 	doneCh         chan struct{}
 }

+func getMonitorPath(dir string, longPathSupport string) (string, error) {
+	if len(dir) > legacyMaxMonitorPathLen && longPathSupport != "1" {


Instead of taking the longPathSupport string make this a function on the QemuDriver and use the node that is in the embedded DriverCtx to check the version of the qemu driver and determine if there is support.

dadgar · 2017-11-02T17:08:33Z

client/driver/qemu.go

+			d.logger.Printf("[DEBUG] driver.qemu: got monitor path OK: %s", monitorPath)
+			args = append(args, "-monitor", fmt.Sprintf("unix:%s,server,nowait", monitorPath))
+		} else {
+			d.logger.Printf("[WARN] driver.qemu: %s", err)


I think this should fail the task.

Done! This will now return (and log) an error.

dadgar · 2017-11-02T17:10:34Z

client/driver/qemu.go

-		if h.pluginClient.Exited() {
-			return nil
+	// First, try sending a graceful shutdown command via the qemu monitor
+	err := sendQemuShutdown(h.logger, h.monitorPath, h.userPid)


Wrap this in a if check seeing if graceful shutdown is enabled and log the error

dadgar · 2017-11-02T17:11:09Z

client/driver/qemu.go

+	if monitorPath == "" {
+		logger.Printf("[DEBUG] driver.qemu: monitorPath not set; will not attempt graceful shutdown for user process pid %d", userPid)
+		return errors.New("monitorPath not set")
+	} else {


You can remove the else statement since the if returns.

dadgar · 2017-11-02T17:11:44Z

client/driver/qemu.go

+		return errors.New("monitorPath not set")
+	} else {
+		monitorSocket, err := net.Dial("unix", monitorPath)
+		if err == nil {


Swap the checking. if err != nil { log && return err} and de-indent the block.

dadgar · 2017-11-02T17:12:09Z

client/driver/qemu.go

+	// Reference: https://en.wikibooks.org/wiki/QEMU/Monitor
+	qemuGracefulShutdownMsg = "system_powerdown\n"
+	legacyMaxMonitorPathLen = 108
+	qemuMonitorSocketName   = "qemu-monitor.sock"


What about windows?

Good question! Until today, I didn't realize QEMU was available on Windows hosts. I would need to spin up an EC2 instance and experiment a bit with QEMU monitor interaction on that platform.

It appears an existing QEMU driver option can cause trouble on Windows hosts (accelerator = "kvm"). As a first pass, would it be acceptable to fail a task if graceful shutdown has been enabled for the job and runtime.GOOS == "windows" (ensuring it's called out in documentation, of course)?

Yeah I think that is acceptable. Would you mind adding error cases for both graceful and kvm on windows.

cheeseprocedure · 2017-11-03T00:44:30Z

@dadgar thank you very much for your feedback! I believe I've addressed the comments, with the exception of one outstanding question about Windows support.

Remove attribute for long qemu monitor path; misc cleanup; update tests

dadgar

Awesome work so far!

dadgar · 2017-11-03T19:55:10Z

client/driver/qemu.go

-			d.logger.Printf("[WARN] driver.qemu: %s", err)
+		monitorPath, err := d.getMonitorPath(ctx.TaskDir.Dir)
+		if err != nil {
+			d.logger.Printf("[ERR] driver.qemu: could not get qemu monitor path - error: %s", err)


"[ERR] driver.qemu: could not get qemu monitor path: %s"

dadgar · 2017-11-03T19:57:48Z

client/driver/qemu.go

-	// If we did not send a graceful shutdown via the monitor socket, we'll
-	// issue an interrupt to the qemu process as a last resort
-	if err != nil {
+	if err := sendQemuShutdown(h.logger, h.monitorPath, h.userPid); err != nil {


This will log several debug messages relating to graceful shutdown even if the user didn't set the option. We should only attempt the graceful shutdown if the user asked for it.

dadgar · 2017-11-03T19:58:23Z

client/driver/qemu.go

+	// Reference: https://en.wikibooks.org/wiki/QEMU/Monitor
+	qemuGracefulShutdownMsg = "system_powerdown\n"
+	legacyMaxMonitorPathLen = 108
+	qemuMonitorSocketName   = "qemu-monitor.sock"


Yeah I think that is acceptable. Would you mind adding error cases for both graceful and kvm on windows.

cheeseprocedure · 2017-11-03T22:09:43Z

@dadgar thank you for the review! In addition to the fixes above, I've added simple checks for the graceful-shutdown feature and use of the KVM accelerator which return errors if run on a Windows host.

dadgar · 2017-11-03T23:41:30Z

@cheeseprocedure Awesome work! I am going to merge and add some documentation that the graceful shutdown is only supported on Linux for now!

github-actions · 2023-03-18T02:15:24Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch 3 times, most recently from 216285c to 2129de2 Compare October 18, 2017 20:28

cheeseprocedure closed this Oct 18, 2017

cheeseprocedure reopened this Oct 18, 2017

cheeseprocedure mentioned this pull request Oct 25, 2017

[Feature] Migrating of QEMU machines #2323

Open

schmichael requested changes Oct 27, 2017

View reviewed changes

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch 7 times, most recently from 35176dc to 95e07da Compare November 1, 2017 22:15

Matt Mercer and others added 8 commits November 1, 2017 15:30

Qemu driver: add graceful shutdown feature

00e3cc8

Standardize driver.qemu logging prefix

3f6fdfc

Wrap qemu docs at 80 characters

16a3614

Qemu driver: minor logging fixes

22f390d

Qemu driver: ensure proper cleanup of resources

200a12c

Qemu driver: include PIDs in log output

1ff9703

Qemu driver: basic testing of graceful shutdown feature

2924bad

Use strings.Replace() instead of custom function

60030d8

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch from 95e07da to 60030d8 Compare November 1, 2017 22:33

schmichael approved these changes Nov 1, 2017

View reviewed changes

schmichael reviewed Nov 2, 2017

View reviewed changes

cheeseprocedure added 2 commits November 1, 2017 17:21

Qemu driver: clean up test logging; retry integration test for longer

15d7565

Qemu driver: defer cleanup sooner

1856585

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch from b4fc5e7 to 1856585 Compare November 2, 2017 00:38

dadgar requested changes Nov 2, 2017

View reviewed changes

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch 2 times, most recently from 101dba8 to 80495d7 Compare November 3, 2017 02:25

Qemu driver: tweaks in response to PR feedback

66f9840

Remove attribute for long qemu monitor path; misc cleanup; update tests

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch from 80495d7 to 66f9840 Compare November 3, 2017 18:30

dadgar requested changes Nov 3, 2017

View reviewed changes

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch from c3121f1 to 046b774 Compare November 3, 2017 22:22

Qemu driver: clean up logging; fail unsupported features on Windows

f734d84

cheeseprocedure force-pushed the f-qemu-graceful-shutdown branch from 046b774 to f734d84 Compare November 3, 2017 22:40

dadgar merged commit 80b434d into hashicorp:master Nov 3, 2017

cheeseprocedure mentioned this pull request Jan 29, 2018

[QEMU] Fixing an unintentional variable shadowing #3798

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023

Qemu driver: graceful shutdown feature #3411

Qemu driver: graceful shutdown feature #3411

Conversation

cheeseprocedure commented Oct 18, 2017

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure commented Nov 1, 2017

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure commented Nov 3, 2017

dadgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseprocedure commented Nov 3, 2017

dadgar commented Nov 3, 2017

github-actions bot commented Mar 18, 2023

cheeseprocedure Nov 3, 2017 •

edited

Loading

cheeseprocedure Nov 2, 2017 •

edited

Loading