Re: [PATCH v2 RESEND] lxc: Cleanup after failed startup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 03, 2020 at 09:46:20PM +0100, Michal Privoznik wrote:
If starting an container fails, the virLXCProcessStop() is
called. But since vm->def->id is not set until libvirt_lxc is
spawned (the domain's ID is PID of that process),
virLXCProcessStop() returns early as virDomainObjIsActive()
returns false. But doing so leaves behind resources reserved for
the containers during the startup process. Most notably, hostdevs
are not re-attached to the host, the domain's transient XML is
not removed, etc.

To resolve this, virLXCProcessCleanup() is called in this case.
However, it is modified to accept @flags which allows caller to
run only specific cleanups (depending how far in container
creation the failure occurred). There is plenty of cleanups which
don't need this guard because either they detect a NULL pointer
or try to release an unique resource.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---

This is just a resend of this patch:

https://www.redhat.com/archives/libvir-list/2020-November/msg00525.html

src/lxc/lxc_process.c | 52 ++++++++++++++++++++++++++++++++++---------
1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 7e07f49f9a..0f818e2ee0 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -145,18 +145,27 @@ lxcProcessRemoveDomainStatus(virLXCDriverConfigPtr cfg,
}


+typedef enum {
+    VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL = (1 << 0),
+    VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL = (1 << 1),
+    VIR_LXC_PROCESS_CLEANUP_REMOVE_TRANSIENT = (1 << 2),
+} virLXCProcessCleanupFlags;
+
/**
 * virLXCProcessCleanup:
 * @driver: pointer to driver structure
 * @vm: pointer to VM to clean up
 * @reason: reason for switching the VM to shutoff state
+ * @flags: allows to run selective cleanups only
 *
- * Cleanout resources associated with the now dead VM
- *
+ * Clean out resources associated with the now dead VM.
+ * If @flags is zero then whole cleanup process is done,
+ * otherwise only selected sections are run.

This sounds more like a "limit" than "flags" to me.

 */
static void virLXCProcessCleanup(virLXCDriverPtr driver,
                                 virDomainObjPtr vm,
-                                 virDomainShutoffReason reason)
+                                 virDomainShutoffReason reason,
+                                 unsigned int flags)
{
    size_t i;
    virLXCDomainObjPrivatePtr priv = vm->privateData;
@@ -164,8 +173,11 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
    virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
    virConnectPtr conn = NULL;

-    VIR_DEBUG("Cleanup VM name=%s pid=%d reason=%d",
-              vm->def->name, (int)vm->pid, (int)reason);
+    VIR_DEBUG("Cleanup VM name=%s pid=%d reason=%d flags=0x%x",
+              vm->def->name, (int)vm->pid, (int)reason, flags);
+
+    if (flags == 0)
+        flags = ~0;


And even though documented this looks error-prone.

    /* now that we know it's stopped call the hook if present */
    if (virHookPresent(VIR_HOOK_DRIVER_LXC)) {
@@ -177,9 +189,15 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
                    NULL, xml, NULL);
    }

-    virSecurityManagerRestoreAllLabel(driver->securityManager,
-                                      vm->def, false, false);
-    virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+    if (flags & VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL) {
+        virSecurityManagerRestoreAllLabel(driver->securityManager,
+                                          vm->def, false, false);
+    }
+
+    if (flags & VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL) {
+        virSecurityManagerReleaseLabel(driver->securityManager, vm->def);
+    }
+
    /* Clear out dynamically assigned labels */
    if (vm->def->nseclabels &&
        vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
@@ -258,7 +276,9 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver,
                    NULL, xml, NULL);
    }

-    virDomainObjRemoveTransientDef(vm);
+    if (flags & VIR_LXC_PROCESS_CLEANUP_REMOVE_TRANSIENT)
+        virDomainObjRemoveTransientDef(vm);
+
    virObjectUnref(cfg);
    virObjectUnref(conn);
}
@@ -904,7 +924,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
    }

 cleanup:
-    virLXCProcessCleanup(driver, vm, reason);
+    virLXCProcessCleanup(driver, vm, reason, 0);

    return 0;
}
@@ -1198,6 +1218,7 @@ int virLXCProcessStart(virConnectPtr conn,
    g_autoptr(virCgroup) selfcgroup = NULL;
    int status;
    g_autofree char *pidfile = NULL;
+    unsigned int stopFlags = 0;

    if (virCgroupNewSelf(&selfcgroup) < 0)
        return -1;
@@ -1265,6 +1286,7 @@ int virLXCProcessStart(virConnectPtr conn,
    VIR_DEBUG("Setting current domain def as transient");
    if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0)
        goto cleanup;
+    stopFlags |= VIR_LXC_PROCESS_CLEANUP_REMOVE_TRANSIENT;

    /* Run an early hook to set-up missing devices */
    if (virHookPresent(VIR_HOOK_DRIVER_LXC)) {
@@ -1312,11 +1334,13 @@ int virLXCProcessStart(virConnectPtr conn,
        goto cleanup;
    }
    virDomainAuditSecurityLabel(vm, true);
+    stopFlags |= VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL;

    VIR_DEBUG("Setting domain security labels");
    if (virSecurityManagerSetAllLabel(driver->securityManager,
                                      vm->def, NULL, false, false) < 0)
        goto cleanup;
+    stopFlags |= VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL;

    VIR_DEBUG("Setting up consoles");
    for (i = 0; i < vm->def->nconsoles; i++) {
@@ -1525,7 +1549,13 @@ int virLXCProcessStart(virConnectPtr conn,
    }
    if (rc != 0) {
        virErrorPreserveLast(&err);
-        virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
+        if (virDomainObjIsActive(vm)) {
+            virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
+        } else {
+            /* virLXCProcessStop() is NOP if the container is not active.
+             * If there was a failure whilst creating it, cleanup manually. */
+            virLXCProcessCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stopFlags);
+        }

This looks like there could be a better way.  But I'm not going to insist on the
suggested changes, the LXC driver would need more than that and it looks like it
is not really used much anyway.  So anything we can make better is good.

Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx>

    }
    virCommandFree(cmd);
    for (i = 0; i < nttyFDs; i++)
--
2.26.2

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux