Skip to content
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

Test QEMU graceful shutdown #3819

Merged
merged 3 commits into from
Feb 12, 2018
Merged

Test QEMU graceful shutdown #3819

merged 3 commits into from
Feb 12, 2018

Conversation

schmichael
Copy link
Member

@schmichael schmichael commented Jan 31, 2018

There's a lot going on here as this feature has proven quite hard to test:

  • Add a new Alpine QEMU image was added as the existing Linux image doesn't shutdown on the system_powerdown command.
  • Use git-lfs to manage the new 40MB Alpine image and hopefully make future image/large-file changes easier to manage.
  • Add a README to QEMU's test images as I had to build an Alpine image myself.
  • Add a new SlowTest(t) test helper to skip slow tests unless NOMAD_SLOW_TEST=1 is set (it is set in Travis). Right now only the QemuDriver_GracefulShutdown test has this check as it takes 40s or more to run and adds about 20s to the driver packages run time even when run in parallel with other tests.

Uses an Alpine image which supports ACPI poweroff signal handling.
Handling is only enabled after the VM has booted, so this test blocks
until sshd starts before issuing the command.
@schmichael schmichael changed the title [DO NOT MERGE] Qemu graceful shutdown alpine Test QEMU graceful shutdown Jan 31, 2018
Hopefully we can reuse the SkipSlow helper elsewhere.
GNUmakefile Outdated
@@ -245,6 +245,7 @@ test-nomad: dev ## Run Nomad test suites
@echo "==> Running Nomad test suites:"
@NOMAD_TEST_RKT=1 \
go test $(if $(VERBOSE),-v) \
-v \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this, because all the changes I made in PR #3626 were because we thought that Travis should not have verbose logging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks for catching that. The entire 7b8d9d4 commit was intended to be a temporary test and reverted. Killing it now.

Driver: "qemu",
Config: map[string]interface{}{
"image_path": "linux-0.2.img",
"accelerator": "tcg",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is accelerator and why do we not need it anymore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the default if accelerator is not specified:

nomad/client/driver/qemu.go

Lines 218 to 221 in 32174fd

accelerator := "tcg"
if d.driverConfig.Accelerator != "" {
accelerator = d.driverConfig.Accelerator
}

QEMU has a number of accelerators depending on the host and vm architectures. I think tcg is the best you can get when you run in as many different environments as our tests do (linux hosts, virtualbox, vmware, travis).

At any rate this test shouldn't have anything to do with the accelerator being used, so I'd rather let it use the default than imply the test actually cares about the accelerator by specifying it.

Copy link
Contributor

@chelseakomlo chelseakomlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small tweaks, but most importantly, add the ability to optionally run slow tests locally.


via https://en.wikibooks.org/wiki/QEMU/Images

Does not support graceful shutdown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we should keep both images around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other one will live forever in git's history, so there's not much benefit in dropping it. It's also very small and fast, and so for tests that don't care about what's running in the VM it's ideal.


select {
case <-resp.Handle.WaitCh():
// VM exited gracefully!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe test-level logging?

@@ -14,7 +14,7 @@ if [ "$RUN_STATIC_CHECKS" ]; then
fi
fi

make test
NOMAD_SLOW_TEST=1 make test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define a make flag to optionally run these locally

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we need a make flag, eg I set TRAVIS=1 to pretend to be travis when running locally, and can similarly set NOMAD_SLOW_TEST=1 too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does TRAVIS=1 do? I know we intentionally lower the parallelism and raise timeouts when running on Travis which you do not want when running this slow test as it would only make it run more slowly.

I was trying to follow the example of NOMAD_TEST_RKT with this, although I guess their naming schemes differ.

@dadgar
Copy link
Contributor

dadgar commented Feb 5, 2018

LGTM

@schmichael schmichael merged commit ea860e9 into hashicorp:master Feb 12, 2018
@schmichael schmichael deleted the qemu-graceful-shutdown-alpine branch February 12, 2018 20:32
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants