Merge lp:~ack/landscape-client/detect-openstack-vm into lp:~landscape/landscape-client/trunk

[Alberto Donato]

This changes the detection logic for kvm virtual machines, looking for the "Bochs" as reported by OpenStack Folsom) or "OpenStack" (as reported by Grizzly) vendor in /sys rather than the CPU model.
It also refactors a bit the existing logic, and only tries to detect HyperV/VmWare/KVM if the "hypervisor" flag is present in /proc/cpuinfo, which is meant to report that the machine is running under an hypervisor.

This changes the detection logic for kvm virtual machines, looking for the "Bochs" as reported by OpenStack Folsom) or "OpenStack" (as reported by Grizzly) vendor in /sys rather than the CPU model.
It also refactors a bit the existing logic, and only tries to detect HyperV/VmWare/KVM if the "hypervisor" flag is present in /proc/cpuinfo, which is meant to report that the machine is running under an hypervisor.

[Björn Tillenius (bjornt)]

Looks good! I have some nitpicks only.

[1]

Have you confirmed that this works on lucid and precise?

[2]

+ if line.startswith("flags") and "hypervisor" in line:
+ has_hypervisor_flag = True
+ break

Can multiple lines start with flags? If not, it might be better to
write it as:

    if line.startswith("flags"):
        has_hypervisor_flag = "hypervisor" in line
        break

[3]

+ if has_hypervisor_flag:
+ sys_vendor_path = join_root_path("sys/class/dmi/id/sys_vendor")
+ if os.path.exists(sys_vendor_path):
+ content = read_file(sys_vendor_path)
+ if "VMware, Inc." in content:

I tend to prefer early returns. I.e. at the top, return "" if there is
not hypervisor flag. That way you don't have to indent the main logic.

review: Approve

[Alberto Donato (ack)]

> Looks good! I have some nitpicks only.
>
> [1]
>
> Have you confirmed that this works on lucid and precise?
>

Yeah, I tried it on Canonistack.

>
> [2]
>
> + if line.startswith("flags") and "hypervisor" in line:
> + has_hypervisor_flag = True
> + break
>
> Can multiple lines start with flags? If not, it might be better to
> write it as:
>
> if line.startswith("flags"):
> has_hypervisor_flag = "hypervisor" in line
> break

There might actually be more than one "flags" line, it's one for each stanza in /proc/cpuinfo (so one for each CPU core/thread).

>
> [3]
>

Fixed.

[Geoff Teale (tealeg)]

+1 Looks good.

[1] An empty what exactly? I think you the word "string". ;-)

+ L{get_vm_info} should return an empty when the sys_vendor is unknown.

review: Approve

=== modified file 'landscape/lib/tests/test_vm_info.py'
--- landscape/lib/tests/test_vm_info.py	2012-12-06 10:16:31 +0000
+++ landscape/lib/tests/test_vm_info.py	2013-06-28 08:49:23 +0000
@@ -7,7 +7,7 @@
 
 class VMInfoTest(LandscapeTest):
 
-    sample_kvm_cpuinfo = """
+    sample_cpuinfo = """
 processor	: 0
 vendor_id	: GenuineIntel
 cpu family	: 6
@@ -20,7 +20,7 @@
 fpu_exception	: yes
 cpuid level	: 4
 wp		: yes
-flags		: fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca
+flags		: {flags}
 bogomips	: 5306.22
 clflush size	: 64
 cache_alignment	: 64
@@ -28,146 +28,161 @@
 power management:
 """
 
+    def setUp(self):
+        super(VMInfoTest, self).setUp()
+        self.root_path = self.makeDir()
+        self.proc_path = self.makeDir(
+            path=os.path.join(self.root_path, "proc"))
+        self.sys_path = self.makeDir(path=os.path.join(self.root_path, "sys"))
+        self.proc_sys_path = self.makeDir(
+            path=os.path.join(self.proc_path, "sys"))
+
+    def make_cpuinfo(self, flags=""):
+        """Build a sample /proc/cpuinfo with specified processor flags."""
+        cpuinfo_path = os.path.join(self.root_path, "proc/cpuinfo")
+        self.makeFile(
+            path=cpuinfo_path, content=self.sample_cpuinfo.format(flags=flags))
+
     def test_get_vm_info_empty_when_no_virtualization_is_found(self):
         """
         L{get_vm_info} should be empty when there's no virtualisation.
         """
-        root_path = self.makeDir()
-        self.assertEqual(u"", get_vm_info(root_path=root_path))
+        self.assertEqual(u"", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_openvz_when_proc_vz_exists(self):
         """
         L{get_vm_info} should return 'openvz' when /proc/vz exists.
         """
-        root_path = self.makeDir()
-        proc_path = os.path.join(root_path, "proc")
-        self.makeDir(path=proc_path)
-
-        proc_vz_path = os.path.join(proc_path, "vz")
+        proc_vz_path = os.path.join(self.proc_path, "vz")
         self.makeFile(path=proc_vz_path, content="foo")
 
-        self.assertEqual("openvz", get_vm_info(root_path=root_path))
+        self.assertEqual("openvz", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_xen_when_proc_sys_xen_exists(self):
         """
         L{get_vm_info} should return 'xen' when /proc/sys/xen exists.
         """
-        root_path = self.makeDir()
-        proc_path = os.path.join(root_path, "proc")
-        self.makeDir(path=proc_path)
-
-        proc_sys_path = os.path.join(proc_path, "sys")
-        self.makeDir(path=proc_sys_path)
-
-        proc_sys_xen_path = os.path.join(proc_sys_path, "xen")
+        proc_sys_xen_path = os.path.join(self.proc_sys_path, "xen")
         self.makeFile(path=proc_sys_xen_path, content="foo")
 
-        self.assertEqual("xen", get_vm_info(root_path=root_path))
+        self.assertEqual("xen", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_xen_when_sys_bus_xen_is_non_empty(self):
         """
         L{get_vm_info} should return 'xen' when /sys/bus/xen exists and has
         devices.
         """
-        root_path = self.makeDir()
-        sys_path = os.path.join(root_path, "sys")
-        self.makeDir(path=sys_path)
-
-        sys_bus_path = os.path.join(sys_path, "bus")
-        self.makeDir(path=sys_bus_path)
-
-        sys_bus_xen_path = os.path.join(sys_bus_path, "xen")
-        self.makeDir(path=sys_bus_xen_path)
-
-        devices_xen_path = os.path.join(sys_bus_xen_path, "devices")
+        devices_xen_path = os.path.join(self.sys_path, "bus/xen/devices")
         self.makeDir(path=devices_xen_path)
-
         foo_devices_path = os.path.join(devices_xen_path, "foo")
         self.makeFile(path=foo_devices_path, content="bar")
 
-        self.assertEqual("xen", get_vm_info(root_path=root_path))
+        self.assertEqual("xen", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_xen_when_proc_xen_exists(self):
         """
         L{get_vm_info} should return 'xen' when /proc/xen exists.
         """
-        root_path = self.makeDir()
-        proc_path = os.path.join(root_path, "proc")
-        self.makeDir(path=proc_path)
-
-        proc_xen_path = os.path.join(proc_path, "xen")
+        proc_xen_path = os.path.join(self.proc_path, "xen")
         self.makeFile(path=proc_xen_path, content="foo")
 
-        self.assertEqual("xen", get_vm_info(root_path=root_path))
-
-    def test_get_vminfo_is_kvm_when_qemu_is_found_in_proc_cpuinfo(self):
-        """
-        L{get_vm_info} should return 'kvm' when QEMU Virtual CPU is found in
-        /proc/cpuinfo.
-        """
-        root_path = self.makeDir()
-        proc_path = os.path.join(root_path, "proc")
-        self.makeDir(path=proc_path)
-
-        cpuinfo_path = os.path.join(proc_path, "cpuinfo")
-        self.makeFile(path=cpuinfo_path, content=self.sample_kvm_cpuinfo)
-
-        self.assertEqual("kvm", get_vm_info(root_path=root_path))
-
-    def test_get_vm_info_is_empty_when_qemu_is_not_found_in_proc_cpuinfo(self):
-        """
-        L{get_vm_info} should have an empty string when QEMU Virtual CPU is not
-        found in /proc/cpuinfo.
-        """
-        root_path = self.makeDir()
-        proc_path = os.path.join(root_path, "proc")
-        self.makeDir(path=proc_path)
-
-        cpuinfo_path = os.path.join(proc_path, "cpuinfo")
-        self.makeFile(path=cpuinfo_path, content="foo")
-        self.assertEqual("", get_vm_info(root_path=root_path))
+        self.assertEqual("xen", get_vm_info(root_path=self.root_path))
+
+    def test_get_vm_info_is_empty_when_no_hypervisor_in_proc_cpuinfo(self):
+        """
+        L{get_vm_info} returns an empty string when the "hypervisor" flag is
+        not found in found in /proc/cpuinfo.
+        """
+        self.make_cpuinfo(flags="fpu vme")
+
+        # The content of sys_vendor is not checked if the "hypervisor" flag is
+        # not present.
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"), content="Bochs")
+
+        self.assertEqual("", get_vm_info(root_path=self.root_path))
+
+    def test_get_vm_info_with_bochs_sys_vendor(self):
+        """
+        L{get_vm_info} should return "kvm" when we detect the sys_vendor is
+        Bochs.
+        """
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"), content="Bochs")
+
+        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+
+    def test_get_vm_info_with_openstack_sys_vendor(self):
+        """
+        L{get_vm_info} should return "kvm" when we detect the sys_vendor is
+        Openstack.
+        """
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"),
+            content="OpenStack Foundation")
+
+        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_vmware_sys_vendor(self):
         """
         L{get_vm_info} should return "vmware" when we detect the sys_vendor is
         VMware Inc.
         """
-        root_path = self.makeDir()
-        dmi_path = os.path.join(root_path, "sys", "class", "dmi", "id")
-        os.makedirs(dmi_path)
-        fd = file(os.path.join(dmi_path, "sys_vendor"), "w")
-        fd.write("VMware, Inc.")
-        fd.close()
-        self.assertEqual("vmware", get_vm_info(root_path=root_path))
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"), content="VMware, Inc.")
+
+        self.assertEqual("vmware", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_empty_without_xen_devices(self):
         """
         L{get_vm_info} returns an empty string if the /sys/bus/xen/devices
         directory exists and but doesn't contain any file.
         """
-        root_path = self.makeDir()
-        sys_path = os.path.join(root_path, "sys")
-        self.makeDir(path=sys_path)
-
-        sys_bus_path = os.path.join(sys_path, "bus")
-        self.makeDir(path=sys_bus_path)
-
-        sys_bus_xen_path = os.path.join(sys_bus_path, "xen")
-        self.makeDir(path=sys_bus_xen_path)
-
-        devices_xen_path = os.path.join(sys_bus_xen_path, "devices")
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        devices_xen_path = os.path.join(self.sys_path, "bus/xen/devices")
         self.makeDir(path=devices_xen_path)
 
-        self.assertEqual("", get_vm_info(root_path=root_path))
+        self.assertEqual("", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_microsoft_sys_vendor(self):
         """
         L{get_vm_info} returns "hyperv" if the sys_vendor is Microsoft.
         """
-        root_path = self.makeDir()
-        dmi_path = os.path.join(root_path, "sys", "class", "dmi", "id")
-        os.makedirs(dmi_path)
-        fd = file(os.path.join(dmi_path, "sys_vendor"), "w")
-        fd.write("Microsoft Corporation")
-        fd.close()
-        self.assertEqual("hyperv", get_vm_info(root_path=root_path))
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"),
+            content="Microsoft Corporation")
+        self.assertEqual("hyperv", get_vm_info(root_path=self.root_path))
+
+    def test_get_vm_info_with_other_vendor(self):
+        """
+        L{get_vm_info} should return an empty string when the sys_vendor is
+        unknown.
+        """
+        self.make_cpuinfo(flags="fpu hypervisor vme")
+
+        dmi_path = os.path.join(self.root_path, "sys/class/dmi/id")
+        self.makeDir(path=dmi_path)
+        self.makeFile(
+            path=os.path.join(dmi_path, "sys_vendor"),
+            content="Some other vendor")
+
+        self.assertEqual("", get_vm_info(root_path=self.root_path))

=== modified file 'landscape/lib/vm_info.py'
--- landscape/lib/vm_info.py	2012-12-06 10:09:19 +0000
+++ landscape/lib/vm_info.py	2013-06-28 08:49:23 +0000
@@ -3,6 +3,8 @@
 """
 import os
 
