proc_loadavg: Sleep in loadavg thread should not slow down reloads

[DeyanSG]

This resolves the issue with reload described in #678

[Blub]

The signal handler will set need_reload=1, which is a bit of a weird side effect.
It should be okay, since when we reach this code, we're either in do_reload() where that's already the case, or in main() during shutdown, but personally I don't like depending on this kind of knowledge, as it is a maintenance burden.
(I wonder if we should introduce a more specific signal there (SIGALRM is common to wake up sleeps I think, or some RT signal)

[mihalicyn]

Hello @DeyanSG,

first of all, thanks for your report and proposed fix!

From #678:

This led us to conclude that we missed the wake-up time for the timer started by the usleep in the loadavg thread. We calculated that it would take about 70 minutes for the timer to overflow and reach the same value again, allowing normal operations to continue.

you are referring to this part of code:

	clock_t time1, time2;

	for (;;) {
		if (loadavg_stop == 1)
			return NULL;

		time1 = clock();
<...>
		if (loadavg_stop == 1)
			return NULL;

		time2 = clock();
		usleep(FLUSH_TIME * 1000000 -
		       (int)((time2 - time1) * 1000000 / CLOCKS_PER_SEC));

right?

Please, can you test the following simple fix:

--- a/src/proc_loadavg.c
+++ b/src/proc_loadavg.c
@@ -504,6 +504,7 @@ static void *load_begin(void *arg)
        int first_node, sum;
        struct load_node *f;
        clock_t time1, time2;
+       int sleep_time;
 
        for (;;) {
                if (loadavg_stop == 1)
@@ -542,8 +543,9 @@ static void *load_begin(void *arg)
                        return NULL;
 
                time2 = clock();
-               usleep(FLUSH_TIME * 1000000 -
-                      (int)((time2 - time1) * 1000000 / CLOCKS_PER_SEC));
+               sleep_time = FLUSH_TIME - (int)((time2 - time1) / CLOCKS_PER_SEC);
+               if ((sleep_time > 0) && (sleep_time <= FLUSH_TIME))
+                       usleep(sleep_time * 1000000);
        }
 }

Kind regards,
Alex

[Blub]

Ah yes, catching the overflow definitely makes sense. We'd still be a bit delayed, but that's much less problematic than a near-endless sleep call and can still be addressed separately.

[DeyanSG]

Hi @mihalicyn and @Blub,

Thanks for looking into this.

It seems I somehow missed the overflow in the code and went on to look into the kernel code. Thanks for spotting this, @mihalicyn. We will deploy a patched version today that includes the suggested fix and some additional logging in case we skip the sleep, so we can see all the values.

The issue is tricky to reproduce, as we do not trigger these live migrations. They are part of some GCE maintenance, and so far, the issue during a migration seems to occur rarely. Nevertheless, we will do our best to test and see if we can determine whether this patch will solve the issue.

@Blub, I tested with SIGALRM and SIGVTALRM. However, sending these to the thread seems to kill the whole process. I am also not sure how good of an idea it is to set a custom handler for these signals. I also tested with SIGUSR2, and even without installing a handler for this signal, it seems to work correctly. Let me know if you are keen on merging this with SIGUSR2 (or another signal), or if we will stick to just the solution provided by @mihalicyn if it works.

Regards,
Deyan

[Blub]

SIGUSR2 is already used up by existing lxcfs code.

I think installing a handler for SIGALRM should be fine, we can choose ourselves how such signals should be dealt with, and SIGALRM is commonly for timeouts (eg. using the alarm(2) call which is meant to interrupt (without killing the process). (Eg. there are no file-locking methods with a timeout, so the only way to try such locks with a time out is to set a timer or alarm or emulate such a thing via threads+sleep)).
If we ever need that behavior we'd want to use timers (timer_create(2)) instead of alarm(2) anyway, as with timers you can target specific threads.

[DeyanSG]

Hi @Blub ,

I've modified it to use SIGALRM as suggested and did a quick test. It seems to work fine this way.

Feel free to suggest more improvements.

Regards,
Deyan