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

[Mert Kirpici]

LP #2009536

 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-forwarder-ha charm to be present
      on the same machine as part of the default shipped rules.

[🤖 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)

[🤖 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

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

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

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

PASSED: Continuous integration, rev:c68c21a336f36834e503b660c277f89bf441a427
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/5/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/491/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/182/

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

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

Looks good overall, I'd just want a test case to be a bit clearer.

I also have a few non-blocking suggestions.

review: Needs Fixing

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

PASSED: Continuous integration, rev:b7582951625b6b66b021e31fb1599d9aa9dd8738
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/6/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/493/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/183/

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

review: Approve (continuous-integration)

[Gabriel Cocenza (gabrielcocenza)]

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

review: Approve

[Mert Kirpici (mertkirpici)]

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

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

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

review: Approve (continuous-integration)

[Andrea Ieri (aieri)]

thank you for the refactor, I think the change is clearer now.

review: Approve

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

Change successfully merged at revision 1c8b9c00d72021c309bccbddf0801c695e129f8e

diff --git a/contrib/includes/base.yaml b/contrib/includes/base.yaml
index 8ea49b1..ddf089d 100644
--- a/contrib/includes/base.yaml
+++ b/contrib/includes/base.yaml
@@ -45,6 +45,8 @@ subordinates:
   where: all
  local-users:
   where: all or nothing
+ rsyslog-forwarder-ha:
+  allow-multiple: true
 
 # openstack and k8s should check nrpe relations. See LP#1965762
 relations base check: &relations-base-check
diff --git a/jujulint/lint.py b/jujulint/lint.py
index d263feb..5f764a9 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -208,8 +208,10 @@ class Linter:
             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"
+                    allow_multiple = (
+                        self.lint_rules["subordinates"]
+                        .get(charm, {})
+                        .get("allow-multiple", False)
                     )
                     if not allow_multiple:
                         self.model.duelling_subs.setdefault(sub, set())
@@ -615,7 +617,12 @@ class Linter:
             self.model.missing_subs.setdefault(required_sub, set())
             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"]
+            where = self.lint_rules["subordinates"][required_sub].get("where")
+            if where is None:
+                self._log_with_header(
+                    "Where clause not defined. Skipping placement checks."
+                )
+                continue
             for machine in self.model.subs_on_machines:
                 self._log_with_header("Checking on {}".format(machine))
                 present_subs = self.model.subs_on_machines[machine]
diff --git a/tests/unit/test_jujulint.py b/tests/unit/test_jujulint.py
index c0f7c7a..b98474b 100644
--- a/tests/unit/test_jujulint.py
+++ b/tests/unit/test_jujulint.py
@@ -368,6 +368,30 @@ applications:
         assert errors[0]["principals"] == "ubuntu"
         assert errors[0]["subordinate"] == "ntp"
 
+    def test_subordinate_missing_where_clause(self, linter, juju_status):
+        """Test that a missing where clause means we don't care about placement."""
+        linter.lint_rules["subordinates"]["ntp"].pop("where")
+
+        # Another ubuntu application that does not have the ntp subordinate
+        juju_status["applications"]["ubuntu2"] = {
+            "application-status": {"current": "active"},
+            "charm": "cs:ubuntu-18",
+            "charm-name": "ubuntu",
+            "relations": {},
+            "units": {
+                "ubuntu2/0": {
+                    "juju-status": {"current": "idle"},
+                    "machine": "1",
+                    "subordinates": {},
+                    "workload-status": {"current": "active"},
+                }
+            },
+        }
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert not errors
+
     def test_subordinate_extraneous(self, linter, juju_status):
         """Test that extraneous subordinate charms are detected."""
         # this check triggers on subordinates on containers that should only
@@ -455,6 +479,45 @@ applications:
         errors = linter.output_collector["errors"]
         assert not errors
 
+    def test_subordinate_optional_duplicates_produce_error(self, linter, juju_status):
+        """Test optional subordinates produce error when duplicate units exist on machine."""
+        template_status = {
+            "juju-status": {"current": "idle"},
+            "workload-status": {"current": "active"},
+        }
+
+        # Make ntp an optional sub.
+        # Meaning: still exists in "known charms" but not defined in "subordinates"
+        linter.lint_rules["subordinates"].pop("ntp")
+
+        # Add a second ubuntu app to the same machine with ntp subordinate
+        juju_status["applications"]["ubuntu2"] = {
+            "application-status": {"current": "active"},
+            "charm": "cs:ubuntu-18",
+            "charm-name": "ubuntu",
+            "relations": {"juju-info": ["ntp"]},
+            "units": {
+                "ubuntu2/0": {
+                    "juju-status": {"current": "idle"},
+                    "machine": "0",
+                    "subordinates": {"ntp/1": template_status},
+                    "workload-status": {"current": "active"},
+                }
+            },
+        }
+
+        # Reflect subordinate relations in the ntp side of the juju status
+        juju_status["applications"]["ntp"]["relations"]["juju-info"].append("ubuntu2")
+        juju_status["applications"]["ntp"]["subordinate-to"].append("ubuntu2")
+
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "subordinate-duplicate"
+        assert errors[0]["machines"] == "0"
+        assert errors[0]["subordinate"] == "ntp"
+
     def test_ops_subordinate_metal_only1(self, linter, juju_status):
         """
         Test that missing ops subordinate charms are detected.