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

[POC] any-event #1554

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

PietroPasotti
Copy link
Contributor

@PietroPasotti PietroPasotti commented Jan 31, 2025

brainstorming proof of concept API for introducing an 'any' event in ops

Issue 1:

class MyCharm(ops.CharmBase):
    def __init__...
        ...
        self.reconcile()
    def reconcile(self):
        # holistic charm state reconciler

examples:

Issue 2:

class MyCharm(ops.CharmBase):
    def __init__...
        ...
        for event in [list of all possible events]:
            self.framework.observe(event, self._on_any)

    def _on_any(self, _event):
        # holistic charm state reconciler

Proposed solution:

class MyCharm(ops.CharmBase):
    def __init__...
        ...
        self.framework.observe(self.on.any, self._on_any)

    def _on_any(self, _event):
        # holistic charm state reconciler

@benhoyt
Copy link
Collaborator

benhoyt commented Feb 3, 2025

This is a duplicate of #1108, and I think the reason we closed that one still mostly applies. However, we are seeing/promoting the holistic pattern more now, so a few additional thoughts:

  • Doing it in __init__ is okay, but is probably going to get you more events than you want. In addition, you probably don't want to reconcile on events like secret-rotate and other "non holistic" events. See holistic vs delta doc.
  • We still believe the for loop over explicit event names is the way to go. Part of the value of an event system is the producer (Juju, or sometimes ops) can produce new event types whenever it needs to, and the consumer will only get notified about those it's listening for. (Gustavo made this point fairly strongly when we were working on the change-updated stuff.)
  • Even if we did something like this, what would "any" mean? You probably only want to listen to holistic-friendly events, which is roughly the same thing as defer-able. So perhaps an observe_any_derrable or some list which lists all the deferrable events?
  • Another approach is opting into observing event groups, something like self.framework.observe_groups(self._reconcile, relation=True, config=True). But this will still suffer from the problem of Juju adding new events to a group.

Regarding your solution in #1108 and the three events you're seeing as a result of collect_app_status and collect_unit_status, you could filter out LifecycleEvent types. Something like:

for event in self.on.events().values():
    if not issubclass(event.event_type, ops.LifecycleEvent):
         self.framework.observe(event, self._on_event)

I'll leave this issue open for comments for another week. And it might be good to have further discussion -- if so, let us know and we'll schedule a nice little discussion for Frankfurt.

@PietroPasotti
Copy link
Contributor Author

  • Doing it in __init__ is okay, but is probably going to get you more events than you want. In addition, you probably don't want to reconcile on events like secret-rotate and other "non holistic" events. See holistic vs delta doc.

I think the whole point of a well-written reconciler is that it truly is idempotent. There's no 'running it too often'. The only case in which there can be confusion is when the event doesn't match the state (e.g. you get a relation-removed, but the relation and its data are still readable).
I want it running on every single event, and possibly even more often than that (e.g. using a resurrect mechanism, in case the user has set update-status-hook-interval to something longer.

  • We still believe the for loop over explicit event names is the way to go. Part of the value of an event system is the producer (Juju, or sometimes ops) can produce new event types whenever it needs to, and the consumer will only get notified about those it's listening for. (Gustavo made this point fairly strongly when we were working on the change-updated stuff.)

I think this argument only holds for charms that don't use a reconciler. A holistic charm will only look at the parts of the state it knows, and if juju adds an event type it will only mean it will reconcile 'more often'. So long as the semantics of the state don't change we're safe.

  • Even if we did something like this, what would "any" mean? You probably only want to listen to holistic-friendly events, which is roughly the same thing as defer-able. So perhaps an observe_any_derrable or some list which lists all the deferrable events?

Side-note: we also realized along the way that the only domain where 'true delta charming' really is useful is machine charms: there 'start'/'remove' really mean something. In k8s world, where everything can churn at any moment and go up and down at any time, that's where holistic charming is more natural.
I don't think 'deferable-or-not' is the line we should be looking at. For charms like the ones we (o11y) write, even non-deferrable events are a good reason to run the reconciler.
The reason is that we don't really have a careful upgrade/shutdown path. Database charms are very different in that regard.
I wonder if we should be special-casing on the workload type instead. All charms I'm aware of except database charms don't care if they skip observing a 'remove' or a 'stop' or an 'upgrade'.
Could we call them stateful-workload-charms and talk about that instead?

Regarding your solution in #1108 and the three events you're seeing as a result of collect_app_status and collect_unit_status, you could filter out LifecycleEvent types. Something like:

Yes, absolutely.

I'll leave this issue open for comments for another week. And it might be good to have further discussion -- if so, let us know and we'll schedule a nice little discussion for Frankfurt.

I wonder if the workload-statefulness thing is a distinction that more people feel we should be philosophizing about; if so, I'd happily discuss it in frankfurt.

Since we already have a framework.on.commit we can (abuse) to do this, and the only difference with what we want is that we'd like our reconciler to run before all other events are emitted (and before the framework has closed all databases) rather than after... would you feel differently about adding a framework.on.init or similar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants