Backwards incompatible change to config.changed states

Mon May 2 21:08:27 UTC 2016

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
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju/attachments/20160502/14af612c/attachment.html>