Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harness and Juju behave differently when using model.get_relation in leader-elected #1153

Closed
tonyandrewmeyer opened this issue Mar 16, 2024 · 1 comment · Fixed by #1156
Closed
Assignees
Labels
bug Something isn't working small item

Comments

@tonyandrewmeyer
Copy link
Contributor

tonyandrewmeyer commented Mar 16, 2024

In the leader-elected hook, trying to get a relation fails with an error when using Harness, but works (returning None) with Juju. For example, with this charm:

requires:
    nonoptrel:
        interface: foo
        optional: false
    optrel:
        interface: bar
        optional: true
import logging

import ops

logger = logging.getLogger(__name__)


class LeaderRelationCharm(ops.CharmBase):
    """Charm the application."""

    def __init__(self, framework):
        super().__init__(framework)
        framework.observe(self.on.leader_elected, self._on_leader)

    def _on_leader(self, event):
        logger.info("Optional rel: %s", self.model.get_relation("optrel"))
        logger.info("Non-optional rel: %s", self.model.get_relation("nonoptrel"))


if __name__ == "__main__":  # pragma: nocover
    ops.main(LeaderRelationCharm)  # type: ignore

There's this debug-log output:

unit-leader-relation-0: 21:36:03 INFO unit.leader-relation/0.juju-log Optional rel: None
unit-leader-relation-0: 21:36:03 INFO unit.leader-relation/0.juju-log Non-optional rel: None

But with this test:

class TestCharm(unittest.TestCase):
    def setUp(self):
        self.harness = ops.testing.Harness(LeaderRelationCharm)
        self.addCleanup(self.harness.cleanup)
        self.harness.begin()

    def test_leader_elected(self):
        self.harness.set_leader(True)

you get:

Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
[..]
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/framework.py", line 955, in _reemit
    custom_handler(event)
  File "/home/tameyer/scratch/leader-relation/src/charm.py", line 22, in _on_leader
    logger.info("Optional rel: %s", self.model.get_relation("optrel"))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/model.py", line 242, in get_relation
    return self.relations._get_unique(relation_name, relation_id)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/model.py", line 868, in _get_unique
    self._backend._validate_relation_access(
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/testing.py", line 2121, in _validate_relation_access
    raise RuntimeError(
RuntimeError: cannot access relation data without first adding the relation: use Harness.add_relation('optrel', <app>) before calling set_leader

This was introduced in #733, but from the PR description, the intention was to protect against trying to access a peer relation, because the peer relation will always be set up (by Juju) prior to leader-elected, so you need to add the peer relation in Harness as well.

However, this isn't the case with other relations, and it doesn't seem right that Harness is behaving differently to Juju in this respect.

It seems to me that it's maybe ok for Harness to protect users in the peer-relation case: it's saying "this can't possibly happen with Juju - you need to adjust your test" (although maybe it could say it a bit clearer?). However, in the non-peer case, I think we should stick with the Juju behaviour. I'm not totally sure about the peer-relation case - it seems consistency and a warning might be better? It would also be good to verify that Juju does indeed always set up the peer relation before emitting leader-elected.

@tonyandrewmeyer tonyandrewmeyer added bug Something isn't working small item labels Mar 16, 2024
@benhoyt
Copy link
Collaborator

benhoyt commented Mar 17, 2024

Thanks for the report.

consistency and a warning might be better

Without reference to the specific details here, I agree with that.

@benhoyt benhoyt added the 24.04 label Mar 18, 2024
benhoyt pushed a commit that referenced this issue Mar 22, 2024
…ted (#1156)

In Juju, using `get_relation()` in a leader-elected event will return
`None` if the relation does not yet exist. This makes Harness consistent
with that behaviour.

This does remove a safety-check for badly written tests - where you mean
for the relation to exist but forget to do an `add_relation()` call
first in the test. However, we don't have any sort of check like this
for any other events, or any other types of relations, and it should be
obvious when the test fails that it's failing to find an expected
relation, and the solution is to set that relation up. I don't think
it's worth having a warning for this specific case (we don't warn in
other similar situations).

This also means we can remove a check in model.py that's a no-op in
production and does work in Harness, which is a bad smell, in my
opinion.

Done:
* [x] [Check with Juju team whether peer-relations are actually
guaranteed to be set up before
leader-elected](https://matrix.to/#/!xzmWHtGpPfVCXKivIh:ubuntu.com/$JUrZzqWEpE5nSQPelnpsG3Mbrd5sGOEiGY989MD-3hs?via=ubuntu.com&via=matrix.org&via=fsfe.org)
* [x] Quick super-tox check to make sure that no-one is somehow relying
on this

Fixes #1153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working small item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants