[SSSD] [PATCH] Restart services with a delay in case they are restarted too often

Pavel Březina pbrezina at redhat.com
Mon Nov 19 12:29:39 UTC 2012


On 11/18/2012 06:09 PM, Jakub Hrozek wrote:
> On Fri, Nov 16, 2012 at 11:41:50AM +0100, Pavel Březina wrote:
>> On 11/15/2012 07:51 PM, Jakub Hrozek wrote:
>>> In case a service is restarted while the DP is not ready yet, it gets
>>> restarted again immediatelly, which means the DP might still not be
>>> ready. The allowed number of restarts is then depleted quickly.
>>>
>>> This patch changes the restart mechanism such that the first restart
>>> happens immediatelly, the second is scheduled after 2 second, then 4
>>> etc..
>>>
>>> https://fedorahosted.org/sssd/ticket/1528
>>
>> Hi,
>>
>>> +    /* restarts are schedule after 0, 2, 4, ...seconds */
>>> +    tv = tevent_timeval_current_ofs((svc->restarts << 1), 0);
>>> +    te = tevent_add_timer(svc->mt_ctx->ev, svc, tv, mt_svc_restart, svc);
>>
>> I would consider setting 4 seconds as a maximum delay instead of
>> going to infinity. Any opinions?
>
> I agree. I wanted to make the delays self-configurable but setting an
> upper limit is the right thing to do.
>
>>
>>> +static void mt_svc_restart(struct tevent_context *ev,
>>> +                           struct tevent_timer *te,
>>> +                           struct timeval t, void *ptr)
>>> +{
>>> +    struct mt_svc *svc;
>>> +
>>> +    svc = talloc_get_type(ptr, struct mt_svc);
>>> +    if (svc == NULL) {
>>> +        return;
>>> +    }
>>> +
>>> +    DEBUG(SSSDBG_TRACE_FUNC,
>>> +          ("Scheduling a service for restart %d\n", svc->restarts));
>>
>> It should say svc->restarts + 1. Can you also add a service name to the
>> message?
>
> Done and done. Thank you for the review.

Hi,
nitpicks...

> +/* TODO: get the restart related values from config */
> +#define MONITOR_RESTART_CNT_RESET   30

I had some troubles interpreting this macro. Can you change the name in
such way that it is more clear that we are resetting restart counter 
after 30 seconds? Maybe
MONITOR_RESET_RESTART_COUNTER_INTERVAL

> +    /* restarts are schedule after 0, 2, 4 seconds */
> +    restart_delay = svc->restarts << 1;
> +    restart_delay = (restart_delay > MONITOR_MAX_RESTART_DELAY) ? \
> +                    MONITOR_MAX_RESTART_DELAY : \
> +                    restart_delay;

I think this is easier to read:

/* restarts are schedule after 0, 2, 4 seconds */
restart_delay = svc->restarts << 1;
if (restart_delay > MONITOR_MAX_RESTART_DELAY) {
     restart_delay = MONITOR_MAX_RESTART_DELAY;
}




More information about the sssd-devel mailing list