reinitialize charm on event in harness

[PietroPasotti]

Extended the Harness with an opt-in feature to reinitialize the charm on every event.

Documentation changes

This will likely have a moderate impact on user tests. It might reveal some bugs (which is good!), but it might also give some false negatives where people wrote tests (as we did too) using mock charms that attached state to the instance.

We need to communicate very clearly the scope of this change, and give time to adapt.
We should also probably add some api to hook into the harness' emit_event for recording purposes.

Bug reference

Bug: #736
Prior art: #758

Changelog

• added an opt-in feature (to become default in the future) to reinitialize the charm every time the harness fires an event on it.

[PietroPasotti]

@sed-i @pengale @rwcarlsen WDYT?

[sed-i]

Looks good. Next, I would add tests for:

• leadership status retained
• peer relations (data) retained
• regular relations (data) retained
• unit status retained

[jameinel]

FWIW, I don't think this is actually enough. AIUI the issue is that the Python code itself is actually adding bound methods to classes that point to this framework. So if we really wanted this, we would need to go around and remove all of those methods. (I'm not 100% sure what else, but there are a lot of Global modifications that are being done during initialization and registration of classes.)

[jameinel]

I'm a bit concerned about these being module values. It means that if you have any test that starts opting into this behavior, suddenly the rest of your test suite becomes a bit indeterminate.

[jameinel]

this comment could be a bit clearer

[benhoyt]

Just coming back to this, I don't think this is the right direction, due to the following issues:

• As John pointed out, it's not clearing/resetting enough.
• Multiple events are called with the same charm instance today when there are deferred events, and that's as expected.
• In future we may want to extend ops, for efficiency when there are 100s or 1000s of events, to allow multiple events to be executed in a single execution of the Python. This could be done either by querying Juju to "give me more events" after the current event is processed, or by Juju somehow telling the hook up-front that there are multiple events.

Given those points, I tend to think we should move in the opposite direction of making it normal for the charm to expect multiple events. (We could still decide to create a new charm instance for every event, but I'm not sure that's a good signal.)

Leaving this open for another couple of days in case @PietroPasotti has comments.

[PietroPasotti]

Given those points, I tend to think we should move in the opposite direction of making it normal for the charm to expect multiple events. (We could still decide to create a new charm instance for every event, but I'm not sure that's a good signal.)

I think I see your point, and can't quite wrap my head around a good reply. I think that never reinitializing the charm, even between two harness.update_relation_data calls, does not reflect the reality of what's happening today if some remote charm touches its databags twice. The unit agent would ./dispatch twice, which would result in a new charm instance receiving the second event.

At the level of abstraction that the Harness operates, one could say, this is fine. If you want more realistic simulations, go to Scenario. I think of Scenario as a dispatch simulation: it simulates what would happen if a unit agent decided to call dispatch. What does Harness simulate then? What kind of testing does it enable?

I can see why this level of detail is too much for harness, but I can't figure out why.
We can close this, but it's maybe good to think about this in some other context.