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

Run deferred Juju events on a fresh charm instance #1174

Open
tonyandrewmeyer opened this issue Apr 2, 2024 · 2 comments
Open

Run deferred Juju events on a fresh charm instance #1174

tonyandrewmeyer opened this issue Apr 2, 2024 · 2 comments
Labels
feature New feature or request needs design needs more thought or spec

Comments

@tonyandrewmeyer
Copy link
Contributor

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:

  • original Juju event: observer A defers, observer B does not
  • next Juju event of same type: deferred observer A defers, observer A defers, observer B defers

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
Copy link

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
Copy link
Collaborator

benhoyt commented May 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request needs design needs more thought or spec
Projects
None yet
Development

No branches or pull requests

3 participants