[libvirt] [PATCH] storage: Check and possibly update config file after build
https://bugzilla.redhat.com/show_bug.cgi?id=1464313 As it turns out, the on-disk config file (e.g. not the stateDir file) needs to be updated when --override is provided since it's possible and highly probable that the def->source.format has been adjusted and could cause a future start after perhaps a libvirtd restart to have the older format from a define operation from the backend. So in the 2 places where it's possible a write would be needed (create after define and build), let's perform a check and do the save/write operation on the configFile if it's necessary. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7cf5943..afb0404 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -651,6 +651,22 @@ storagePoolIsPersistent(virStoragePoolPtr pool) } +/* After a pool build, it's possible the inactive configFile needs to + * be updated especially since overwriting the pool more than likely + * changes the source format and may change/update a few other fields. */ +static int +storagePoolBuildCheckUpdateConfig(virStoragePoolObjPtr obj, + unsigned int flags) +{ + int ret = 0; + + if ((flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && obj->configFile) + ret = virStoragePoolSaveConfig(obj->configFile, obj->def); + + return ret; +} + + static virStoragePoolPtr storagePoolCreateXML(virConnectPtr conn, const char *xml, @@ -916,6 +932,9 @@ storagePoolCreate(virStoragePoolPtr pool, if (backend->buildPool(pool->conn, obj, build_flags) < 0) goto cleanup; } + + if (storagePoolBuildCheckUpdateConfig(obj, build_flags) < 0) + goto cleanup; } VIR_INFO("Starting up storage pool '%s'", obj->def->name); @@ -980,6 +999,10 @@ storagePoolBuild(virStoragePoolPtr pool, if (backend->buildPool && backend->buildPool(pool->conn, obj, flags) < 0) goto cleanup; + + if (storagePoolBuildCheckUpdateConfig(obj, flags) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.9.5
On Wed, Sep 06, 2017 at 15:24:18 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1464313
As it turns out, the on-disk config file (e.g. not the stateDir file) needs to be updated when --override is provided since it's possible and highly probable that the def->source.format has been adjusted and could cause a future start after perhaps a libvirtd restart to have the older format from a define operation from the backend.
So in the 2 places where it's possible a write would be needed (create after define and build), let's perform a check and do the save/write operation on the configFile if it's necessary.
I don't think this is correct. If you use the "create" API with a custom XML this should not in any way touch the persistent config of the pool, similarly as we do with VMs. The provided XML may describe a completely different storage pool and using the 'create' API should not overwrite anything stored persistently. If a user messes up the storage state via the live config, he needs to fix everything by themselves. This would just overwrite persistent config which is not desired. NACK
On 09/11/2017 07:34 AM, Peter Krempa wrote:
On Wed, Sep 06, 2017 at 15:24:18 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1464313
As it turns out, the on-disk config file (e.g. not the stateDir file) needs to be updated when --override is provided since it's possible and highly probable that the def->source.format has been adjusted and could cause a future start after perhaps a libvirtd restart to have the older format from a define operation from the backend.
So in the 2 places where it's possible a write would be needed (create after define and build), let's perform a check and do the save/write operation on the configFile if it's necessary.
I don't think this is correct. If you use the "create" API with a custom XML this should not in any way touch the persistent config of the pool, similarly as we do with VMs. The provided XML may describe a completely different storage pool and using the 'create' API should not overwrite anything stored persistently.
If a user messes up the storage state via the live config, he needs to fix everything by themselves.
This would just overwrite persistent config which is not desired.
NACK
To both Create and Build or just Create? Not sure I can visualize the Create path noted above, but it's still early in the morning with not enough coffee yet. Still the logic to decide to write out the config file is gated on whether the caller wanted overwrite (via VIR_STORAGE_POOL_BUILD_OVERWRITE flag being provided), but unlike VM's there's no flags to indicate whether we're "active" or "inactive". So if someone has indicated to use the overwrite flag, then that says to me all bets are off and not only should the live config be updated (as it is), but the on disk config as well since the decision by the consumer is to overwrite. Without doing so we end up with an unusable pool the next time we start which is the crux of the bz. John
On Mon, Sep 11, 2017 at 08:18:41 -0400, John Ferlan wrote:
On 09/11/2017 07:34 AM, Peter Krempa wrote:
On Wed, Sep 06, 2017 at 15:24:18 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1464313
As it turns out, the on-disk config file (e.g. not the stateDir file) needs to be updated when --override is provided since it's possible and highly probable that the def->source.format has been adjusted and could cause a future start after perhaps a libvirtd restart to have the older format from a define operation from the backend.
So in the 2 places where it's possible a write would be needed (create after define and build), let's perform a check and do the save/write operation on the configFile if it's necessary.
I don't think this is correct. If you use the "create" API with a custom XML this should not in any way touch the persistent config of the pool, similarly as we do with VMs. The provided XML may describe a completely different storage pool and using the 'create' API should not overwrite anything stored persistently.
If a user messes up the storage state via the live config, he needs to fix everything by themselves.
This would just overwrite persistent config which is not desired.
NACK
To both Create and Build or just Create?
This was for CreateXML but you are changing just "Create" which makes more sense, since that only touches the config which was already defined. I've mistaken those two, since CreateXML was at the end of the context of the first hunk. The code addition is actually in a different function. But that leads to another question ... why is that even necessary? If you are overwriting a disk, then the source format (and all possible other config) should be taken from what is provided in the definition which already exists (that is the persistent config) and thus no update should be required.
Not sure I can visualize the Create path noted above, but it's still early in the morning with not enough coffee yet. Still the logic to decide to write out the config file is gated on whether the caller wanted overwrite (via VIR_STORAGE_POOL_BUILD_OVERWRITE flag being provided), but unlike VM's there's no flags to indicate whether we're "active" or "inactive".
virStoragePoolObjPtr->active? Some pools can't be inactive, but e.g. gluster pools can, so that is not universally true.
So if someone has indicated to use the overwrite flag, then that says to me all bets are off and not only should the live config be updated (as it is), but the on disk config as well since the decision by the consumer is to overwrite.
In such case the live config should be completely deleted and replaced by new config which is based on the persistent config. Same way as when we start VMs.
Without doing so we end up with an unusable pool the next time we start which is the crux of the bz.
I think the root cause here is somewhere else. If we are adding anything automatic (e.g. the format), it should be recorded in the original config (and XML) and not overwritten once you are going to build/start it. Since I was mistaken I'm retracting the NACK, but I still think this patch is not justified enough. Rewriting of the persistent config should not be necessary.
On 09/11/2017 08:55 AM, Peter Krempa wrote:
On Mon, Sep 11, 2017 at 08:18:41 -0400, John Ferlan wrote:
On 09/11/2017 07:34 AM, Peter Krempa wrote:
On Wed, Sep 06, 2017 at 15:24:18 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1464313
As it turns out, the on-disk config file (e.g. not the stateDir file) needs to be updated when --override is provided since it's possible and highly probable that the def->source.format has been adjusted and could cause a future start after perhaps a libvirtd restart to have the older format from a define operation from the backend.
So in the 2 places where it's possible a write would be needed (create after define and build), let's perform a check and do the save/write operation on the configFile if it's necessary.
I don't think this is correct. If you use the "create" API with a custom XML this should not in any way touch the persistent config of the pool, similarly as we do with VMs. The provided XML may describe a completely different storage pool and using the 'create' API should not overwrite anything stored persistently.
If a user messes up the storage state via the live config, he needs to fix everything by themselves.
This would just overwrite persistent config which is not desired.
NACK
To both Create and Build or just Create?
This was for CreateXML but you are changing just "Create" which makes more sense, since that only touches the config which was already defined. I've mistaken those two, since CreateXML was at the end of the context of the first hunk. The code addition is actually in a different function.
But that leads to another question ... why is that even necessary? If you are overwriting a disk, then the source format (and all possible other config) should be taken from what is provided in the definition which already exists (that is the persistent config) and thus no update should be required.
On input "format" is optional. It's only used for some pools and "default" values at definition/parse time written out based on a table in storage_conf.c The storage_conf format definition for a disk pool is: ... static virStoragePoolTypeInfo poolTypeInfo[] = { ... {.poolType = VIR_STORAGE_POOL_DISK, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_DEVICE), .defaultFormat = VIR_STORAGE_POOL_DISK_UNKNOWN, ... Thus, if not supplied, the value written out is "unknown". For comparison, other build-able pool types: ... {.poolType = VIR_STORAGE_POOL_LOGICAL, ... .defaultFormat = VIR_STORAGE_POOL_LOGICAL_LVM2, ... {.poolType = VIR_STORAGE_POOL_FS, ... .defaultFormat = VIR_STORAGE_POOL_FS_AUTO, ... {.poolType = VIR_STORAGE_POOL_ZFS, ... .defaultFormat = VIR_STORAGE_FILE_RAW, ... {.poolType = VIR_STORAGE_POOL_VSTORAGE, ... .defaultFormat = VIR_STORAGE_FILE_RAW, ... We could alter the default format to be VIR_STORAGE_POOL_DISK_DOS so that when not specified, "dos" is written out at define time. This follows how the others do things. It could be argued they break the rules having the define/parse/format code perform the adjustment rather than having that adjustment only occur in the storage backend. Still if someone does supply "unknown" in the input XML, then the output has "unknown" as well since virStoragePoolDefParseSource finds something in the format type field.
Not sure I can visualize the Create path noted above, but it's still early in the morning with not enough coffee yet. Still the logic to decide to write out the config file is gated on whether the caller wanted overwrite (via VIR_STORAGE_POOL_BUILD_OVERWRITE flag being provided), but unlike VM's there's no flags to indicate whether we're "active" or "inactive".
virStoragePoolObjPtr->active? Some pools can't be inactive, but e.g. gluster pools can, so that is not universally true.
So if someone has indicated to use the overwrite flag, then that says to me all bets are off and not only should the live config be updated (as it is), but the on disk config as well since the decision by the consumer is to overwrite.
In such case the live config should be completely deleted and replaced by new config which is based on the persistent config. Same way as when we start VMs.
Without doing so we end up with an unusable pool the next time we start which is the crux of the bz.
I think the root cause here is somewhere else. If we are adding anything automatic (e.g. the format), it should be recorded in the original config (and XML) and not overwritten once you are going to build/start it.
Some more digging and experimenting led me to adjusting the Disk Start code to check if the config has UNKNOWN, then "assume" DOS and try the start (similar to the build code finds UNKNOWN and uses DOS). I don't think changing the storage_conf to use _DOS as the defaultFormat is really right, but if you believe so I have no issue altering that as well....
Since I was mistaken I'm retracting the NACK, but I still think this patch is not justified enough. Rewriting of the persistent config should not be necessary.
I'll send a v2 in a bit. It won't change the persistent on disk format, but will allow the start to occur and to have the expected format displayed. John
participants (2)
-
John Ferlan -
Peter Krempa