From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 426A2905F8 for ; Thu, 22 Sep 2022 10:25:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 22B466564 for ; Thu, 22 Sep 2022 10:25:04 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 22 Sep 2022 10:25:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 72CD644492 for ; Thu, 22 Sep 2022 10:25:02 +0200 (CEST) Date: Thu, 22 Sep 2022 10:24:55 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220921124911.3224970-1-d.csapak@proxmox.com> <20220921124911.3224970-2-d.csapak@proxmox.com> In-Reply-To: <<20220921124911.3224970-2-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1663834293.mozxwx9wgy.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.147 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH qemu-server 1/3] qmeventd: rework 'forced_cleanup' handling and set timeout to 60s X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Sep 2022 08:25:34 -0000 On September 21, 2022 2:49 pm, 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. >=20 > Accidentally, it could be triggered earlier than 5 seconds, if a > SIGALRM triggers in the timespan directly before setting it again. >=20 > 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. >=20 > To improve this situation, rework the way we deal with this cleanup, by > saving the time incl. timeout in the CleanupData, and trigger a > forced_cleanup every 10 seconds. If that happends, remove it from > the forced_cleanup list. >=20 > 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. >=20 > Signed-off-by: Dominik Csapak > --- > qmeventd/qmeventd.c | 75 +++++++++++++++++++++++++-------------------- > qmeventd/qmeventd.h | 2 ++ > 2 files changed, 44 insertions(+), 33 deletions(-) >=20 > diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c > index 8d32827..e9ff5b3 100644 > --- a/qmeventd/qmeventd.c > +++ b/qmeventd/qmeventd.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -36,15 +37,18 @@ > #include > #include > #include > +#include > =20 > #include "qmeventd.h" > =20 > +#define DEFAULT_KILL_TIMEOUT 60 > + > static int verbose =3D 0; > +static int kill_timeout =3D DEFAULT_KILL_TIMEOUT; > static int epoll_fd =3D 0; > static const char *progname; > GHashTable *vm_clients; // key=3Dvmid (freed on remove), value=3D*Client= (free manually) > GSList *forced_cleanups; > -volatile sig_atomic_t alarm_triggered =3D 0; > =20 > /* > * Helper functions > @@ -54,9 +58,10 @@ static void > 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, " PATH use PATH for socket\n"); > + fprintf(stderr, " -f run in foreground (default: false)\n= "); > + fprintf(stderr, " -v verbose (default: false)\n"); > + fprintf(stderr, " -t timeout kill timeout (default: %ds)\n", DEFA= ULT_KILL_TIMEOUT); > + fprintf(stderr, " PATH use PATH for socket\n"); > } > =20 > static pid_t > @@ -469,16 +474,16 @@ terminate_client(struct Client *client) > int err =3D kill(client->pid, SIGTERM); > log_neg(err, "kill"); > =20 > + time_t timeout =3D time(NULL) + kill_timeout; > + > struct CleanupData *data_ptr =3D malloc(sizeof(struct CleanupData)); > struct CleanupData data =3D { > .pid =3D client->pid, > - .pidfd =3D pidfd > + .pidfd =3D pidfd, > + .timeout =3D timeout, > }; > *data_ptr =3D data; > forced_cleanups =3D g_slist_prepend(forced_cleanups, (void *)data_pt= r); > - > - // resets any other alarms, but will fire eventually and cleanup all > - alarm(5); > } > =20 > void > @@ -551,27 +556,16 @@ handle_client(struct Client *client) > json_tokener_free(tok); > } > =20 > - > -/* > - * 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 lis= t and > - * attempt to issue SIGKILL to all processes which haven't yet stopped. > - */ > - > -static void > -alarm_handler(__attribute__((unused)) int signum) > -{ > - alarm_triggered =3D 1; > -} > - wasn't this intentionally decoupled like this? alarm_handler just sets the flag actual force cleanup is conditionalized on the alarm having triggered,=20 but the cleanup happens outside of the signal handler.. is there a reason from switching away from these scheme? we don't need=20 to do the cleanup in the signal handler (timing is already plenty fuzzy=20 anyway ;)) > static void > sigkill(void *ptr, __attribute__((unused)) void *unused) > { > struct CleanupData data =3D *((struct CleanupData *)ptr); > int err; > =20 > + if (data.timeout > time(NULL)) { nit: current time / cutoff could be passed in via the currently unused=20 user_data parameter.. > + return; > + } > + > if (data.pidfd > 0) { > err =3D pidfd_send_signal(data.pidfd, SIGKILL, NULL, 0); > (void)close(data.pidfd); > @@ -588,21 +582,29 @@ sigkill(void *ptr, __attribute__((unused)) void *un= used) > fprintf(stderr, "cleanup failed, terminating pid '%d' with SIGKILL\n", > data.pid); > } > + > + // remove ourselves from the list > + forced_cleanups =3D g_slist_remove(forced_cleanups, ptr); > + free(ptr); > } > =20 > +/* > + * SIGALRM and cleanup handling > + * > + * handles the cleanup on non terminated qemu processes, will be called = every > + * 10 seconds by setting 'alarm(10)' at the end again > + */ > + > static void > -handle_forced_cleanup() > +alarm_handler(__attribute__((unused)) int signum) > { > - if (alarm_triggered) { > + if (g_slist_length(forced_cleanups) > 0) { > VERBOSE_PRINT("clearing forced cleanup backlog\n"); > - alarm_triggered =3D 0; > g_slist_foreach(forced_cleanups, sigkill, NULL); > - g_slist_free_full(forced_cleanups, free); > - forced_cleanups =3D NULL; > } > + alarm(10); > } > =20 > - > int > main(int argc, char *argv[]) > { > @@ -611,7 +613,7 @@ main(int argc, char *argv[]) > char *socket_path =3D NULL; > progname =3D argv[0]; > =20 > - while ((opt =3D getopt(argc, argv, "hfv")) !=3D -1) { > + while ((opt =3D getopt(argc, argv, "hfvt:")) !=3D -1) { > switch (opt) { > case 'f': > daemonize =3D 0; > @@ -619,6 +621,15 @@ main(int argc, char *argv[]) > case 'v': > verbose =3D 1; > break; > + case 't': > + errno =3D 0; > + char *endptr =3D NULL; > + kill_timeout =3D strtoul(optarg, &endptr, 10); > + if (errno !=3D 0 || *endptr !=3D '\0' || kill_timeout =3D=3D 0) { > + usage(); > + exit(EXIT_FAILURE); > + } > + break; > case 'h': > usage(); > exit(EXIT_SUCCESS); > @@ -668,10 +679,10 @@ main(int argc, char *argv[]) > =20 > int nevents; > =20 > + alarm(10); > for(;;) { > nevents =3D epoll_wait(epoll_fd, events, 1, -1); > if (nevents < 0 && errno =3D=3D EINTR) { > - handle_forced_cleanup(); > continue; > } > bail_neg(nevents, "epoll_wait"); > @@ -688,7 +699,5 @@ 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 @@ > */ > =20 > #include > +#include > =20 > #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; > }; > =20 > void handle_qmp_handshake(struct Client *client); > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20