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

[Steffen Allner]

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/3714/

review: Approve (test results)

Unmerged revisions

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2017-03-21 13:00:17 +0000
+++ landscape/broker/exchange.py	2017-03-23 12:58:06 +0000
@@ -715,7 +715,7 @@
         return payload
 
     def _hash_types(self, types):
-        accepted_types_str = ";".join(types)
+        accepted_types_str = ";".join(types).encode("ascii")
         return md5(accepted_types_str).digest()
 
     def _handle_result(self, payload, result):

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2017-02-24 09:38:26 +0000
+++ landscape/broker/registration.py	2017-03-23 12:58:06 +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_exchange.py'
--- landscape/broker/tests/test_exchange.py	2016-06-15 17:33:13 +0000
+++ landscape/broker/tests/test_exchange.py	2017-03-23 12:58:06 +0000
@@ -104,7 +104,7 @@
         messages = self.transport.payloads[0]["messages"]
         self.assertEqual(messages, [{"type": "empty",
                                      "timestamp": 0,
-                                     "api": "3.2"}])
+                                     "api": b"3.2"}])
 
     def test_send_urgent(self):
         """
@@ -149,7 +149,7 @@
         """
         payload = self.exchanger._make_payload()
         self.assertIn("accepted-types", payload)
-        self.assertEqual(payload["accepted-types"], md5("").digest())
+        self.assertEqual(payload["accepted-types"], md5(b"").digest())
 
     def test_handle_message_sets_accepted_types(self):
         """
@@ -170,8 +170,8 @@
             {"type": "accepted-types", "types": ["a", "b"]})
         self.exchanger.handle_message(
             {"type": "accepted-types", "types": ["b", "c"]})
-        self.assertEqual(stash, [("a", True), ("b", True),
-                                 ("a", False), ("c", True)])
+        self.assertCountEqual(stash, [("a", True), ("b", True),
+                                      ("a", False), ("c", True)])
 
     def test_wb_accepted_types_roundtrip(self):
         """
@@ -183,7 +183,7 @@
         payload = self.exchanger._make_payload()
         self.assertIn("accepted-types", payload)
         self.assertEqual(payload["accepted-types"],
-                         md5("ack;bar").digest())
+                         md5(b"ack;bar").digest())
 
     def test_accepted_types_causes_urgent_if_held_messages_exist(self):
         """
@@ -523,33 +523,33 @@
         payload = self.transport.payloads[-1]
         self.assertMessages(payload["messages"], [])
         self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "3.2")
-        self.assertEqual(self.transport.message_api, "3.2")
-
-        self.mstore.add({"type": "a", "api": "1.0"})
-        self.mstore.add({"type": "b", "api": "1.0"})
-        self.mstore.add({"type": "c", "api": "1.1"})
-        self.mstore.add({"type": "d", "api": "1.1"})
-
-        self.exchanger.exchange()
-
-        payload = self.transport.payloads[-1]
-        self.assertMessages(payload["messages"],
-                            [{"type": "a", "api": "1.0"},
-                             {"type": "b", "api": "1.0"}])
-        self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "1.0")
-        self.assertEqual(self.transport.message_api, "1.0")
-
-        self.exchanger.exchange()
-
-        payload = self.transport.payloads[-1]
-        self.assertMessages(payload["messages"],
-                            [{"type": "c", "api": "1.1"},
-                             {"type": "d", "api": "1.1"}])
-        self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "1.1")
-        self.assertEqual(self.transport.message_api, "1.1")
+        self.assertEqual(payload.get("server-api"), b"3.2")
+        self.assertEqual(self.transport.message_api, b"3.2")
+
+        self.mstore.add({"type": "a", "api": b"1.0"})
+        self.mstore.add({"type": "b", "api": b"1.0"})
+        self.mstore.add({"type": "c", "api": b"1.1"})
+        self.mstore.add({"type": "d", "api": b"1.1"})
+
+        self.exchanger.exchange()
+
+        payload = self.transport.payloads[-1]
+        self.assertMessages(payload["messages"],
+                            [{"type": "a", "api": b"1.0"},
+                             {"type": "b", "api": b"1.0"}])
+        self.assertEqual(payload.get("client-api"), CLIENT_API)
+        self.assertEqual(payload.get("server-api"), b"1.0")
+        self.assertEqual(self.transport.message_api, b"1.0")
+
+        self.exchanger.exchange()
+
+        payload = self.transport.payloads[-1]
+        self.assertMessages(payload["messages"],
+                            [{"type": "c", "api": b"1.1"},
+                             {"type": "d", "api": b"1.1"}])
+        self.assertEqual(payload.get("client-api"), CLIENT_API)
+        self.assertEqual(payload.get("server-api"), b"1.1")
+        self.assertEqual(self.transport.message_api, b"1.1")
 
     def test_exchange_token(self):
         """
