Backwards incompatible change to config.changed states

Mon May 2 21:13:22 UTC 2016

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.sebrechts at 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.johns at 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.sebrechts at 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.johns at 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 at lists.ubuntu.com
>>>> Modify settings or unsubscribe at:
>>>> https://lists.ubuntu.com/mailman/listinfo/juju
>>>>
>>>>
>>>
>>
> --
> Juju mailing list
> Juju at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju/attachments/20160502/58480845/attachment.html>