| Message ID | 1442563153-680-1-git-send-email-helmut.schaa@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi On 18/09/2015 09:59, Helmut Schaa wrote: > Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT > and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the > default respawn behavior. > technically ok but why cant you tweak them in your packages initd script ? i am wondering what the use case is and if there are other possible solutions > This can be used to set the default respawn mode to infinite retries > for example. > it can already to that. John > Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com> > --- > CMakeLists.txt | 12 ++++++++++++ > service/instance.c | 4 +++- > service/instance.h | 12 +++++++++++- > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 6af17a3..721a381 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -23,6 +23,18 @@ SET(SOURCES procd.c signal.c watchdog.c state.c inittab.c rcS.c ubus.c system.c > > SET(LIBS ubox ubus json-c blobmsg_json json_script) > > +IF(RESPAWN_THESHOLD_DEFAULT) > + ADD_DEFINITIONS(-DRESPAWN_THESHOLD_DEFAULT=${RESPAWN_THESHOLD_DEFAULT}) > +ENDIF() > + > +IF(RESPAWN_TIMEOUT_DEFAULT) > + ADD_DEFINITIONS(-DRESPAWN_TIMEOUT_DEFAULT=${RESPAWN_TIMEOUT_DEFAULT}) > +ENDIF() > + > +IF(RESPAWN_RETRY_DEFAULT) > + ADD_DEFINITIONS(-DRESPAWN_RETRY_DEFAULT=${RESPAWN_RETRY_DEFAULT}) > +ENDIF() > + > IF(DEBUG) > ADD_DEFINITIONS(-DDEBUG -g3) > ENDIF() > diff --git a/service/instance.c b/service/instance.c > index 40ff021..9d4df7c 100644 > --- a/service/instance.c > +++ b/service/instance.c > @@ -709,7 +709,9 @@ instance_config_parse(struct service_instance *in) > > if (tb[INSTANCE_ATTR_RESPAWN]) { > int i = 0; > - uint32_t vals[3] = { 3600, 5, 5}; > + uint32_t vals[3] = { RESPAWN_THRESHOLD_DEFAULT, > + RESPAWN_TIMEOUT_DEFAULT, > + RESPAWN_RETRY_DEFAULT }; > > blobmsg_for_each_attr(cur2, tb[INSTANCE_ATTR_RESPAWN], rem) { > if ((i >= 3) && (blobmsg_type(cur2) == BLOBMSG_TYPE_STRING)) > diff --git a/service/instance.h b/service/instance.h > index 3fb33e9..ab05f20 100644 > --- a/service/instance.h > +++ b/service/instance.h > @@ -20,7 +20,17 @@ > #include <libubox/ustream.h> > #include "../utils/utils.h" > > -#define RESPAWN_ERROR (5 * 60) > +#ifndef RESPAWN_THRESHOLD_DEFAULT > +#define RESPAWN_THRESHOLD_DEFAULT 3600 > +#endif > + > +#ifndef RESPAWN_TIMEOUT_DEFAULT > +#define RESPAWN_TIMEOUT_DEFAULT 5 > +#endif > + > +#ifndef RESPAWN_RETRY_DEFAULT > +#define RESPAWN_RETRY_DEFAULT 5 > +#endif > > struct jail { > bool procfs; >
Hi John, On Fri, Sep 18, 2015 at 10:18 AM, John Crispin <blogic@openwrt.org> wrote: > Hi > > On 18/09/2015 09:59, Helmut Schaa wrote: >> Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT >> and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the >> default respawn behavior. >> > > technically ok but why cant you tweak them in your packages initd script > ? i am wondering what the use case is and if there are other possible > solutions In our tree we've patched most (maybe even all) services to respawn forever (respawn_retry=-1). Including all OpenWrt provided services. Instead of keeping these local modifications to several packages it's easier to just override procds default behavior. I think there might be other people around running OpenWrt on headless boxes where the respawn retry should not be limited by default. However, this is of course not suitable for a default OpenWrt box. If there are good reasons not to include this in procd feel free to drop this patch. However, it causes zero runtime overhead and is quite simple. As a followup I'd add a config flag for procd "respawn_forver_mode" or so that just sets respawn_retry to -1. Helmut
Hi, 2015-09-18 11:03 GMT+02:00 Helmut Schaa <helmut.schaa@googlemail.com>: > Hi John, > > On Fri, Sep 18, 2015 at 10:18 AM, John Crispin <blogic@openwrt.org> wrote: > > Hi > > > > On 18/09/2015 09:59, Helmut Schaa wrote: > >> Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT > >> and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the > >> default respawn behavior. > >> > > > > technically ok but why cant you tweak them in your packages initd script > > ? i am wondering what the use case is and if there are other possible > > solutions > > In our tree we've patched most (maybe even all) services to respawn forever > (respawn_retry=-1). Including all OpenWrt provided services. Instead of > keeping > these local modifications to several packages it's easier to just > override procds > default behavior. > > I think there might be other people around running OpenWrt on headless > boxes > where the respawn retry should not be limited by default. However, > this is of course > not suitable for a default OpenWrt box. > > If there are good reasons not to include this in procd feel free to > drop this patch. > However, it causes zero runtime overhead and is quite simple. > > As a followup I'd add a config flag for procd "respawn_forver_mode" or so > that > just sets respawn_retry to -1. > > Helmut > > It would be great to be able to configure these settings at runtime (ie in an uci file) Etienne
On 18/09/2015 11:40, Etienne Champetier wrote: > Hi, > > 2015-09-18 11:03 GMT+02:00 Helmut Schaa <helmut.schaa@googlemail.com > <mailto:helmut.schaa@googlemail.com>>: > > Hi John, > > On Fri, Sep 18, 2015 at 10:18 AM, John Crispin <blogic@openwrt.org > <mailto:blogic@openwrt.org>> wrote: > > Hi > > > > On 18/09/2015 09:59, Helmut Schaa wrote: > >> Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT > >> and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the > >> default respawn behavior. > >> > > > > technically ok but why cant you tweak them in your packages initd script > > ? i am wondering what the use case is and if there are other possible > > solutions > > In our tree we've patched most (maybe even all) services to respawn > forever > (respawn_retry=-1). Including all OpenWrt provided services. Instead > of keeping > these local modifications to several packages it's easier to just > override procds > default behavior. > > I think there might be other people around running OpenWrt on > headless boxes > where the respawn retry should not be limited by default. However, > this is of course > not suitable for a default OpenWrt box. > > If there are good reasons not to include this in procd feel free to > drop this patch. > However, it causes zero runtime overhead and is quite simple. > > As a followup I'd add a config flag for procd "respawn_forver_mode" > or so that > just sets respawn_retry to -1. > > Helmut > > It would be great to be able to configure these settings at runtime (ie > in an uci file) > > Etienne > agreed, i think the best would be to add support for -1 == endless respawn and then change procd.sh to use current values as default but allow overriding them vom /etc/config/system or similar. would that solve your use case ? John
On Fri, Sep 18, 2015 at 11:52 AM, John Crispin <blogic@openwrt.org> wrote: > > > On 18/09/2015 11:40, Etienne Champetier wrote: >> Hi, >> >> 2015-09-18 11:03 GMT+02:00 Helmut Schaa <helmut.schaa@googlemail.com >> <mailto:helmut.schaa@googlemail.com>>: >> >> Hi John, >> >> On Fri, Sep 18, 2015 at 10:18 AM, John Crispin <blogic@openwrt.org >> <mailto:blogic@openwrt.org>> wrote: >> > Hi >> > >> > On 18/09/2015 09:59, Helmut Schaa wrote: >> >> Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT >> >> and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the >> >> default respawn behavior. >> >> >> > >> > technically ok but why cant you tweak them in your packages initd script >> > ? i am wondering what the use case is and if there are other possible >> > solutions >> >> In our tree we've patched most (maybe even all) services to respawn >> forever >> (respawn_retry=-1). Including all OpenWrt provided services. Instead >> of keeping >> these local modifications to several packages it's easier to just >> override procds >> default behavior. >> >> I think there might be other people around running OpenWrt on >> headless boxes >> where the respawn retry should not be limited by default. However, >> this is of course >> not suitable for a default OpenWrt box. >> >> If there are good reasons not to include this in procd feel free to >> drop this patch. >> However, it causes zero runtime overhead and is quite simple. >> >> As a followup I'd add a config flag for procd "respawn_forver_mode" >> or so that >> just sets respawn_retry to -1. >> >> Helmut >> >> It would be great to be able to configure these settings at runtime (ie >> in an uci file) >> >> Etienne >> > > agreed, i think the best would be to add support for -1 == endless > respawn and then change procd.sh to use current values as default but > allow overriding them vom /etc/config/system or similar. would that > solve your use case ? Hmm, could work. However, changing the setting in /etc/config/system would only affect all services started after the config file change. In our case we'd just add it to the default /etc/config/system ... Let me look into it next week. Helmut
diff --git a/CMakeLists.txt b/CMakeLists.txt index 6af17a3..721a381 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,6 +23,18 @@ SET(SOURCES procd.c signal.c watchdog.c state.c inittab.c rcS.c ubus.c system.c SET(LIBS ubox ubus json-c blobmsg_json json_script) +IF(RESPAWN_THESHOLD_DEFAULT) + ADD_DEFINITIONS(-DRESPAWN_THESHOLD_DEFAULT=${RESPAWN_THESHOLD_DEFAULT}) +ENDIF() + +IF(RESPAWN_TIMEOUT_DEFAULT) + ADD_DEFINITIONS(-DRESPAWN_TIMEOUT_DEFAULT=${RESPAWN_TIMEOUT_DEFAULT}) +ENDIF() + +IF(RESPAWN_RETRY_DEFAULT) + ADD_DEFINITIONS(-DRESPAWN_RETRY_DEFAULT=${RESPAWN_RETRY_DEFAULT}) +ENDIF() + IF(DEBUG) ADD_DEFINITIONS(-DDEBUG -g3) ENDIF() diff --git a/service/instance.c b/service/instance.c index 40ff021..9d4df7c 100644 --- a/service/instance.c +++ b/service/instance.c @@ -709,7 +709,9 @@ instance_config_parse(struct service_instance *in) if (tb[INSTANCE_ATTR_RESPAWN]) { int i = 0; - uint32_t vals[3] = { 3600, 5, 5}; + uint32_t vals[3] = { RESPAWN_THRESHOLD_DEFAULT, + RESPAWN_TIMEOUT_DEFAULT, + RESPAWN_RETRY_DEFAULT }; blobmsg_for_each_attr(cur2, tb[INSTANCE_ATTR_RESPAWN], rem) { if ((i >= 3) && (blobmsg_type(cur2) == BLOBMSG_TYPE_STRING)) diff --git a/service/instance.h b/service/instance.h index 3fb33e9..ab05f20 100644 --- a/service/instance.h +++ b/service/instance.h @@ -20,7 +20,17 @@ #include <libubox/ustream.h> #include "../utils/utils.h" -#define RESPAWN_ERROR (5 * 60) +#ifndef RESPAWN_THRESHOLD_DEFAULT +#define RESPAWN_THRESHOLD_DEFAULT 3600 +#endif + +#ifndef RESPAWN_TIMEOUT_DEFAULT +#define RESPAWN_TIMEOUT_DEFAULT 5 +#endif + +#ifndef RESPAWN_RETRY_DEFAULT +#define RESPAWN_RETRY_DEFAULT 5 +#endif struct jail { bool procfs;
Allow to pass RESPAWN_THESHOLD_DEFAULT, DRESPAWN_TIMEOUT_DEFAULT and RESPAWN_RETRY_DEFAULT as parameters to cmake to change the default respawn behavior. This can be used to set the default respawn mode to infinite retries for example. Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com> --- CMakeLists.txt | 12 ++++++++++++ service/instance.c | 4 +++- service/instance.h | 12 +++++++++++- 3 files changed, 26 insertions(+), 2 deletions(-)