Merge lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message into lp:~landscape/landscape-client/trunk

[Fernando Correa Neto]

This branch adds and extra optional field, vm-info, to computer info messages, so that the server knows if the computer running the client, is virtualized or not.

[Thomas Herve (therve)]

[1] You don't have any specific tests for _get_vm_info? I thought that was the point of passing root_path.

[2]
+ xen_info = [u"proc/sys/xen", u"sys/bus/xen", u"proc/xen"]

There is no need to have unicode paths.

[3]
+ elif True in map(exists, xen_info):

It's slightly more explicit to me to do "elif filter(exists, xen_info):"

[4]
+ elif "QEMU Virtual CPU" in open(
+ join_root_path(u"proc/cpuinfo")).read():
+ virt_info = "KVM"

You should close the open file explicitly.

[5]
+import os.path

You can import os directly.

Thanks!

review: Needs Fixing

[Free Ekanayaka (free.ekanayaka)]

What Thomas said (especially about testing) plus some other comments.

Just to elaborate on test coverage thing, a rule of thumb is that any change in the code (like removing a line, an if branch, etc.) should result in a test failure. In this case replacing the entire body of ComputerInfo._get_vm_info with a simple 'return ""' doesn't make any tests fail, which means that its behavior is untested.

[1]

+ def _get_vm_info(self, root_path="/"):

Please pass root_path to the constructor instead, like:

    def __init__(self, get_fqdn=get_fqdn,
                 meminfo_file="/proc/meminfo",
                 lsb_release_filename=LSB_RELEASE_FILENAME,
                 root_path="/"):
        self._get_fqdn = get_fqdn
        self._meminfo_file = meminfo_file
        self._lsb_release_filename = lsb_release_filename
        self._root_path = root_path

and then you can have something like

    root_path = self.makeDir()
    plugin = ComputerInfo(root_path=root_path)

in your vm-info-related tests, where the test populates root_path by hand for checking the behavior of the method.

To avoid cluttering the constructor with arguments that are only meaningful for tests, I think it would be better to use class variables that one can override in the tests:

class ComputerInfo(MonitorPlugin):

    get_fqdn = get_fqdn
    meminfo_file = "/proc/meminfo"
    lsb_release_filename = LSB_RELEASE_FILENAME
    root_path = "/"

and then

    plugin = ComputerInfo()
    plugin.root_path = self.makeDir()

We use this pattern in other places, but consider that this piece of code already uses test-oriented constructor parameters, it's fine to be consistent.

[2]

+ virt_info = "OpenVZ"

Please use lowercase strings, like "openvz" "xen" and "kvm", that's one chance less to get spelling errors while writing server side code/tests that use them. If this information must be eventually shown in some UI, it will be the UI's job to map those lowercase strings to nice human-oriented strings/phrases.

[3]

- def test_resynchronize(self):
+ def test_wb_resynchronize(self):

and

+ plugin._get_vm_info = lambda: ""

Once [1] is addressed I believe you can revert these changes.

[4]

+ xen_info = [u"proc/sys/xen", u"sys/bus/xen", u"proc/xen"]
+ xen_info = map(join_root_path, xen_info)

Maybe this variable would be better named "xen_paths".

review: Needs Fixing

[lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message]

331. By Fernando Correa Neto on 2011-04-20

- Addressing therve and free view comments

332. By Fernando Correa Neto on 2011-04-20

- Backporting tests from old revision

333. By Fernando Correa Neto on 2011-04-20

- Linting

[Thomas Herve (therve)]

Looks good!

[6]
+ elif exists(join_root_path("proc/cpuinfo")):
+ with open(join_root_path("proc/cpuinfo")) as cpuinfo:
+ if "QEMU Virtual CPU" in cpuinfo.read():
+ virt_info = "kvm"

You don't need to check for the existence of cpuinfo (it should always be there). You can't use "with" because we're still supporting python 2.5 in the client. You could use a future import, but I'd rather see you use try/finally here.

+1!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Looks good, just a few more minor things. +1 with those fixed!

[5]

+ path_join = os.path.join

and

+ def join_root_path(path):
+ return path_join(self._root_path, path)

I'd avoid setting the the path_join variable, just call os.path.join directly, like:

         def join_root_path(path):
             return os.path.join(self._root_path, path)

it's one less line of code and little tiny bit easier to parse (you don't have to know that path_join is os.path.join).

