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

Proposed by Simon Poirier on 2017-04-19
Status: Merged
Approved by: Simon Poirier on 2017-04-20
Approved revision: 1018
Merged at revision: 1020
Proposed branch: lp:~simpoir/landscape-client/py3fix-network-usage
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 197 lines (+51/-22)
6 files modified
landscape/broker/store.py (+2/-1)
landscape/lib/amp.py (+2/-1)
landscape/lib/bpickle.py (+22/-16)
landscape/lib/tests/test_amp.py (+15/-0)
landscape/lib/tests/test_bpickle.py (+8/-2)
landscape/monitor/tests/test_networkactivity.py (+2/-2)
To merge this branch: bzr merge lp:~simpoir/landscape-client/py3fix-network-usage
Reviewer Review Type Date Requested Status
Landscape Builder test results Approve on 2017-04-19
Eric Snow (community) 2017-04-19 Approve on 2017-04-19
Review via email: mp+322794@code.launchpad.net

Commit message

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).

Description of the change

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.

To post a comment you must log in.
review: Abstain (executing tests)

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) wrote :

LGTM

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

review: Approve
review: Abstain (executing tests)

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/broker/store.py'
2--- landscape/broker/store.py 2017-04-06 18:27:36 +0000
3+++ landscape/broker/store.py 2017-04-19 20:54:09 +0000
4@@ -262,7 +262,8 @@
5 break
6 data = read_binary_file(self._message_dir(filename))
7 try:
8- message = bpickle.loads(data)
9+ # don't reinterpret messages that are meant to be sent out
10+ message = bpickle.loads(data, as_is=True)
11 except ValueError as e:
12 logging.exception(e)
13 self._add_flags(filename, BROKEN)
14
15=== modified file 'landscape/lib/amp.py'
16--- landscape/lib/amp.py 2017-03-17 09:26:45 +0000
17+++ landscape/lib/amp.py 2017-04-19 20:54:09 +0000
18@@ -165,7 +165,8 @@
19 chunks.append(arguments)
20 arguments = b"".join(chunks)
21
22- args, kwargs = bpickle.loads(arguments)
23+ # Pass the the arguments as-is without reinterpreting strings.
24+ args, kwargs = bpickle.loads(arguments, as_is=True)
25
26 # We encoded the method name in `send_method_call` and have to decode
27 # it here again.
28
29=== modified file 'landscape/lib/bpickle.py'
30--- landscape/lib/bpickle.py 2017-04-11 14:13:28 +0000
31+++ landscape/lib/bpickle.py 2017-04-19 20:54:09 +0000
32@@ -45,13 +45,19 @@
33 raise ValueError("Unsupported type: %s" % e)
34
35
36-def loads(byte_string, _lt=loads_table):
37+def loads(byte_string, _lt=loads_table, as_is=False):
38+ """Load a serialized byte_string.
39+
40+ @param byte_string: the serialized data
41+ @param _lt: the conversion map
42+ @param as_is: don't reinterpret dict keys as str
43+ """
44 if not byte_string:
45 raise ValueError("Can't load empty string")
46 try:
47 # To avoid python3 turning byte_string[0] into an int,
48 # we slice the bytestring instead.
49- return _lt[byte_string[0:1]](byte_string, 0)[0]
50+ return _lt[byte_string[0:1]](byte_string, 0, as_is=as_is)[0]
51 except KeyError as e:
52 raise ValueError("Unknown type character: %s" % e)
53 except IndexError:
54@@ -107,59 +113,59 @@
55 return b"n"
56
57
58-def loads_bool(bytestring, pos):
59+def loads_bool(bytestring, pos, as_is=False):
60 return bool(int(bytestring[pos+1:pos+2])), pos+2
61
62
63-def loads_int(bytestring, pos):
64+def loads_int(bytestring, pos, as_is=False):
65 endpos = bytestring.index(b";", pos)
66 return int(bytestring[pos+1:endpos]), endpos+1
67
68
69-def loads_float(bytestring, pos):
70+def loads_float(bytestring, pos, as_is=False):
71 endpos = bytestring.index(b";", pos)
72 return float(bytestring[pos+1:endpos]), endpos+1
73
74
75-def loads_bytes(bytestring, pos):
76+def loads_bytes(bytestring, pos, as_is=False):
77 startpos = bytestring.index(b":", pos)+1
78 endpos = startpos+int(bytestring[pos+1:startpos-1])
79 return bytestring[startpos:endpos], endpos
80
81
82-def loads_unicode(bytestring, pos):
83+def loads_unicode(bytestring, pos, as_is=False):
84 startpos = bytestring.index(b":", pos)+1
85 endpos = startpos+int(bytestring[pos+1:startpos-1])
86 return bytestring[startpos:endpos].decode("utf-8"), endpos
87
88
89-def loads_list(bytestring, pos, _lt=loads_table):
90+def loads_list(bytestring, pos, _lt=loads_table, as_is=False):
91 pos += 1
92 res = []
93 append = res.append
94 while bytestring[pos:pos+1] != b";":
95- obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
96+ obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
97 append(obj)
98 return res, pos+1
99
100
101-def loads_tuple(bytestring, pos, _lt=loads_table):
102+def loads_tuple(bytestring, pos, _lt=loads_table, as_is=False):
103 pos += 1
104 res = []
105 append = res.append
106 while bytestring[pos:pos+1] != b";":
107- obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
108+ obj, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
109 append(obj)
110 return tuple(res), pos+1
111
112
113-def loads_dict(bytestring, pos, _lt=loads_table):
114+def loads_dict(bytestring, pos, _lt=loads_table, as_is=False):
115 pos += 1
116 res = {}
117 while bytestring[pos:pos+1] != b";":
118- key, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
119- val, pos = _lt[bytestring[pos:pos+1]](bytestring, pos)
120- if _PY3 and isinstance(key, bytes):
121+ key, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
122+ val, pos = _lt[bytestring[pos:pos+1]](bytestring, pos, as_is=as_is)
123+ if _PY3 and not as_is and isinstance(key, bytes):
124 # Although the wire format of dictionary keys is ASCII bytes, the
125 # code actually expects them to be strings, so we convert them
126 # here.
127@@ -168,7 +174,7 @@
128 return res, pos+1
129
130
131-def loads_none(str, pos):
132+def loads_none(str, pos, as_is=False):
133 return None, pos+1
134
135
136
137=== modified file 'landscape/lib/tests/test_amp.py'
138--- landscape/lib/tests/test_amp.py 2017-03-09 13:41:17 +0000
139+++ landscape/lib/tests/test_amp.py 2017-04-19 20:54:09 +0000
140@@ -213,6 +213,21 @@
141 self.connection.flush()
142 self.assertEqual("barfoobarfoobarfoo", self.successResultOf(deferred))
143
144+ def test_with_bytes_dictionary_arguments(self):
145+ """
146+ Method arguments passed to a MethodCall can be a dict of bytes.
147+ """
148+ arg = {b"byte_key": 1}
149+ self.object.method = lambda d: ",".join([
150+ type(x).__name__ for x in d.keys()])
151+ deferred = self.sender.send_method_call(
152+ method="method",
153+ args=[arg],
154+ kwargs={})
155+ self.connection.flush()
156+ # str under python2, bytes under python3
157+ self.assertEqual(type(b"").__name__, self.successResultOf(deferred))
158+
159 def test_with_non_serializable_return_value(self):
160 """
161 If the target object method returns an object that can't be serialized,
162
163=== modified file 'landscape/lib/tests/test_bpickle.py'
164--- landscape/lib/tests/test_bpickle.py 2017-04-10 21:28:13 +0000
165+++ landscape/lib/tests/test_bpickle.py 2017-04-19 20:54:09 +0000
166@@ -49,8 +49,14 @@
167 {True: False})
168
169 def test_dict_bytes_keys(self):
170- data = bpickle.dumps({b"hello": True})
171- self.assertEqual(bpickle.loads(data), {"hello": True})
172+ """Check loading dict bytes keys without reinterpreting."""
173+ # Happens in amp and broker. Since those messages are meant to be
174+ # forwarded to the server without changing schema, keys shouldn't be
175+ # decoded in this case.
176+ initial_data = {b"hello": True}
177+ data = bpickle.dumps(initial_data)
178+ result = bpickle.loads(data, as_is=True)
179+ self.assertEqual(initial_data, result)
180
181 def test_long(self):
182 long = 99999999999999999999999999999
183
184=== modified file 'landscape/monitor/tests/test_networkactivity.py'
185--- landscape/monitor/tests/test_networkactivity.py 2017-04-05 13:08:02 +0000
186+++ landscape/monitor/tests/test_networkactivity.py 2017-04-19 20:54:09 +0000
187@@ -184,8 +184,8 @@
188 self.assertMessages(self.mstore.get_pending_messages(),
189 [{"type": "network-activity",
190 "activities": {
191- "lo": [(step_size, 0, 1000)],
192- "eth0": [(step_size, 0, 1000)]}}])
193+ b"lo": [(step_size, 0, 1000)],
194+ b"eth0": [(step_size, 0, 1000)]}}])
195
196 def test_config(self):
197 """The network activity plugin is enabled by default."""

Subscribers

People subscribed via source and target branches

to all changes: