Implement backend healthcheck #948

townsend2010 · 2019-08-08T18:55:19Z

This enables the backends to define how they check if their prerequisites are met before running and to also handle error conditions better and surface those to the user,

Fixes #6, Fixes #770, Fixes #775, Fixes #811

townsend2010 · 2019-08-08T21:00:06Z

Tests are segfaulting...will look tomorrow.

Also, only run the check if there are no instances running.

Also, run check on any CPU type

This will check if /dev/kvm is already in use before trying to launch/start an instance. Fixes #6

On any calls into the Libvirt backend, try a connection to libvirtd to ensure it is there instead of keeping a presistent connection that can become stale. Also, modify the whole state machine to better get the actual current state from libvirt. Modify and add new tests as seen fit for these modifications.

…ding Fixes #811

codecov · 2019-08-13T19:38:32Z

Codecov Report

Merging #948 into master will increase coverage by 0.14%.
The diff coverage is 63.86%.

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   69.49%   69.63%   +0.14%     
==========================================
  Files         192      192              
  Lines        6788     6790       +2     
==========================================
+ Hits         4717     4728      +11     
+ Misses       2071     2062       -9

Impacted Files	Coverage Δ
...backends/libvirt/libvirt_virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
...tform/backends/qemu/qemu_virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
include/multipass/virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	`85.71% <0%> (-2.19%)`	⬇️
...c/platform/backends/shared/linux/backend_utils.cpp	`55.84% <42.85%> (+11.79%)`	⬆️
src/daemon/daemon.cpp	`30.44% <55.55%> (-0.06%)`	⬇️
...tform/backends/libvirt/libvirt_virtual_machine.cpp	`52.12% <69.01%> (+8.67%)`	⬆️
...ckends/libvirt/libvirt_virtual_machine_factory.cpp	`48% <72.72%> (-2%)`	⬇️
...c/platform/backends/shared/linux/linux_process.cpp	`77.55% <0%> (-7.64%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d28b857...b6d0ae7. Read the comment docs.

codecov · 2019-08-13T19:38:32Z

Codecov Report

Merging #948 into master will increase coverage by 0.21%.
The diff coverage is 71.09%.

@@            Coverage Diff            @@
##           master    #948      +/-   ##
=========================================
+ Coverage   69.49%   69.7%   +0.21%     
=========================================
  Files         192     192              
  Lines        6788    6852      +64     
=========================================
+ Hits         4717    4776      +59     
- Misses       2071    2076       +5

Impacted Files	Coverage Δ
...backends/libvirt/libvirt_virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
...tform/backends/qemu/qemu_virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
include/multipass/virtual_machine_factory.h	`0% <ø> (ø)`	⬆️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	`87.34% <0%> (-0.56%)`	⬇️
src/daemon/daemon.cpp	`30.65% <55.55%> (+0.15%)`	⬆️
...tform/backends/libvirt/libvirt_virtual_machine.cpp	`52.12% <69.01%> (+8.67%)`	⬆️
...ckends/libvirt/libvirt_virtual_machine_factory.cpp	`56.66% <80.64%> (+6.66%)`	⬆️
...c/platform/backends/shared/linux/backend_utils.cpp	`55.05% <85.71%> (+11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d28b857...4eba7c8. Read the comment docs.

townsend2010 · 2019-08-13T19:40:24Z

I'm working on a fix for #325 and was going to include it in this PR, but that should be in its own PR. So this is good for review 😁

Saviq · 2019-08-14T10:53:13Z

Having broken KVM (renamed the module files), I'm getting this:

$ multipass start snapcraft-multipass
start failed: modprobe: ERROR: ../libkmod/libkmod-module.c:192 kmod_module_parse_depline() ctx=0x55dabd9ea260 path=/lib/modules/5.0.0-25-generic/kernel/arch/x86/kvm/kvm.ko error=No such file or directory
modprobe: ERROR: ../libkmod/libkmod-module.c:192 kmod_module_parse_depline() ctx=0x55dabd9ea260 path=/lib/modules/5.0.0-25-generic/kernel/arch/x86/kvm/kvm.ko error=No such file or directory
modprobe: ERROR: could not insert 'kvm_intel': Unknown symbol in module, or unknown parameter (see dmesg)

Could we wrap this in a better error message?

Saviq · 2019-08-14T10:59:26Z

Having disabled VT, qemu behaves nice:

$ multipass start snapcraft-multipass
start failed: KVM (vmx) is disabled by your BIOS.                               
Enter your BIOS setup and enable Virtualization Technology (VT).

But after switching to libvirt:

$ multipass start snapcraft-multipass           
start failed: invalid domain pointer in virDomainCreate

src/platform/backends/shared/linux/backend_utils.cpp

townsend2010 · 2019-08-14T12:51:26Z

@Saviq

Could we wrap this in a better error message?

Yeah, I need to redirect modprobe's output to /dev/null there.

But after switching to libvirt:

Hmm, seems libvirt is quite lacking when it comes to checking for the actually support of the hypervisor. I'll see if there is something in the native API, and if not, then I'll just have the libvirt backend health check run the check_kvm_support script as well.

- check_kvm_support: Output "modprobe" to /dev/null - libvirt: Run check_kvm_support in health check - check_if_kvm_is_in_use: Use "virtual machine manager" instead of "hypervisor"

townsend2010 · 2019-08-14T13:40:34Z

@Saviq,

I addressed your concerns and pushed.

Saviq · 2019-08-14T15:35:53Z

Having broken KVM (renamed the module files), I'm getting this:

So that's now (both qemu and libvirt):

$ multipass start snapcraft-multipass
start failed:

meh.

townsend2010 · 2019-08-14T16:24:21Z

meh.

Oh right, I have the script set to stop execution on errors, so it doesn't get to the output of not being able to load the module. I'll fix that 😁

gerboland

Couple of niggles inline. Still testing

gerboland · 2019-08-15T13:31:40Z

src/platform/backends/qemu/check_kvm_support

 check_kvm_module

-echo "Unable to load '$kvm_mod'."
+echo "Unable to load kvm support. Please ensure kvm is installed on your machine."


Nitpick: you capitalize KVM in other messages above. Why not here? Also, can KVM actually be installed?

Checking the return code of the modprobe, and if failure, printing its error message would help people debug the issue. And us, if people paste that output into a bug.

you capitalize KVM in other messages above. Why not here?

Oops, I will do that.

Also, can KVM actually be installed?

Well, it's a kernel module, so perhaps it's not compiled in the kernel being used? I'm not sure how else to word that without being too technical.

Checking the return code of the modprobe, and if failure, printing its error message would help people debug the issue. And us, if people paste that output into a bug.

I originally had the output of modprobe, but @Saviq pointed out that it's not what we want in #811 and #948 (comment). So I'm not sure how else to handle that other to be generic.

Ok, pushed the change for capitalizing 'KVM'.

You mean @Saviq 's comment "Could we wrap this in a better error message?" ?

Could we compromise by printing the modprobe error to stderr, and setting QProcess::ForwardedErrorChannel so that it'll end up in the journal?

I just hate throwing away a useful error message.

src/platform/backends/shared/linux/backend_utils.cpp

- check_kvm_support: Capitalize 'KVM' - backend::check_for_kvm_support(): Detect if check_kvm_support() is in multipassd's PATH

townsend2010 · 2019-08-15T14:53:01Z

@gerboland,
I also pushed the change to catch if check_kvm_support is not in multipassd's PATH.

gerboland

I've tested it as much as I can, and healthcheck appears to do the job for both backends. libvirt improvements welcome!

townsend2010 · 2019-08-15T15:57:21Z

bors r=Saviq,gerboland

948: Implement backend healthcheck r=Saviq,gerboland a=townsend2010 This enables the backends to define how they check if their prerequisites are met before running and to also handle error conditions better and surface those to the user, Fixes #6, Fixes #770, Fixes #775, Fixes #811 Co-authored-by: Chris Townsend <[email protected]>

bors · 2019-08-15T16:32:50Z

Build failed

continuous-integration/travis-ci/push

townsend2010 force-pushed the implement-backend-healthcheck branch from b4a68ed to b8dcc74 Compare August 9, 2019 13:35

townsend2010 added the no-merge label Aug 12, 2019

Chris Townsend added 7 commits August 13, 2019 15:37

virtual_machine_factory: Rename hypervisor check function

48f1d7e

Also, only run the check if there are no instances running.

check_kvm_support: Add support for other CPU types

f883d9e

backend_utils: Rename function for checking if kvm is enabled

8ff7e52

Also, run check on any CPU type

backend_utils: Add function to test if /dev/kvm is in use

7e6d486

This will check if /dev/kvm is already in use before trying to launch/start an instance. Fixes #6

check_kvm_support: Use better error message when msr module fails loa…

893a8bc

…ding Fixes #811

tests: Fix segfaulting test due to new libvirt mock change

b6d0ae7

townsend2010 force-pushed the implement-backend-healthcheck branch from b8dcc74 to b6d0ae7 Compare August 13, 2019 19:38

townsend2010 removed the no-merge label Aug 13, 2019

backend/libvirt: Fix build & test failure due to last rebase on master

89017af

Saviq mentioned this pull request Aug 13, 2019

Windows instructions on Discourse are out of date #963

Closed

Saviq reviewed Aug 14, 2019

View reviewed changes

src/platform/backends/shared/linux/backend_utils.cpp Outdated Show resolved Hide resolved

backends: Changes based on review feedback

0aabf6a

- check_kvm_support: Output "modprobe" to /dev/null - libvirt: Run check_kvm_support in health check - check_if_kvm_is_in_use: Use "virtual machine manager" instead of "hypervisor"

check_kvm_support: Drop 'set -e' as we don't want to error immediately

e707ce6

gerboland reviewed Aug 15, 2019

View reviewed changes

backends: A couple of fixes based on code review

4eba7c8

- check_kvm_support: Capitalize 'KVM' - backend::check_for_kvm_support(): Detect if check_kvm_support() is in multipassd's PATH

gerboland approved these changes Aug 15, 2019

View reviewed changes

townsend2010 merged commit 4eba7c8 into master Aug 15, 2019

bors bot deleted the implement-backend-healthcheck branch August 15, 2019 16:13

Implement backend healthcheck #948

Implement backend healthcheck #948

Uh oh!

Conversation

townsend2010 commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

townsend2010 commented Aug 8, 2019

Uh oh!

codecov bot commented Aug 13, 2019

Codecov Report

Uh oh!

codecov bot commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

townsend2010 commented Aug 13, 2019

Uh oh!

Saviq commented Aug 14, 2019

Uh oh!

Saviq commented Aug 14, 2019

Uh oh!

Uh oh!

townsend2010 commented Aug 14, 2019

Uh oh!

townsend2010 commented Aug 14, 2019

Uh oh!

Saviq commented Aug 14, 2019

Uh oh!

townsend2010 commented Aug 14, 2019

Uh oh!

gerboland left a comment

Choose a reason for hiding this comment

Uh oh!

gerboland Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

gerboland Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

townsend2010 Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

townsend2010 Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

gerboland Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

townsend2010 commented Aug 15, 2019

Uh oh!

gerboland left a comment

Choose a reason for hiding this comment

Uh oh!

townsend2010 commented Aug 15, 2019

Uh oh!

bors bot commented Aug 15, 2019

Build failed

Uh oh!

Uh oh!

townsend2010 commented Aug 8, 2019 •

edited

Loading

codecov bot commented Aug 13, 2019 •

edited

Loading