[PATCH 4/4] lxc: Let the driver detect CGroups earlier

[Martin Kletzander]

On Tue, Apr 20, 2021 at 04:15:18PM +0200, Michal Privoznik wrote:
>This is the bug I'm facing. I deliberately configured a container
>so that the source of a <filesystem/> to passthrough doesn't
>exist. The start fails with:
>
>  lxcContainerPivotRoot:669 : Failed to create /non-existent/path/.oldroot: Permission denied
>
>which is expected. But what is NOT expected is that CGroup
>hierarchy is left behind. This is because the controller sets up
>the CGroup hierarchy, user namespace, moves interfaces, etc. and
>finally checks whether container setup (done in a separate
>process) succeeded. Only after all this the error is propagated
>to the LXC driver. The driver aborts the startup and tries to
>perform the cleanup, but this is missing CGroups because those
>weren't detected yet.
>
>Ideally, whenever a function fails, it tries to unroll back so
>that is has no artifacts left behind (look at all those frees/FD
>closes/etc. at end of functions). But with CGroups it is
>different - the controller process can't clean up after itself,
>because it is still running inside that CGroup.
>
>Therefore, what we have to do is to let the driver detect CGroups
>as soon as they are created, and proceed with controller
>execution only after that.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/lxc/lxc_controller.c | 19 +++++++++++++++++--
> src/lxc/lxc_process.c    | 20 ++++++++++++++++++++
> 2 files changed, 37 insertions(+), 2 deletions(-)
>

[...]

>diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>index 493e19f03d..55e74e913b 100644
>--- a/src/lxc/lxc_process.c
>+++ b/src/lxc/lxc_process.c
>@@ -1473,6 +1473,7 @@ int virLXCProcessStart(virConnectPtr conn,
>     if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback)
>         driver->inhibitCallback(true, driver->inhibitOpaque);
>
>+    /* The first synchronization point is when the controller creates CGroups. */
>     if (lxcContainerWaitForContinue(handshakefds[0]) < 0) {
>         char out[1024];
>
>@@ -1504,6 +1505,25 @@ int virLXCProcessStart(virConnectPtr conn,
>         goto cleanup;
>     }
>
>+    if (lxcContainerSendContinue(handshakefds[3]) < 0) {
>+        virReportSystemError(errno, "%s",
>+                             _("Failed to send continue signal to controller"));
>+        goto cleanup;
>+    }
>+
>+    /* The second synchronization point is when the controller finished
>+     * creating the container. */
>+    if (lxcContainerWaitForContinue(handshakefds[0]) < 0) {
>+        char out[1024];
>+
>+        if (!(virLXCProcessReadLogOutput(vm, logfile, pos, out, 1024) < 0)) {
>+            virReportError(VIR_ERR_INTERNAL_ERROR,
>+                           _("guest failed to start: %s"), out);

It would be nice to differentiate the error message reported here to the
one reported originally handful of lines above.  Just for the sake of
future debugging.

Other than that the patches could be nicer to read if the variables were
split or named differently, but after reading through a lot of the code
around these parts I see that such a request would be unreasonable for
this patch.  Maybe a rewrite of the signalling could be an item for a
TODO list, although I am not sure what would be the best way to handle
this.  Can we use for example eventfd()s?  Would that help us at all?

Anyway, with one of the error messages tweaked a bit:

Reviewed-by: Martin Kletzander <mkletzan at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210422/4c556325/attachment-0001.sig>