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

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

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

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

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

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

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

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

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.

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

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

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.

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>
  ```