[libvirt] [PATCH 0/4] storage: Fix a few issues with pool state tracking
First patch fixes a regression introduced with pool state patches Last 3 patches help fix other issues with the state tracking Cole Robinson (4): storage: Fix autostart dir for qemu:///session storage: Don't leave stale state file if pool startup fails storage: Break out storageDriverLoadPoolState storage: If driver startup state syncing fails, delete statefile src/storage/storage_driver.c | 109 ++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 42 deletions(-) -- 2.3.6
--- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 306d98e..3b7746b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -237,7 +237,7 @@ storageStateInitialize(bool privileged, if ((virAsprintf(&driver->configDir, "%s/storage", configdir) < 0) || (virAsprintf(&driver->autostartDir, - "%s/storage", configdir) < 0) || + "%s/storage/autostart", configdir) < 0) || (virAsprintf(&driver->stateDir, "%s/storage/run", rundir) < 0)) goto error; -- 2.3.6
After pool startup we call refreshPool(). If that fails, we leave a stale pool state file hanging around. Hit this trying to create a pool with qemu:///session containing root owned files. --- src/storage/storage_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3b7746b..06686bf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -180,6 +180,8 @@ storageDriverAutostart(void) virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); + if (stateFile) + unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), @@ -690,6 +692,8 @@ storagePoolCreateXML(virConnectPtr conn, if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { + if (stateFile) + unlink(stateFile); if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool); @@ -856,6 +860,8 @@ storagePoolCreate(virStoragePoolPtr obj, if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(obj->conn, pool) < 0) { + if (stateFile) + unlink(stateFile); if (backend->stopPool) backend->stopPool(obj->conn, pool); goto cleanup; -- 2.3.6
Will simplify a future patch --- src/storage/storage_driver.c | 88 +++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 06686bf..9abdc68 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -75,14 +75,58 @@ static void storageDriverUnlock(void) } static void +storagePoolUpdateState(virStoragePoolObjPtr pool) +{ + bool active; + virStorageBackendPtr backend; + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { + VIR_ERROR(_("Missing backend %d"), pool->def->type); + goto error; + } + + /* Backends which do not support 'checkPool' are considered + * inactive by default. + */ + active = false; + if (backend->checkPool && + backend->checkPool(pool, &active) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + goto error; + } + + /* We can pass NULL as connection, most backends do not use + * it anyway, but if they do and fail, we want to log error and + * continue with other pools. + */ + if (active) { + virStoragePoolObjClearVols(pool); + if (backend->refreshPool(NULL, pool) < 0) { + virErrorPtr err = virGetLastError(); + if (backend->stopPool) + backend->stopPool(NULL, pool); + VIR_ERROR(_("Failed to restart storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + goto error; + } + } + + pool->active = active; + error: + return; +} + +static void storagePoolUpdateAllState(void) { size_t i; - bool active; for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; - virStorageBackendPtr backend; virStoragePoolObjLock(pool); if (!virStoragePoolObjIsActive(pool)) { @@ -90,45 +134,7 @@ storagePoolUpdateAllState(void) continue; } - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); - virStoragePoolObjUnlock(pool); - continue; - } - - /* Backends which do not support 'checkPool' are considered - * inactive by default. - */ - active = false; - if (backend->checkPool && - backend->checkPool(pool, &active) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); - virStoragePoolObjUnlock(pool); - continue; - } - - /* We can pass NULL as connection, most backends do not use - * it anyway, but if they do and fail, we want to log error and - * continue with other pools. - */ - if (active) { - virStoragePoolObjClearVols(pool); - if (backend->refreshPool(NULL, pool) < 0) { - virErrorPtr err = virGetLastError(); - if (backend->stopPool) - backend->stopPool(NULL, pool); - VIR_ERROR(_("Failed to restart storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); - virStoragePoolObjUnlock(pool); - continue; - } - } - - pool->active = active; + storagePoolUpdateState(pool); virStoragePoolObjUnlock(pool); } } -- 2.3.6
If you end up with a state file for a pool that no longer starts up or refreshes correctly, the state file is never removed and adds noise to the logs everytime libvirtd is started. If the initial state syncing fails, delete the statefile. --- src/storage/storage_driver.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9abdc68..ac4a74a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -79,6 +79,12 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) { bool active; virStorageBackendPtr backend; + int ret = -1; + char *stateFile; + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto error; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { VIR_ERROR(_("Missing backend %d"), pool->def->type); @@ -116,7 +122,14 @@ storagePoolUpdateState(virStoragePoolObjPtr pool) } pool->active = active; + ret = 0; error: + if (ret < 0) { + if (stateFile) + unlink(stateFile); + } + VIR_FREE(stateFile); + return; } -- 2.3.6
On Mon, Apr 27, 2015 at 10:44:54AM -0400, Cole Robinson wrote:
First patch fixes a regression introduced with pool state patches Last 3 patches help fix other issues with the state tracking
Cole Robinson (4): storage: Fix autostart dir for qemu:///session storage: Don't leave stale state file if pool startup fails storage: Break out storageDriverLoadPoolState storage: If driver startup state syncing fails, delete statefile
src/storage/storage_driver.c | 109 ++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 42 deletions(-)
ACK series Jan
On 04/28/2015 08:07 AM, Ján Tomko wrote:
On Mon, Apr 27, 2015 at 10:44:54AM -0400, Cole Robinson wrote:
First patch fixes a regression introduced with pool state patches Last 3 patches help fix other issues with the state tracking
Cole Robinson (4): storage: Fix autostart dir for qemu:///session storage: Don't leave stale state file if pool startup fails storage: Break out storageDriverLoadPoolState storage: If driver startup state syncing fails, delete statefile
src/storage/storage_driver.c | 109 ++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 42 deletions(-)
ACK series
Thanks, pushed now - Cole
participants (2)
-
Cole Robinson -
Ján Tomko