lxc: Cleanup after failed startup

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@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 7e07f49f9a8d3c7b1c195054fc8d7162d6732ff0..0f818e2ee019f775417c1925d3e0848213e6a8ac 100644 (file)
--- 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.
  */
 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;
 
     /* 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);
+        }
     }
     virCommandFree(cmd);
     for (i = 0; i < nttyFDs; i++)