[pve-devel] [PATCH qemu-server v2 0/3] qmeventd: improve shutdown behaviour

[pve-devel] [PATCH qemu-server v2 0/3] qmeventd: improve shutdown behaviour

[Dominik Csapak]

includes the following improvements:
* increases 'force cleanup' timeout to 60s (from 5)
* saves individual timeout for each vm
* don't force cleanup for vms where normal cleanup worked
* sending QMP quit instead of SIGTERM (less log noise)

i opted for variant 1 of wbumillers suggestions, as it yielded the least
change and still results in clean code

changes from v1:
* remove 'alarm' calls altogether and use epoll_waits' timeout mechanic
   instead
* call 'time()' only once and give it as user data to the function
* change the function singatures and cast on callsite with '(GFunc)'
   for the g_slist_foreach calls
* change to <s> for the usage output for timeouts

Dominik Csapak (3):
  qmeventd: rework 'forced_cleanup' handling and set timeout to 60s
  qmeventd: cancel 'forced cleanup' when normal cleanup succeeds
  qmeventd: send QMP 'quit' command instead of SIGTERM

 qmeventd/qmeventd.c | 104 +++++++++++++++++++++++++++-----------------
 qmeventd/qmeventd.h |   2 +
 2 files changed, 67 insertions(+), 39 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server v2 1/3] qmeventd: rework 'forced_cleanup' handling and set timeout to 60s

[Dominik Csapak]

currently, the 'forced_cleanup' (sending SIGKILL to the qemu process),
is intended to be triggered 5 seconds after sending the initial shutdown
signal (SIGTERM) which is sadly not enough for some setups.

Accidentally, it could be triggered earlier than 5 seconds, if a
SIGALRM triggers in the timespan directly before setting it again.

Also, this approach means that depending on when machines are shutdown
their forced cleanup may happen after 5 seconds, or any time after, if
new vms are shut off in the meantime.

Improve this situation by reworking the way we deal with this cleanup.
We save the time incl. timeout in the CleanupData, and set a timeout
to 'epoll_wait' of 10 seconds, which will then trigger a forced_cleanup.
Remove entries from the forced_cleanup list when that entry is killed,
or when the normal cleanup took place.

To improve the shutdown behaviour, increase the default timeout to 60
seconds, which should be enough, but add a commandline toggle where
users can set it to a different value.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qmeventd/qmeventd.c | 73 +++++++++++++++++++++++----------------------
 qmeventd/qmeventd.h |  2 ++
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
index 8d32827..46bc7eb 100644
--- a/qmeventd/qmeventd.c
+++ b/qmeventd/qmeventd.c
@@ -29,6 +29,7 @@
 #include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <sys/epoll.h>
 #include <sys/socket.h>
@@ -36,15 +37,19 @@
 #include <sys/un.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <time.h>
 
 #include "qmeventd.h"
 
+#define DEFAULT_KILL_TIMEOUT 60
+
 static int verbose = 0;
+static int kill_timeout = DEFAULT_KILL_TIMEOUT;
 static int epoll_fd = 0;
 static const char *progname;
 GHashTable *vm_clients; // key=vmid (freed on remove), value=*Client (free manually)
 GSList *forced_cleanups;
-volatile sig_atomic_t alarm_triggered = 0;
+static int needs_cleanup = 0;
 
 /*
  * Helper functions
@@ -56,6 +61,7 @@ usage()
     fprintf(stderr, "Usage: %s [-f] [-v] PATH\n", progname);
     fprintf(stderr, "  -f       run in foreground (default: false)\n");
     fprintf(stderr, "  -v       verbose (default: false)\n");
+    fprintf(stderr, "  -t <s>   kill timeout (default: %ds)\n", DEFAULT_KILL_TIMEOUT);
     fprintf(stderr, "  PATH     use PATH for socket\n");
 }
 
@@ -469,16 +475,17 @@ terminate_client(struct Client *client)
     int err = kill(client->pid, SIGTERM);
     log_neg(err, "kill");
 
+    time_t timeout = time(NULL) + kill_timeout;
+
     struct CleanupData *data_ptr = malloc(sizeof(struct CleanupData));
     struct CleanupData data = {
 	.pid = client->pid,
-	.pidfd = pidfd
+	.pidfd = pidfd,
+	.timeout = timeout,
     };
     *data_ptr = data;
     forced_cleanups = g_slist_prepend(forced_cleanups, (void *)data_ptr);
-
-    // resets any other alarms, but will fire eventually and cleanup all
-    alarm(5);
+    needs_cleanup = 1;
 }
 
 void
@@ -551,27 +558,16 @@ handle_client(struct Client *client)
     json_tokener_free(tok);
 }
 
-
-/*
- * SIGALRM and cleanup handling
- *
- * terminate_client will set an alarm for 5 seconds and add its client's PID to
- * the forced_cleanups list - when the timer expires, we iterate the list and
- * attempt to issue SIGKILL to all processes which haven't yet stopped.
- */
-
-static void
-alarm_handler(__attribute__((unused)) int signum)
-{
-    alarm_triggered = 1;
-}
-
 static void
-sigkill(void *ptr, __attribute__((unused)) void *unused)
+sigkill(struct CleanupData *ptr, time_t *cur_time)
 {
-    struct CleanupData data = *((struct CleanupData *)ptr);
+    struct CleanupData data = *ptr;
     int err;
 
+    if (data.timeout > *cur_time) {
+	return;
+    }
+
     if (data.pidfd > 0) {
 	err = pidfd_send_signal(data.pidfd, SIGKILL, NULL, 0);
 	(void)close(data.pidfd);
@@ -588,21 +584,23 @@ sigkill(void *ptr, __attribute__((unused)) void *unused)
 	fprintf(stderr, "cleanup failed, terminating pid '%d' with SIGKILL\n",
 		data.pid);
     }
+
+    // remove ourselves from the list
+    forced_cleanups = g_slist_remove(forced_cleanups, ptr);
+    free(ptr);
 }
 
 static void
 handle_forced_cleanup()
 {
-    if (alarm_triggered) {
+    if (g_slist_length(forced_cleanups) > 0) {
 	VERBOSE_PRINT("clearing forced cleanup backlog\n");
-	alarm_triggered = 0;
-	g_slist_foreach(forced_cleanups, sigkill, NULL);
-	g_slist_free_full(forced_cleanups, free);
-	forced_cleanups = NULL;
+	time_t cur_time = time(NULL);
+	g_slist_foreach(forced_cleanups, (GFunc)sigkill, &cur_time);
     }
+    needs_cleanup = g_slist_length(forced_cleanups) > 0;
 }
 
-
 int
 main(int argc, char *argv[])
 {
@@ -611,7 +609,7 @@ main(int argc, char *argv[])
     char *socket_path = NULL;
     progname = argv[0];
 
-    while ((opt = getopt(argc, argv, "hfv")) != -1) {
+    while ((opt = getopt(argc, argv, "hfvt:")) != -1) {
 	switch (opt) {
 	    case 'f':
 		daemonize = 0;
@@ -619,6 +617,15 @@ main(int argc, char *argv[])
 	    case 'v':
 		verbose = 1;
 		break;
+	    case 't':
+		errno = 0;
+		char *endptr = NULL;
+		kill_timeout = strtoul(optarg, &endptr, 10);
+		if (errno != 0 || *endptr != '\0' || kill_timeout == 0) {
+		    usage();
+		    exit(EXIT_FAILURE);
+		}
+		break;
 	    case 'h':
 		usage();
 		exit(EXIT_SUCCESS);
@@ -635,7 +642,6 @@ main(int argc, char *argv[])
     }
 
     signal(SIGCHLD, SIG_IGN);
-    signal(SIGALRM, alarm_handler);
 
     socket_path = argv[optind];
 
@@ -669,11 +675,7 @@ main(int argc, char *argv[])
     int nevents;
 
     for(;;) {
-	nevents = epoll_wait(epoll_fd, events, 1, -1);
-	if (nevents < 0 && errno == EINTR) {
-	    handle_forced_cleanup();
-	    continue;
-	}
+	nevents = epoll_wait(epoll_fd, events, 1, needs_cleanup ? 10*1000 : -1);
 	bail_neg(nevents, "epoll_wait");
 
 	for (int n = 0; n < nevents; n++) {
@@ -688,7 +690,6 @@ main(int argc, char *argv[])
 		handle_client((struct Client *)events[n].data.ptr);
 	    }
 	}
-
 	handle_forced_cleanup();
     }
 }
diff --git a/qmeventd/qmeventd.h b/qmeventd/qmeventd.h
index 2cf1947..1b9cea1 100644
--- a/qmeventd/qmeventd.h
+++ b/qmeventd/qmeventd.h
@@ -7,6 +7,7 @@
 */
 
 #include <sys/syscall.h>
+#include <time.h>
 
 #ifndef __NR_pidfd_open
 #define __NR_pidfd_open 434
@@ -86,6 +87,7 @@ struct Client {
 struct CleanupData {
     pid_t pid;
     int pidfd;
+    time_t timeout;
 };
 
 void handle_qmp_handshake(struct Client *client);
-- 
2.30.2

[pve-devel] [PATCH qemu-server v2 2/3] qmeventd: cancel 'forced cleanup' when normal cleanup succeeds

[Dominik Csapak]

instead of always sending a SIGKILL to the target pid.
It was not that much of a problem since the timeout previously was 5
seconds and we used pifds where possible, thus the chance of killing the
wrong process was rather slim.

Now we increased the timeout to 60s which makes the race a bit more likely
(when not using pidfds), so remove it from the 'forced_cleanups' list when
the normal cleanup succeeds.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qmeventd/qmeventd.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
index 46bc7eb..eebc19d 100644
--- a/qmeventd/qmeventd.c
+++ b/qmeventd/qmeventd.c
@@ -416,6 +416,22 @@ cleanup_qemu_client(struct Client *client)
     }
 }
 
+static void
+remove_cleanup_data(struct CleanupData *data, struct Client *client) {
+    if (data->pid == client->pid) {
+	forced_cleanups = g_slist_remove(forced_cleanups, data);
+	free(data);
+    }
+}
+
+static void
+remove_from_forced_cleanup(struct Client *client) {
+    if (g_slist_length(forced_cleanups) > 0) {
+	VERBOSE_PRINT("removing %s from forced cleanups\n", client->qemu.vmid);
+	g_slist_foreach(forced_cleanups, (GFunc)remove_cleanup_data, client);
+    }
+}
+
 void
 cleanup_client(struct Client *client)
 {
@@ -442,6 +458,7 @@ cleanup_client(struct Client *client)
 	    break;
     }
 
+    remove_from_forced_cleanup(client);
     free(client);
 }
 
