Merge lp:~gocept/landscape-client/py3-broker-registration into lp:~landscape/landscape-client/trunk

[Steffen Allner]

This is the port of landscape.broker.registration to Python 3.

This involved fixing bytes/unicode issues. One situation in the tests involved incompatibilities involving io.StringIO. The fix there is not ideal. Applying a better solution is tabled for later.

As the diff for the landscape.broker module would be to large, I MP the sub-modules separately.

Here we have landscape.broker.registration, where the tests were failing because bytes were not uses as input when required by a schema, in particular "api" and "vm-info". The restriction on "vm-info" required larger modifications on landscape.lib.vm_info which returns now bytes for all cases of "vm-info".

The case with the conditional encoding to compare bytes of log messages in unfortunate. I tried a few different approaches but a drop-in replacement of cStringIO with io.StringIO would result in a lot of test failures so I would rather suggest to put that on a trello card and look at this problem separately.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 976
Branch: lp:~gocept/landscape-client/py3-broker-registration
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3715/

review: Approve (test results)

[Данило Шеган (danilo)]

Looks good: I agree with your suggestion of adding a separate card to look at at the end, and thanks for putting in an XXX comment too!

review: Approve

[Daniel Havlik (nilo)]

+1

review: Approve

[Eric Snow (ericsnowcurrently)]

LGTM

review: Approve

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2017-02-24 09:38:26 +0000
+++ landscape/broker/registration.py	2017-03-23 13:00:45 +0000
@@ -196,7 +196,7 @@
         # version 14.01 has a different format for the juju-info field,
         # so this makes sure that the correct schema is used by the server
         # when validating our message.
