Re: Backwards incompatible change to config.changed states
I would strongly prefer not to add states lightly. Can we take some time
to see if this is avoidable?
Mark
On 22/04/16 16:02, Cory Johns wrote:
> 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
Re: Backwards incompatible change to config.changed states
We can skip the new state if no one is depending on or finds the existing
behavior useful (I suspect that may be true).
On Fri, Apr 22, 2016 at 5:37 PM, Mark Shuttleworth wrote:
>
> I would strongly prefer not to add states lightly. Can we take some time
> to see if this is avoidable?
>
> Mark
>
> On 22/04/16 16:02, Cory Johns wrote:
>
> 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
Re: Backwards incompatible change to config.changed states
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 :
> 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
Re: Backwards incompatible change to config.changed states
I think this is an excellent idea! The the initial setting of the
config.changed hook has caused contention in charm design and
implementation for me. Not having to account for the config.changed hooks
firing, when logically it seems that one shouldn't have to (e.g. no configs
have changed yet), would probably ease uptake for new-comers who aren't
familiar with the intrinsics of why a config.changed hook fires without
configs changing. I think Cory's implementation is a non-intrusive way of
handling this issue whilst also providing backwards compatibility for the
legacy functionality.
> 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
Re: Backwards incompatible change to config.changed states
On 23 April 2016 at 04:02, Cory Johns wrote: > Is anyone depending on the current behavior? Are there any objections to > this change? I can argue both designs, but think that the most useful one is what you are proposing (config.changed not being set in the first hook, which is not necessarily the install hook). If you are clarifying this, you should also clarify how things work in the upgrade-charm hook when new config options are added or removed. Your proposed change does not affect my work, and will allow me to simplify some things. -- Stuart Bishop -- Juju mailing list Juju@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju
Re: Backwards incompatible change to config.changed states
I'd ask Cory to explore Merlijn's suggestions more deeply before we add
another state.
Mark
On 25/04/16 02:11, James Beedy wrote:
> I think this is an excellent idea! The the initial setting of the
> config.changed hook has caused contention in charm design and
> implementation for me. Not having to account for the config.changed hooks
> firing, when logically it seems that one shouldn't have to (e.g. no configs
> have changed yet), would probably ease uptake for new-comers who aren't
> familiar with the intrinsics of why a config.changed hook fires without
> configs changing. I think Cory's implementation is a non-intrusive way of
> handling this issue whilst also providing backwards compatibility for the
> legacy functionality.
>
>> 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
Re: Backwards incompatible change to config.changed states
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 :
>
>> 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
Re: Backwards incompatible change to config.changed states
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 :
> 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 i
Re: Backwards incompatible change to config.changed states
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 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 : > >> 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
Re: Backwards incompatible change to config.changed states
Great suggestion, Marco! When would the next office hour be? 2016-05-02 23:13 GMT+02:00 Marco Ceppi : > 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 : >> >>> 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"
Re: Backwards incompatible change to config.changed states
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.sebrec...@gmail.com> wrote: > Great suggestion, Marco! When would the next office hour be? > > 2016-05-02 23:13 GMT+02:00 Marco Ceppi : > >> 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 : >>> 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,
Re: Backwards incompatible change to config.changed states
Office Hours will indeed be May 13th this time around, I'll send out a separate EU-friendly time. Merlijn, if you have a specific timeslot in mind just send me a mail offlist and we'll put it where it's most convenient. On Mon, May 2, 2016 at 5:32 PM, Marco Ceppi wrote: > 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 > wrote: >> >> Great suggestion, Marco! When would the next office hour be? >> >> 2016-05-02 23:13 GMT+02:00 Marco Ceppi : >>> >>> 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 >>> 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 : > > 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 >
