Re: [libvirt] [PATCH] storage: Check and possibly update config file after build
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.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)