-        if self._juju_data and is_version_higher(server_api, "3.3"):
+        if self._juju_data and is_version_higher(server_api, b"3.3"):
             message["juju-info"] = {
                 "environment-uuid": self._juju_data["environment-uuid"],
                 "api-addresses": self._juju_data["api-addresses"],

=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2017-02-24 09:38:26 +0000
+++ landscape/broker/tests/test_registration.py	2017-03-23 13:00:45 +0000
@@ -3,6 +3,8 @@
 import socket
 import mock
 
+from twisted.python.compat import _PY3
+
 from landscape.broker.registration import RegistrationError, Identity
 from landscape.tests.helpers import LandscapeTest
 from landscape.broker.tests.helpers import (
@@ -167,13 +169,13 @@
         secure_id is set, and an exchange is about to happen,
         queue a registration message with VM information.
         """
-        get_vm_info_mock.return_value = "vmware"
+        get_vm_info_mock.return_value = b"vmware"
         self.mstore.set_accepted_types(["register"])
         self.config.computer_title = "Computer Title"
         self.config.account_name = "account_name"
         self.reactor.fire("pre-exchange")
         messages = self.mstore.get_pending_messages()
-        self.assertEqual("vmware", messages[0]["vm-info"])
+        self.assertEqual(b"vmware", messages[0]["vm-info"])
         self.assertEqual(self.logfile.getvalue().strip(),
                          "INFO: Queueing message to register with account "
                          "'account_name' without a password.")
@@ -261,10 +263,19 @@
         expected = u"prova\N{LATIN SMALL LETTER J WITH CIRCUMFLEX}o"
         self.assertEqual(expected, messages[0]["tags"])
 
-        self.assertEqual(self.logfile.getvalue().strip(),
-                         "INFO: Queueing message to register with account "
-                         "'account_name' and tags prova\xc4\xb5o "
-                         "with a password.")
+        logs = self.logfile.getvalue().strip()
+        # XXX This is not nice, as it has the origin in a non-consistent way of
+        # using logging. self.logfile is a cStringIO in Python 2 and
+        # io.StringIO in Python 3. This results in reading bytes in Python 2
+        # and unicode in Python 3, but a drop-in replacement of cStringIO with
+        # io.StringIO in Python 2 is not working. However, we compare bytes
+        # here, to circumvent that problem.
+        if _PY3:
+            logs = logs.encode("utf-8")
+        self.assertEqual(logs,
+                         b"INFO: Queueing message to register with account "
+                         b"'account_name' and tags prova\xc4\xb5o "
+                         b"with a password.")
 
     def test_queue_message_on_exchange_with_access_group(self):
         """
@@ -541,7 +552,7 @@
         the registration message.
         """
         self.mstore.set_accepted_types(["register"])
-        self.mstore.set_server_api("3.3")
+        self.mstore.set_server_api(b"3.3")
         self.config.account_name = "account_name"
         self.reactor.fire("run")
         self.reactor.fire("pre-exchange")
@@ -559,7 +570,7 @@
         isn't included in the message.
         """
         self.mstore.set_accepted_types(["register"])
-        self.mstore.set_server_api("3.2")
+        self.mstore.set_server_api(b"3.2")
         self.config.account_name = "account_name"
         self.reactor.fire("run")
         self.reactor.fire("pre-exchange")

=== modified file 'landscape/lib/tests/test_vm_info.py'
--- landscape/lib/tests/test_vm_info.py	2016-09-23 12:51:20 +0000
+++ landscape/lib/tests/test_vm_info.py	2017-03-23 13:00:45 +0000
@@ -25,7 +25,7 @@
         """
         L{get_vm_info} should be empty when there's no virtualisation.
         """
-        self.assertEqual(u"", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_openvz_when_proc_vz_exists(self):
         """
@@ -34,7 +34,7 @@
         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=self.root_path))
+        self.assertEqual(b"openvz", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_xen_when_sys_bus_xen_is_non_empty(self):
         """
@@ -46,7 +46,7 @@
         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=self.root_path))
+        self.assertEqual(b"xen", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_is_empty_without_xen_devices(self):
         """
@@ -56,7 +56,7 @@
         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=self.root_path))
+        self.assertEqual(b"", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_bochs_sys_vendor(self):
         """
@@ -64,7 +64,7 @@
         Bochs.
         """
         self.make_sys_vendor("Bochs")
-        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_openstack_sys_vendor(self):
         """
@@ -72,7 +72,7 @@
         Openstack.
         """
         self.make_sys_vendor("OpenStack Foundation")
-        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_qemu_sys_vendor(self):
         """
@@ -80,7 +80,7 @@
         QEMU.
         """
         self.make_sys_vendor("QEMU")
-        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_vmware_sys_vendor(self):
         """
@@ -88,7 +88,7 @@
         VMware Inc.
         """
         self.make_sys_vendor("VMware, Inc.")
-        self.assertEqual("vmware", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"vmware", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_virtualbox_sys_vendor(self):
         """
@@ -96,28 +96,28 @@
         is innotek. GmbH.
         """
         self.make_sys_vendor("innotek GmbH")
-        self.assertEqual("virtualbox", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"virtualbox", 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.
         """
         self.make_sys_vendor("Microsoft Corporation")
-        self.assertEqual("hyperv", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"hyperv", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_google_sys_vendor(self):
         """
         L{get_vm_info} returns "gce" if the sys_vendor is Google.
         """
         self.make_sys_vendor("Google")
-        self.assertEqual("gce", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"gce", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_matches_insensitive(self):
         """
         L{get_vm_info} matches the vendor string in a case-insentive way.
         """
         self.make_sys_vendor("openstack foundation")
-        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_kvm_on_other_architecture(self):
         """
@@ -130,7 +130,7 @@
             "model	: Some CPU (emulated by qemu)\n"
             "machine	: Some Machine (emulated by qemu)\n")
         self.makeFile(path=cpuinfo_path, content=cpuinfo)
-        self.assertEqual("kvm", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"kvm", get_vm_info(root_path=self.root_path))
 
     def test_get_vm_info_with_other_vendor(self):
         """
@@ -138,7 +138,7 @@
         unknown.
         """
         self.make_sys_vendor("Some other vendor")
-        self.assertEqual("", get_vm_info(root_path=self.root_path))
+        self.assertEqual(b"", get_vm_info(root_path=self.root_path))
 
 
 class GetContainerInfoTest(LandscapeTest):

=== modified file 'landscape/lib/vm_info.py'
--- landscape/lib/vm_info.py	2017-03-13 15:38:09 +0000
+++ landscape/lib/vm_info.py	2017-03-23 13:00:45 +0000
@@ -8,16 +8,16 @@
 
 def get_vm_info(root_path="/"):
     """
-    Return a string with the virtualization type if it's known, an empty string
-    otherwise.
+    Return a bytestring with the virtualization type if it's known, an empty
+    bytestring otherwise.
 
-    It loops through some possible configurations and return a string with
+    It loops through some possible configurations and return a bytestring with
     the name of the technology being used or None if there's no match
     """
     if _is_vm_openvz(root_path):
-        return "openvz"
+        return b"openvz"
     if _is_vm_xen(root_path):
-        return "xen"
+        return b"xen"
 
     sys_vendor_path = os.path.join(root_path, "sys/class/dmi/id/sys_vendor")
     if os.path.exists(sys_vendor_path):
@@ -51,22 +51,23 @@
 
 
 def _get_vm_by_vendor(sys_vendor_path):
-    """Return the VM type string (possibly empty) based on the vendor."""
+    """Return the VM type byte string (possibly empty) based on the vendor."""
     vendor = read_text_file(sys_vendor_path).lower()
     # Use lower-key string for vendors, since we do case-insentive match.
+    # We need bytes here as required by the message schema.
     content_vendors_map = (
-        ("bochs", "kvm"),
-        ("google", "gce"),
-        ("innotek", "virtualbox"),
-        ("microsoft", "hyperv"),
-        ("openstack", "kvm"),
-        ("qemu", "kvm"),
-        ("vmware", "vmware"))
+        ("bochs", b"kvm"),
+        ("google", b"gce"),
+        ("innotek", b"virtualbox"),
+        ("microsoft", b"hyperv"),
+        ("openstack", b"kvm"),
+        ("qemu", b"kvm"),
+        ("vmware", b"vmware"))
     for name, vm_type in content_vendors_map:
         if name in vendor:
             return vm_type
 
-    return ""
+    return b""
 
 
 def _get_vm_legacy(root_path):
@@ -74,9 +75,9 @@
     try:
         cpuinfo = read_text_file(os.path.join(root_path, "proc/cpuinfo"))
     except (IOError, OSError):
-        return ""
+        return b""
 
     if "qemu" in cpuinfo:
-        return "kvm"
+        return b"kvm"
 
-    return ""
+    return b""

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2017-03-17 10:08:19 +0000
+++ landscape/message_schemas.py	2017-03-23 13:00:45 +0000
@@ -199,7 +199,7 @@
                            "api-addresses": List(Unicode()),
                            "machine-id": Unicode()}),
      "access_group": Unicode()},
-    api="3.3",
+    api=b"3.3",
     optional=["registration_password", "hostname", "tags", "vm-info",
               "container-info", "access_group", "juju-info"])
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-23 13:00:45 +0000
+++ py3_ready_tests	2017-03-23 13:00:45 +0000
@@ -7,6 +7,6 @@
 
 
 
-
+landscape.broker.tests.test_registration
 landscape.broker.tests.test_exchange