From 707458cc8cc78f5162d6ee76d01fc112499313be Mon Sep 17 00:00:00 2001
From: Rick Elrod <rick@elrod.me>
Date: Tue, 7 Jul 2020 17:28:13 -0500
Subject: [PATCH] Make netbsd virtualization facts more specific (#70467)

Change:
Our handling of NetBSD virtualization facts led to facts that were just
plain incorrect. One example is reporting Xen even when the system is
running on something completely different (like KVM).

As stated by the reporter of #69352, NetBSD has a better sysctl setting
to use for this information, machdep.hypervisor.

This PR does the following:

- Try to use machdep.hypervisor sysctl value if the other sysctl values
  we check don't end up with enough information to be useful
- Only look for /dev/xencons and assume Xen if nothing else works
  (Really this should probably return 'unknown' since the file exists on
  non-Xen systems and is not very useful).
- Add a few more patterns (Xen matches and also Hyper-V) to
  VirtualSysctlDetectionMixin#detect_virt_product.

This change is slightly breaking:
- If the first two attempts at using sysctl worked before,
  (machdep.dmi.system-product and machdep.dmi.system-vendor), they will
  continue to work.
- For cases when those values didn't work, previously the existence of
  /dev/xencons was checked, and if found, we reported 'xen' (even on
  non-Xen systems when the file existed). After this PR, we try the
  machdep.hypervisor sysctl key before still falling back to
  /dev/xencons. This means that in some cases, we might go from
  (wrongly) saying "xen" to giving a more accurate value such as "kvm"
  or "Hyper-V".

Test Plan:
- Tested with local NetBSD VM and got 'kvm' instead of 'xen' back.

Tickets:
- Fixes #69352

Signed-off-by: Rick Elrod <rick@elrod.me>
---
 changelogs/fragments/69352-netbsd-virtual-facts.yml    |  2 ++
 docs/docsite/rst/porting_guides/porting_guide_2.11.rst |  2 +-
 lib/ansible/module_utils/facts/virtual/netbsd.py       | 10 +++++++++-
 lib/ansible/module_utils/facts/virtual/sysctl.py       |  5 ++++-
 4 files changed, 16 insertions(+), 3 deletions(-)
 create mode 100644 changelogs/fragments/69352-netbsd-virtual-facts.yml

diff --git a/changelogs/fragments/69352-netbsd-virtual-facts.yml b/changelogs/fragments/69352-netbsd-virtual-facts.yml
new file mode 100644
index 00000000000..eb7e8ef6b76
--- /dev/null
+++ b/changelogs/fragments/69352-netbsd-virtual-facts.yml
@@ -0,0 +1,2 @@
+breaking_changes:
+  - NetBSD virtualization facts (specifically ``ansible_virtualization_type``) now returns a more accurate value by checking the value of the ``machdep.hypervisor`` ``sysctl`` key. This change is breaking because in some cases previously, we would erroneously report ``xen`` even when the target is not running on Xen. This prevents that behavior in most cases. (https://github.com/ansible/ansible/issues/69352)
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.11.rst b/docs/docsite/rst/porting_guides/porting_guide_2.11.rst
index b186be79c5f..d6e1968b493 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.11.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.11.rst
@@ -57,7 +57,7 @@ No notable changes
 Noteworthy module changes
 -------------------------
 
-No notable changes
+* facts - ``ansible_virtualization_type`` now tries to report a more accurate result than ``xen`` when virtualized and not running on Xen.
 
 
 Plugins
diff --git a/lib/ansible/module_utils/facts/virtual/netbsd.py b/lib/ansible/module_utils/facts/virtual/netbsd.py
index 514ef8596ea..eb37b595af4 100644
--- a/lib/ansible/module_utils/facts/virtual/netbsd.py
+++ b/lib/ansible/module_utils/facts/virtual/netbsd.py
@@ -38,7 +38,15 @@ class NetBSDVirtual(Virtual, VirtualSysctlDetectionMixin):
             virtual_vendor_facts = self.detect_virt_vendor('machdep.dmi.system-vendor')
             virtual_facts.update(virtual_vendor_facts)
 
-        if os.path.exists('/dev/xencons'):
+        # The above logic is tried first for backwards compatibility. If
+        # something above matches, use it. Otherwise if the result is still
+        # empty, try machdep.hypervisor.
+        if virtual_facts['virtualization_type'] == '':
+            virtual_vendor_facts = self.detect_virt_vendor('machdep.hypervisor')
+            virtual_facts.update(virtual_vendor_facts)
+
+        if (virtual_facts['virtualization_type'] == '' and
+                os.path.exists('/dev/xencons')):
             virtual_facts['virtualization_type'] = 'xen'
             virtual_facts['virtualization_role'] = 'guest'
 
diff --git a/lib/ansible/module_utils/facts/virtual/sysctl.py b/lib/ansible/module_utils/facts/virtual/sysctl.py
index a159cc15d91..62bd8d406d7 100644
--- a/lib/ansible/module_utils/facts/virtual/sysctl.py
+++ b/lib/ansible/module_utils/facts/virtual/sysctl.py
@@ -38,9 +38,12 @@ class VirtualSysctlDetectionMixin(object):
                 elif out.rstrip() == 'VirtualBox':
                     virtual_product_facts['virtualization_type'] = 'virtualbox'
                     virtual_product_facts['virtualization_role'] = 'guest'
-                elif out.rstrip() == 'HVM domU':
+                elif re.match('(HVM domU|XenPVH|XenPV|XenPVHVM).*', out):
                     virtual_product_facts['virtualization_type'] = 'xen'
                     virtual_product_facts['virtualization_role'] = 'guest'
+                elif out.rstrip() == 'Hyper-V':
+                    virtual_product_facts['virtualization_type'] = 'Hyper-V'
+                    virtual_product_facts['virtualization_role'] = 'guest'
                 elif out.rstrip() == 'Parallels':
                     virtual_product_facts['virtualization_type'] = 'parallels'
                     virtual_product_facts['virtualization_role'] = 'guest'