fix: don't include private-address in default testing.Relation databags on Juju 4

[tonyandrewmeyer]

At exec time, remove the private-address key from the databags, if it's set to our default value and the mock Juju version is 4+.

I don't love this solution, since we're editing the State after the user has finished with it, but it's only when we have the Context that the version is available, so there doesn't seem to be a backwards-compatible alternative. It would probably be better if the runtime injected the automated keys and they weren't there when the user had the State, or maybe if you had to provide a version when getting a fresh Relation, but we can't do either of those in 8.x. This change at least gives the Juju behaviour, even if it's a bit quirky.

Regarding the comment in the issue, what I am proposing is that we do this workaround so that the Scenario 8.x behaviour matches Juju 3 and 4, and we do something more elaborate and backwards incompatible in 9.0 (I added to the list in #2350). We could instead reject this change and opt in to new behaviour with a env flag or similar, but I feel even if we do that we would want this change so that the default behaviour is the Juju one.

Fixes #2185

[dwilding]

Main comment is about (lack of) JujuVersion

[dwilding]

My review agent tells me that this would remove private-address from local_app_data and remote_app_data - if someone has set private-address to the default value in those databags. I feel like we're safe to assume nobody would ever do that... What do you think?

[tonyandrewmeyer]

I did wonder about this. There's some chance a charm could do this to make their Juju 4 databag look like a Juju 3 one. I'd argue that it's not a good idea, but it's not impossible.

If someone does that their tests would break and they wouldn't be able to get around that.

The only ideas I have for avoiding that are ugly (like a special object we set instead, which we can recognise later, but pretends to be the normal one when people work with their state).

I think this comes down to a decision about whether we want to have a fix in Scenario 8 so the Juju 4 behaviour is surfaced, only have a fix with something opt-in (where we could do this more cleanly), or only fix it in 9.0 (definitely could do it clean, but people's tests may need updates). Or someone comes up with an idea I haven't thought of, of course.

I lean towards what's in the PR, but I can definitely be convinced we should go another way.

[dwilding]

I'm OK with what's in this PR. My knowledge is a bit limited here, so no objection if you and James decide to go in a different direction.

[james-garner-canonical]

• How breaky is this PR?
• I'd be curious to see just how breaky changing the behaviour in the desired way instead would be: changing the default factories to provide empty dicts and populating the correct Juju-inserted keys at exec time (+/- having the Juju-inserted keys flow through to the output state).

I think it would be great to quantify the difference in breaky-ness so we can make an informed choice about whether this is the right fix for now.

PS: An idea for making this PR's approach safer -- track if the default factory was called with a private marker on the relation object, and only replace in that case. But maybe that falls apart as soon as we hit dataclasses.replace ...

[tonyandrewmeyer]

• How breaky is this PR?

In terms of running Hyrum? Or in my opinion given what code changes?

• I'd be curious to see just how breaky changing the behaviour in the desired way instead would be: changing the default factories to provide empty dicts and populating the correct Juju-inserted keys at exec time (+/- having the Juju-inserted keys flow through to the output state).

This feels objectively more break-y to me, because it impacts Juju 3 as well as Juju 4, and also the keys that are still injected in both. But I could implement it and run it over the fleet tests to get actual numbers.

PS: An idea for making this PR's approach safer -- track if the default factory was called with a private marker on the relation object, and only replace in that case. But maybe that falls apart as soon as we hit dataclasses.replace ...

Yeah, that sort of thing (as well as using a dict-subclass or something) is the kind of thing I was thinking of when I was saying there are "ugly" solutions. My preference would be to avoid those, and go with a clean solution (that I can't think of), the one in this PR, or an opt-in fix that becomes the default in 9.0.

[james-garner-canonical]

• How breaky is this PR?

In terms of running Hyrum? Or in my opinion given what code changes?

• I'd be curious to see just how breaky changing the behaviour in the desired way instead would be: changing the default factories to provide empty dicts and populating the correct Juju-inserted keys at exec time (+/- having the Juju-inserted keys flow through to the output state).

This feels objectively more break-y to me, because it impacts Juju 3 as well as Juju 4, and also the keys that are still injected in both. But I could implement it and run it over the fleet tests to get actual numbers.

You're probably right, but with how cheap implementation is I'd love to see the Hyrum numbers for the different approaches.

[james-garner-canonical]

Comment review to allow explicit re-review request after we have the Hyrum numbers.

[tonyandrewmeyer]

Alternative implementation in #2637 - the "inject at runtime" solution.

Run	ops source	Passed	Pass rate
baseline	canonical/operator@main	175/267	66%
databag	tonyandrewmeyer/operator@rainy/2185-juju-default-databag-at-exec	172/267	64%
no-private	tonyandrewmeyer/operator@rainy/2185-no-private-address-juju-4	178/267	67%

hyrum compare against the main baseline reported 6 new failures for databag and 0 for no-private.

Verdict

rainy/2185-no-private-address-juju-4 — clean

Zero new failures across 267 charms. The small change is because this branch is behind main.

rainy/2185-juju-default-databag-at-exec — 5 real regressions

Each passes at the branch's own merge-base and fails on the branch.

Charm	Failing test	Error
alertmanager-k8s-operator	test_log_forwarding.py::test_charm_active_on_logging_departed	InconsistentScenarioError
cos-proxy-operator	test_alerts.py::test_relation[1,5,10]	InconsistentScenarioError ×3
prometheus-k8s-operator	test_receive_ca_cert.py::test_ca_forwarded_over_rel_data	InconsistentScenarioError
trino-k8s-operator	test_charm.py::test_trino_worker_relation_broken	InconsistentScenarioError
microcluster-token-distributor-operator	test_provides_lib.py::test_handle_mirror_mirror_hostnames	AssertionError

Two distinct consumer assumptions are being broken.

1. Relation identity (4 charms). Scenario's consistency checker rejects the event:

cannot emit monitors_relation_changed because relation 1 is not in the state
(a relation with the same ID is not sufficient - you must pass the object in
the state to the event)

These tests build a Relation for the event that is equal by ID to the one in the state but is not the same object.

2. Default databag keys (1 charm). microcluster-token-distributor asserts the exact key list of its own unit databag:

assert list(cluster_relation.data[manager.charm.unit].keys()) == [...]
# Left contains 3 more items, first extra item: 'egress-subnets'

The branch adds Juju's defaults (egress-subnets, ingress-address, private-address), so the list grows. This is the branch working as intended; the charm relied on the databag holding only what it wrote.

[tonyandrewmeyer]

So this method is definitely the more conservative and safer approach. The question is whether there is enough breakage that we wouldn't want to do the more correct change, probably after submitting PRs downstream.

[james-garner-canonical]

Thanks for running the numbers! There are few enough failures that I'm definitely interested in if we can get away with doing the 'more correct' change after pre-emptively fixing up the charm tests.

Run ops source Passed Pass rate
baseline canonical/operator@main 175/267 66%
databag tonyandrewmeyer/operator@rainy/2185-juju-default-databag-at-exec 172/267 64%
no-private tonyandrewmeyer/operator@rainy/2185-no-private-address-juju-4 178/267 67%
hyrum compare against the main baseline reported 6 new failures for databag and 0 for no-private.

The apparently 3 new passes in this branch jumped out at me. Do you know why they are?

1. Relation identity (4 charms). Scenario's consistency checker rejects the event:

cannot emit monitors_relation_changed because relation 1 is not in the state
(a relation with the same ID is not sufficient - you must pass the object in
the state to the event)

These tests build a Relation for the event that is equal by ID to the one in the state but is not the same object.

I had a look through #2637, and I'm struggling to understand how this category of test failure is a consequence of it. Could you explain, please?

2. Default databag keys (1 charm). microcluster-token-distributor asserts the exact key list of its own unit databag:

assert list(cluster_relation.data[manager.charm.unit].keys()) == [...]
# Left contains 3 more items, first extra item: 'egress-subnets'

The branch adds Juju's defaults (egress-subnets, ingress-address, private-address), so the list grows. This is the branch working as intended; the charm relied on the databag holding only what it wrote.

This does seem like the kind of breakage we'd expect, and seems easy enough to provide a fix for.

[james-garner-canonical]

Comment review so you can re-request review after addressing the comment above.