Fix SystemVMs running in Xen HVM mode are not configured (#2760) by dpassante · Pull Request #2824 · apache/cloudstack

dpassante · 2018-08-28T08:43:53Z

Set hypervisor to xen-hvm when virt-what detects both HyperV cpuid and xen-domU

Description

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Fixes: #2760

Screenshots (if appropriate):

How Has This Been Tested?

Tested on Cloudstack 4.11.1 + XenServer 7.1.1 with a patched systemvm template.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

rafaelweingartner · 2018-08-28T13:21:25Z

 hypervisor() {
-  local try=$([ -x /usr/sbin/virt-what ] && virt-what | tail -1)
-  [ "$try" != "" ] && echo $try && return 0
+  [ -x /usr/sbin/virt-what ] && local facts=( $(virt-what) )


I have a question here. What is the return if it is a PV VM? Is it only xen-pv?

Good catch, according to the virt-what script, the returns should be:
Xen PV:

xen + xen-domU

or just xen

Xen HVM:

xen + xen-hvm

virt-what never returns xen-pv.
I think the xen-pv case in the get_boot_params() function should be xen-pv|xen-domU|xen) to ensure that any PV VMs read their configuration from /proc/cmdline.

Also, when I use virt-what in an HVM machine I get:

xen xen-hvm

virt-what always returns xen + xen-hvm on Xen HVM guests when it's cpuid helper detects XenVMMXenVMM signature.
You can have a look here to see in details how virt-what detects Xen guests.

yadvr · 2018-08-30T09:05:13Z

+  [ -x /usr/sbin/virt-what ] && local facts=( $(virt-what) )
+  if [ "$facts" != "" ]; then
+    # Xen HVM is recognized as Hyperv when Viridian extensions are enabled
+    if [ "${facts[-1]}" == "xen-domU" ] && [ "${facts[0]}" == "hyperv" ]; then


We have support for hyperv, it may be wrong to return 'xen-hvm' when fact is hyperv? I'm not sure how this may affect hyperv.

If I'm not mistaken, we will get xen-hvm only if the latest fact returned by virt-what is "xen-domU" when the first fact is hyperv. On all other cases, we will get the latest fact.

yadvr · 2018-08-30T09:06:09Z

-  [ "$try" != "" ] && echo $try && return 0
+  [ -x /usr/sbin/virt-what ] && local facts=( $(virt-what) )
+  if [ "$facts" != "" ]; then
+    # Xen HVM is recognized as Hyperv when Viridian extensions are enabled


What is a Viridian extension?

I'm not an expert about it but, as I understand, it's a Xen feature which speeds up Windows guests by providing some hyperV extensions. Viridian was the codename of HyperV.

yadvr · 2018-08-30T09:07:25Z


 get_boot_params() {
  case $HYPERVISOR in
-     xen-pv|xen-domU)


What if xen-domU is returned, removing the case would mean we no longer could process hypervisor where xen-domU would be returned.

Right, it's my mistake. This line should not be changed.

In that case, please remove/revert the change @dpassante

yadvr

Outstanding questions

dpassante · 2018-08-30T10:26:49Z

I also ran virt-what (original and 1.15 versions) in an older PV SSVM (ACS 4.3) running on XenServer 6.2 and I get:
xen
xen-hvm

I'm sure the VM is a PV guest since the following lines are present in the dmesg:

Booting paravirtualized kernel on Xen
Xen: using vcpuop timer interface
Initialising Xen virtual ethernet driver.

and there is no reference to HVM.

Perhaps we should use the dmesg method that seems to be more accurate than virt-what for Xen guests?

yadvr · 2018-08-31T08:34:48Z

@dpassante sounds good to me if you want to grep dmesg for accuracy

- Set hypervisor to xen-hvm when virt-what detects both HyperV cpuid and xen-domU

yadvr

LGTM, but this will require regression testing across all supported hypervisors. There is no way to do it without building a new systemvmtemplate, so I'll merge this tentatively. Note: If something comes up in testing, we may have to revert it.

yadvr · 2018-09-04T10:57:35Z

@DaanHoogland @nvazquez @PaulAngus @GabrielBrascher @wido @khos2ow can you please review this? I won't be able to test it without building a new systemvmtemplate, if we agree we can merge this based on review and run testing after #2829 is merged, revert if necessary.

DaanHoogland

one small doubt, no objections

DaanHoogland · 2018-09-04T16:06:53Z

+    fi
+  fi
+
+  grep -q QEMU /proc/cpuinfo  && echo "kvm" && return 0


I realize this is old code moved but it does not seem reliable or future proof. How about a call to virsh (--version or so)?

virsh is only the hypervisor host, not in the guest VM.

khos2ow

I'm approving this PR with one comment below. IMO if condition on line 51 can be removed if by removing, it still works on Hyper-V which I haven't tested.

khos2ow

I'm approving this PR with one comment below. IMO if condition on line 51 can be removed if by removing, it still works on Hyper-V which I haven't tested.

khos2ow · 2018-09-05T03:14:44Z

+  [ -x /usr/sbin/virt-what ] && local facts=( $(virt-what) )
+  if [ "$facts" != "" ]; then
+    # Xen HVM is recognized as Hyperv when Viridian extensions are enabled
+    if [ "${facts[-1]}" == "xen-domU" ] && [ "${facts[0]}" == "hyperv" ]; then


From XenServer only point of view, this block of code is redundant. Because result of dmesg | grep -q "Xen HVM" on HVM instance on XenServer, line 41, will work 100% of the time. The gotcha point of this function is to look for hvm on xen the first thing and if not a match let the rest of the function try to detect the type.

A while back I had the same issue and did a non-conclusive investigation about how virt-what works, and was actually getting all sorts of results/order on HVM and PV instance on Xen Server. Essentially the workaround I came up was the one I mentioned above.

khos2ow · 2018-09-05T03:21:06Z

@rhtyd IIRC you don't need to build a new systemvm template for this to be pushed to instances, this will be populated and pushed to instances with systemvm.iso. A new set of rpms will suffice.

yadvr · 2018-09-05T11:36:43Z

@khos2ow the changes are in cloud-early-config which patches using systemvm.iso, so the first time a systemvm boots it won't be able to patch and use the new cloud-early-config from a (new) systemvm.iso. On after 2+ boots, it will be able to use any new changes in cloud-early-config. Therefore a systemvmtemplate change is needed.

yadvr · 2018-09-07T10:40:32Z

Merging based on commentary, we'll proceed with testing a new systemvmtemplate from latest 4.11 branch and conclude next week if this needs to be reverted at all.

rafaelweingartner approved these changes Aug 28, 2018

View reviewed changes

yadvr reviewed Aug 30, 2018

View reviewed changes

yadvr requested changes Aug 30, 2018

View reviewed changes

fix SystemVMs running in Xen HVM mode are not configured (apache#2760)

470bf3f

- Set hypervisor to xen-hvm when virt-what detects both HyperV cpuid and xen-domU

dpassante force-pushed the fix_viridian_vrouter branch from 26ce558 to 470bf3f Compare August 31, 2018 09:57

Try to detect Xen mode with dmesg prior to virt-what for accuracy.

a393ac0

yadvr approved these changes Sep 4, 2018

View reviewed changes

yadvr requested review from DaanHoogland, GabrielBrascher, PaulAngus, nvazquez and wido September 4, 2018 08:35

yadvr added this to the 4.11.2.0 milestone Sep 4, 2018

DaanHoogland approved these changes Sep 4, 2018

View reviewed changes

khos2ow approved these changes Sep 5, 2018

View reviewed changes

yadvr merged commit 4b4555b into apache:4.11 Sep 7, 2018

dpassante deleted the fix_viridian_vrouter branch September 17, 2018 07:11

Conversation

dpassante commented Aug 28, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

dpassante commented Aug 30, 2018

Uh oh!

yadvr commented Aug 31, 2018

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 4, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khos2ow left a comment

Choose a reason for hiding this comment

Uh oh!

khos2ow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khos2ow Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khos2ow commented Sep 5, 2018

Uh oh!

yadvr commented Sep 5, 2018

Uh oh!

yadvr commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yadvr Aug 30, 2018 •

edited

Loading

khos2ow left a comment •

edited

Loading

khos2ow Sep 5, 2018 •

edited

Loading