Re: [libvirt] [PATCH] logical: Allow [no]overwrite option for poolBuild

On 11/16/2016 07:42 AM, Erik Skultety wrote: > On Tue, Nov 15, 2016 at 04:55:10PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1373711 >> >> When using pool-create --build or pool-define and pool-build to create/define >> and build/initialize the logical pool, the 'pvcreate' command could fail if >> some previous attempt had been used on the source path previously. >> >> So add support for the --override option in order to force usage of the >> magic override option "-ff" and the option "-y" in order to force creation >> and answer any questions with yes. >> >> NB: Although the reality is part of the process of building the logical >> pool involves wiping out the first 512 bytes of the disk block to be >> added (e.g. the partition table) because pvcreate requires that. So, to >> a degree the device being added is already altered. I suspect the knowlege >> of whether the disk was in a vg already could be contained outside the >> range of the first 512 bytes. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/storage/storage_backend_logical.c | 26 +++++++++++++++++++++----- >> 1 file changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c >> index ca05fe1..9c76156 100644 >> --- a/src/storage/storage_backend_logical.c >> +++ b/src/storage/storage_backend_logical.c >> @@ -682,13 +682,18 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, >> virStoragePoolObjPtr pool, >> unsigned int flags) >> { >> - virCommandPtr vgcmd; >> + virCommandPtr vgcmd = NULL; >> int fd; >> char zeros[PV_BLANK_SECTOR_SIZE]; >> int ret = -1; >> size_t i; >> >> - virCheckFlags(0, -1); >> + virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | >> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); >> + >> + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE, >> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, >> + cleanup); >> >> memset(zeros, 0, sizeof(zeros)); >> >> @@ -731,10 +736,21 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, >> /* >> * Initialize the physical volume because vgcreate is not >> * clever enough todo this for us :-( >> + * >> + * If this API is called twice for the same device, then because >> + * vgcmd adds the device to the volume group, the pvcreate will >> + * fail since the pv is already part of the vg. Allow use of the >> + * override option inorder to overrule! >> */ >> - pvcmd = virCommandNewArgList(PVCREATE, >> - pool->def->source.devices[i].path, >> - NULL); >> + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) >> + pvcmd = virCommandNewArgList(PVCREATE, >> + "-ff", "-y", >> + pool->def->source.devices[i].path, >> + NULL); >> + else >> + pvcmd = virCommandNewArgList(PVCREATE, >> + pool->def->source.devices[i].path, >> + NULL); >> if (virCommandRun(pvcmd, NULL) < 0) { >> virCommandFree(pvcmd); >> goto cleanup; >> -- >> 2.7.4 > > NACK. > > I'm not really convinced whether we should really do this. I'm not quite > familiar with LVM, so I tried a couple usecases. Since a forceful recreation of > a physical partition on a device by pvcreate will permanently wipe all the > volume group metadata, once you're done with the volume group you cannot remove > it in a straightforward manner: > > [root@f23-a ~]# pvcreate /dev/vdc > Physical volume "/dev/vdc" successfully created > [root@f23-a ~]# vgcreate myvg /dev/vdb /dev/vdc > Volume group "myvg" successfully created > [root@f23-a ~]# pvcreate /dev/vdc -ff -y > WARNING: Forcing physical volume creation on /dev/vdc of volume group "myvg" > Physical volume "/dev/vdc" successfully created > [root@f23-a ~]# vgremove myvg > WARNING: Device for PV zConQ1-eSy3-xvQu-5mWv-kVdT-v3yI-LwZBk3 not found or > rejected by a filter. > Volume group "myvg" not found, is inconsistent or has PVs missing. > > Now, I also tried removing it forcefully (-f) which worked and which is also > something pool-delete does, for me it just doesn't seem right to allow the user > to corrupt the volume group by allowing --overwrite, thus -ff to pvcreate. > Another thing is that if you do pvcreate whilst having the volume group active, > what you get is: > > [root@f23-a ~]# pvcreate /dev/vdc -ff -y > Can't open /dev/vdc exclusively. Mounted filesystem? > > So, even the -ff isn't enough. Anyway, since you were rather skeptical about > this BZ in your comment, I suggest we close it as WONTFIX and the testcase > should either be adjusted or dropped completely because as you say, there > basically isn't any practical usage for that and the only thing you get out of > it is a corrupted LVM anyway. > > Erik > Fair enough - Still the 'Build' process is doing: In a loop for all source devices: dd if=/dev/zero of=/dev/sdN bs=512 count=1 pvcreate /dev/sdN

Then once done, all the vgcreate the devices vgcreate $name /dev/sdN [/dev/sdX] Where the 'dd' is always done and the vgcreate fails for this bug. So I wonder if the build code should be augmented to get a list of devices already in the vg and either skip or error if there's an attempt to add it again.

We have code virStorageBackendLogicalGetPoolSources to get a list of pv's for all vg's, so we could take the other approach... In fact, if the vg already exists, one would think we shouldn't be building it again. Adding a pv to an existing vg is something left "outside" of libvirt too IIRC. John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list