@@ -827,27 +827,27 @@
         """
         self.transport.extra.pop("server-api", None)
         self.exchanger.exchange()
-        self.assertEqual("3.2", self.mstore.get_server_api())
+        self.assertEqual(b"3.2", self.mstore.get_server_api())
 
     def test_wb_client_with_older_api_and_server_with_newer(self):
         """
         If a server notifies us that it case use a very new API, but we
         don't know how to speak it, we keep using ours.
         """
-        self.exchanger._api = "3.3"
-        self.transport.extra["server-api"] = "3.4"
+        self.exchanger._api = b"3.3"
+        self.transport.extra["server-api"] = b"3.4"
         self.exchanger.exchange()
-        self.assertEqual("3.3", self.mstore.get_server_api())
+        self.assertEqual(b"3.3", self.mstore.get_server_api())
 
     def test_wb_client_with_newer_api_and_server_with_older(self):
         """
         If a server notifies us that it can use an API which is older
         than the one we support, we'll just use the server API.
         """
-        self.exchanger._api = "3.4"
-        self.transport.extra["server-api"] = "3.3"
+        self.exchanger._api = b"3.4"
+        self.transport.extra["server-api"] = b"3.3"
         self.exchanger.exchange()
-        self.assertEqual("3.3", self.mstore.get_server_api())
+        self.assertEqual(b"3.3", self.mstore.get_server_api())
 
     def test_server_uuid_is_stored_on_message_store(self):
         self.transport.extra["server-uuid"] = "first-uuid"
@@ -1078,10 +1078,10 @@
         """
         If we get a 404, we try to donwgrade our server API version.
         """
-        self.mstore.set_server_api("3.3")
+        self.mstore.set_server_api(b"3.3")
         self.transport.responses.append(HTTPCodeError(404, ""))
         self.exchanger.exchange()
-        self.assertEqual("3.2", self.mstore.get_server_api())
+        self.assertEqual(b"3.2", self.mstore.get_server_api())
 
 
 class AcceptedTypesMessageExchangeTest(LandscapeTest):
@@ -1120,7 +1120,7 @@
         self.exchanger.register_client_accepted_message_type("type-A")
         self.exchanger.register_client_accepted_message_type("type-B")
         types = sorted(["type-A", "type-B"] + DEFAULT_ACCEPTED_TYPES)
-        accepted_types_digest = md5(";".join(types)).digest()
+        accepted_types_digest = md5(";".join(types).encode("ascii")).digest()
         self.transport.extra["client-accepted-types-hash"] = \
             accepted_types_digest
         self.exchanger.exchange()
@@ -1146,7 +1146,7 @@
         client will send a new list to the server.
         """
         self.exchanger.register_client_accepted_message_type("type-A")
-        types_hash = md5("type-A").digest()
+        types_hash = md5(b"type-A").digest()
         self.transport.extra["client-accepted-types-hash"] = types_hash
         self.exchanger.exchange()
         self.exchanger.register_client_accepted_message_type("type-B")
@@ -1162,7 +1162,7 @@
         send a new list of types.
         """
         self.exchanger.register_client_accepted_message_type("type-A")
-        types_hash = md5("type-A").digest()
+        types_hash = md5(b"type-A").digest()
         self.transport.extra["client-accepted-types-hash"] = types_hash
         self.exchanger.exchange()
         self.transport.extra["client-accepted-types-hash"] = "lol"

=== 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 12:58:06 +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 12:58:06 +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 12:58:06 +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 12:58:06 +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 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2017-03-21 17:57:12 +0000
+++ landscape/tests/helpers.py	2017-03-23 12:58:06 +0000
@@ -148,6 +148,11 @@
         else:
             return result[0]
 
+    if not _PY3:
+        def assertCountEqual(self, *args, **kwargs):
+            """An alias for C{TestCase.assertItemsEqual} for py2/3 compat."""
+            self.assertItemsEqual(*args, **kwargs)
+
     def assertNoResult(self, deferred):
         """See C{twisted.trial._synctest._Assertions.assertNoResult}.
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-23 07:28:20 +0000
+++ py3_ready_tests	2017-03-23 12:58:06 +0000
@@ -7,6 +7,6 @@
 
 
 
-
-
+landscape.broker.tests.test_registration
+landscape.broker.tests.test_exchange