From a94ff361dd26e3fae62eeee5a1d2285caed5ade7 Mon Sep 17 00:00:00 2001 From: Marcus Sorensen Date: Wed, 13 Mar 2013 15:29:09 -0600 Subject: [PATCH] CLOUDSTACK-1648 - KVM - make storage pools non-persistent in libvirt. Persisting cloud-defined resources on the host has caused various problems. As a backward compatible fix, if an existing pool with a different name collides with a pool being created (by path), the pool will be redefined with the name cloudstack knows about. This is actually what brought up the bug, a persisted storage pool cloudstack wasn't managing. Signed-off-by: Marcus Sorensen 1363210149 -0600 --- .../resource/LibvirtComputingResource.java | 2 +- .../kvm/storage/LibvirtStorageAdaptor.java | 109 +++++++++++++++--- 2 files changed, 91 insertions(+), 20 deletions(-) diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 4e1779b9441..c2d5a94cd31 100755 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -3496,7 +3496,7 @@ ServerResource { sscmd.setDataCenter(_dcId); sscmd.setResourceType(Storage.StorageResourceType.STORAGE_POOL); } catch (CloudRuntimeException e) { - + s_logger.debug("Unable to initialize local storage pool: " + e); } if (sscmd != null) { diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java index 070c1327ba3..d5e6ad6fe00 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java @@ -120,14 +120,18 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { StoragePool sp = null; try { s_logger.debug(spd.toString()); - sp = conn.storagePoolDefineXML(spd.toString(), 0); - sp.create(0); + sp = conn.storagePoolCreateXML(spd.toString(), 0); return sp; } catch (LibvirtException e) { s_logger.error(e.toString()); if (sp != null) { try { - sp.undefine(); + if (sp.isPersistent() == 1) { + sp.destroy(); + sp.undefine(); + } else { + sp.destroy(); + } sp.free(); } catch (LibvirtException l) { s_logger.debug("Failed to define nfs storage pool with: " @@ -150,15 +154,18 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { StoragePool sp = null; try { s_logger.debug(spd.toString()); - sp = conn.storagePoolDefineXML(spd.toString(), 0); - sp.create(0); - + sp = conn.storagePoolCreateXML(spd.toString(), 0); return sp; } catch (LibvirtException e) { s_logger.error(e.toString()); if (sp != null) { try { - sp.undefine(); + if (sp.isPersistent() == 1) { + sp.destroy(); + sp.undefine(); + } else { + sp.destroy(); + } sp.free(); } catch (LibvirtException l) { s_logger.debug("Failed to define shared mount point storage pool with: " @@ -181,14 +188,18 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { StoragePool sp = null; try { s_logger.debug(spd.toString()); - sp = conn.storagePoolDefineXML(spd.toString(), 0); - sp.create(0); + sp = conn.storagePoolCreateXML(spd.toString(), 0); return sp; } catch (LibvirtException e) { s_logger.error(e.toString()); if (sp != null) { try { - sp.undefine(); + if (sp.isPersistent() == 1) { + sp.destroy(); + sp.undefine(); + } else { + sp.destroy(); + } sp.free(); } catch (LibvirtException l) { s_logger.debug("Failed to define clvm storage pool with: " @@ -236,14 +247,18 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { try { s_logger.debug(spd.toString()); - sp = conn.storagePoolDefineXML(spd.toString(), 0); - sp.create(0); + sp = conn.storagePoolCreateXML(spd.toString(), 0); return sp; } catch (LibvirtException e) { s_logger.debug(e.toString()); if (sp != null) { try { - sp.undefine(); + if (sp.isPersistent() == 1) { + sp.destroy(); + sp.undefine(); + } else { + sp.destroy(); + } sp.free(); } catch (LibvirtException l) { s_logger.debug("Failed to define RBD storage pool with: " + l.toString()); @@ -385,15 +400,59 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { try { sp = conn.storagePoolLookupByUUIDString(name); - if (sp.getInfo().state != StoragePoolState.VIR_STORAGE_POOL_RUNNING) { + if (sp != null && sp.isActive() == 0) { sp.undefine(); sp = null; + s_logger.debug("Found existing defined storage pool " + name + ". It wasn't running, so we undefined it."); + } + if (sp != null) { + s_logger.debug("Found existing defined storage pool " + name + ", using it."); } } catch (LibvirtException e) { + sp = null; + s_logger.debug("createStoragePool didn't find existing running pool: " + e + ", need to create it"); + } + // libvirt strips trailing slashes off of path, we will too in order to match + // existing paths + if (path.endsWith("/")) { + path = path.substring(0, path.length() - 1); } if (sp == null) { + // see if any existing pool by another name is using our storage path. + // if anyone is, undefine the pool so we can define it as requested. + // This should be safe since a pool in use can't be removed, and no + // volumes are affected by unregistering the pool with libvirt. + s_logger.debug("Didn't find an existing storage pool " + name + + " by UUID, checking for pools with duplicate paths"); + + try { + String[] poolnames = conn.listStoragePools(); + for (String poolname : poolnames) { + s_logger.debug("Checking path of existing pool " + poolname + + " against pool we want to create"); + StoragePool p = conn.storagePoolLookupByName(poolname); + LibvirtStoragePoolDef pdef = getStoragePoolDef(conn, p); + + if (pdef.getTargetPath().equals(path)) { + s_logger.debug("Storage pool utilizing path '" + path + "' already exists as pool " + + poolname + ", undefining so we can re-define with correct name " + name); + if (p.isPersistent() == 1) { + p.destroy(); + p.undefine(); + } else { + p.destroy(); + } + } + } + } catch (LibvirtException e) { + s_logger.error("Failure in attempting to see if an existing storage pool might " + + "be using the path of the pool to be created:" + e); + } + + s_logger.debug("Attempting to create storage pool " + name); + if (type == StoragePoolType.NetworkFilesystem) { sp = createNfsStoragePool(conn, name, host, path); } else if (type == StoragePoolType.SharedMountPoint @@ -407,8 +466,8 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } try { - StoragePoolInfo spi = sp.getInfo(); - if (spi.state != StoragePoolState.VIR_STORAGE_POOL_RUNNING) { + if (sp.isActive() == 0) { + s_logger.debug("attempting to activate pool " + name); sp.create(0); } @@ -427,7 +486,15 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { return pool; } catch (LibvirtException e) { - throw new CloudRuntimeException(e.toString()); + String error = e.toString(); + if (error.contains("Storage source conflict")) { + throw new CloudRuntimeException("A pool matching this location already exists in libvirt, " + + " but has a different UUID/Name. Cannot create new pool without first " + + " removing it. Check for inactive pools via 'virsh pool-list --all'. " + + error); + } else { + throw new CloudRuntimeException(error); + } } } @@ -459,8 +526,12 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { } try { - sp.destroy(); - sp.undefine(); + if (sp.isPersistent() == 1) { + sp.destroy(); + sp.undefine(); + } else { + sp.destroy(); + } sp.free(); if (s != null) { s.undefine();