The same holds for

+ exists = os.path.exists

and

+ elif filter(exists, xen_paths):
+ virt_info = "xen"

[6]

There should be a test checking that if "proc/cpuinfo" exists and does *not* contain the "QEMU Virtual CPU" string, then the vm-info will be empty. Without such a test one could simply remove the "if" and "open" check and all tests would pass, like:

        elif exists(join_root_path("proc/cpuinfo")):
            virt_info = "kvm"

[7]

- "total-swap": 1584}
+ "total-swap": 1584, "vm-info": ""}

I think in this test (test_resynchronize) we should also pass a root_path=self.makeDir() to the plugin constructor, otherwise the test would fail when run on KVM machine (which is not unlikely if we want to run the suite in an Eucalyptus cloud instance)

[8]

There's a test missing for the openvz case.

review: Approve

[lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message]

334. By Fernando Correa Neto on 2011-04-20

- Addressing final therve and free review comments

335. By Fernando Correa Neto on 2011-04-20

- Adding missing test for openvz and also adding root_path to the constructor of ComputerInfo in test_resynchronize test

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2010-06-28 12:52:39 +0000
+++ landscape/message_schemas.py	2011-04-20 21:43:28 +0000
@@ -68,10 +68,11 @@
     "computer-info",
     {"hostname": utf8,
      "total-memory": Int(),
-     "total-swap": Int()},
+     "total-swap": Int(),
+     "vm-info": String()},
     # Not sure why these are all optional, but it's explicitly tested
     # in the server
-    optional=["hostname", "total-memory", "total-swap"])
+    optional=["hostname", "total-memory", "total-swap", "vm-info"])
 
 DISTRIBUTION_INFO = Message(
     "distribution-info",

=== modified file 'landscape/monitor/computerinfo.py'
--- landscape/monitor/computerinfo.py	2010-12-10 13:57:11 +0000
+++ landscape/monitor/computerinfo.py	2011-04-20 21:43:28 +0000
@@ -1,4 +1,5 @@
 import logging
+import os
 
 from landscape.lib.lsb_release import LSB_RELEASE_FILENAME, parse_lsb_release
 from landscape.lib.network import get_fqdn
@@ -16,10 +17,12 @@
 
     def __init__(self, get_fqdn=get_fqdn,
                  meminfo_file="/proc/meminfo",
-                 lsb_release_filename=LSB_RELEASE_FILENAME):
+                 lsb_release_filename=LSB_RELEASE_FILENAME,
+                 root_path="/"):
         self._get_fqdn = get_fqdn
         self._meminfo_file = meminfo_file
         self._lsb_release_filename = lsb_release_filename
+        self._root_path = root_path
 
     def register(self, registry):
         super(ComputerInfo, self).register(registry)
@@ -57,6 +60,8 @@
         self._add_if_new(message, "total-memory",
                          total_memory)
         self._add_if_new(message, "total-swap", total_swap)
+        vm_info = self._get_vm_info()
+        self._add_if_new(message, "vm-info", vm_info)
         return message
 
     def _add_if_new(self, message, key, value):
@@ -90,3 +95,37 @@
         message = {}
         message.update(parse_lsb_release(self._lsb_release_filename))
         return message
