Merge ~mertkirpici/juju-lint:lp/2009536 into juju-lint:master

[Mert Kirpici]

LP #2009536

lint: check multiple subs for required ones only

    Starting with this commit, juju-lint only generates an error in case of
    a subordinate charm having multiple units on a given machine if:
    - the subordinate charm is declared under the top level "subordinates"
      YAML object
    - it does not set allow-multiple or sets it to false. this essentially
      defines the default value for the "allow-multiple" config as false.

    The previous implementation did not care about the subordinate charm
    being listed under the "subordinates" at all. This behavior caused a
    crash when an optional subordinate is deployed multiple times on a
    machine.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Robert Gildein (rgildein)]

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://git.launchpad.net/juju-lint/tree/pyproject.toml#n43

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:2a4082f2ebaae09dfe7217a86f812dbbc56fa643
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/1/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/378/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/137/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/1//rebuild

review: Needs Fixing (continuous-integration)

7c63022... by Mert Kirpici on 2023-03-28

tests/functional: pin juju<3

Works-on: 2007834
Signed-off-by: Mert Kırpıcı <email address hidden>

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:7c63022749deca3e7c21576e57a9e37793199d4e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/2/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/380/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/139/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/2//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

review: Needs Fixing

[Gabriel Cocenza (gabrielcocenza)]

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-forwarder-ha be deployed more than once in a machine?
- 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-forwarder-ha?

Having at least an unit test covering this corner case is important.

Thanks

review: Needs Information

[Eric Chen (eric-chen)]

review: Approve

[Tianqi Xiao (txiao)]

LGTM. I changed the target repo in bug #2009536 as it was wrongly pointed to the charm instead of the snap.

review: Approve

[Eric Chen (eric-chen)]

Based on Gabriel's comment, I change the status to "needs fixing" until we clarify the questions.

review: Needs Fixing

[Mert Kirpici (mertkirpici)]

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-forwarder-ha be deployed more than once in a machine?

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-forwarder-ha?

 That depends. We could resolve this issue by only making an addition like the following in the contrib/includes/base.yaml file:

 subordinates:
   rsyslog-forwarder-ha:
     allow-multiple: true # this fixes the unhandled exception
     where: ???

However this change would make rsyslog-forwarder-ha subordinate charm mandatory and we would need to specify where we expect to find this subordinate charm in the deployments using the "where" key.

The reason I did not go in that direction is because I realized that rsyslog-forwarder-ha is already listed as an optional operations subordinate in contrib/includes/operations.yaml file. Which, during runtime after every included yaml file is merged, will only end up in the top level "known charms" object but not in "subordinates" object(which is the cause of the unhandled KeyError in the first place).

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_subordinate_duplicates_allow. Since the check was designed to run against a mandatory charm(nrpe), this issue was not caught. Coincidentally this also is the reason why coverage did not go down.

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.

[Gabriel Cocenza (gabrielcocenza)]

>> Can rsyslog-forwarder-ha be deployed more than once in a machine?

> 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-forwarder-ha in the discussion. Just nrpe that is a special case. lp #2009536 there is also no comments if rsyslog-forwarder-ha could be deployed more than once in a machine.

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

> 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/suggestions.

[Gabriel Cocenza (gabrielcocenza)]

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-forwarder-ha is a special case like nrpe.

review: Needs Fixing

[Andrea Ieri (aieri)]

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-forwarder-ha:
* 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-forwarder-ha:
      allow-multiple: true
  ```
* requiring rsyslog-forwarder-ha to be present (or absent) can be expressed as
  ```
  subordinates:
    rsyslog-forwarder-ha:
      where: <a suitable rule>
  ```

review: Needs Fixing

Unmerged commits

7c63022... by Mert Kirpici on 2023-03-28

tests/functional: pin juju<3

Works-on: 2007834
Signed-off-by: Mert Kırpıcı <email address hidden>

2a4082f... by Mert Kirpici on 2023-03-28

lint: check multiple subs for required ones only

Starting with this commit, juju-lint only generates an error in case of
a subordinate charm having multiple units on a given machine if:
- the subordinate charm is declared under the top level "subordinates"
  YAML object
- it does not set allow-multiple or sets it to false. this essentially
  defines the default value for the "allow-multiple" config as false.

The previous implementation did not care about the subordinate charm
being listed under the "subordinates" at all. This behavior caused a
crash when an optional subordinate is deployed multiple times on a
machine.

Works-on: 2009536
Signed-off-by: Mert Kırpıcı <email address hidden>

diff --git a/jujulint/lint.py b/jujulint/lint.py
index d263feb..4886ad7 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -207,13 +207,8 @@ class Linter:
             self.model.subs_on_machines.setdefault(machine, set())
             for sub in subordinates:
                 if sub in self.model.subs_on_machines[machine]:
-                    charm = self.model.app_to_charm[sub]
-                    allow_multiple = self.lint_rules["subordinates"][charm].get(
-                        "allow-multiple"
-                    )
-                    if not allow_multiple:
-                        self.model.duelling_subs.setdefault(sub, set())
-                        self.model.duelling_subs[sub].add(machine)
+                    self.model.duelling_subs.setdefault(sub, set())
+                    self.model.duelling_subs[sub].add(machine)
                 self.model.subs_on_machines[machine].add(sub)
             self.model.subs_on_machines[machine] = (
                 set(subordinates) | self.model.subs_on_machines[machine]
@@ -607,6 +602,7 @@ class Linter:
     def check_subs(self, machines_data):  # pragma: no cover
         """Check the subordinates in the model."""
         all_or_nothing = set()
+        multiple_not_allowed = set()
         for machine in self.model.subs_on_machines:
             for sub in self.model.subs_on_machines[machine]:
                 all_or_nothing.add(sub)
@@ -616,6 +612,10 @@ class Linter:
             self.model.extraneous_subs.setdefault(required_sub, set())
             self._log_with_header("Checking for sub {}".format(required_sub))
             where = self.lint_rules["subordinates"][required_sub]["where"]
+
+            if not self.lint_rules["subordinates"][required_sub].get("allow-multiple"):
+                multiple_not_allowed.add(required_sub)
+
             for machine in self.model.subs_on_machines:
                 self._log_with_header("Checking on {}".format(machine))
                 present_subs = self.model.subs_on_machines[machine]
@@ -739,6 +739,9 @@ class Linter:
         for sub in list(self.model.extraneous_subs.keys()):
             if not self.model.extraneous_subs[sub]:
                 del self.model.extraneous_subs[sub]
+        for sub in list(self.model.duelling_subs.keys()):
+            if sub not in multiple_not_allowed:
+                del self.model.duelling_subs[sub]
 
     def check_relations(self, input_file):
         """Check the relations in the rules file.
diff --git a/tests/functional/requirements.txt b/tests/functional/requirements.txt
index 6a1cad4..e8c24d8 100644
--- a/tests/functional/requirements.txt
+++ b/tests/functional/requirements.txt
@@ -1,3 +1,4 @@
+juju<3
 pytest
 pytest-operator
 pytest-httpserver