Merge lp:~simpoir/landscape-client/py3fix-network-usage into lp:~landscape/landscape-client/trunk

[Simon Poirier]

This branch fixes reporting of network usage under python3. It adds a parameter to bpicle to allow loading without casting bytes keys to string in dict in the cases where data need to be passed as-is (broker and messaging serialization).

This branch fixes reporting of network usage under python3. It adds a parameter to bpicle to allow loading without casting bytes keys to string in dict in the cases where data need to be passed as-is (broker and messaging serialization).

Testing instructions:

make check2 check3
./scripts/landscape-client -c root-config

Accept the client and check in Monitoring for network graphs a few minutes later.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Fail
Revno: 1018
Branch: lp:~simpoir/landscape-client/py3fix-network-usage
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3877/

review: Needs Fixing (test results)

[Eric Snow (ericsnowcurrently)]

LGTM

Interestingly, we had something similar before but then someone-I-won't-name decided to simplify... :)

review: Approve

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 1018
Branch: lp:~simpoir/landscape-client/py3fix-network-usage
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3878/

review: Approve (test results)

=== modified file 'landscape/broker/store.py'
--- landscape/broker/store.py	2017-04-06 18:27:36 +0000
+++ landscape/broker/store.py	2017-04-19 20:54:09 +0000
@@ -262,7 +262,8 @@
                 break
             data = read_binary_file(self._message_dir(filename))
             try:
-                message = bpickle.loads(data)
+                # don't reinterpret messages that are meant to be sent out
+                message = bpickle.loads(data, as_is=True)
             except ValueError as e:
                 logging.exception(e)
                 self._add_flags(filename, BROKEN)

=== modified file 'landscape/lib/amp.py'
--- landscape/lib/amp.py	2017-03-17 09:26:45 +0000
+++ landscape/lib/amp.py	2017-04-19 20:54:09 +0000
@@ -165,7 +165,8 @@
             chunks.append(arguments)
             arguments = b"".join(chunks)
 
-        args, kwargs = bpickle.loads(arguments)
+        # Pass the the arguments as-is without reinterpreting strings.
+        args, kwargs = bpickle.loads(arguments, as_is=True)
 
         # We encoded the method name in `send_method_call` and have to decode
         # it here again.

=== modified file 'landscape/lib/bpickle.py'
--- landscape/lib/bpickle.py	2017-04-11 14:13:28 +0000
+++ landscape/lib/bpickle.py	2017-04-19 20:54:09 +0000
@@ -45,13 +45,19 @@
         raise ValueError("Unsupported type: %s" % e)
 
 
-def loads(byte_string, _lt=loads_table):
+def loads(byte_string, _lt=loads_table, as_is=False):
+    """Load a serialized byte_string.
+
+    @param byte_string: the serialized data
+    @param _lt: the conversion map
+    @param as_is: don't reinterpret dict keys as str
+    """
     if not byte_string:
         raise ValueError("Can't load empty string")
     try:
         # To avoid python3 turning byte_string[0] into an int,
         # we slice the bytestring instead.
-        return _lt[byte_string[0:1]](byte_string, 0)[0]
+        return _lt[byte_string[0:1]](byte_string, 0, as_is=as_is)[0]
     except KeyError as e:
         raise ValueError("Unknown type character: %s" % e)
     except IndexError:
@@ -107,59 +113,59 @@
     return b"n"
 
 
-def loads_bool(bytestring, pos):
+def loads_bool(bytestring, pos, as_is=False):
     return bool(int(bytestring[pos+1:pos+2])), pos+2
 
 
-def loads_int(bytestring, pos):
+def loads_int(bytestring, pos, as_is=False):
     endpos = bytestring.index(b";", pos)
     return int(bytestring[pos+1:endpos]), endpos+1
 
 
-def loads_float(bytestring, pos):
+def loads_float(bytestring, pos, as_is=False):
     endpos = bytestring.index(b";", pos)
     return float(bytestring[pos+1:endpos]), endpos+1
 
 
-def loads_bytes(bytestring, pos):
+def loads_bytes(bytestring, pos, as_is=False):
     startpos = bytestring.index(b":", pos)+1
     endpos = startpos+int(bytestring[pos+1:startpos-1])
     return bytestring[startpos:endpos], endpos
 
 
-def loads_unicode(bytestring, pos):
+def loads_unicode(bytestring, pos, as_is=False):
     startpos = bytestring.index(b":", pos)+1
     endpos = startpos+int(bytestring[pos+1:startpos-1])
     return bytestring[startpos:endpos].decode("utf-8"), endpos
 
 
-def loads_list(bytestring, pos, _lt=loads_table):
+def loads_list(bytestring, pos, _lt=loads_table, as_is=False):
     pos += 1
     res = []
     append = res.append
     while bytestring[pos:pos+1] != b";":
-        obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
+        obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
         append(obj)
     return res, pos+1
 
 
-def loads_tuple(bytestring, pos, _lt=loads_table):
+def loads_tuple(bytestring, pos, _lt=loads_table, as_is=False):
     pos += 1
     res = []
     append = res.append
     while bytestring[pos:pos+1] != b";":
-        obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
+        obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
         append(obj)
     return tuple(res), pos+1
 
 
-def loads_dict(bytestring, pos, _lt=loads_table):
+def loads_dict(bytestring, pos, _lt=loads_table, as_is=False):
     pos += 1
     res = {}
     while bytestring[pos:pos+1] != b";":
-        key, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
-        val, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
-        if _PY3 and isinstance(key, bytes):
+        key, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
+        val, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
+        if _PY3 and not as_is and isinstance(key, bytes):
             # Although the wire format of dictionary keys is ASCII bytes, the
             # code actually expects them to be strings, so we convert them
             # here.
@@ -168,7 +174,7 @@
     return res, pos+1
 
 
-def loads_none(str, pos):
+def loads_none(str, pos, as_is=False):
     return None, pos+1
 
 

=== modified file 'landscape/lib/tests/test_amp.py'
--- landscape/lib/tests/test_amp.py	2017-03-09 13:41:17 +0000
+++ landscape/lib/tests/test_amp.py	2017-04-19 20:54:09 +0000
@@ -213,6 +213,21 @@
         self.connection.flush()
         self.assertEqual("barfoobarfoobarfoo", self.successResultOf(deferred))
 
+    def test_with_bytes_dictionary_arguments(self):
+        """
+        Method arguments passed to a MethodCall can be a dict of bytes.
+        """
+        arg = {b"byte_key": 1}
+        self.object.method = lambda d: ",".join([
+            type(x).__name__ for x in d.keys()])
+        deferred = self.sender.send_method_call(
+            method="method",
+            args=[arg],
+            kwargs={})
+        self.connection.flush()
+        # str under python2, bytes under python3
+        self.assertEqual(type(b"").__name__, self.successResultOf(deferred))
+
     def test_with_non_serializable_return_value(self):
         """
         If the target object method returns an object that can't be serialized,

=== modified file 'landscape/lib/tests/test_bpickle.py'
--- landscape/lib/tests/test_bpickle.py	2017-04-10 21:28:13 +0000
+++ landscape/lib/tests/test_bpickle.py	2017-04-19 20:54:09 +0000
@@ -49,8 +49,14 @@
                          {True: False})
 
     def test_dict_bytes_keys(self):
-        data = bpickle.dumps({b"hello": True})
-        self.assertEqual(bpickle.loads(data), {"hello": True})
+        """Check loading dict bytes keys without reinterpreting."""
+        # Happens in amp and broker. Since those messages are meant to be
+        # forwarded to the server without changing schema, keys shouldn't be
+        # decoded in this case.
+        initial_data = {b"hello": True}
+        data = bpickle.dumps(initial_data)
+        result = bpickle.loads(data, as_is=True)
+        self.assertEqual(initial_data, result)
 
     def test_long(self):
         long = 99999999999999999999999999999

=== modified file 'landscape/monitor/tests/test_networkactivity.py'
--- landscape/monitor/tests/test_networkactivity.py	2017-04-05 13:08:02 +0000
+++ landscape/monitor/tests/test_networkactivity.py	2017-04-19 20:54:09 +0000
@@ -184,8 +184,8 @@
         self.assertMessages(self.mstore.get_pending_messages(),
                         [{"type": "network-activity",
                           "activities": {
-                              "lo": [(step_size, 0, 1000)],
-                              "eth0": [(step_size, 0, 1000)]}}])
+                              b"lo": [(step_size, 0, 1000)],
+                              b"eth0": [(step_size, 0, 1000)]}}])
 
     def test_config(self):
         """The network activity plugin is enabled by default."""