operator

operator copied to clipboard

The ConfigChanged event doesn't provide any information today as to what changed. It seems necessary to properly processes this hook to have a view of what changed.

After checking the ConfigData it's simply a cached JSON. Expanding the ConfigChanged event to automatically store data like the StoredStateData on commit would provide N-1 historical view of the config. Then the event could provide a property to compare current to historical and just return what has changed.

This seems fairly critical to properly processing ConfigChagned. Is it already addressed with another feature of the Framework that I'm just not connecting here?

Many applications don't need a delta, they just need to recompute their current state based on the current value of config. As such, the framework doesn't currently provide the N-1 of config. As you say, it isn't hard for the charm to do something like:

def __init__(self):
  self.state.set_default(previous_config=None)

def on_config_changed(event):
  previous_config = self.state.previous_config
  if previous_config is not None:
    diff(previous_config, self.model.config)
  self.state.previous_config = self.model.config

That is something we could do automatically in the framework if we find that people need it all the time. However, many ways of dealing with config don't really care what keys have changed, as much as they just want the opportunity to see the new config and react to it.

On Mon, Mar 23, 2020 at 11:33 PM Chris Sanders [email protected] wrote:

The ConfigChanged event doesn't provide any information today as to what changed. It seems necessary to properly processes this hook to have a view of what changed.

After checking the ConfigData it's simply a cached JSON. Expanding the ConfigChanged event to automatically store data like the StoredStateData on commit would provide N-1 historical view of the config. Then the event could provide a property to compare current to historical and just return what has changed.

This seems fairly critical to properly processing ConfigChagned. Is it already addressed with another feature of the Framework that I'm just not connecting here?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/canonical/operator/issues/189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7LM4WBQC2A7FIKUROTRI62P7ANCNFSM4LSD2FQA .

Wouldn't it be a good best practice to have available for new charmers? It's standard practice for me with reactive charming to only trigger functions on changes to specific configuration options. I can do as you're showing above, but I felt it would be better for the framework to build it in so it's available to people, if it's not needed it can be ignored.

One of the most important reasons why I do think you need to check configurations are that a lot of services require some form of restart to pickup new configs. From an operational perspective this is a big deal and charms shouldn't be doing that randomly. Changing a config option that doesn't impact a service shouldn't restart it. Also config change get's called for events that don't actually change a config value (see attach-resource for example).

With production charms today I see this addressed by using a separate tool that the charm registers configuration files with. That tools keeps a hash of the config file, and after any change all config files are checked and any that changed get the registered service restarted. While something like that could be used, I find it leaves the charm author unsure if/when a service gets restarted and personally think it would be better if the framework provided the tools to handle this from the start.

If I implemented this in a PR for the framework where would it be best implemented? What you describe above works fine in the charm code, but I'm not clear how it would be implemented as a proper part of the framework. Would simply adding something like the above to CharmBase be appropriate? I felt like the right place to put it was onto the ConfigChanged event itself, but I'm not clear where/how to store information since events are created and destroyed. What happens with the implementation if a ConfigChanged is deferred, or if multiple are deferred?

Once you start thinking in terms of strict deltas you have to be a lot more careful to never miss an event and be careful about your synchronization. Juju itself will know if config-changed has returned successfully and will continue to trigger config-changed as long as the version of config in the database isn't the last successful version from the charm.

What we have to be careful of in a charm delta code, is that we don't record the "last known" config unless we know that it has been successfully handled.

eg config-changed is fired, the charm computes a delta, runs some code that wants to change some relation data, but then encounters an error. It is tempting to compute the delta and then save the new state, before triggering handlers. We'll want to make sure we don't snapshot the new config until the handlers have fired.

John =:->

On Tue, Mar 24, 2020, 17:40 Chris Sanders [email protected] wrote:

Wouldn't the be a good best practice to have available for new charmers? It's standard practice for me with reactive charming to only trigger functions on changes to specific configuration options. I can do as you're showing above, but I felt it would be better for the framework to build it in so it's available to people, if it's not needed it can be ignored.