-- 
2.30.2

[pve-devel] [PATCH qemu-server v2 3/3] qmeventd: send QMP 'quit' command instead of SIGTERM

[Dominik Csapak]

this is functionally the same, but sending SIGTERM has the ugly side
effect of printing the following to the log:

> QEMU[<pid>]: kvm: terminating on signal 15 from pid <pid> (/usr/sbin/qmeventd)

while sending a QMP quit command does not.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qmeventd/qmeventd.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
index eebc19d..75490f4 100644
--- a/qmeventd/qmeventd.c
+++ b/qmeventd/qmeventd.c
@@ -287,8 +287,10 @@ handle_qmp_return(struct Client *client, struct json_object *data, bool error)
 	    VERBOSE_PRINT("%s: QMP handshake complete\n", client->qemu.vmid);
 	    break;
 
-	case STATE_IDLE:
+	// we expect an empty return object after sending quit
 	case STATE_TERMINATING:
+	    break;
+	case STATE_IDLE:
 	    VERBOSE_PRINT("%s: spurious return value received\n",
 			  client->qemu.vmid);
 	    break;
@@ -489,8 +491,14 @@ terminate_client(struct Client *client)
 	}
     }
 
-    int err = kill(client->pid, SIGTERM);
-    log_neg(err, "kill");
+    // try to send a 'quit' command first, fallback to SIGTERM of the pid
+    static const char qmp_quit_command[] = "{\"execute\":\"quit\"}\n";
+    VERBOSE_PRINT("%s: sending 'quit' via QMP\n", client->qemu.vmid);
+    if (!must_write(client->fd, qmp_quit_command, sizeof(qmp_quit_command) - 1)) {
+	VERBOSE_PRINT("%s: sending 'SIGTERM' to pid %d\n", client->qemu.vmid, client->pid);
+	int err = kill(client->pid, SIGTERM);
+	log_neg(err, "kill");
+    }
 
     time_t timeout = time(NULL) + kill_timeout;
 
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server v2 1/3] qmeventd: rework 'forced_cleanup' handling and set timeout to 60s

[Wolfgang Bumiller]

On Thu, Sep 22, 2022 at 04:19:33PM +0200, Dominik Csapak wrote:
> currently, the 'forced_cleanup' (sending SIGKILL to the qemu process),
> is intended to be triggered 5 seconds after sending the initial shutdown
> signal (SIGTERM) which is sadly not enough for some setups.
> 
> Accidentally, it could be triggered earlier than 5 seconds, if a
> SIGALRM triggers in the timespan directly before setting it again.
> 
> Also, this approach means that depending on when machines are shutdown
> their forced cleanup may happen after 5 seconds, or any time after, if
> new vms are shut off in the meantime.
> 
> Improve this situation by reworking the way we deal with this cleanup.
> We save the time incl. timeout in the CleanupData, and set a timeout
> to 'epoll_wait' of 10 seconds, which will then trigger a forced_cleanup.
> Remove entries from the forced_cleanup list when that entry is killed,
> or when the normal cleanup took place.
> 
> To improve the shutdown behaviour, increase the default timeout to 60
> seconds, which should be enough, but add a commandline toggle where
> users can set it to a different value.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  qmeventd/qmeventd.c | 73 +++++++++++++++++++++++----------------------
>  qmeventd/qmeventd.h |  2 ++
>  2 files changed, 39 insertions(+), 36 deletions(-)
> 
> diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
> index 8d32827..46bc7eb 100644
> --- a/qmeventd/qmeventd.c
> +++ b/qmeventd/qmeventd.c
> @@ -551,27 +558,16 @@ handle_client(struct Client *client)
>      json_tokener_free(tok);
>  }
>  
> -
> -/*
> - * SIGALRM and cleanup handling
> - *
> - * terminate_client will set an alarm for 5 seconds and add its client's PID to
> - * the forced_cleanups list - when the timer expires, we iterate the list and
> - * attempt to issue SIGKILL to all processes which haven't yet stopped.
> - */
> -
> -static void
> -alarm_handler(__attribute__((unused)) int signum)
> -{
> -    alarm_triggered = 1;
> -}
> -
>  static void
> -sigkill(void *ptr, __attribute__((unused)) void *unused)

If you change the style here... (which I'm not a fan of btw.)

> +sigkill(struct CleanupData *ptr, time_t *cur_time)
>  {
> -    struct CleanupData data = *((struct CleanupData *)ptr);
> +    struct CleanupData data = *ptr;

...at least get rid of this line completely ^
and just use `ptr->` instea of `data.`, I see no reason to keep copying
the data onto the stack?
(or with the old style, make `data` a pointer and skip the cast)

>      int err;
>  
> +    if (data.timeout > *cur_time) {
> +	return;
> +    }
> +
>      if (data.pidfd > 0) {
>  	err = pidfd_send_signal(data.pidfd, SIGKILL, NULL, 0);
>  	(void)close(data.pidfd);

Re: [pve-devel] [PATCH qemu-server v2 2/3] qmeventd: cancel 'forced cleanup' when normal cleanup succeeds

[Wolfgang Bumiller]

On Thu, Sep 22, 2022 at 04:19:34PM +0200, Dominik Csapak wrote:
> instead of always sending a SIGKILL to the target pid.
> It was not that much of a problem since the timeout previously was 5
> seconds and we used pifds where possible, thus the chance of killing the
> wrong process was rather slim.
> 
> Now we increased the timeout to 60s which makes the race a bit more likely
> (when not using pidfds), so remove it from the 'forced_cleanups' list when
> the normal cleanup succeeds.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  qmeventd/qmeventd.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
> index 46bc7eb..eebc19d 100644
> --- a/qmeventd/qmeventd.c
> +++ b/qmeventd/qmeventd.c
> @@ -416,6 +416,22 @@ cleanup_qemu_client(struct Client *client)
>      }
>  }
>  
> +static void
> +remove_cleanup_data(struct CleanupData *data, struct Client *client) {
> +    if (data->pid == client->pid) {
> +	forced_cleanups = g_slist_remove(forced_cleanups, data);
> +	free(data);
> +    }
> +}
> +
> +static void
> +remove_from_forced_cleanup(struct Client *client) {
> +    if (g_slist_length(forced_cleanups) > 0) {
> +	VERBOSE_PRINT("removing %s from forced cleanups\n", client->qemu.vmid);
> +	g_slist_foreach(forced_cleanups, (GFunc)remove_cleanup_data, client);

Foreach + remove feels awkward to me. Sure, it's a linked list and
should be fine™, but I don't like the lack of documentation of
interactions here as a non-glib user. (I mean, eg. for C++ iterator
invalidation is *usually* documented...)

Can't we just give `struct Client` a `struct CleanupData` pointer and
call `g_slist_remove` right here without the iteration?

Or better yet, your previous idea of dropping `CleanupData` sounds
better.
We should be able to just add `struct Client*` to the list, after all,
according to the glib docs `g_slist_remove` should simply leave the list
unchanged if the data is not part of the list, so when we free up the
`Client` we could even call `g_slist_remove` unconditionally (though
we'll know whether it's in there by having a timeout set then...)

(or use `g_slist_find_custom`)

> +    }
> +}
> +
>  void
>  cleanup_client(struct Client *client)
>  {
> @@ -442,6 +458,7 @@ cleanup_client(struct Client *client)
>  	    break;
>      }
>  
> +    remove_from_forced_cleanup(client);
>      free(client);
>  }
>  
> -- 
> 2.30.2

