Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-04-12 Thread Michael Olbrich 
On Fri, Apr 11, 2014 at 04:07:46PM +0200, Lennart Poettering wrote:
 On Fri, 11.04.14 09:48, Michael Olbrich (m.olbr...@pengutronix.de) wrote:
+else if (allow_restart 
   
   I would drop the else here, I think. Is there a reason not to do the
   restart thing anyway? If it is configured, it should run I think, just
   in case the failure action doesn't work or so...
  
  Are you sure? With Restart=always and FailureAction=reboot this would
  try to start the unit while shutting down. Will the Conflicts= from the
  default dependencies handle this correctly?
 
 It should handle this correctly, and if it doesn't we should fix this. I
 mean, my thinking here is that combining FailureAction= and
 Restart=failure might not make much sense but there isn't really a
 strong reason to totally prohibit it...

With lots of debugging enabled it looks like this:

wd-test.service failed, rebooting.
Trying to enqueue job reboot.target/start/replace
Installed new job reboot.target/start as 157
Installed new job systemd-reboot.service/start as 158
Installed new job shutdown.target/start as 159
[...]
wd-test.service changed failed - auto-restart
wd-test.service: cgroup is empty
Accepted new private connection.
wd-test.service holdoff time over, scheduling restart.
Trying to enqueue job wd-test.service/restart/fail
[...]
wd-test.service failed to schedule restart job: Transaction is destructive.
wd-test.service changed auto-restart - failed
Unit wd-test.service entered failed state.
wd-test.service failed, rebooting.
Trying to enqueue job reboot.target/start/replace
Merged into installed job reboot.target/start as 157
Merged into installed job systemd-reboot.service/start as 158
Merged into installed job shutdown.target/start as 159
[...]

The restart is canceled and the new reboot jobs are merged into the already
running jobs. I'd say the falls under 'handle this correctly'.
I'll send out a new patch when I'm done testing.

Michael

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-04-11 Thread Michael Olbrich 
On Fri, Apr 11, 2014 at 03:34:42AM +0200, Lennart Poettering wrote:
 On Wed, 26.03.14 10:02, Michael Olbrich (m.olbr...@pengutronix.de) wrote:
 
  It has the same possible values as StartLimitAction= and is executed
  immediately if a service fails.
 
 I think the enum type should probably be renamed to FailureAction, since
 that now sounds like the more generic name. 

Ok.

  ---
  
  Hi Lennart,
  
  Something like this maybe? I'm not quite sure about the condition in
  service_enter_dead(). I don't think the action should be executed when the
  service is explicitly stopped. Maybe it should depend on !forbid_restart?
  
  If you like, I'll add some documentation. An maybe a follow-up patch to
  rename the StartLimitAction type? To what though?
 
  index ae3695a..ab161a5 100644
  --- a/src/core/service.c
  +++ b/src/core/service.c
  @@ -1835,6 +1835,8 @@ static int cgroup_good(Service *s) {
   return !r;
   }
   
  +static int service_execute_action(Service *s, StartLimitAction action, 
  const char *reason);
  +
   static void service_enter_dead(Service *s, ServiceResult f, bool 
  allow_restart) {
   int r;
   assert(s);
  @@ -1844,7 +1846,9 @@ static void service_enter_dead(Service *s, 
  ServiceResult f, bool allow_restart)
   
   service_set_state(s, s-result != SERVICE_SUCCESS ? SERVICE_FAILED 
  : SERVICE_DEAD);
   
  -if (allow_restart 
  +if (s-result != SERVICE_SUCCESS  s-failure_action != 
  SERVICE_START_LIMIT_NONE)
  +service_execute_action(s, s-failure_action,
  failed);
 
 I'd prefer to move the check for SERVICE_START_LIMIT_NONE to the top of 
 service_execute_action().

Hmmm, the check is already there, but for the start limit it produces a
warning. I'll see if I can find a nice way to handle this.

  +else if (allow_restart 
 
 I would drop the else here, I think. Is there a reason not to do the
 restart thing anyway? If it is configured, it should run I think, just
 in case the failure action doesn't work or so...

Are you sure? With Restart=always and FailureAction=reboot this would
try to start the unit while shutting down. Will the Conflicts= from the
default dependencies handle this correctly?

Regards,
Michael

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-04-11 Thread Lennart Poettering 
On Fri, 11.04.14 09:48, Michael Olbrich (m.olbr...@pengutronix.de) wrote:

   +else if (allow_restart 
  
  I would drop the else here, I think. Is there a reason not to do the
  restart thing anyway? If it is configured, it should run I think, just
  in case the failure action doesn't work or so...
 
 Are you sure? With Restart=always and FailureAction=reboot this would
 try to start the unit while shutting down. Will the Conflicts= from the
 default dependencies handle this correctly?

It should handle this correctly, and if it doesn't we should fix this. I
mean, my thinking here is that combining FailureAction= and
Restart=failure might not make much sense but there isn't really a
strong reason to totally prohibit it...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-04-10 Thread Lennart Poettering 
On Wed, 26.03.14 10:02, Michael Olbrich (m.olbr...@pengutronix.de) wrote:

 It has the same possible values as StartLimitAction= and is executed
 immediately if a service fails.

I think the enum type should probably be renamed to FailureAction, since
that now sounds like the more generic name. 

 ---
 
 Hi Lennart,
 
 Something like this maybe? I'm not quite sure about the condition in
 service_enter_dead(). I don't think the action should be executed when the
 service is explicitly stopped. Maybe it should depend on !forbid_restart?
 
 If you like, I'll add some documentation. An maybe a follow-up patch to
 rename the StartLimitAction type? To what though?

 index ae3695a..ab161a5 100644
 --- a/src/core/service.c
 +++ b/src/core/service.c
 @@ -1835,6 +1835,8 @@ static int cgroup_good(Service *s) {
  return !r;
  }
  
 +static int service_execute_action(Service *s, StartLimitAction action, const 
 char *reason);
 +
  static void service_enter_dead(Service *s, ServiceResult f, bool 
 allow_restart) {
  int r;
  assert(s);
 @@ -1844,7 +1846,9 @@ static void service_enter_dead(Service *s, 
 ServiceResult f, bool allow_restart)
  
  service_set_state(s, s-result != SERVICE_SUCCESS ? SERVICE_FAILED : 
 SERVICE_DEAD);
  
 -if (allow_restart 
 +if (s-result != SERVICE_SUCCESS  s-failure_action != 
 SERVICE_START_LIMIT_NONE)
 +service_execute_action(s, s-failure_action,
 failed);

I'd prefer to move the check for SERVICE_START_LIMIT_NONE to the top of 
service_execute_action().

 +else if (allow_restart 

I would drop the else here, I think. Is there a reason not to do the
restart thing anyway? If it is configured, it should run I think, just
in case the failure action doesn't work or so...

Otherwise looks pretty good.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-03-26 Thread Michael Olbrich 
It has the same possible values as StartLimitAction= and is executed
immediately if a service fails.
---

Hi Lennart,

Something like this maybe? I'm not quite sure about the condition in
service_enter_dead(). I don't think the action should be executed when the
service is explicitly stopped. Maybe it should depend on !forbid_restart?

If you like, I'll add some documentation. An maybe a follow-up patch to
rename the StartLimitAction type? To what though?

Regards,
Michael

 src/core/dbus-service.c   |  1 +
 src/core/load-fragment-gperf.gperf.m4 |  1 +
 src/core/service.c| 31 ---
 src/core/service.h|  2 ++
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c
index 0451790..1e710e3 100644
--- a/src/core/dbus-service.c
+++ b/src/core/dbus-service.c
@@ -50,6 +50,7 @@ const sd_bus_vtable bus_service_vtable[] = {
 SD_BUS_PROPERTY(StartLimitInterval, t, bus_property_get_usec, 
offsetof(Service, start_limit.interval), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(StartLimitBurst, u, bus_property_get_unsigned, 
offsetof(Service, start_limit.burst), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(StartLimitAction, s, 
property_get_start_limit_action, offsetof(Service, start_limit_action), 
SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(FailureAction, s, property_get_start_limit_action, 
offsetof(Service, failure_action), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(PermissionsStartOnly, b, bus_property_get_bool, 
offsetof(Service, permissions_start_only), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(RootDirectoryStartOnly, b, bus_property_get_bool, 
offsetof(Service, root_directory_start_only), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(RemainAfterExit, b, bus_property_get_bool, 
offsetof(Service, remain_after_exit), SD_BUS_VTABLE_PROPERTY_CONST),
diff --git a/src/core/load-fragment-gperf.gperf.m4 
b/src/core/load-fragment-gperf.gperf.m4
index dbb5d13..dc83247 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -180,6 +180,7 @@ Service.WatchdogSec, config_parse_sec,  
 0,
 Service.StartLimitInterval,  config_parse_sec,   0,
 offsetof(Service, start_limit.interval)
 Service.StartLimitBurst, config_parse_unsigned,  0,
 offsetof(Service, start_limit.burst)
 Service.StartLimitAction,config_parse_start_limit_action,0,
 offsetof(Service, start_limit_action)
+Service.FailureAction,   config_parse_start_limit_action,0,
 offsetof(Service, failure_action)
 Service.Type,config_parse_service_type,  0,
 offsetof(Service, type)
 Service.Restart, config_parse_service_restart,   0,
 offsetof(Service, restart)
 Service.PermissionsStartOnly,config_parse_bool,  0,
 offsetof(Service, permissions_start_only)
diff --git a/src/core/service.c b/src/core/service.c
index ae3695a..ab161a5 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1835,6 +1835,8 @@ static int cgroup_good(Service *s) {
 return !r;
 }
 
+static int service_execute_action(Service *s, StartLimitAction action, const 
char *reason);
+
 static void service_enter_dead(Service *s, ServiceResult f, bool 
allow_restart) {
 int r;
 assert(s);
@@ -1844,7 +1846,9 @@ static void service_enter_dead(Service *s, ServiceResult 
f, bool allow_restart)
 
 service_set_state(s, s-result != SERVICE_SUCCESS ? SERVICE_FAILED : 
SERVICE_DEAD);
 
-if (allow_restart 
+if (s-result != SERVICE_SUCCESS  s-failure_action != 
SERVICE_START_LIMIT_NONE)
+service_execute_action(s, s-failure_action, failed);
+else if (allow_restart 
 !s-forbid_restart 
 (s-restart == SERVICE_RESTART_ALWAYS ||
  (s-restart == SERVICE_RESTART_ON_SUCCESS  s-result == 
SERVICE_SUCCESS) ||
@@ -2366,18 +2370,14 @@ fail:
 service_enter_stop(s, SERVICE_FAILURE_RESOURCES);
 }
 
-static int service_start_limit_test(Service *s) {
+static int service_execute_action(Service *s, StartLimitAction action, const 
char *reason) {
 assert(s);
 
-if (ratelimit_test(s-start_limit))
-return 0;
-
-switch (s-start_limit_action) {
+switch (action) {
 
 case SERVICE_START_LIMIT_NONE:
 log_warning_unit(UNIT(s)-id,
- %s start request repeated too quickly, 
refusing to start.,
- UNIT(s)-id);
+ %s %s, refusing to start., UNIT(s)-id, 
reason);

Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-03-26 Thread Jóhann B. Guðmundsson 


On 03/26/2014 09:02 AM, Michael Olbrich wrote:

It has the same possible values as StartLimitAction= and is executed
immediately if a service fails.
---

Hi Lennart,

Something like this maybe? I'm not quite sure about the condition in
service_enter_dead(). I don't think the action should be executed when the
service is explicitly stopped. Maybe it should depend on !forbid_restart?

If you like, I'll add some documentation. An maybe a follow-up patch to
rename the StartLimitAction type? To what though?


Dont we already have OnFailure= for this ( or can be used for this 
instead of introducing FailureAction= which can be confusing to users )?


JBG
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC PATCH] service: add FailureAction= option

2014-03-26 Thread Michael Olbrich 
On Wed, Mar 26, 2014 at 10:19:53AM +, Jóhann B. Guðmundsson wrote:
 On 03/26/2014 09:02 AM, Michael Olbrich wrote:
 It has the same possible values as StartLimitAction= and is executed
 immediately if a service fails.
 ---
 
 Hi Lennart,
 
 Something like this maybe? I'm not quite sure about the condition in
 service_enter_dead(). I don't think the action should be executed when the
 service is explicitly stopped. Maybe it should depend on !forbid_restart?
 
 If you like, I'll add some documentation. An maybe a follow-up patch to
 rename the StartLimitAction type? To what though?
 
 Dont we already have OnFailure= for this ( or can be used for this
 instead of introducing FailureAction= which can be confusing to
 users )?

No. Take a look at the possible actions. This is intended to be used e.g.
in combination with watchdogs. A unit that may or may not start cannot
express 'reboot right now'.

Michael

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel