[libvirt] [PATCH 3/3]: Implement logical stopPool and deletePool more fully
This patch does a few things. 1) Clean up a bunch of comments which I believe are no longer valid, or just slightly wrong 2) Enable the stopPool command for the logical backend. The comment in the code worries about a situation where you try to "stop" the logical volume that your rootfs is on, for example. However, that situation is already taken care of by the LVM tools; if the logical volume you try to stop is active, it will throw an error saying that the LV is in use, and won't stop it. Therefore, it should be pretty safe to try to stop logical volumes; it will fail for ones that are in use, but will stop volumes that have been properly torn down ahead of time. 3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc. 4) In deletePool, implement pvremove of all source devices. Note that this should also be relatively safe; it will only pvremove devices that we had previously pvcreate'd in createPool, which means they were all under control of libvirt. It will not pvremove devices on an LVM that you previously created, but were just scanning with libvirt. Signed-off-by: Chris Lalancette <clalance@redhat.com>
On Fri, Sep 19, 2008 at 10:37:30AM +0200, Chris Lalancette wrote:
This patch does a few things.
1) Clean up a bunch of comments which I believe are no longer valid, or just slightly wrong
ACK
2) Enable the stopPool command for the logical backend. The comment in the code worries about a situation where you try to "stop" the logical volume that your rootfs is on, for example. However, that situation is already taken care of by the LVM tools; if the logical volume you try to stop is active, it will throw an error saying that the LV is in use, and won't stop it. Therefore, it should be pretty safe to try to stop logical volumes; it will fail for ones that are in use, but will stop volumes that have been properly torn down ahead of time.
Ok, that's the scenario I was concerned about, so looks safe to enable this then.
3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc.
Hmm, are you sure it won't prompt for a y/n confirmation ? IIRC, that was the reason I put in the -f. Perhaps simply ensuring stdin is /dev/null takes care of that problem.
4) In deletePool, implement pvremove of all source devices. Note that this should also be relatively safe; it will only pvremove devices that we had previously pvcreate'd in createPool, which means they were all under control of libvirt. It will not pvremove devices on an LVM that you previously created, but were just scanning with libvirt.
ACK. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Sep 19, 2008 at 10:27:33AM +0100, Daniel P. Berrange wrote:
On Fri, Sep 19, 2008 at 10:37:30AM +0200, Chris Lalancette wrote:
This patch does a few things.
1) Clean up a bunch of comments which I believe are no longer valid, or just slightly wrong
ACK
2) Enable the stopPool command for the logical backend. The comment in the code worries about a situation where you try to "stop" the logical volume that your rootfs is on, for example. However, that situation is already taken care of by the LVM tools; if the logical volume you try to stop is active, it will throw an error saying that the LV is in use, and won't stop it. Therefore, it should be pretty safe to try to stop logical volumes; it will fail for ones that are in use, but will stop volumes that have been properly torn down ahead of time.
Ok, that's the scenario I was concerned about, so looks safe to enable this then.
3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc.
Hmm, are you sure it won't prompt for a y/n confirmation ? IIRC, that was the reason I put in the -f. Perhaps simply ensuring stdin is /dev/null takes care of that problem.
Hum, I hope the behaviour of the command didn't changed that way between RHEL-5/F-8 and now. I just committed but maybe we can make sure to pass /dev/null, since that's the main specific option of vgremove, I guess it will ask by default at least in recent versions. I didn't find an easy way to nullify stdin in virRun, seems we need to do another wrapper for virExec, or I'm mistaken.
4) In deletePool, implement pvremove of all source devices. Note that this should also be relatively safe; it will only pvremove devices that we had previously pvcreate'd in createPool, which means they were all under control of libvirt. It will not pvremove devices on an LVM that you previously created, but were just scanning with libvirt.
ACK.
I commited that patch but apparently we need at least to check that pool deletion still works on F-9/10, so that's not completely done Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel P. Berrange wrote:
3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc.
Hmm, are you sure it won't prompt for a y/n confirmation ? IIRC, that was the reason I put in the -f. Perhaps simply ensuring stdin is /dev/null takes care of that problem.
This was already committed, but just to follow-up: First, I was wrong about RHEL-5. Apparently we've rebased the LVM tools in RHEL-5, so it does have the -f flag for vgremove. That being said, it's not actually necessary, based on my testing. Here's what I did: 1) pvcreate /dev/sdb 2) vgcreate MyVG2 /dev/sdb 3) lvcreate -L1G 0n guest1 /dev/MyVG2 At this point, I have a valid volume at /dev/MyVG2/guest1 4) lvremove -f /dev/MyVG2/guest1 (this corresponds to the code in virStorageBackendLogicalDeleteVol) 5) vgremove /dev/MyVG2 (this corresponds to the new code in virStorageBackendLogicalDeletePool) 6) pvremove /dev/sdb All commands completed without prompts, and remove the logical volumes. In step 4), if I do a vgremove, *then* I get a prompt saying: "Do you really want to remove volume group "MyVG2" containing 1 logical volumes? [y/n]: n" But that's only because you are trying to de-activate a volume group that has active logical volumes. The bottom-line is that I think we are OK with the new code, but we will need to do further testing/use cases to make sure. Chris Lalancette
On Tue, Sep 23, 2008 at 09:30:46AM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc.
Hmm, are you sure it won't prompt for a y/n confirmation ? IIRC, that was the reason I put in the -f. Perhaps simply ensuring stdin is /dev/null takes care of that problem.
This was already committed, but just to follow-up:
First, I was wrong about RHEL-5. Apparently we've rebased the LVM tools in RHEL-5, so it does have the -f flag for vgremove. That being said, it's not actually necessary, based on my testing. Here's what I did:
1) pvcreate /dev/sdb 2) vgcreate MyVG2 /dev/sdb 3) lvcreate -L1G 0n guest1 /dev/MyVG2
At this point, I have a valid volume at /dev/MyVG2/guest1
4) lvremove -f /dev/MyVG2/guest1 (this corresponds to the code in virStorageBackendLogicalDeleteVol) 5) vgremove /dev/MyVG2 (this corresponds to the new code in virStorageBackendLogicalDeletePool) 6) pvremove /dev/sdb
All commands completed without prompts, and remove the logical volumes. In step 4), if I do a vgremove, *then* I get a prompt saying:
"Do you really want to remove volume group "MyVG2" containing 1 logical volumes? [y/n]: n"
But that's only because you are trying to de-activate a volume group that has active logical volumes.
Ah well that's the edge case then. There is no requirement in the libvirt virStoragePoolDelete method that says all volumes must be removed before calling it. So its certainly possible we'll ht this scenario. We either have to explicitly remove all volumes ourselves in the Delete method, or we can trap & raise an error indicating that the app must remove them all. I tend towards the latter. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Sep 23, 2008 at 09:59:35AM +0100, Daniel P. Berrange wrote:
On Tue, Sep 23, 2008 at 09:30:46AM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
3) In deletePool, remove the -f from the vgremove command. Besides the fact that we probably don't usually want to force things, the -f option doesn't exist prior to F-9, so this would fail on F-8, RHEL-5, etc.
Hmm, are you sure it won't prompt for a y/n confirmation ? IIRC, that was the reason I put in the -f. Perhaps simply ensuring stdin is /dev/null takes care of that problem.
This was already committed, but just to follow-up:
First, I was wrong about RHEL-5. Apparently we've rebased the LVM tools in RHEL-5, so it does have the -f flag for vgremove. That being said, it's not actually necessary, based on my testing. Here's what I did:
1) pvcreate /dev/sdb 2) vgcreate MyVG2 /dev/sdb 3) lvcreate -L1G 0n guest1 /dev/MyVG2
At this point, I have a valid volume at /dev/MyVG2/guest1
4) lvremove -f /dev/MyVG2/guest1 (this corresponds to the code in virStorageBackendLogicalDeleteVol) 5) vgremove /dev/MyVG2 (this corresponds to the new code in virStorageBackendLogicalDeletePool) 6) pvremove /dev/sdb
All commands completed without prompts, and remove the logical volumes. In step 4), if I do a vgremove, *then* I get a prompt saying:
"Do you really want to remove volume group "MyVG2" containing 1 logical volumes? [y/n]: n"
But that's only because you are trying to de-activate a volume group that has active logical volumes.
Ah well that's the edge case then. There is no requirement in the libvirt virStoragePoolDelete method that says all volumes must be removed before calling it. So its certainly possible we'll ht this scenario. We either have to explicitly remove all volumes ourselves in the Delete method, or we can trap & raise an error indicating that the app must remove them all. I tend towards the latter.
Okay, so at this point i will rather revert that part of the patch adding back the -f flag, it feels safer for the upcoming release. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard