diff mbox

[LEDE-DEV,2/3,RFC] base-files: add option to rc.common to disable default reload behavior

Message ID 1490018935-16947-2-git-send-email-alexandru.ardelean@riverbed.com
State Superseded
Delegated to: Felix Fietkau
Headers show

Commit Message

Alexandru Ardelean March 20, 2017, 2:08 p.m. UTC 
From: Alexandru Ardelean <ardeleanalex@gmail.com>

Traditionally if a reload script fails, it will fallback to restart.

That seems to be the default behavior in case no reload
handler has been specified, and `reload` will return 1.

That also has the disadvantage of masking reload errors
from `/etc/init.d/<service> reload`.

Still, it's a pretty old behavior, and in most cases
it should be fine.

For other cases, the `RESTART_ON_RELOAD_ERR=0` can
be specified to override this.

Not sure about the correctness of this approach,
so this patch is RFC.

Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 package/base-files/files/etc/rc.common | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Hans Dedecker March 28, 2017, 8:36 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> From: Alexandru Ardelean <ardeleanalex@gmail.com>
>
> Traditionally if a reload script fails, it will fallback to restart.
>
> That seems to be the default behavior in case no reload
> handler has been specified, and `reload` will return 1.
>
> That also has the disadvantage of masking reload errors
> from `/etc/init.d/<service> reload`.
>
> Still, it's a pretty old behavior, and in most cases
> it should be fine.
>
> For other cases, the `RESTART_ON_RELOAD_ERR=0` can
> be specified to override this.
>
> Not sure about the correctness of this approach,
> so this patch is RFC.
Discussed the restart-on-reload-fail behaviour with Felix and
Matthias; general consensus is to remove the restart-on-reload-fail
behaviour in rc.common. Packages do not seem to depend on this
restart-on-reload fail behaviour and it's more logical to handle
reload failure in the packages itself if extra logic is required.

Hans
>
> Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> ---
>  package/base-files/files/etc/rc.common | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/package/base-files/files/etc/rc.common b/package/base-files/files/etc/rc.common
> index 95cf956..a893b09 100755
> --- a/package/base-files/files/etc/rc.common
> +++ b/package/base-files/files/etc/rc.common
> @@ -139,7 +139,13 @@ ${INIT_TRACE:+set -x}
>         }
>  }
>
> +RESTART_ON_RELOAD_ERR=${RESTART_ON_RELOAD_ERR:-1}
> +
>  ALL_COMMANDS="start stop reload restart boot shutdown enable disable enabled depends ${EXTRA_COMMANDS}"
>  list_contains ALL_COMMANDS "$action" || action=help
> -[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
> +[ "$action" = "reload" ] && {
> +       if [ "$RESTART_ON_RELOAD_ERR" == "1" ] ; then
> +               action='eval reload "$@" || restart "$@" && :'
> +       fi
> +}
>  $action "$@"
> --
> 2.7.4
>
Alexandru Ardelean March 29, 2017, 7:17 a.m. UTC | #2 
On Tue, Mar 28, 2017 at 11:36 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
>> From: Alexandru Ardelean <ardeleanalex@gmail.com>
>>
>> Traditionally if a reload script fails, it will fallback to restart.
>>
>> That seems to be the default behavior in case no reload
>> handler has been specified, and `reload` will return 1.
>>
>> That also has the disadvantage of masking reload errors
>> from `/etc/init.d/<service> reload`.
>>
>> Still, it's a pretty old behavior, and in most cases
>> it should be fine.
>>
>> For other cases, the `RESTART_ON_RELOAD_ERR=0` can
>> be specified to override this.
>>
>> Not sure about the correctness of this approach,
>> so this patch is RFC.
> Discussed the restart-on-reload-fail behaviour with Felix and
> Matthias; general consensus is to remove the restart-on-reload-fail
> behaviour in rc.common. Packages do not seem to depend on this
> restart-on-reload fail behaviour and it's more logical to handle
> reload failure in the packages itself if extra logic is required.

Thanks for the consideration.

I'll submit another patch series just for this restart-on-reload fail.

I feel this mechanism was maybe intended to act as default reload
implementation.
In the sense, that a default reload hook is implemented returning
non-zero, and if no specific reload is implemented, this logic [
restart on reload fail ] would work.

Will think about it a bit.

Thanks
Alex

>
> Hans
>>
>> Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
>> ---
>>  package/base-files/files/etc/rc.common | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/package/base-files/files/etc/rc.common b/package/base-files/files/etc/rc.common
>> index 95cf956..a893b09 100755
>> --- a/package/base-files/files/etc/rc.common
>> +++ b/package/base-files/files/etc/rc.common
>> @@ -139,7 +139,13 @@ ${INIT_TRACE:+set -x}
>>         }
>>  }
>>
>> +RESTART_ON_RELOAD_ERR=${RESTART_ON_RELOAD_ERR:-1}
>> +
>>  ALL_COMMANDS="start stop reload restart boot shutdown enable disable enabled depends ${EXTRA_COMMANDS}"
>>  list_contains ALL_COMMANDS "$action" || action=help
>> -[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
>> +[ "$action" = "reload" ] && {
>> +       if [ "$RESTART_ON_RELOAD_ERR" == "1" ] ; then
>> +               action='eval reload "$@" || restart "$@" && :'
>> +       fi
>> +}
>>  $action "$@"
>> --
>> 2.7.4
>>
Eric Luehrsen March 29, 2017, 11:43 p.m. UTC | #3 
On 03/29/2017 03:17 AM, Alexandru Ardelean wrote:
> On Tue, Mar 28, 2017 at 11:36 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>> On Mon, Mar 20, 2017 at 3:08 PM, Alexandru Ardelean
>> <ardeleanalex@gmail.com> wrote:
>>> From: Alexandru Ardelean <ardeleanalex@gmail.com>
>>>
>>> Traditionally if a reload script fails, it will fallback to restart.
>>>
>>> That seems to be the default behavior in case no reload
>>> handler has been specified, and `reload` will return 1.
>>>
>>> That also has the disadvantage of masking reload errors
>>> from `/etc/init.d/<service> reload`.
>>>
>>> Still, it's a pretty old behavior, and in most cases
>>> it should be fine.
>>>
>>> For other cases, the `RESTART_ON_RELOAD_ERR=0` can
>>> be specified to override this.
>>>
>>> Not sure about the correctness of this approach,
>>> so this patch is RFC.
>> Discussed the restart-on-reload-fail behaviour with Felix and
>> Matthias; general consensus is to remove the restart-on-reload-fail
>> behaviour in rc.common. Packages do not seem to depend on this
>> restart-on-reload fail behaviour and it's more logical to handle
>> reload failure in the packages itself if extra logic is required.
>
> Thanks for the consideration.
>
> I'll submit another patch series just for this restart-on-reload fail.
>
> I feel this mechanism was maybe intended to act as default reload
> implementation.
> In the sense, that a default reload hook is implemented returning
> non-zero, and if no specific reload is implemented, this logic [
> restart on reload fail ] would work.
>
> Will think about it a bit.
>
> Thanks
> Alex

I would have concerns with the lack of documentation on how to control 
the outcome of procd triggers. Restart on reload behavior may fix things 
without people realizing it. Cross platform contributers may not fully 
appreciate the quirks of procd vs systemd vs ... This is to be expected 
without a user guide.

For example of a gotcha, triggers on interfaces may be noisy. Look at 
how often dnsmasq.conf is rewritten on "interface.*" raw trigger. It 
isn't restarted because the command line doesn't change. The delay timer 
is a nice robustness action over hotplug scripts. Hotplug scripts are 
easier to target. Reload needs to be used with this procd trigger but 
not restart.
diff mbox

Patch

diff --git a/package/base-files/files/etc/rc.common b/package/base-files/files/etc/rc.common
index 95cf956..a893b09 100755
--- a/package/base-files/files/etc/rc.common
+++ b/package/base-files/files/etc/rc.common
@@ -139,7 +139,13 @@  ${INIT_TRACE:+set -x}
 	}
 }
 
+RESTART_ON_RELOAD_ERR=${RESTART_ON_RELOAD_ERR:-1}
+
 ALL_COMMANDS="start stop reload restart boot shutdown enable disable enabled depends ${EXTRA_COMMANDS}"
 list_contains ALL_COMMANDS "$action" || action=help
-[ "$action" = "reload" ] && action='eval reload "$@" || restart "$@" && :'
+[ "$action" = "reload" ] && {
+	if [ "$RESTART_ON_RELOAD_ERR" == "1" ] ; then
+		action='eval reload "$@" || restart "$@" && :'
+	fi
+}
 $action "$@"