Hi Cory
Thanks for your consideration. I strongly agree that any sort of automatic state removal is a bad idea. That was the reason why I started thinking about making the differentiation between states and events. I would have loved to discuss this more thoroughly with you and Ben. Although I understand the decision has been made, I would still like to explain my take on this, especially since we agree on so much of the fundamentals. Each state is a fact. A fact can only become un-true when an action reverses it. x.installed will becomes untrue when something uninstalls x. If you interpret y.changed as a fact, then it will only become untrue when y has reverted to its original value. Only then does it become un-changed. This behavior is clearly useless. So in contrary to all the other states, "x.changed" was not interpreted as a fact. It has been interpreted as "x.changed since the last hook run" by removing this state after a hook run. I am glad that we agree that this behavior isn't consistent and that it has to change. Now I'm not so sure about the fix. Removing the "x.changed" hook manually in a handler has the exact same issue. "x.changed" has not been made un-true because some handler reacts to it. "x.changed" is still a fact. By removing it, the handlers are actually lying to the framework. This will cause all sorts of issues. Am I correct that you will modify the reactive framework to not retest the queue on a state removal? I understand the reasoning behind it, however, this will create new issues. Retesting the queue ensures a hook run has the same outcome no matter what order the handlers are executed in. A handler should not be allowed to run when its conditions aren't satisfied anymore. Please see the following example: Handler A requires the service to be running. Handler B stops the service. When the queue is A-B, you will have a successful run. When the queue is B-A, you will have an error. The order in which handlers are executed is not determined, so this means that *this hook would crash sometimes, and run successfully other times*. This will cause errors that are not reproducible. Reproducability and repeatability are very important in config management... I would love to discuss this more thoroughly with you and Ben. Doing a discussion like this on a mailinglist isn't the easiest way of communicating, although I'm not sure the time difference permits a real-time discussion. Kind regards Merlijn Sebrechts 2016-05-02 21:15 GMT+02:00 Cory Johns <[email protected]>: > Merlijn, > > Apologies for the delayed reply. I realized that I had typed this up but > forgotten to actually send it. > > You're right that there are still cases where the hook-persistent nature > of the config.changed states continue to cause problems. However, after > some discussion with Ben, I actually think that *any* sort of automatic > state removal is the wrong approach, whether it happens at the end of a > hook or at the end of an dispatch loop (essentially what you're proposing > with events). Instead, Ben convinced me that the right thing to do is to > always have states be explicitly acknowledged and removed by the handlers. > This doesn't work as expected currently because of an implementation detail > of how the handler queue is managed on state removals, but I think it's > more appropriate to fix that rather than add a new type of state. > > In that approach, the config.changed state would be set when the change is > detected, all applicable handlers that are watching for it would execute, > each one explicitly acknowledging that it's been handled by removing it, > and then, after all handlers are done, the removals would be applied. Note > that the initial handler (e.g., install or start_service) would also need > to clear the changed state if present to prevent the secondary handler > (reinstall or restart_service) from acting on it. Alternatively, the > approach I took for the ibm-base layer was to remove the gating state and > separate reinstall handlers entirely, and always just drive off the > config.changed states: > https://code.launchpad.net/~johnsca/layer-ibm-base/fix-multi-call/+merge/292845 > > Note that there is still some potential semantic value to having "new" and > "changed" be distinguishable, but perhaps it's not as valuable enough to > worry about. > > On Fri, Apr 22, 2016 at 6:57 PM, Merlijn Sebrechts < > [email protected]> wrote: > >> Hi Cory >> >> >> >> I think this is another symptom of the deeper issue that the reactive >> framework treats events like states. 'config.changed' is an event. The >> following code is something that intuitively seems correct. We want to >> reinstall when the config has changed while the service is installed. >> However, it will still have the unwanted side effect you stated earlier. >> >> @when('installed', 'config.changed.install_source')def reinstall(): >> install() >> >> >> Please note that *your fix will only work when the service is installed >> during the first config-changed hook*. If a service is installed during >> a subsequent config-changed hook, you will again have the same issue. This >> can happen when you have config options such as "(bool)install_plugin-x" >> and "(string)plugin-x-source". >> >> Anticipating these kind of conflicts requires a thorough understanding of >> both the reactive framework and hooks. You are correct in thinking that >> these conflicts should not happen. If we require every Charmer to have full >> understanding of these things, we might miss out on valuable contributions. >> >> >> I would urge you to seriously consider making the differentiation between >> events and states. For people who have used hooks it might seem logical >> that config.changed is active during an entire hook. Newcomers might have >> more difficulty understanding this. >> >> So my suggestion is: >> >> - An event is only active right after the event happens. >> - A handler can only be added to the queue when his events + his states >> are active >> - A handler will be removed from the queue only when one of his states >> becomes inactive. Events of handlers that are in the queue are not >> 'rechecked'. >> >> >> Another use-case for this: >> >> @when('service.running', 'configfile.changed') >> def restart_service() >> >> 1. When the config file changes, and the service is running, restart the >> service. >> 2. When the config file changes and the service is not running, don't >> restart the service. >> 3. When the config file changed before the service was running, and now >> we start the service, don't restart the service. >> 4. When the config file changes, the service restarts, and the config >> file changes again, we want to restart the service again. >> >> 1 and 2 are currently possible. 3 and 4 would be if 'file.changed' would >> be an event. >> >> >> >> >> >> Kind regards >> Merlijn Sebrechts >> >> 2016-04-22 23:02 GMT+02:00 Cory Johns <[email protected]>: >> >>> I have proposed https://github.com/juju-solutions/layer-basic/pull/61 >>> as a slight change to how the config.changed states from the basic layer >>> work. Currently, the changed states are set during the first hook >>> invocation, under the assumption that the values were "changed" from >>> "nothing" (not being set at all). However, this is slightly problematic in >>> a case like the following, where we expect install() to only be called >>> once, unless the value has changed after the fact: >>> >>> @when_not('installed')def install(): >>> # do install >>> set_state('installed') >>> @when('config.changed.install_source')def reinstall(): >>> install() >>> >>> >>> The proposal adds new states, config.new, and changes config.changed to >>> not be set the first time. You could get the old behavior by saying >>> @when_any('config.new.foo', 'config.changed.foo'). >>> >>> Is anyone depending on the current behavior? Are there any objections >>> to this change? >>> >>> -- >>> Juju mailing list >>> [email protected] >>> Modify settings or unsubscribe at: >>> https://lists.ubuntu.com/mailman/listinfo/juju >>> >>> >> >
-- Juju mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju
