1. Patchew
  2. Libvirt
  3. View series

[PATCH] Fix reboot command for LXC containers

Joachim Falk posted 1 patch 2 years, 4 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
src/lxc/lxc_controller.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
[PATCH] Fix reboot command for LXC containers
Posted by Joachim Falk 2 years, 4 months ago 
The virNetDaemonQuit(dmn) command in virLXCControllerSignalChildIO triggers an
early close of all clients of lxc_controller. Here, libvirtd itself is a client
of this controller, and the client connection is used to notify libvirtd if a
reboot of the container is required. However, the client connection was closed
before such a status could be sent to libvirtd. To fix this bug, we will
immediately send the reboot or shutdown status of the container to libvirtd,
and only after client disconnect will we trigger virNetDaemonQuit.

Fixes: #237
Bug: https://gitlab.com/libvirt/libvirt/-/issues/237
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=991773
Signed-off-by: Joachim Falk <joachim.falk@gmx.de>
---
 src/lxc/lxc_controller.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 444f728af4..75107822ba 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -894,8 +894,10 @@ static void virLXCControllerClientCloseHook(virNetServerClient *client)
     virLXCController *ctrl = virNetServerClientGetPrivateData(client);
      VIR_DEBUG("Client %p has closed", client);
-    if (ctrl->client == client)
+    if (ctrl->client == client) {
         ctrl->client = NULL;
+        VIR_DEBUG("Client has gone away");
+    }
     if (ctrl->inShutdown) {
         VIR_DEBUG("Arm timer to quit event loop");
         virEventUpdateTimeout(ctrl->timerShutdown, 0);
@@ -1006,6 +1008,9 @@ static int lxcControllerClearCapabilities(void)
 static bool wantReboot;
 static virMutex lock = VIR_MUTEX_INITIALIZER;
 +static int
+virLXCControllerEventSendExit(virLXCController *ctrl,
+                              int exitstatus);
  static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
                                           siginfo_t *info G_GNUC_UNUSED,
@@ -1015,10 +1020,10 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
     int ret;
     int status;
 +    ignore_value(dmn);
     ret = waitpid(-1, &status, WNOHANG);
     VIR_DEBUG("Got sig child %d vs %lld", ret, (long long)ctrl->initpid);
     if (ret == ctrl->initpid) {
-        virNetDaemonQuit(dmn);
         virMutexLock(&lock);
         if (WIFSIGNALED(status) &&
             WTERMSIG(status) == SIGHUP) {
@@ -1026,6 +1031,7 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
             wantReboot = true;
         }
         virMutexUnlock(&lock);
+        virLXCControllerEventSendExit(ctrl, wantReboot ? 1 : 0);
     }
 }
 @@ -2276,9 +2282,10 @@ virLXCControllerEventSendExit(virLXCController *ctrl,
         VIR_DEBUG("Waiting for client to complete dispatch");
         ctrl->inShutdown = true;
         virNetServerClientDelayedClose(ctrl->client);
-        virNetDaemonRun(ctrl->daemon);
+    } else {
+        VIR_DEBUG("Arm timer to quit event loop");
+        virEventUpdateTimeout(ctrl->timerShutdown, 0);
     }
-    VIR_DEBUG("Client has gone away");
     return 0;
 }
 @@ -2430,8 +2437,6 @@ virLXCControllerRun(virLXCController *ctrl)
      rc = virLXCControllerMain(ctrl);
 -    virLXCControllerEventSendExit(ctrl, rc);
-
  cleanup:
     VIR_FORCE_CLOSE(control[0]);
     VIR_FORCE_CLOSE(control[1]);
--
2.30.2
Re: [PATCH] Fix reboot command for LXC containers
Posted by Michal Prívozník 2 years, 4 months ago 
On 12/1/21 22:38, Joachim Falk wrote:
> The virNetDaemonQuit(dmn) command in virLXCControllerSignalChildIO triggers an
> early close of all clients of lxc_controller. Here, libvirtd itself is a client
> of this controller, and the client connection is used to notify libvirtd if a
> reboot of the container is required. However, the client connection was closed
> before such a status could be sent to libvirtd. To fix this bug, we will
> immediately send the reboot or shutdown status of the container to libvirtd,
> and only after client disconnect will we trigger virNetDaemonQuit.
> 
> Fixes: #237
> Bug: https://gitlab.com/libvirt/libvirt/-/issues/237
> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=991773
> Signed-off-by: Joachim Falk <joachim.falk@gmx.de>
> ---
>  src/lxc/lxc_controller.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

This patch is broken - I'm unable to apply it. Did you hand edit it
perhaps? Also please make sure that the patch is rebased onto current
master.

> 
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 444f728af4..75107822ba 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -894,8 +894,10 @@ static void virLXCControllerClientCloseHook(virNetServerClient *client)
>      virLXCController *ctrl = virNetServerClientGetPrivateData(client);
>       VIR_DEBUG("Client %p has closed", client);
> -    if (ctrl->client == client)
> +    if (ctrl->client == client) {
>          ctrl->client = NULL;
> +        VIR_DEBUG("Client has gone away");
> +    }
>      if (ctrl->inShutdown) {
>          VIR_DEBUG("Arm timer to quit event loop");
>          virEventUpdateTimeout(ctrl->timerShutdown, 0);
> @@ -1006,6 +1008,9 @@ static int lxcControllerClearCapabilities(void)
>  static bool wantReboot;
>  static virMutex lock = VIR_MUTEX_INITIALIZER;
>  +static int
> +virLXCControllerEventSendExit(virLXCController *ctrl,
> +                              int exitstatus);
>   static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>                                            siginfo_t *info G_GNUC_UNUSED,
> @@ -1015,10 +1020,10 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>      int ret;
>      int status;
>  +    ignore_value(dmn);

No. We use G_GNUC_UNUSED, just like you can see in this context ^^^.

>      ret = waitpid(-1, &status, WNOHANG);
>      VIR_DEBUG("Got sig child %d vs %lld", ret, (long long)ctrl->initpid);
>      if (ret == ctrl->initpid) {
> -        virNetDaemonQuit(dmn);
>          virMutexLock(&lock);
>          if (WIFSIGNALED(status) &&
>              WTERMSIG(status) == SIGHUP) {
> @@ -1026,6 +1031,7 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>              wantReboot = true;
>          }
>          virMutexUnlock(&lock);
> +        virLXCControllerEventSendExit(ctrl, wantReboot ? 1 : 0);
>      }
>  }
>  @@ -2276,9 +2282,10 @@ virLXCControllerEventSendExit(virLXCController *ctrl,
>          VIR_DEBUG("Waiting for client to complete dispatch");
>          ctrl->inShutdown = true;
>          virNetServerClientDelayedClose(ctrl->client);
> -        virNetDaemonRun(ctrl->daemon);
> +    } else {
> +        VIR_DEBUG("Arm timer to quit event loop");
> +        virEventUpdateTimeout(ctrl->timerShutdown, 0);
>      }
> -    VIR_DEBUG("Client has gone away");
>      return 0;
>  }
>  @@ -2430,8 +2437,6 @@ virLXCControllerRun(virLXCController *ctrl)
>       rc = virLXCControllerMain(ctrl);
>  -    virLXCControllerEventSendExit(ctrl, rc);
> -
>   cleanup:
>      VIR_FORCE_CLOSE(control[0]);
>      VIR_FORCE_CLOSE(control[1]);

Otherwise this approach looks reasonable.

Michal
Re: [PATCH] Fix reboot command for LXC containers
Posted by Joachim Falk 2 years, 4 months ago 
Hi Michal,

Am 02.12.21 um 16:02 schrieb Michal Prívozník:
> On 12/1/21 22:38, Joachim Falk wrote:
>> The virNetDaemonQuit(dmn) command in virLXCControllerSignalChildIO triggers an
>> early close of all clients of lxc_controller. Here, libvirtd itself is a client
>> of this controller, and the client connection is used to notify libvirtd if a
>> reboot of the container is required. However, the client connection was closed
>> before such a status could be sent to libvirtd. To fix this bug, we will
>> immediately send the reboot or shutdown status of the container to libvirtd,
>> and only after client disconnect will we trigger virNetDaemonQuit.
>>
>> Fixes: #237
>> Bug: https://gitlab.com/libvirt/libvirt/-/issues/237
>> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=991773
>> Signed-off-by: Joachim Falk <joachim.falk@gmx.de>
>> ---
>>  src/lxc/lxc_controller.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> This patch is broken - I'm unable to apply it. Did you hand edit it
> perhaps? Also please make sure that the patch is rebased onto current
> master.

it is on the current master. However, I had to resend it due to a spam bounce.
Maybe it got mangled by this process.

>
>>
>> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
>> index 444f728af4..75107822ba 100644
>> --- a/src/lxc/lxc_controller.c
>> +++ b/src/lxc/lxc_controller.c
>> @@ -894,8 +894,10 @@ static void virLXCControllerClientCloseHook(virNetServerClient *client)
>>      virLXCController *ctrl = virNetServerClientGetPrivateData(client);
>>       VIR_DEBUG("Client %p has closed", client);
>> -    if (ctrl->client == client)
>> +    if (ctrl->client == client) {
>>          ctrl->client = NULL;
>> +        VIR_DEBUG("Client has gone away");
>> +    }
>>      if (ctrl->inShutdown) {
>>          VIR_DEBUG("Arm timer to quit event loop");
>>          virEventUpdateTimeout(ctrl->timerShutdown, 0);
>> @@ -1006,6 +1008,9 @@ static int lxcControllerClearCapabilities(void)
>>  static bool wantReboot;
>>  static virMutex lock = VIR_MUTEX_INITIALIZER;
>>  +static int
>> +virLXCControllerEventSendExit(virLXCController *ctrl,
>> +                              int exitstatus);
>>   static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>>                                            siginfo_t *info G_GNUC_UNUSED,
>> @@ -1015,10 +1020,10 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>>      int ret;
>>      int status;
>>  +    ignore_value(dmn);
>
> No. We use G_GNUC_UNUSED, just like you can see in this context ^^^.

Thanks for the heads up. It is quite surprising that one can miss such obvious stuff.

>
>>      ret = waitpid(-1, &status, WNOHANG);
>>      VIR_DEBUG("Got sig child %d vs %lld", ret, (long long)ctrl->initpid);
>>      if (ret == ctrl->initpid) {
>> -        virNetDaemonQuit(dmn);
>>          virMutexLock(&lock);
>>          if (WIFSIGNALED(status) &&
>>              WTERMSIG(status) == SIGHUP) {
>> @@ -1026,6 +1031,7 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn,
>>              wantReboot = true;
>>          }
>>          virMutexUnlock(&lock);
>> +        virLXCControllerEventSendExit(ctrl, wantReboot ? 1 : 0);
>>      }
>>  }
>>  @@ -2276,9 +2282,10 @@ virLXCControllerEventSendExit(virLXCController *ctrl,
>>          VIR_DEBUG("Waiting for client to complete dispatch");
>>          ctrl->inShutdown = true;
>>          virNetServerClientDelayedClose(ctrl->client);
>> -        virNetDaemonRun(ctrl->daemon);
>> +    } else {
>> +        VIR_DEBUG("Arm timer to quit event loop");
>> +        virEventUpdateTimeout(ctrl->timerShutdown, 0);
>>      }
>> -    VIR_DEBUG("Client has gone away");
>>      return 0;
>>  }
>>  @@ -2430,8 +2437,6 @@ virLXCControllerRun(virLXCController *ctrl)
>>       rc = virLXCControllerMain(ctrl);
>>  -    virLXCControllerEventSendExit(ctrl, rc);
>> -
>>   cleanup:
>>      VIR_FORCE_CLOSE(control[0]);
>>      VIR_FORCE_CLOSE(control[1]);
>
> Otherwise this approach looks reasonable.

I resend the patch with the required changes. However, you might need to check
your SPAM filter. Patch should be send from address joachim.falk@jfalk.de.

Best,

Joachim

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.