One of the most important reasons why I do think you need to check configurations are that a lot of services require some form of restart to pickup new configs. From an operational perspective this is a big deal and charms shouldn't be doing that randomly. Changing a config option that doesn't impact a service shouldn't restart it. Also config change get's called for events that don't actually change a config value (see attach-resource for example).

With production charms today I see this addressed by using a separate tool that the charm registers configuration files with. That tools keeps a hash of the config file, and after any change all config files are checked and any that changed get the registered service restarted. While something like that could be used, I find it leaves the charm author unsure if/when a service gets restarted and personally think it would be better if the framework provided the tools to handle this from the start.

Isn't this the other side of the equation? Registering the output config vs the input. (output config depends on a lot more potential input sources. config-get as well as relation-get) Currently we also don't give deltas for relation data. (what keys in the relation data have changed since I last acknowledged it).

I'm not sure what tools you want to handle this, as it isn't the same question that you started with. The nice thing about using output rendering is you don't make mistakes thinking that this output doesn't depend on this input, but then in a follow up you change it, but don't update the source tracking. (It works because in testing some other key changes at the same time, so it is getting updated.)

I'm a little concerned (though not very) about the amount of state we would be tracking in case the charmer wanted to know. (essentially we need a second copy of all data the charm interacts with).

If I implemented this in a PR for the framework where would it be best implemented? What you describe above works fine in the charm code, but I'm not clear how it would be implemented as a proper part of the framework. Would simply adding something like the above to CharmBase be appropriate? I felt like the right place to put it was onto the ConfigChanged event itself, but I'm not clear where/how to store information since events are created and destroyed. What happens with the implementation if a ConfigChanged is deferred, or if multiple are deferred?

We can define the behavior here. But from an infrastructure today, config-get will always return the current config. I'd rather have the behaviour that we give you the last acknowledged config vs the most current config. (rather than the small delta at the time of the event, replayed one after the other.) Also, be aware that multiple event handlers can look at the config, and 9 of 10 can handle it, but if the 7th one decided to defer, then all 10 will get the event again. And I don't think we want to introduce per-handler state tracking. (each handler can have their state, but the framework itself shouldn't implicitly do this).

—

You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/canonical/operator/issues/189#issuecomment-603244618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQ7PUSNPFKZC6MVQDGXTRJCZ33ANCNFSM4LSD2FQA .

I do see your point about tracking relation data as well and honestly while I hadn't thought about that it's probably worth doing as well.

For now don't worry about the 'rendering' issue I was only using that as an example of why you might want to know what changed rather than re-processing the full set of config on every config-changed.

What we have to be careful of in a charm delta code, is that we don't record the "last known" config unless we know that it has been successfully handled.

This is a great point, I think taking care of details like this in the framework prevents the user from implementing it without this sort of understanding and getting into weird edge cases.

Given this, do you have any advice for how to properly implement this in the framework? Is there a preferred process to blueprint something like this for development in the framework?

It sounds reasonable to have a way to decorate your config-changed handler to keep track of, and deliver, deltas in a usable way. I don't think it's something that we should be doing unconditionally for everybody, as what we're seeing is that a majority of config-changed handlers do not look at deltas but handle things in a stateless way.

This isn't a priority for us, but patches welcome -- talk to us before diving in just so we can sync up on exactly how to do it, and caveats that we might care about.

In other frameworks I've seen, where "events" are tracked. Its even possible to subscribe to specific events.

I mean, it would be fantastic to be able to observe a configuration, just as we would observe actions or relations.

framework.observe(config.myConfigKey.changed, myeventHandler)

This feature is also that I see alot when people are trying to develop charms. It comes up alot.

I think I could bloat this thread with many such posts in the forum, so I would say this would be a very welcome feature.

@erik78se

Currently, the pattern is to use the StoredState and check changes of stored state (which you should only update after handling the tasks related to that config state change.

Here's an example of how to handle each config value change for stateful reconfiguration. https://git.launchpad.net/charm-bitwarden-k8s/tree/src/charm.py#n94

You could create an object that mimics how the current config would reflect in the StoredState and then diff the StoredState against the new StoredState to get a list of keys/attributes that have changed if you don't want to run if statements in this manner.

This is not going to happen in the Operator Framework. If anything, we could push on Juju for a feature like this, but for now, closing.