+from landscape.lib.fs import read_file
+
 
 def get_vm_info(root_path="/"):
     """
@@ -16,11 +18,10 @@
 
     xen_paths = ["proc/sys/xen", "proc/xen"]
     xen_paths = map(join_root_path, xen_paths)
+    vz_path = join_root_path("proc/vz")
 
-    vz_path = os.path.join(root_path, "proc/vz")
     if os.path.exists(vz_path):
         return "openvz"
-
     elif filter(os.path.exists, xen_paths):
         return "xen"
 
@@ -30,27 +31,28 @@
     if os.path.isdir(sys_xen_path) and os.listdir(sys_xen_path):
         return "xen"
 
-    cpu_info_path = os.path.join(root_path, "proc/cpuinfo")
+    has_hypervisor_flag = False
+    cpu_info_path = join_root_path("proc/cpuinfo")
     if os.path.exists(cpu_info_path):
-        try:
-            fd = open(cpu_info_path)
-            cpuinfo = fd.read()
-            if "QEMU Virtual CPU" in cpuinfo:
-                return "kvm"
-        finally:
-            fd.close()
-
-    sys_vendor_path = os.path.join(root_path, "sys", "class", "dmi", "id",
-                                   "sys_vendor")
-    if os.path.exists(sys_vendor_path):
-        try:
-            fd = open(sys_vendor_path)
-            file_content = fd.read()
-            if "VMware, Inc." in file_content:
-                return "vmware"
-            elif "Microsoft Corporation" in file_content:
-                return "hyperv"
-        finally:
-            fd.close()
+        content = read_file(cpu_info_path)
+        for line in content.split("\n"):
+            if line.startswith("flags") and "hypervisor" in line:
+                has_hypervisor_flag = True
+                break
+
+    if not has_hypervisor_flag:
+        return ""
+
+    sys_vendor_path = join_root_path("sys/class/dmi/id/sys_vendor")
+    if not os.path.exists(sys_vendor_path):
+        return ""
+
+    content = read_file(sys_vendor_path)
+    if "VMware, Inc." in content:
+        return "vmware"
+    elif "Microsoft Corporation" in content:
+        return "hyperv"
+    elif "Bochs" in content or "OpenStack" in content:
+        return "kvm"
 
     return ""