Re: [pve-devel] [PATCH qemu-server v2 2/3] qmeventd: cancel 'forced cleanup' when normal cleanup succeeds

[Dominik Csapak]

On 9/23/22 10:31, Wolfgang Bumiller wrote:
> On Thu, Sep 22, 2022 at 04:19:34PM +0200, Dominik Csapak wrote:
>> instead of always sending a SIGKILL to the target pid.
>> It was not that much of a problem since the timeout previously was 5
>> seconds and we used pifds where possible, thus the chance of killing the
>> wrong process was rather slim.
>>
>> Now we increased the timeout to 60s which makes the race a bit more likely
>> (when not using pidfds), so remove it from the 'forced_cleanups' list when
>> the normal cleanup succeeds.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   qmeventd/qmeventd.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
>> index 46bc7eb..eebc19d 100644
>> --- a/qmeventd/qmeventd.c
>> +++ b/qmeventd/qmeventd.c
>> @@ -416,6 +416,22 @@ cleanup_qemu_client(struct Client *client)
>>       }
>>   }
>>   
>> +static void
>> +remove_cleanup_data(struct CleanupData *data, struct Client *client) {
>> +    if (data->pid == client->pid) {
>> +	forced_cleanups = g_slist_remove(forced_cleanups, data);
>> +	free(data);
>> +    }
>> +}
>> +
>> +static void
>> +remove_from_forced_cleanup(struct Client *client) {
>> +    if (g_slist_length(forced_cleanups) > 0) {
>> +	VERBOSE_PRINT("removing %s from forced cleanups\n", client->qemu.vmid);
>> +	g_slist_foreach(forced_cleanups, (GFunc)remove_cleanup_data, client);
> 
> Foreach + remove feels awkward to me. Sure, it's a linked list and
> should be fine™, but I don't like the lack of documentation of
> interactions here as a non-glib user. (I mean, eg. for C++ iterator
> invalidation is *usually* documented...)

just to clarify: it's documented in glibs docs[0]:
 > It is safe for func to remove the element from list, but it must not modify any part of the list 
after that element.

> 
> Can't we just give `struct Client` a `struct CleanupData` pointer and
> call `g_slist_remove` right here without the iteration?
> 
> Or better yet, your previous idea of dropping `CleanupData` sounds
> better.
> We should be able to just add `struct Client*` to the list, after all,
> according to the glib docs `g_slist_remove` should simply leave the list
> unchanged if the data is not part of the list, so when we free up the
> `Client` we could even call `g_slist_remove` unconditionally (though
> we'll know whether it's in there by having a timeout set then...)
> 
> (or use `g_slist_find_custom`)

sounds good to me

> 
>> +    }
>> +}
>> +
>>   void
>>   cleanup_client(struct Client *client)
>>   {
>> @@ -442,6 +458,7 @@ cleanup_client(struct Client *client)
>>   	    break;
>>       }
>>   
>> +    remove_from_forced_cleanup(client);
>>       free(client);
>>   }
>>   
>> -- 
>> 2.30.2

0: https://docs.gtk.org/glib/type_func.SList.foreach.html