canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
244 stars 119 forks source link

Run deferred Juju events on a fresh charm instance #1174

Open tonyandrewmeyer opened 6 months ago

tonyandrewmeyer commented 6 months ago

The main benefit here would be that it would be simpler to reason about (Juju) event behaviour, because it would be consistent both when an event was deferred and when it is executed normally.

This would not apply to custom events, which are executed synchronously, and are already different from Juju events. This would also not apply to LifecycleEvents (framework generated events), because those cannot be deferred.

We will need to use the same charm instance for multiple deferred (event, handler) pairs when more than one handler defers the event. If something like this happens:

I don't think we can tell that these were different Juju events, and we would maybe collapse the duplicates (?) and probably have to use the same charm instance for both observers. We would otherwise need to add some sort of ID to the deferred event to be able to group them correctly.

I believe the cleanest change here is to lift out the deferred reemit from _Manager in [ops/main.py], and handle the deferred events in main(), creating new _Managers for each one (we need to check that setup_root_logging can be called twice, and avoid calling legacy hooks twice - probably the legacy hook should be moved out of _Manager too). This provides a fresh Model, Framework and _Dispatcher as well, which is also most consistent (and avoids having to have the framework forget/unobserve the old charm).

There would be some small behaviour changes here: the pre-commit and commit events would get emitted twice (just as if the event had not been deferred, but not like now), which I think is fine, and we would collect the status twice (again just as if the event had not been deferred, but not like now), which might break some expectations.

See also the discussion in #736. Note that we will not be able to make Harness mimic this behaviour (because it would break too many existing charm tests) but we will be encouraging using Scenario, which can handle it (right now Scenario copies the ops behaviour, saving the deferred events to a queue that ops then runs ahead of the Juju event, but the Scenario API would not need changing in order to match the new ops behaviour).

ca-scribner commented 5 months ago

I haven't thought about this specific case a ton, but I suspect there's a number of charms in the wild that have bugs around this and don't realize it. I know at least some charms have self.x = self.model.config['x'] statements in their Charm.__init__() which feels like a natural thing to do in a charm, but can have unexpected consequences when Charm objects are sometimes reused. My guess is there's more bugs like that in the wild atm, too. For those reasons I'm +1 on making events closer to "every event gets its own object"

benhoyt commented 4 months ago

Per Madrid discussion: it still seems to us like a good idea to run deferred events on their own charm instance so they're more like other events. However, we don't want to spend time on this until we've worked through the "Juju-aware deferral" mechanism we're planning for 24.10.