12:42 p.m.
On 03/24/2015 06:06 AM, Erik Skultety wrote:
The update was originally part of the storageDriverAutostart
function,
s/update/checkPool
but the pools have to be checked earlier during initialization phase,
so
s/have/need
the 'checkPool' block has been moved to
storagePoolUpdateAllState.
move the checkPool block into storageDriverInitialize. This ensures
activating pools earlier allowing domains using volumes from a pool to
be able to find the volume during qemuProcessReconnect ...
[e.g. picking up the commit message from 1/7 into here...]
Prior to this update virStoragePoolLoadAllConfigs and
virStoragePoolLoadAllState functions gather all existing pool in the
system, so first it is necessary to filter out the ones that are
inactive (only config XML found), then determine storage backends for
the rest of the pools and run checkPool on each one of them to update
their 'active' property.
I think you're trying to indicate prior to this function, the
virStoragePoolLoadAllState and virStoragePoolLoadAllConfigs functions
have generated a list all the existing configs (active or inactive)...
The purpose of this function is to take any that were deemed active and
ensure that the checkPool function is called along with the refreshPool
function in order to ensure that the volumes within the pool are present
when the domain reconnection occurs.
After update, pools have to be refreshed, otherwise the list of
volumes
s/update/checkPool
stays empty. Once again we need the connection, but all the
storage backends ignore this argument except for RBD, however RBD
doesn't support 'checkPool' callback, therefore it is safe to pass
connection as NULL pointer.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
src/storage/storage_driver.c | 73 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 61 insertions(+), 12 deletions(-)
This one I'm struggling with... For file based pools there's mostly
truth to the claim in the commit message; however, iSCSI and SCSI pools
are a bit different. Perhaps along the same issues as rbd and even
gluster. I guess I just have to research a bit more of any edge
conditions and make sure the changes actually "work" for my iSCSI
config... My fc_host config is a bit trickier since I'm sharing the host
with someone else who I have to coordinate with (so I'll wait for the v2
patches before going down that path).
diff --git a/src/storage/storage_driver.c
b/src/storage/storage_driver.c
index d09acce..2899521 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -75,6 +75,64 @@ static void storageDriverUnlock(void)
}
static void
+storagePoolUpdateAllState(void)
+{
+ size_t i;
+ bool active = false;
+
+ for (i = 0; i < driver->pools.count; i++) {
+ virStoragePoolObjPtr pool = driver->pools.objs[i];
+ virStorageBackendPtr backend;
+
+ virStoragePoolObjLock(pool);
+ if (!virStoragePoolObjIsActive(pool)) {
+ virStoragePoolObjUnlock(pool);
+ continue;
+ }
+
+ if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
+ VIR_ERROR(_("Missing backend %d"), pool->def->type);
+ virStoragePoolObjUnlock(pool);
+ continue;
+ }
Redundant check with DriverAutostart and since that's the only place
that calls us.... I think dropping the VIR_ERROR is fine...
In fact I think:
if ((backend = virStorageBackendForType(pool->def->type)) == NULL ||
!backend->checkPool) {
virStoragepoolObjUnlock(pool);
continue;
}
Then you can assume that whatever follows has a 'checkPool' function
+
+ /* Backends which do not support 'checkPool' are considered
+ * inactive by default.
+ */
+ if (backend->checkPool &&
+ backend->checkPool(pool, &active) < 0) {
See I understand why patch 1 removes the conn; however, I'm still not
convinced we can do that - let's see if anyone else says anything.
+ 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;
+ }
Could add a :
if (!active) {
virStoragePoolObjUnlock(pool);
continue;
}
then the rest is just inline...
+
+ /* 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;
(ahem) I assume this only matters is active true...
John
+ virStoragePoolObjUnlock(pool);
+ }
+}
+
+static void
storageDriverAutostart(void)
{
size_t i;
@@ -99,18 +157,7 @@ storageDriverAutostart(void)
continue;
}
- if (backend->checkPool &&
- backend->checkPool(pool, &started) < 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;
- }
-
- if (!started &&
- pool->autostart &&
+ if (pool->autostart &&
!virStoragePoolObjIsActive(pool)) {
if (backend->startPool &&
backend->startPool(conn, pool) < 0) {
@@ -207,6 +254,8 @@ storageStateInitialize(bool privileged,
driver->autostartDir) < 0)
goto error;
+ storagePoolUpdateAllState();
+
storageDriverUnlock();
ret = 0;