Merge ~mertkirpici/juju-lint:lp/2009536-2 into juju-lint:master
- Git
- lp:~mertkirpici/juju-lint
- lp/2009536-2
- Merge into master
Status: | Merged |
---|---|
Approved by: | Erhan Sunar |
Approved revision: | dde3f5b57d00fa879b012e6a65e9c3ba414bca3c |
Merged at revision: | 1c8b9c00d72021c309bccbddf0801c695e129f8e |
Proposed branch: | ~mertkirpici/juju-lint:lp/2009536-2 |
Merge into: | juju-lint:master |
Diff against target: |
125 lines (+75/-3) 3 files modified
contrib/includes/base.yaml (+2/-0) jujulint/lint.py (+10/-3) tests/unit/test_jujulint.py (+63/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andrea Ieri | Approve | ||
🤖 prod-jenkaas-bootstack (community) | continuous-integration | Approve | |
Gabriel Cocenza | Approve | ||
Eric Chen | Pending | ||
Tianqi Xiao | Pending | ||
BootStack Reviewers | Pending | ||
Review via email: mp+440425@code.launchpad.net |
This proposal supersedes a proposal from 2023-03-28.
Commit message
LP #2009536
Description of the change
lint: issue error for optional duplicate subs
This commit adds the following behavior to juju-lint:
- Issue error if a subordinate not declared in the top level subordinates
object but has multiple units on the same machine
- Ignore any placement checks for subordinate charms that are declared
in the top level subordinates object however missing the "where"
clause
- Allow multiple units of the rsyslog-
on the same machine as part of the default shipped rules.
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : Posted in a previous version of this proposal | # |
Robert Gildein (rgildein) wrote : Posted in a previous version of this proposal | # |
Overall LGTM, but we are missing unit tests + those tests will be
needed otherwise coverage will failed, since we have 100% [1] requirement.
[1]: https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:2a4082f2eba
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:7c63022749d
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Eric Chen (eric-chen) : Posted in a previous version of this proposal | # |
Mert Kirpici (mertkirpici) : Posted in a previous version of this proposal | # |
Gabriel Cocenza (gabrielcocenza) wrote (last edit ): Posted in a previous version of this proposal | # |
I think I didn't understand completely what we are trying to achieve here.
IMO the bug is a corner case where an `optional` charm in the deployment is subordinate and can be deployed more than once in a machine. E.g: when relating to a principal and another subordinate charm.
My questions are:
- Can rsyslog-
- If it can, I'm guessing that we will jump from KeyError to juju-lint error?
- Don't we need to change the rules file to accept this corner case with rsyslog-
Having at least an unit test covering this corner case is important.
Thanks
Eric Chen (eric-chen) : Posted in a previous version of this proposal | # |
Tianqi Xiao (txiao) wrote : Posted in a previous version of this proposal | # |
LGTM. I changed the target repo in bug #2009536 as it was wrongly pointed to the charm instead of the snap.
Robert Gildein (rgildein) : Posted in a previous version of this proposal | # |
Eric Chen (eric-chen) wrote : Posted in a previous version of this proposal | # |
Based on Gabriel's comment, I change the status to "needs fixing" until we clarify the questions.
Mert Kirpici (mertkirpici) wrote : Posted in a previous version of this proposal | # |
I believe checking out the mp and discussions related to lp #1855858(linked to this mp) will clarify most of the questions. However let me try to answer them to the best of my understanding.
> Can rsyslog-
Yes. See discussion around hyperconverged architecture in lp #1855858 also the status file provided by the reporter for lp #2009536(also linked).
> If it can, I'm guessing that we will jump from KeyError to juju-lint error?
I don't fully understand this question. The reported bug has an attached juju status file where this already is the case. juju-lint is crashing with an unhandled KeyError when run against that status file with a ruleset that is shipped by the snap.
> Don't we need to change the rules file to accept this corner case with rsyslog-
That depends. We could resolve this issue by only making an addition like the following in the contrib/
subordinates:
rsyslog-
allow-
where: ???
However this change would make rsyslog-
The reason I did not go in that direction is because I realized that rsyslog-
Instead, this proposed solution completely discards the checks about duplicate existence of subordinates that are not declared as mandatory(in top level "subordinates" object).
This change already has a unit test written during handling of lp #1855858, which is called test_subordinat
I do agree that although the coverage did not go down, this particular scenario is still not tested. Therefore I am planning to include this scenario as well for completeness' sake.
Mert Kirpici (mertkirpici) : Posted in a previous version of this proposal | # |
Gabriel Cocenza (gabrielcocenza) wrote : Posted in a previous version of this proposal | # |
>> Can rsyslog-
> Yes. See discussion around hyperconverged architecture in lp #1855858 also the status file provided by the reporter for lp #2009536(also linked).
I've checked lp #1855858 and I didn't see rsyslog-
That is why I think we should ask confirmation from Bootstack. Probably it's another special case because it makes sense to have it related with nova-compute and ceph-osd, but it would be good to confirm it just in case.
> Instead, this proposed solution completely discards the checks about duplicate existence of subordinates that are not declared as mandatory(in top level "subordinates" object).
What if we have a optional subordinate charm that shouldn't be duplicated? What is the behavior? We just don't check it? It doesn't looks good to me. Maybe we should differentiate optional principal charms from optional subordinate charms?
Mert Kirpici (mertkirpici) wrote : Posted in a previous version of this proposal | # |
> Based on Gabriel's comment, I change the status to "needs fixing" until we
> clarify the questions.
I am failing to see the reasoning here. What "fix" is "needed" according to _you_ as a reviewer? I do not see any opinions/
Gabriel Cocenza (gabrielcocenza) wrote : Posted in a previous version of this proposal | # |
Summarizing what I would like to see in this patch/bug-fix:
- Optional subordinate charms that can be deployed more than once in a machine and expected behavior(no error message) on unit test.
- Optional subordinate charms that can't be deployed more than once in a machine and expected behavior(error message) on unit test.
- Simple confirmation with Bootstack that rsyslog-
Andrea Ieri (aieri) wrote : Posted in a previous version of this proposal | # |
I suspect juju-lint has tried to track too closely the specific needs of BootStack and this caused the software to evolve convoluted and confusing behaviors.
The concept of mandatory/optional subordinates seems unwieldy to me. Overall, I would propose tending towards something like:
* multiple instances of a unit (principal or subordinate) on the same *machine* generate an error by default (i.e. there's a top-level allow-multiple key that default to False)
* if a charm isn't listed in the ruleset its presence or absence should not generate an error, meaning that "unknown" charms no longer generate errors
* the ruleset acts as a list of constraints, which only apply to the indicated charms. In other words, "mandatory" equals "there is a rule about this charm defining a where clause"
The consequences would then be that in regard to rsyslog-
* if multiple instances on the same machine should generate an error, no special rule needs to be written
* allowing multiple instances can be expressed as
```
subordinates:
rsyslog-
allow-
```
* requiring rsyslog-
```
subordinates:
rsyslog-
where: <a suitable rule>
```
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:c68c21a336f
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Andrea Ieri (aieri) wrote : | # |
Looks good overall, I'd just want a test case to be a bit clearer.
I also have a few non-blocking suggestions.
Mert Kirpici (mertkirpici) : | # |
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:b7582951625
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Gabriel Cocenza (gabrielcocenza) wrote : | # |
LGTM. Thanks for refactoring it. I think this is a much cleanner solution.
I left just one small comment, but it's not a blocker.
Gabriel Cocenza (gabrielcocenza) : | # |
Mert Kirpici (mertkirpici) wrote : | # |
> LGTM. Thanks for refactoring it. I think this is a much cleanner solution.
>
> I left just one small comment, but it's not a blocker.
Thanks for the review, I addressed it in the update.
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:dde3f5b57d0
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Andrea Ieri (aieri) wrote : | # |
thank you for the refactor, I think the change is clearer now.
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Change successfully merged at revision 1c8b9c00d72021c
Preview Diff
1 | diff --git a/contrib/includes/base.yaml b/contrib/includes/base.yaml | |||
2 | index 8ea49b1..ddf089d 100644 | |||
3 | --- a/contrib/includes/base.yaml | |||
4 | +++ b/contrib/includes/base.yaml | |||
5 | @@ -45,6 +45,8 @@ subordinates: | |||
6 | 45 | where: all | 45 | where: all |
7 | 46 | local-users: | 46 | local-users: |
8 | 47 | where: all or nothing | 47 | where: all or nothing |
9 | 48 | rsyslog-forwarder-ha: | ||
10 | 49 | allow-multiple: true | ||
11 | 48 | 50 | ||
12 | 49 | # openstack and k8s should check nrpe relations. See LP#1965762 | 51 | # openstack and k8s should check nrpe relations. See LP#1965762 |
13 | 50 | relations base check: &relations-base-check | 52 | relations base check: &relations-base-check |
14 | diff --git a/jujulint/lint.py b/jujulint/lint.py | |||
15 | index d263feb..5f764a9 100755 | |||
16 | --- a/jujulint/lint.py | |||
17 | +++ b/jujulint/lint.py | |||
18 | @@ -208,8 +208,10 @@ class Linter: | |||
19 | 208 | for sub in subordinates: | 208 | for sub in subordinates: |
20 | 209 | if sub in self.model.subs_on_machines[machine]: | 209 | if sub in self.model.subs_on_machines[machine]: |
21 | 210 | charm = self.model.app_to_charm[sub] | 210 | charm = self.model.app_to_charm[sub] |
24 | 211 | allow_multiple = self.lint_rules["subordinates"][charm].get( | 211 | allow_multiple = ( |
25 | 212 | "allow-multiple" | 212 | self.lint_rules["subordinates"] |
26 | 213 | .get(charm, {}) | ||
27 | 214 | .get("allow-multiple", False) | ||
28 | 213 | ) | 215 | ) |
29 | 214 | if not allow_multiple: | 216 | if not allow_multiple: |
30 | 215 | self.model.duelling_subs.setdefault(sub, set()) | 217 | self.model.duelling_subs.setdefault(sub, set()) |
31 | @@ -615,7 +617,12 @@ class Linter: | |||
32 | 615 | self.model.missing_subs.setdefault(required_sub, set()) | 617 | self.model.missing_subs.setdefault(required_sub, set()) |
33 | 616 | self.model.extraneous_subs.setdefault(required_sub, set()) | 618 | self.model.extraneous_subs.setdefault(required_sub, set()) |
34 | 617 | self._log_with_header("Checking for sub {}".format(required_sub)) | 619 | self._log_with_header("Checking for sub {}".format(required_sub)) |
36 | 618 | where = self.lint_rules["subordinates"][required_sub]["where"] | 620 | where = self.lint_rules["subordinates"][required_sub].get("where") |
37 | 621 | if where is None: | ||
38 | 622 | self._log_with_header( | ||
39 | 623 | "Where clause not defined. Skipping placement checks." | ||
40 | 624 | ) | ||
41 | 625 | continue | ||
42 | 619 | for machine in self.model.subs_on_machines: | 626 | for machine in self.model.subs_on_machines: |
43 | 620 | self._log_with_header("Checking on {}".format(machine)) | 627 | self._log_with_header("Checking on {}".format(machine)) |
44 | 621 | present_subs = self.model.subs_on_machines[machine] | 628 | present_subs = self.model.subs_on_machines[machine] |
45 | diff --git a/tests/unit/test_jujulint.py b/tests/unit/test_jujulint.py | |||
46 | index c0f7c7a..b98474b 100644 | |||
47 | --- a/tests/unit/test_jujulint.py | |||
48 | +++ b/tests/unit/test_jujulint.py | |||
49 | @@ -368,6 +368,30 @@ applications: | |||
50 | 368 | assert errors[0]["principals"] == "ubuntu" | 368 | assert errors[0]["principals"] == "ubuntu" |
51 | 369 | assert errors[0]["subordinate"] == "ntp" | 369 | assert errors[0]["subordinate"] == "ntp" |
52 | 370 | 370 | ||
53 | 371 | def test_subordinate_missing_where_clause(self, linter, juju_status): | ||
54 | 372 | """Test that a missing where clause means we don't care about placement.""" | ||
55 | 373 | linter.lint_rules["subordinates"]["ntp"].pop("where") | ||
56 | 374 | |||
57 | 375 | # Another ubuntu application that does not have the ntp subordinate | ||
58 | 376 | juju_status["applications"]["ubuntu2"] = { | ||
59 | 377 | "application-status": {"current": "active"}, | ||
60 | 378 | "charm": "cs:ubuntu-18", | ||
61 | 379 | "charm-name": "ubuntu", | ||
62 | 380 | "relations": {}, | ||
63 | 381 | "units": { | ||
64 | 382 | "ubuntu2/0": { | ||
65 | 383 | "juju-status": {"current": "idle"}, | ||
66 | 384 | "machine": "1", | ||
67 | 385 | "subordinates": {}, | ||
68 | 386 | "workload-status": {"current": "active"}, | ||
69 | 387 | } | ||
70 | 388 | }, | ||
71 | 389 | } | ||
72 | 390 | linter.do_lint(juju_status) | ||
73 | 391 | |||
74 | 392 | errors = linter.output_collector["errors"] | ||
75 | 393 | assert not errors | ||
76 | 394 | |||
77 | 371 | def test_subordinate_extraneous(self, linter, juju_status): | 395 | def test_subordinate_extraneous(self, linter, juju_status): |
78 | 372 | """Test that extraneous subordinate charms are detected.""" | 396 | """Test that extraneous subordinate charms are detected.""" |
79 | 373 | # this check triggers on subordinates on containers that should only | 397 | # this check triggers on subordinates on containers that should only |
80 | @@ -455,6 +479,45 @@ applications: | |||
81 | 455 | errors = linter.output_collector["errors"] | 479 | errors = linter.output_collector["errors"] |
82 | 456 | assert not errors | 480 | assert not errors |
83 | 457 | 481 | ||
84 | 482 | def test_subordinate_optional_duplicates_produce_error(self, linter, juju_status): | ||
85 | 483 | """Test optional subordinates produce error when duplicate units exist on machine.""" | ||
86 | 484 | template_status = { | ||
87 | 485 | "juju-status": {"current": "idle"}, | ||
88 | 486 | "workload-status": {"current": "active"}, | ||
89 | 487 | } | ||
90 | 488 | |||
91 | 489 | # Make ntp an optional sub. | ||
92 | 490 | # Meaning: still exists in "known charms" but not defined in "subordinates" | ||
93 | 491 | linter.lint_rules["subordinates"].pop("ntp") | ||
94 | 492 | |||
95 | 493 | # Add a second ubuntu app to the same machine with ntp subordinate | ||
96 | 494 | juju_status["applications"]["ubuntu2"] = { | ||
97 | 495 | "application-status": {"current": "active"}, | ||
98 | 496 | "charm": "cs:ubuntu-18", | ||
99 | 497 | "charm-name": "ubuntu", | ||
100 | 498 | "relations": {"juju-info": ["ntp"]}, | ||
101 | 499 | "units": { | ||
102 | 500 | "ubuntu2/0": { | ||
103 | 501 | "juju-status": {"current": "idle"}, | ||
104 | 502 | "machine": "0", | ||
105 | 503 | "subordinates": {"ntp/1": template_status}, | ||
106 | 504 | "workload-status": {"current": "active"}, | ||
107 | 505 | } | ||
108 | 506 | }, | ||
109 | 507 | } | ||
110 | 508 | |||
111 | 509 | # Reflect subordinate relations in the ntp side of the juju status | ||
112 | 510 | juju_status["applications"]["ntp"]["relations"]["juju-info"].append("ubuntu2") | ||
113 | 511 | juju_status["applications"]["ntp"]["subordinate-to"].append("ubuntu2") | ||
114 | 512 | |||
115 | 513 | linter.do_lint(juju_status) | ||
116 | 514 | |||
117 | 515 | errors = linter.output_collector["errors"] | ||
118 | 516 | assert len(errors) == 1 | ||
119 | 517 | assert errors[0]["id"] == "subordinate-duplicate" | ||
120 | 518 | assert errors[0]["machines"] == "0" | ||
121 | 519 | assert errors[0]["subordinate"] == "ntp" | ||
122 | 520 | |||
123 | 458 | def test_ops_subordinate_metal_only1(self, linter, juju_status): | 521 | def test_ops_subordinate_metal_only1(self, linter, juju_status): |
124 | 459 | """ | 522 | """ |
125 | 460 | Test that missing ops subordinate charms are detected. | 523 | Test that missing ops subordinate charms are detected. |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.