Backwards incompatible change to config.changed states

Marco Ceppi marco.ceppi at canonical.com
Mon May 2 21:32:41 UTC 2016 

I'll have to check with Jorge Castro, but I imagine either the 13th or 27th
of this month. I'll confirm and this will likely be a "Europe" friendly
time.

Marco

On Mon, May 2, 2016 at 5:19 PM Merlijn Sebrechts <
merlijn.sebrechts at gmail.com> wrote:

> Great suggestion, Marco! When would the next office hour be?
>
> 2016-05-02 23:13 GMT+02:00 Marco Ceppi <marco.ceppi at 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.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/ea274055/attachment.html>

More information about the Juju mailing list