Merge lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message into lp:~landscape/landscape-client/trunk
- add-vminfo-to-computer-info-message
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Free Ekanayaka |
Approved revision: | 333 |
Merge reported by: | Thomas Herve |
Merged at revision: | not available |
Proposed branch: | lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message |
Merge into: | lp:~landscape/landscape-client/trunk |
Diff against target: |
291 lines (+211/-5) 3 files modified
landscape/message_schemas.py (+3/-2) landscape/monitor/computerinfo.py (+40/-1) landscape/monitor/tests/test_computerinfo.py (+168/-2) |
To merge this branch: | bzr merge lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Free Ekanayaka (community) | Approve | ||
Thomas Herve (community) | Approve | ||
Landscape | Pending | ||
Review via email:
|
Commit message
Description of the change
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Free Ekanayaka (free.ekanayaka) wrote : | # |
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.
[1]
+ def _get_vm_info(self, root_path="/"):
Please pass root_path to the constructor instead, like:
def __init__(self, get_fqdn=get_fqdn,
and then you can have something like
root_path = self.makeDir()
plugin = ComputerInfo(
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(
get_fqdn = get_fqdn
meminfo_file = "/proc/meminfo"
lsb_
root_path = "/"
and then
plugin = ComputerInfo()
plugin.
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_resynchron
+ def test_wb_
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".
- 331. By Fernando Correa Neto
-
- Addressing therve and free view comments
- 332. By Fernando Correa Neto
-
- Backporting tests from old revision
- 333. By Fernando Correa Neto
-
- Linting
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Thomas Herve (therve) wrote : | # |
Looks good!
[6]
+ elif exists(
+ with open(join_
+ 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!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Free Ekanayaka (free.ekanayaka) wrote : | # |
Looks good, just a few more minor things. +1 with those fixed!
[5]
+ path_join = os.path.join
and
+ def join_root_
+ return path_join(
I'd avoid setting the the path_join variable, just call os.path.join directly, like:
def join_root_
return os.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(
[7]
- "total-swap": 1584}
+ "total-swap": 1584, "vm-info": ""}
I think in this test (test_resynchro
[8]
There's a test missing for the openvz case.
- 334. By Fernando Correa Neto
-
- Addressing final therve and free review comments
- 335. By Fernando Correa Neto
-
- Adding missing test for openvz and also adding root_path to the constructor of ComputerInfo in test_resynchronize test
Preview Diff
1 | === modified file 'landscape/message_schemas.py' | |||
2 | --- landscape/message_schemas.py 2010-06-28 12:52:39 +0000 | |||
3 | +++ landscape/message_schemas.py 2011-04-20 21:43:28 +0000 | |||
4 | @@ -68,10 +68,11 @@ | |||
5 | 68 | "computer-info", | 68 | "computer-info", |
6 | 69 | {"hostname": utf8, | 69 | {"hostname": utf8, |
7 | 70 | "total-memory": Int(), | 70 | "total-memory": Int(), |
9 | 71 | "total-swap": Int()}, | 71 | "total-swap": Int(), |
10 | 72 | "vm-info": String()}, | ||
11 | 72 | # Not sure why these are all optional, but it's explicitly tested | 73 | # Not sure why these are all optional, but it's explicitly tested |
12 | 73 | # in the server | 74 | # in the server |
14 | 74 | optional=["hostname", "total-memory", "total-swap"]) | 75 | optional=["hostname", "total-memory", "total-swap", "vm-info"]) |
15 | 75 | 76 | ||
16 | 76 | DISTRIBUTION_INFO = Message( | 77 | DISTRIBUTION_INFO = Message( |
17 | 77 | "distribution-info", | 78 | "distribution-info", |
18 | 78 | 79 | ||
19 | === modified file 'landscape/monitor/computerinfo.py' | |||
20 | --- landscape/monitor/computerinfo.py 2010-12-10 13:57:11 +0000 | |||
21 | +++ landscape/monitor/computerinfo.py 2011-04-20 21:43:28 +0000 | |||
22 | @@ -1,4 +1,5 @@ | |||
23 | 1 | import logging | 1 | import logging |
24 | 2 | import os | ||
25 | 2 | 3 | ||
26 | 3 | from landscape.lib.lsb_release import LSB_RELEASE_FILENAME, parse_lsb_release | 4 | from landscape.lib.lsb_release import LSB_RELEASE_FILENAME, parse_lsb_release |
27 | 4 | from landscape.lib.network import get_fqdn | 5 | from landscape.lib.network import get_fqdn |
28 | @@ -16,10 +17,12 @@ | |||
29 | 16 | 17 | ||
30 | 17 | def __init__(self, get_fqdn=get_fqdn, | 18 | def __init__(self, get_fqdn=get_fqdn, |
31 | 18 | meminfo_file="/proc/meminfo", | 19 | meminfo_file="/proc/meminfo", |
33 | 19 | lsb_release_filename=LSB_RELEASE_FILENAME): | 20 | lsb_release_filename=LSB_RELEASE_FILENAME, |
34 | 21 | root_path="/"): | ||
35 | 20 | self._get_fqdn = get_fqdn | 22 | self._get_fqdn = get_fqdn |
36 | 21 | self._meminfo_file = meminfo_file | 23 | self._meminfo_file = meminfo_file |
37 | 22 | self._lsb_release_filename = lsb_release_filename | 24 | self._lsb_release_filename = lsb_release_filename |
38 | 25 | self._root_path = root_path | ||
39 | 23 | 26 | ||
40 | 24 | def register(self, registry): | 27 | def register(self, registry): |
41 | 25 | super(ComputerInfo, self).register(registry) | 28 | super(ComputerInfo, self).register(registry) |
42 | @@ -57,6 +60,8 @@ | |||
43 | 57 | self._add_if_new(message, "total-memory", | 60 | self._add_if_new(message, "total-memory", |
44 | 58 | total_memory) | 61 | total_memory) |
45 | 59 | self._add_if_new(message, "total-swap", total_swap) | 62 | self._add_if_new(message, "total-swap", total_swap) |
46 | 63 | vm_info = self._get_vm_info() | ||
47 | 64 | self._add_if_new(message, "vm-info", vm_info) | ||
48 | 60 | return message | 65 | return message |
49 | 61 | 66 | ||
50 | 62 | def _add_if_new(self, message, key, value): | 67 | def _add_if_new(self, message, key, value): |
51 | @@ -90,3 +95,37 @@ | |||
52 | 90 | message = {} | 95 | message = {} |
53 | 91 | message.update(parse_lsb_release(self._lsb_release_filename)) | 96 | message.update(parse_lsb_release(self._lsb_release_filename)) |
54 | 92 | return message | 97 | return message |
55 | 98 | |||
56 | 99 | def _get_vm_info(self): | ||
57 | 100 | """ | ||
58 | 101 | This is a utility that returns the virtualization type | ||
59 | 102 | |||
60 | 103 | It loops through some possible configurations and return a string with | ||
61 | 104 | the name of the technology being used or None if there's no match | ||
62 | 105 | """ | ||
63 | 106 | virt_info = "" | ||
64 | 107 | |||
65 | 108 | def join_root_path(path): | ||
66 | 109 | return os.path.join(self._root_path, path) | ||
67 | 110 | |||
68 | 111 | xen_paths = ["proc/sys/xen", "sys/bus/xen", "proc/xen"] | ||
69 | 112 | xen_paths = map(join_root_path, xen_paths) | ||
70 | 113 | |||
71 | 114 | vz_path = os.path.join(self._root_path, "proc/vz") | ||
72 | 115 | if os.path.exists(vz_path): | ||
73 | 116 | virt_info = "openvz" | ||
74 | 117 | |||
75 | 118 | elif filter(os.path.exists, xen_paths): | ||
76 | 119 | virt_info = "xen" | ||
77 | 120 | |||
78 | 121 | cpu_info_path = os.path.join(self._root_path, "proc/cpuinfo") | ||
79 | 122 | if os.path.exists(cpu_info_path): | ||
80 | 123 | try: | ||
81 | 124 | fd = open(cpu_info_path) | ||
82 | 125 | cpuinfo = fd.read() | ||
83 | 126 | if "QEMU Virtual CPU" in cpuinfo: | ||
84 | 127 | virt_info = "kvm" | ||
85 | 128 | finally: | ||
86 | 129 | fd.close() | ||
87 | 130 | |||
88 | 131 | return virt_info | ||
89 | 93 | 132 | ||
90 | === modified file 'landscape/monitor/tests/test_computerinfo.py' | |||
91 | --- landscape/monitor/tests/test_computerinfo.py 2010-04-28 13:07:53 +0000 | |||
92 | +++ landscape/monitor/tests/test_computerinfo.py 2011-04-20 21:43:28 +0000 | |||
93 | @@ -1,4 +1,5 @@ | |||
94 | 1 | import re | 1 | import re |
95 | 2 | import os | ||
96 | 2 | 3 | ||
97 | 3 | from landscape.monitor.computerinfo import ComputerInfo | 4 | from landscape.monitor.computerinfo import ComputerInfo |
98 | 4 | from landscape.tests.helpers import LandscapeTest, MonitorHelper | 5 | from landscape.tests.helpers import LandscapeTest, MonitorHelper |
99 | @@ -44,6 +45,27 @@ | |||
100 | 44 | VmallocChunk: 107432 kB | 45 | VmallocChunk: 107432 kB |
101 | 45 | """ | 46 | """ |
102 | 46 | 47 | ||
103 | 48 | sample_kvm_cpuinfo = """ | ||
104 | 49 | processor : 0 | ||
105 | 50 | vendor_id : GenuineIntel | ||
106 | 51 | cpu family : 6 | ||
107 | 52 | model : 2 | ||
108 | 53 | model name : QEMU Virtual CPU version 0.14.0 | ||
109 | 54 | stepping : 3 | ||
110 | 55 | cpu MHz : 2653.112 | ||
111 | 56 | cache size : 4096 KB | ||
112 | 57 | fpu : yes | ||
113 | 58 | fpu_exception : yes | ||
114 | 59 | cpuid level : 4 | ||
115 | 60 | wp : yes | ||
116 | 61 | flags : fpu de pse tsc msr pae mce cx8 apic sep mtrr pge mca | ||
117 | 62 | bogomips : 5306.22 | ||
118 | 63 | clflush size : 64 | ||
119 | 64 | cache_alignment : 64 | ||
120 | 65 | address sizes : 40 bits physical, 48 bits virtual | ||
121 | 66 | power management: | ||
122 | 67 | """ | ||
123 | 68 | |||
124 | 47 | def setUp(self): | 69 | def setUp(self): |
125 | 48 | LandscapeTest.setUp(self) | 70 | LandscapeTest.setUp(self) |
126 | 49 | self.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE) | 71 | self.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE) |
127 | @@ -272,14 +294,15 @@ | |||
128 | 272 | meminfo_filename = self.makeFile(self.sample_memory_info) | 294 | meminfo_filename = self.makeFile(self.sample_memory_info) |
129 | 273 | plugin = ComputerInfo(get_fqdn=get_fqdn, | 295 | plugin = ComputerInfo(get_fqdn=get_fqdn, |
130 | 274 | meminfo_file=meminfo_filename, | 296 | meminfo_file=meminfo_filename, |
132 | 275 | lsb_release_filename=self.lsb_release_filename) | 297 | lsb_release_filename=self.lsb_release_filename, |
133 | 298 | root_path=self.makeDir()) | ||
134 | 276 | self.monitor.add(plugin) | 299 | self.monitor.add(plugin) |
135 | 277 | plugin.exchange() | 300 | plugin.exchange() |
136 | 278 | self.reactor.fire("resynchronize") | 301 | self.reactor.fire("resynchronize") |
137 | 279 | plugin.exchange() | 302 | plugin.exchange() |
138 | 280 | computer_info = {"type": "computer-info", "hostname": "ooga.local", | 303 | computer_info = {"type": "computer-info", "hostname": "ooga.local", |
139 | 281 | "timestamp": 0, "total-memory": 1510, | 304 | "timestamp": 0, "total-memory": 1510, |
141 | 282 | "total-swap": 1584} | 305 | "total-swap": 1584, "vm-info": ""} |
142 | 283 | dist_info = {"type": "distribution-info", | 306 | dist_info = {"type": "distribution-info", |
143 | 284 | "code-name": "dapper", "description": "Ubuntu 6.06.1 LTS", | 307 | "code-name": "dapper", "description": "Ubuntu 6.06.1 LTS", |
144 | 285 | "distributor-id": "Ubuntu", "release": "6.06"} | 308 | "distributor-id": "Ubuntu", "release": "6.06"} |
145 | @@ -324,3 +347,146 @@ | |||
146 | 324 | 347 | ||
147 | 325 | self.mstore.set_accepted_types(["distribution-info", "computer-info"]) | 348 | self.mstore.set_accepted_types(["distribution-info", "computer-info"]) |
148 | 326 | self.assertMessages(list(self.mstore.get_pending_messages()), []) | 349 | self.assertMessages(list(self.mstore.get_pending_messages()), []) |
149 | 350 | |||
150 | 351 | def test_vminfo_empty_when_no_virtualization_is_found(self): | ||
151 | 352 | """ | ||
152 | 353 | L{ComputerInfo} should always have the vm-info key even when no | ||
153 | 354 | virtualization is used. | ||
154 | 355 | |||
155 | 356 | And it should be an empty string. | ||
156 | 357 | """ | ||
157 | 358 | self.mstore.set_accepted_types(["computer-info"]) | ||
158 | 359 | root_path = self.makeDir() | ||
159 | 360 | plugin = ComputerInfo(root_path=root_path) | ||
160 | 361 | self.monitor.add(plugin) | ||
161 | 362 | |||
162 | 363 | plugin.exchange() | ||
163 | 364 | message = self.mstore.get_pending_messages()[0] | ||
164 | 365 | self.assertTrue("vm-info" in message) | ||
165 | 366 | self.assertEqual("", message["vm-info"]) | ||
166 | 367 | |||
167 | 368 | def test_vminfo_is_openvz_when_proc_vz_exists(self): | ||
168 | 369 | """ | ||
169 | 370 | L{ComputerInfo} should have 'openvz' as the value of vm-info when | ||
170 | 371 | /proc/vz exists. | ||
171 | 372 | """ | ||
172 | 373 | root_path = self.makeDir() | ||
173 | 374 | proc_path = os.path.join(root_path, "proc") | ||
174 | 375 | self.makeDir(path=proc_path) | ||
175 | 376 | |||
176 | 377 | proc_vz_path = os.path.join(proc_path, "vz") | ||
177 | 378 | self.makeFile(path=proc_vz_path, content="foo") | ||
178 | 379 | |||
179 | 380 | self.mstore.set_accepted_types(["computer-info"]) | ||
180 | 381 | plugin = ComputerInfo(root_path=root_path) | ||
181 | 382 | self.monitor.add(plugin) | ||
182 | 383 | |||
183 | 384 | plugin.exchange() | ||
184 | 385 | message = self.mstore.get_pending_messages()[0] | ||
185 | 386 | self.assertEquals('openvz', message["vm-info"]) | ||
186 | 387 | |||
187 | 388 | def test_vminfo_is_xen_when_proc_sys_xen_exists(self): | ||
188 | 389 | """ | ||
189 | 390 | L{ComputerInfo} should have 'xen' as the value of vm-info when | ||
190 | 391 | /proc/sys/xen exists. | ||
191 | 392 | """ | ||
192 | 393 | root_path = self.makeDir() | ||
193 | 394 | proc_path = os.path.join(root_path, "proc") | ||
194 | 395 | self.makeDir(path=proc_path) | ||
195 | 396 | |||
196 | 397 | proc_sys_path = os.path.join(proc_path, "sys") | ||
197 | 398 | self.makeDir(path=proc_sys_path) | ||
198 | 399 | |||
199 | 400 | proc_sys_xen_path = os.path.join(proc_sys_path, "xen") | ||
200 | 401 | self.makeFile(path=proc_sys_xen_path, content="foo") | ||
201 | 402 | |||
202 | 403 | self.mstore.set_accepted_types(["computer-info"]) | ||
203 | 404 | plugin = ComputerInfo(root_path=root_path) | ||
204 | 405 | self.monitor.add(plugin) | ||
205 | 406 | |||
206 | 407 | plugin.exchange() | ||
207 | 408 | message = self.mstore.get_pending_messages()[0] | ||
208 | 409 | self.assertEquals('xen', message["vm-info"]) | ||
209 | 410 | |||
210 | 411 | def test_vminfo_is_xen_when_sys_bus_xen_exists(self): | ||
211 | 412 | """ | ||
212 | 413 | L{ComputerInfo} should have 'xen' as value of vm-info when | ||
213 | 414 | /sys/bus/xen exists | ||
214 | 415 | """ | ||
215 | 416 | root_path = self.makeDir() | ||
216 | 417 | sys_path = os.path.join(root_path, "sys") | ||
217 | 418 | self.makeDir(path=sys_path) | ||
218 | 419 | |||
219 | 420 | sys_bus_path = os.path.join(sys_path, "bus") | ||
220 | 421 | self.makeDir(path=sys_bus_path) | ||
221 | 422 | |||
222 | 423 | sys_bus_xen_path = os.path.join(sys_bus_path, "xen") | ||
223 | 424 | self.makeFile(path=sys_bus_xen_path, content="foo") | ||
224 | 425 | |||
225 | 426 | self.mstore.set_accepted_types(["computer-info"]) | ||
226 | 427 | plugin = ComputerInfo(root_path=root_path) | ||
227 | 428 | self.monitor.add(plugin) | ||
228 | 429 | |||
229 | 430 | plugin.exchange() | ||
230 | 431 | message = self.mstore.get_pending_messages()[0] | ||
231 | 432 | self.assertEquals('xen', message["vm-info"]) | ||
232 | 433 | |||
233 | 434 | def test_vminfo_is_xen_when_proc_xen_exists(self): | ||
234 | 435 | """ | ||
235 | 436 | L{ComputerInfo} should have 'xen' as value of vm-info when | ||
236 | 437 | /proc/xen exists. | ||
237 | 438 | """ | ||
238 | 439 | root_path = self.makeDir() | ||
239 | 440 | proc_path = os.path.join(root_path, "proc") | ||
240 | 441 | self.makeDir(path=proc_path) | ||
241 | 442 | |||
242 | 443 | proc_xen_path = os.path.join(proc_path, "xen") | ||
243 | 444 | self.makeFile(path=proc_xen_path, content="foo") | ||
244 | 445 | |||
245 | 446 | self.mstore.set_accepted_types(["computer-info"]) | ||
246 | 447 | plugin = ComputerInfo(root_path=root_path) | ||
247 | 448 | self.monitor.add(plugin) | ||
248 | 449 | |||
249 | 450 | plugin.exchange() | ||
250 | 451 | message = self.mstore.get_pending_messages()[0] | ||
251 | 452 | self.assertEquals('xen', message["vm-info"]) | ||
252 | 453 | |||
253 | 454 | def test_vminfo_is_kvm_when_qemu_is_found_in_proc_cpuinfo(self): | ||
254 | 455 | """ | ||
255 | 456 | L{ComputerInfo} should have 'kvm' as value of vm-info when | ||
256 | 457 | QEMU Virtual CPU is found in /proc/cpuinfo. | ||
257 | 458 | """ | ||
258 | 459 | root_path = self.makeDir() | ||
259 | 460 | proc_path = os.path.join(root_path, "proc") | ||
260 | 461 | self.makeDir(path=proc_path) | ||
261 | 462 | |||
262 | 463 | cpuinfo_path = os.path.join(proc_path, "cpuinfo") | ||
263 | 464 | self.makeFile(path=cpuinfo_path, content=self.sample_kvm_cpuinfo) | ||
264 | 465 | |||
265 | 466 | self.mstore.set_accepted_types(["computer-info"]) | ||
266 | 467 | plugin = ComputerInfo(root_path=root_path) | ||
267 | 468 | self.monitor.add(plugin) | ||
268 | 469 | |||
269 | 470 | plugin.exchange() | ||
270 | 471 | message = self.mstore.get_pending_messages()[0] | ||
271 | 472 | self.assertEqual("kvm", message["vm-info"]) | ||
272 | 473 | |||
273 | 474 | def test_vminfo_is_empty_when_qemu_is_not_found_in_proc_cpuinfo(self): | ||
274 | 475 | """ | ||
275 | 476 | L{ComputerInfo} should have an empty string as value of vm-info when | ||
276 | 477 | QEMU Virtual CPU is not found in /proc/cpuinfo. | ||
277 | 478 | """ | ||
278 | 479 | root_path = self.makeDir() | ||
279 | 480 | proc_path = os.path.join(root_path, "proc") | ||
280 | 481 | self.makeDir(path=proc_path) | ||
281 | 482 | |||
282 | 483 | cpuinfo_path = os.path.join(proc_path, "cpuinfo") | ||
283 | 484 | self.makeFile(path=cpuinfo_path, content="foo") | ||
284 | 485 | |||
285 | 486 | self.mstore.set_accepted_types(["computer-info"]) | ||
286 | 487 | plugin = ComputerInfo(root_path=root_path) | ||
287 | 488 | self.monitor.add(plugin) | ||
288 | 489 | |||
289 | 490 | plugin.exchange() | ||
290 | 491 | message = self.mstore.get_pending_messages()[0] | ||
291 | 492 | self.assertEqual("", message["vm-info"]) |
[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] path(u" proc/cpuinfo" )).read( ):
+ elif "QEMU Virtual CPU" in open(
+ join_root_
+ virt_info = "KVM"
You should close the open file explicitly.
[5]
+import os.path
You can import os directly.
Thanks!