Great suggestion, Marco! When would the next office hour be? 2016-05-02 23:13 GMT+02:00 Marco Ceppi <marco.ce...@canonical.com>:
> Might I suggest we do a hangout on air so we can record the discussion > while skipping the back and forth on the list? Possibly during an office > hour? > > Also, I'm not sure the decision is final and I certainly appreciate your > feedback and welcome the continued discussion so we can reach a consensus! > > Marco > > On Mon, May 2, 2016, 4:09 PM Merlijn Sebrechts < > merlijn.sebrec...@gmail.com> wrote: > >> 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 <cory.jo...@canonical.com>: >> >>> 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 < >>> merlijn.sebrec...@gmail.com> 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 <cory.jo...@canonical.com>: >>>> >>>>> 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 >>>>> Juju@lists.ubuntu.com >>>>> Modify settings or unsubscribe at: >>>>> https://lists.ubuntu.com/mailman/listinfo/juju >>>>> >>>>> >>>> >>> >> -- >> Juju mailing list >> Juju@lists.ubuntu.com >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/juju >> >
-- Juju mailing list Juju@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju
