Merge ~jfguedez/juju-lint:bug/1903973 into juju-lint:master
- Git
- lp:~jfguedez/juju-lint
- bug/1903973
- Merge into master
Proposed by
Jose Guedez
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 3b85de9d81bf29c7769febd0a60c7a41fc424a34 | ||||
Proposed branch: | ~jfguedez/juju-lint:bug/1903973 | ||||
Merge into: | juju-lint:master | ||||
Diff against target: |
340 lines (+158/-47) 5 files modified
contrib/includes/base.yaml (+1/-1) jujulint/lint.py (+22/-8) jujulint/util.py (+25/-3) tests/conftest.py (+6/-0) tests/test_jujulint.py (+104/-35) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Edin S (community) | Approve | ||
Juju Lint maintainers | Pending | ||
Review via email: mp+408853@code.launchpad.net |
Commit message
Add the "metal only" condition for subordinates
Description of the change
To post a comment you must log in.
Revision history for this message
Jose Guedez (jfguedez) wrote : | # |
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.
Revision history for this message
Edin S (exsdev) : | # |
review:
Approve
Revision history for this message
James Troup (elmo) wrote : | # |
I had to thought check MAC addresses, FWIW; it's also a heuristic, but a fairly reliable one for detecting the most common virt platforms (KVM/VMWare).
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/contrib/includes/base.yaml b/contrib/includes/base.yaml | |||
2 | index 1815ee2..da558a3 100644 | |||
3 | --- a/contrib/includes/base.yaml | |||
4 | +++ b/contrib/includes/base.yaml | |||
5 | @@ -35,6 +35,6 @@ subordinates: | |||
6 | 35 | thruk-agent: | 35 | thruk-agent: |
7 | 36 | where: on nagios | 36 | where: on nagios |
8 | 37 | hw-health: | 37 | hw-health: |
10 | 38 | where: host only | 38 | where: metal only |
11 | 39 | logrotated: | 39 | logrotated: |
12 | 40 | where: all | 40 | where: all |
13 | diff --git a/jujulint/lint.py b/jujulint/lint.py | |||
14 | index 68fef78..f680370 100755 | |||
15 | --- a/jujulint/lint.py | |||
16 | +++ b/jujulint/lint.py | |||
17 | @@ -33,7 +33,7 @@ import attr | |||
18 | 33 | import dateutil.parser | 33 | import dateutil.parser |
19 | 34 | from dateutil import relativedelta | 34 | from dateutil import relativedelta |
20 | 35 | 35 | ||
22 | 36 | from jujulint.util import flatten_list, is_container, extract_charm_name | 36 | import jujulint.util as utils |
23 | 37 | from jujulint.logging import Logger | 37 | from jujulint.logging import Logger |
24 | 38 | 38 | ||
25 | 39 | VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte") | 39 | VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte") |
26 | @@ -133,7 +133,9 @@ class Linter: | |||
27 | 133 | self.lint_rules["subordinates"][name] = dict(where=where) | 133 | self.lint_rules["subordinates"][name] = dict(where=where) |
28 | 134 | 134 | ||
29 | 135 | # Flatten all entries (to account for nesting due to YAML anchors (templating) | 135 | # Flatten all entries (to account for nesting due to YAML anchors (templating) |
31 | 136 | self.lint_rules = {k: flatten_list(v) for k, v in self.lint_rules.items()} | 136 | self.lint_rules = { |
32 | 137 | k: utils.flatten_list(v) for k, v in self.lint_rules.items() | ||
33 | 138 | } | ||
34 | 137 | 139 | ||
35 | 138 | self._log_with_header( | 140 | self._log_with_header( |
36 | 139 | "Lint Rules: {}".format(pprint.pformat(self.lint_rules)) | 141 | "Lint Rules: {}".format(pprint.pformat(self.lint_rules)) |
37 | @@ -372,7 +374,7 @@ class Linter: | |||
38 | 372 | ) | 374 | ) |
39 | 373 | continue | 375 | continue |
40 | 374 | 376 | ||
42 | 375 | charm_name = extract_charm_name(applications[application]["charm"]) | 377 | charm_name = utils.extract_charm_name(applications[application]["charm"]) |
43 | 376 | if "config" in self.lint_rules: | 378 | if "config" in self.lint_rules: |
44 | 377 | if charm_name in self.lint_rules["config"]: | 379 | if charm_name in self.lint_rules["config"]: |
45 | 378 | lint_rules = self.lint_rules["config"][charm_name].items() | 380 | lint_rules = self.lint_rules["config"][charm_name].items() |
46 | @@ -393,7 +395,7 @@ class Linter: | |||
47 | 393 | lint_rules, | 395 | lint_rules, |
48 | 394 | ) | 396 | ) |
49 | 395 | 397 | ||
51 | 396 | def check_subs(self): | 398 | def check_subs(self, machines_data): |
52 | 397 | """Check the subordinates in the model.""" | 399 | """Check the subordinates in the model.""" |
53 | 398 | all_or_nothing = set() | 400 | all_or_nothing = set() |
54 | 399 | for machine in self.model.subs_on_machines: | 401 | for machine in self.model.subs_on_machines: |
55 | @@ -429,7 +431,7 @@ class Linter: | |||
56 | 429 | self._log_with_header( | 431 | self._log_with_header( |
57 | 430 | "requirement is 'host only' form...." | 432 | "requirement is 'host only' form...." |
58 | 431 | ) | 433 | ) |
60 | 432 | if is_container(machine): | 434 | if utils.is_container(machine): |
61 | 433 | self._log_with_header("... and we are a container, checking") | 435 | self._log_with_header("... and we are a container, checking") |
62 | 434 | # XXX check alternate names? | 436 | # XXX check alternate names? |
63 | 435 | if required_sub in present_subs: | 437 | if required_sub in present_subs: |
64 | @@ -438,6 +440,18 @@ class Linter: | |||
65 | 438 | self.model.extraneous_subs[required_sub].add(app) | 440 | self.model.extraneous_subs[required_sub].add(app) |
66 | 439 | continue | 441 | continue |
67 | 440 | self._log_with_header("... and we are a host, will fallthrough") | 442 | self._log_with_header("... and we are a host, will fallthrough") |
68 | 443 | elif where == "metal only": | ||
69 | 444 | self._log_with_header( | ||
70 | 445 | "requirement is 'metal only' form...." | ||
71 | 446 | ) | ||
72 | 447 | if not utils.is_metal(machine, machines_data.get(machine, {})): | ||
73 | 448 | self._log_with_header("... and we are not a metal, checking") | ||
74 | 449 | if required_sub in present_subs: | ||
75 | 450 | self._log_with_header("... found extraneous sub") | ||
76 | 451 | for app in self.model.apps_on_machines[machine]: | ||
77 | 452 | self.model.extraneous_subs[required_sub].add(app) | ||
78 | 453 | continue | ||
79 | 454 | self._log_with_header("... and we are a metal, will fallthrough") | ||
80 | 441 | 455 | ||
81 | 442 | elif where == "all or nothing" and required_sub not in all_or_nothing: | 456 | elif where == "all or nothing" and required_sub not in all_or_nothing: |
82 | 443 | self._log_with_header( | 457 | self._log_with_header( |
83 | @@ -448,7 +462,7 @@ class Linter: | |||
84 | 448 | # need to change the name we expect to see it as | 462 | # need to change the name we expect to see it as |
85 | 449 | elif where == "container aware": | 463 | elif where == "container aware": |
86 | 450 | self._log_with_header("requirement is 'container aware'.") | 464 | self._log_with_header("requirement is 'container aware'.") |
88 | 451 | if is_container(machine): | 465 | if utils.is_container(machine): |
89 | 452 | suffixes = self.lint_rules["subordinates"][required_sub][ | 466 | suffixes = self.lint_rules["subordinates"][required_sub][ |
90 | 453 | "container-suffixes" | 467 | "container-suffixes" |
91 | 454 | ] | 468 | ] |
92 | @@ -719,7 +733,7 @@ class Linter: | |||
93 | 719 | """Process applications in the model, validating and normalising the names.""" | 733 | """Process applications in the model, validating and normalising the names.""" |
94 | 720 | for app in applications: | 734 | for app in applications: |
95 | 721 | if "charm" in applications[app]: | 735 | if "charm" in applications[app]: |
97 | 722 | charm_name = extract_charm_name(applications[app]["charm"]) | 736 | charm_name = utils.extract_charm_name(applications[app]["charm"]) |
98 | 723 | self.model.charms.add(charm_name) | 737 | self.model.charms.add(charm_name) |
99 | 724 | self.model.app_to_charm[app] = charm_name | 738 | self.model.app_to_charm[app] = charm_name |
100 | 725 | else: | 739 | else: |
101 | @@ -973,7 +987,7 @@ class Linter: | |||
102 | 973 | for app in parsed_yaml[applications]: | 987 | for app in parsed_yaml[applications]: |
103 | 974 | self.process_subordinates(parsed_yaml[applications][app], app) | 988 | self.process_subordinates(parsed_yaml[applications][app], app) |
104 | 975 | 989 | ||
106 | 976 | self.check_subs() | 990 | self.check_subs(parsed_yaml["machines"]) |
107 | 977 | self.check_charms() | 991 | self.check_charms() |
108 | 978 | 992 | ||
109 | 979 | if "relations" not in parsed_yaml: | 993 | if "relations" not in parsed_yaml: |
110 | diff --git a/jujulint/util.py b/jujulint/util.py | |||
111 | index c6eb80e..fc45ee6 100644 | |||
112 | --- a/jujulint/util.py | |||
113 | +++ b/jujulint/util.py | |||
114 | @@ -50,13 +50,35 @@ def is_container(machine): | |||
115 | 50 | return False | 50 | return False |
116 | 51 | 51 | ||
117 | 52 | 52 | ||
118 | 53 | def is_virtual_machine(machine, machine_data): | ||
119 | 54 | """ | ||
120 | 55 | Check if a provided machine is a VM. | ||
121 | 56 | |||
122 | 57 | It is not straightforward to determine if a machine is a VM from juju data | ||
123 | 58 | (bundle/juju status). In some cases a "hardware" key is provided (jsfy), | ||
124 | 59 | and in those cases we can check for the keyword "virtual" since some | ||
125 | 60 | provisioners include a tag there (FCE). We use that criteria as a best | ||
126 | 61 | effort attempt to determine if the machine is a VM. | ||
127 | 62 | """ | ||
128 | 63 | hardware = machine_data.get("hardware") | ||
129 | 64 | return bool(hardware and "virtual" in hardware) | ||
130 | 65 | |||
131 | 66 | |||
132 | 67 | def is_metal(machine, machine_data): | ||
133 | 68 | """ | ||
134 | 69 | Check if a provided machine is a bare metal host. | ||
135 | 70 | |||
136 | 71 | Leverages the other detection methods, if the others fail (e.g. not a | ||
137 | 72 | container or VM), we consider the machine to be bare metal. | ||
138 | 73 | """ | ||
139 | 74 | return not (is_container(machine) or is_virtual_machine(machine, machine_data)) | ||
140 | 75 | |||
141 | 76 | |||
142 | 53 | def extract_charm_name(charm): | 77 | def extract_charm_name(charm): |
143 | 54 | """Extract the charm name using regex.""" | 78 | """Extract the charm name using regex.""" |
144 | 55 | match = re.match( | 79 | match = re.match( |
145 | 56 | r"^(?:\w+:)?(?:~[\w\.-]+/)?(?:\w+/)?([a-zA-Z0-9-]+?)(?:-\d+)?$", charm | 80 | r"^(?:\w+:)?(?:~[\w\.-]+/)?(?:\w+/)?([a-zA-Z0-9-]+?)(?:-\d+)?$", charm |
146 | 57 | ) | 81 | ) |
147 | 58 | if not match: | 82 | if not match: |
151 | 59 | raise InvalidCharmNameError( | 83 | raise InvalidCharmNameError("charm name '{}' is invalid".format(charm)) |
149 | 60 | "charm name '{}' is invalid".format(charm) | ||
150 | 61 | ) | ||
152 | 62 | return match.group(1) | 84 | return match.group(1) |
153 | diff --git a/tests/conftest.py b/tests/conftest.py | |||
154 | index ba9ed77..fc7093e 100644 | |||
155 | --- a/tests/conftest.py | |||
156 | +++ b/tests/conftest.py | |||
157 | @@ -98,6 +98,12 @@ def juju_status(): | |||
158 | 98 | } | 98 | } |
159 | 99 | }, | 99 | }, |
160 | 100 | }, | 100 | }, |
161 | 101 | "ntp": { | ||
162 | 102 | "application-status": {"current": "active"}, | ||
163 | 103 | "charm": "cs:ntp-47", | ||
164 | 104 | "charm-name": "ntp", | ||
165 | 105 | "relations": {"juju-info": ["ubuntu"]}, | ||
166 | 106 | }, | ||
167 | 101 | }, | 107 | }, |
168 | 102 | "machines": { | 108 | "machines": { |
169 | 103 | "0": { | 109 | "0": { |
170 | diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py | |||
171 | index ce123ee..2494c28 100644 | |||
172 | --- a/tests/test_jujulint.py | |||
173 | +++ b/tests/test_jujulint.py | |||
174 | @@ -5,41 +5,59 @@ from datetime import datetime, timezone | |||
175 | 5 | import pytest | 5 | import pytest |
176 | 6 | 6 | ||
177 | 7 | 7 | ||
213 | 8 | def test_flatten_list(utils): | 8 | class TestUtils: |
214 | 9 | """Test the utils flatten_list function.""" | 9 | """Test the jujulint utilities.""" |
215 | 10 | unflattened_list = [1, [2, 3]] | 10 | |
216 | 11 | flattened_list = [1, 2, 3] | 11 | def test_flatten_list(self, utils): |
217 | 12 | assert flattened_list == utils.flatten_list(unflattened_list) | 12 | """Test the utils flatten_list function.""" |
218 | 13 | 13 | unflattened_list = [1, [2, 3]] | |
219 | 14 | unflattened_list = [1, [2, [3, 4]]] | 14 | flattened_list = [1, 2, 3] |
220 | 15 | flattened_list = [1, 2, 3, 4] | 15 | assert flattened_list == utils.flatten_list(unflattened_list) |
221 | 16 | assert flattened_list == utils.flatten_list(unflattened_list) | 16 | |
222 | 17 | 17 | unflattened_list = [1, [2, [3, 4]]] | |
223 | 18 | 18 | flattened_list = [1, 2, 3, 4] | |
224 | 19 | def test_flatten_list_non_list_iterable(utils): | 19 | assert flattened_list == utils.flatten_list(unflattened_list) |
225 | 20 | """Test the utils flatten_list function.""" | 20 | |
226 | 21 | iterable = {1: 2} | 21 | def test_flatten_list_non_list_iterable(self, utils): |
227 | 22 | assert iterable == utils.flatten_list(iterable) | 22 | """Test the utils flatten_list function.""" |
228 | 23 | 23 | iterable = {1: 2} | |
229 | 24 | 24 | assert iterable == utils.flatten_list(iterable) | |
230 | 25 | def test_map_charms(linter, utils): | 25 | |
231 | 26 | """Test the charm name validation code.""" | 26 | def test_is_container(self, utils): |
232 | 27 | applications = { | 27 | """Test the utils is_container function.""" |
233 | 28 | "test-app-1": {"charm": "cs:~USER/SERIES/TEST-CHARM12-123"}, | 28 | assert utils.is_container("1/lxd/0") is True |
234 | 29 | "test-app-2": {"charm": "cs:~USER/TEST-CHARM12-123"}, | 29 | assert utils.is_container("0") is False |
235 | 30 | "test-app-3": {"charm": "cs:TEST-CHARM12-123"}, | 30 | |
236 | 31 | "test-app-4": {"charm": "local:SERIES/TEST-CHARM12"}, | 31 | def test_is_virtual_machine(self, utils): |
237 | 32 | "test-app-5": {"charm": "local:TEST-CHARM12"}, | 32 | """Test the utils is_virtual_machine function.""" |
238 | 33 | "test-app-6": {"charm": "cs:~TEST-CHARMERS/TEST-CHARM12-123"}, | 33 | machine = "0" |
239 | 34 | } | 34 | machine_data = { |
240 | 35 | linter.map_charms(applications) | 35 | "hardware": "arch=amd64 cores=2 mem=4096M tags=virtual,pod-console-logging,vault availability-zone=AZ3" |
241 | 36 | for charm in linter.model.charms: | 36 | } |
242 | 37 | assert "TEST-CHARM12" == charm | 37 | assert utils.is_virtual_machine(machine, machine_data) is True |
243 | 38 | applications = { | 38 | |
244 | 39 | "test-app1": {"charm": "cs:invalid-charm$"}, | 39 | machine_data = {} |
245 | 40 | } | 40 | assert utils.is_virtual_machine(machine, machine_data) is False |
246 | 41 | with pytest.raises(utils.InvalidCharmNameError): | 41 | |
247 | 42 | linter.map_charms(applications) | 42 | def test_is_metal(self, utils): |
248 | 43 | """Test the utils is_metal function.""" | ||
249 | 44 | # A VM should return false | ||
250 | 45 | machine = "0" | ||
251 | 46 | machine_data = { | ||
252 | 47 | "hardware": "arch=amd64 cores=2 mem=4096M tags=virtual,pod-console-logging,vault availability-zone=AZ3" | ||
253 | 48 | } | ||
254 | 49 | assert utils.is_metal(machine, machine_data) is False | ||
255 | 50 | |||
256 | 51 | # A container should return false | ||
257 | 52 | assert utils.is_metal("1/lxd/0", {}) is False | ||
258 | 53 | |||
259 | 54 | # A bare metal should return true | ||
260 | 55 | machine = "1" | ||
261 | 56 | machine_data = { | ||
262 | 57 | "hardware": "arch=amd64 cores=128 mem=2093056M tags=foundation-nodes,hyper-converged-az2 " | ||
263 | 58 | "availability-zone=AZ2" | ||
264 | 59 | } | ||
265 | 60 | assert utils.is_metal(machine, machine_data) is True | ||
266 | 43 | 61 | ||
267 | 44 | 62 | ||
268 | 45 | class TestLinter: | 63 | class TestLinter: |
269 | @@ -77,6 +95,25 @@ class TestLinter: | |||
270 | 77 | assert errors[0]["id"] == "charm-not-mapped" | 95 | assert errors[0]["id"] == "charm-not-mapped" |
271 | 78 | assert errors[0]["application"] == "ubuntu2" | 96 | assert errors[0]["application"] == "ubuntu2" |
272 | 79 | 97 | ||
273 | 98 | def test_map_charms(self, linter, utils): | ||
274 | 99 | """Test the charm name validation code.""" | ||
275 | 100 | applications = { | ||
276 | 101 | "test-app-1": {"charm": "cs:~USER/SERIES/TEST-CHARM12-123"}, | ||
277 | 102 | "test-app-2": {"charm": "cs:~USER/TEST-CHARM12-123"}, | ||
278 | 103 | "test-app-3": {"charm": "cs:TEST-CHARM12-123"}, | ||
279 | 104 | "test-app-4": {"charm": "local:SERIES/TEST-CHARM12"}, | ||
280 | 105 | "test-app-5": {"charm": "local:TEST-CHARM12"}, | ||
281 | 106 | "test-app-6": {"charm": "cs:~TEST-CHARMERS/TEST-CHARM12-123"}, | ||
282 | 107 | } | ||
283 | 108 | linter.map_charms(applications) | ||
284 | 109 | for charm in linter.model.charms: | ||
285 | 110 | assert "TEST-CHARM12" == charm | ||
286 | 111 | applications = { | ||
287 | 112 | "test-app1": {"charm": "cs:invalid-charm$"}, | ||
288 | 113 | } | ||
289 | 114 | with pytest.raises(utils.InvalidCharmNameError): | ||
290 | 115 | linter.map_charms(applications) | ||
291 | 116 | |||
292 | 80 | def test_juju_status_unexpected(self, linter, juju_status): | 117 | def test_juju_status_unexpected(self, linter, juju_status): |
293 | 81 | """Test that juju and workload status is expected.""" | 118 | """Test that juju and workload status is expected.""" |
294 | 82 | # inject invalid status to the application | 119 | # inject invalid status to the application |
295 | @@ -243,6 +280,37 @@ class TestLinter: | |||
296 | 243 | assert errors[0]["machines"] == "0" | 280 | assert errors[0]["machines"] == "0" |
297 | 244 | assert errors[0]["subordinate"] == "ntp" | 281 | assert errors[0]["subordinate"] == "ntp" |
298 | 245 | 282 | ||
299 | 283 | def test_ops_subordinate_metal_only1(self, linter, juju_status): | ||
300 | 284 | """ | ||
301 | 285 | Test that missing ops subordinate charms are detected. | ||
302 | 286 | |||
303 | 287 | Use the "metal only" rule in a bare metal machine, should report the | ||
304 | 288 | missing subordinate | ||
305 | 289 | """ | ||
306 | 290 | linter.lint_rules["subordinates"]["hw-health"] = {"where": "metal only"} | ||
307 | 291 | linter.do_lint(juju_status) | ||
308 | 292 | |||
309 | 293 | errors = linter.output_collector["errors"] | ||
310 | 294 | assert len(errors) == 1 | ||
311 | 295 | assert errors[0]["id"] == "ops-subordinate-missing" | ||
312 | 296 | assert errors[0]["principals"] == "ubuntu" | ||
313 | 297 | assert errors[0]["subordinate"] == "hw-health" | ||
314 | 298 | |||
315 | 299 | def test_ops_subordinate_metal_only2(self, linter, juju_status): | ||
316 | 300 | """ | ||
317 | 301 | Test that missing ops subordinate charms are detected. | ||
318 | 302 | |||
319 | 303 | Use the "metal only" rule in a VM, should ignore it | ||
320 | 304 | """ | ||
321 | 305 | linter.lint_rules["subordinates"]["hw-health"] = {"where": "metal only"} | ||
322 | 306 | |||
323 | 307 | # Turn machine "0" into a "VM" | ||
324 | 308 | juju_status["machines"]["0"]["hardware"] = "tags=virtual availability-zone=rack-1" | ||
325 | 309 | linter.do_lint(juju_status) | ||
326 | 310 | |||
327 | 311 | errors = linter.output_collector["errors"] | ||
328 | 312 | assert not errors | ||
329 | 313 | |||
330 | 246 | def test_openstack_charm_missing(self, linter, juju_status): | 314 | def test_openstack_charm_missing(self, linter, juju_status): |
331 | 247 | """Test that missing openstack mandatory charms are detected.""" | 315 | """Test that missing openstack mandatory charms are detected.""" |
332 | 248 | linter.cloud_type = "openstack" | 316 | linter.cloud_type = "openstack" |
333 | @@ -286,6 +354,7 @@ class TestLinter: | |||
334 | 286 | linter.cloud_type = "kubernetes" | 354 | linter.cloud_type = "kubernetes" |
335 | 287 | linter.lint_rules["kubernetes mandatory"] = [] | 355 | linter.lint_rules["kubernetes mandatory"] = [] |
336 | 288 | linter.lint_rules["operations kubernetes mandatory"] = ["ntp"] | 356 | linter.lint_rules["operations kubernetes mandatory"] = ["ntp"] |
337 | 357 | juju_status["applications"].pop("ntp") # drop the app from the model | ||
338 | 289 | linter.do_lint(juju_status) | 358 | linter.do_lint(juju_status) |
339 | 290 | 359 | ||
340 | 291 | errors = linter.output_collector["errors"] | 360 | errors = linter.output_collector["errors"] |
Add the "metal only" condition for subordinates