No decorator to run handler for every hook

[stub42]

There is no mechanism provided to declare that a handler should be run during the hook phase for each and every hook.

The most obvious fix would be adjusting the existing @hook decorator so that it always runs if no patterns are provided (and an argument never passed to the handler).

In my experience, charms should minimize code that can only run in a specific hook to avoid race conditions. For example, consider a simple service configuration change. This will queue the config-changed hook on each unit, but if there is already a queue of hooks being processed by a unit then these hooks will see the changed configuration in the hook environment before the config-changed hook has had a chance to run. This can be fatal, with hooks crashing attempting to run code paths before the config-changed hook has set things up so they can be run successfully. Similar races can be described for leadership and relations. The simplest way of avoiding this entire class of races is to have a single, general hook instead of several specific ones tied to particular events.

[johnsca]

I would argue that if you think you want a @hook handler that runs for every hook, you're probably thinking about the problem wrong. That is to say, I agree with your premise that you shouldn't restrict your logic to specific hooks, but I think you're not taking it quite far enough; I think that charm layers should be avoiding the use of @hook as much as possible and instead relying on states to drive behavior, and if you do have a @hook handler, the only thing it should be doing is setting one or more states (with the possible exception of install, but even that in many charms should be handled by a runtime layer). In other words, you can think of the reactive dispatch loop as the single general hook that you mentioned.

For your example, I would suggest not handling the config-changed hook explicitly at all, and instead having the state handlers do the work and drive their behavior off of the config values, whether they were just changed or not.

[stub42]

So rather than a @hook handler that runs for every hook in the hook phase, you would recommend a @when decorated handler that runs for every hook in the 'other' phase?

What I'm primarily trying to ensure is keeping the hook environment state in sync with the reactive framework state. I don't want a handler to fail when service configuration (hook environment state) contains unexpected settings because the handler that notices that the service configuration has changed and updates the reactive state has not run yet. Or maybe this is better suited to @preflight discussed elsewhere.

[johnsca]

Right, so let's say your charm has two different "modes," selected by a config option that each depend on a different set of other config options. Your config-changed hook handler could then set one of two "mode states" based on the selector option. You could even have the hook handler do validation on the additional options and remove the mode state if its dependent options are invalid. Then, you'd have separate state handlers for each of those two states that would be guaranteed to see the expected settings because they would only trigger if the hook handler had first confirmed which should be set and that it had the additional settings it needed.

[stub42]

This does not address the skew between the hook environment and the reactive state. The reactive state will lag, as it will not be updated until a config-changed hook can be run. The separate state handlers you describe are not guaranteed to see the expected settings. They will see whatever the current settings are in the hook environment, and that may not match what they expect to see from the lagging reactive state. eg. the charm is running handlers for the peer-relation-changed hook in "mode A" but fails because it finds that all the "mode A" settings are now set to None, despite these same settings having been validated in an earlier config-changed hook.

You can avoid this skew entirely if you update the state in every hook rather than just in config-changed.

[marcoceppi]

Since all reactive events occur within the context of a hook and the state of the juju db is snapshotted at each run (realtion settings, configuration, etc) it shouldn't matter when or where during the event train code is run because it will all always run with the same data for the life of that hook event. I'm not sure what problem you're highlighting @stub42 could you try explaining it in a different way?

[johnsca]

If I understand it correctly, the issue is that, let's say you have hook A running and B queued and the user changes a config value. So now your hook queue is A, B, C (where C is the config-changed). A has a snapshot of the state from when it started, so won't see the new values, but when B runs, it may see the new config values even though C (config-changed) has not run.

Somehow I had it in my head that the snapshot was taken when the hooks were queued, but if they're taken when the hook is started I could see that being a problem. It would be nice if there were a better way to handle this than saying "always use @hook() instead of @hook('config-changed')" (or maybe @hook('*')) but I'm struggling to find a better solution.

At any rate, it should be easy enough to make the hook decorator behave that way.

[johnsca]

The reason I want to resist again a wild-card approach is that it's unclear at a glance why this particular block should be run "unconditionally" vs every other block. Maybe something like @when_config_changed would assuage my concerns, even if it essentially behaved like @hook() (maybe @hook() plus if data_changed(config()):).

[lazypower]

after a long and arduous discussion about hook context isolation vs config state / unit state - i'm +1 for his last assessment of @when_config_changed

[stub42]

Service configuration and leadership are certainly the main problem areas. I have code that implements leadership states at https://git.launchpad.net/~stub/charms/+source/postgresql/tree/reactive/leadership.py?h=reactive , which I think would make a nice layer. I was thinking of a similar approach for service configuration. You may consider layers a better solution than decorators. The layer approach requires being able to run code between discovery and the main reactor loop (hookenv.atstart works for this purpose, and discussion about an @preflight decorator is elsewhere).

[johnsca]

@stub42 That's interesting. I hadn't really understood what you were proposing with your leadership layer before you explained it in context of this discussion.

So, any detected change in a leadership (or config) value would set a corresponding state, which would persist until a charm layer removed it? That would certainly give fine-grained ability to react to changes in specific values, but with the all-or-none nature of @when_all / @when_none, it wouldn't be feasible to group settings together. So, I think there would still be a need for a "when any config changed" case, which could then be used to set more states based on any level of granularity desired. I do like the idea of using a state instead of adding a new decorator, as it both gives better control and avoids the question of how to acknowledge the change and what happens if you are not in a position to react to it right away.

Regarding these leadership and config states, perhaps it would make sense to fold them into the base layer? They seem pretty broadly applicable to any charm.

[stub42]

@johnsca Currently in my work, the leadership.changed.foo state remains set for the duration of a hook. It will not be set next hook (unless the value has changed again). This sort of matches things like @when_file_changed. I think I prefer it this way. It may be a better approach to require the charm to explicitly unset the flag, but this stops it being useful as a way of communicating across layers and restricts you to having a single handler watching for this state.

[stub42]

#20 implements an @setup decorator that deals with most use cases of @everyhook