+
+    def _get_vm_info(self):
+        """
+        This is a utility that returns the virtualization type
+
+        It loops through some possible configurations and return a string with
+        the name of the technology being used or None if there's no match
+        """
+        virt_info = ""
+
+        def join_root_path(path):
+            return os.path.join(self._root_path, path)
+
+        xen_paths = ["proc/sys/xen", "sys/bus/xen", "proc/xen"]
+        xen_paths = map(join_root_path, xen_paths)
+
+        vz_path = os.path.join(self._root_path, "proc/vz")
+        if os.path.exists(vz_path):
+            virt_info = "openvz"
+
+        elif filter(os.path.exists, xen_paths):
+            virt_info = "xen"
+
+        cpu_info_path = os.path.join(self._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:
+                    virt_info = "kvm"
+            finally:
+                fd.close()
+
+        return virt_info

=== modified file 'landscape/monitor/tests/test_computerinfo.py'
--- landscape/monitor/tests/test_computerinfo.py	2010-04-28 13:07:53 +0000
+++ landscape/monitor/tests/test_computerinfo.py	2011-04-20 21:43:28 +0000
@@ -1,4 +1,5 @@
 import re
+import os
 
 from landscape.monitor.computerinfo import ComputerInfo
 from landscape.tests.helpers import LandscapeTest, MonitorHelper
@@ -44,6 +45,27 @@
 VmallocChunk:   107432 kB
 """
 
+    sample_kvm_cpuinfo = """
+processor	: 0
+vendor_id	: GenuineIntel
+cpu family	: 6
+model		: 2
+model name	: QEMU Virtual CPU version 0.14.0
+stepping	: 3
+cpu MHz		: 2653.112
+cache size	: 4096 KB
+fpu		: yes
+fpu_exception	: yes
+cpuid level	: 4
+wp		: yes
+flags		: fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca
+bogomips	: 5306.22
+clflush size	: 64
+cache_alignment	: 64
+address sizes	: 40 bits physical, 48 bits virtual
+power management:
+"""
+
     def setUp(self):
         LandscapeTest.setUp(self)
         self.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE)
@@ -272,14 +294,15 @@
         meminfo_filename = self.makeFile(self.sample_memory_info)
         plugin = ComputerInfo(get_fqdn=get_fqdn,
                               meminfo_file=meminfo_filename,
-                              lsb_release_filename=self.lsb_release_filename)
+                              lsb_release_filename=self.lsb_release_filename,
+                              root_path=self.makeDir())
         self.monitor.add(plugin)
         plugin.exchange()
         self.reactor.fire("resynchronize")
         plugin.exchange()
         computer_info = {"type": "computer-info", "hostname": "ooga.local",
                          "timestamp": 0, "total-memory": 1510,
-                         "total-swap": 1584}
+                         "total-swap": 1584, "vm-info": ""}
         dist_info = {"type": "distribution-info",
                      "code-name": "dapper", "description": "Ubuntu 6.06.1 LTS",
                      "distributor-id": "Ubuntu", "release": "6.06"}
@@ -324,3 +347,146 @@
 
         self.mstore.set_accepted_types(["distribution-info", "computer-info"])
         self.assertMessages(list(self.mstore.get_pending_messages()), [])
+
+    def test_vminfo_empty_when_no_virtualization_is_found(self):
+        """
+        L{ComputerInfo} should always have the vm-info key even when no
+        virtualization is used.
+
+        And it should be an empty string.
+        """
+        self.mstore.set_accepted_types(["computer-info"])
+        root_path = self.makeDir()
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertTrue("vm-info" in message)
+        self.assertEqual("", message["vm-info"])
+
+    def test_vminfo_is_openvz_when_proc_vz_exists(self):
+        """
+        L{ComputerInfo} should have 'openvz' as the value of vm-info 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")
+        self.makeFile(path=proc_vz_path, content="foo")
+
+        self.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEquals('openvz', message["vm-info"])
+
+    def test_vminfo_is_xen_when_proc_sys_xen_exists(self):
+        """
+        L{ComputerInfo} should have 'xen' as the value of vm-info 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")
+        self.makeFile(path=proc_sys_xen_path, content="foo")
+
+        self.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEquals('xen', message["vm-info"])
+
+    def test_vminfo_is_xen_when_sys_bus_xen_exists(self):
+        """
+        L{ComputerInfo} should have 'xen' as value of vm-info when
+        /sys/bus/xen exists
+        """
+        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.makeFile(path=sys_bus_xen_path, content="foo")
+
+        self.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEquals('xen', message["vm-info"])
+
+    def test_vminfo_is_xen_when_proc_xen_exists(self):
+        """
+        L{ComputerInfo} should have 'xen' as value of vm-info 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")
+        self.makeFile(path=proc_xen_path, content="foo")
+
+        self.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEquals('xen', message["vm-info"])
+
+    def test_vminfo_is_kvm_when_qemu_is_found_in_proc_cpuinfo(self):
+        """
+        L{ComputerInfo} should have 'kvm' as value of vm-info 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.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEqual("kvm", message["vm-info"])
+
+    def test_vminfo_is_empty_when_qemu_is_not_found_in_proc_cpuinfo(self):
+        """
+        L{ComputerInfo} should have an empty string as value of vm-info 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.mstore.set_accepted_types(["computer-info"])
+        plugin = ComputerInfo(root_path=root_path)
+        self.monitor.add(plugin)
+
+        plugin.exchange()
+        message = self.mstore.get_pending_messages()[0]
+        self.assertEqual("", message["vm-info"])