[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#375491: yet another lvm_removal_on_demand patch suggestion

David Härdeman wrote:
On Thu, Aug 24, 2006 at 09:27:49PM +0200, Ronny Aasen wrote:
Index: debian/partman-auto-lvm.templates
--- debian/partman-auto-lvm.templates    (revision 40192)
+++ debian/partman-auto-lvm.templates    (working copy)
@@ -37,6 +37,15 @@
You can choose to ignore this warning, but that may result in a failure to
 reboot the system after the installation is completed.

+Template: partman-auto-lvm/purge_lvm_from_device
+Type: boolean
+Default: false
+_Description: Remove all Logical Volume Configuration?
+ Before creating any Logical Volume Groups, we need to remove all exsisting
+ Logical Volumes and Volume Groups from the disk we are partitioning.

In general, I think phrases such as "we" should be avoided, but it would be better if Christian/Kamion/joeyh could review the template.

+ .
+ WARNING     This will erase all data on LVM Partitions
This will erase all data on the LVM volumes and on the disk?

Template: partman-auto-lvm/pv_on_device
Type: error
_Description: Existing physical volume on the selected device
Index: autopartition-lvm
--- autopartition-lvm    (revision 40192)
+++ autopartition-lvm    (working copy)
@@ -26,10 +26,52 @@
    log-output -t update-dev update-dev

# Check if the device already contains any physical volumes
realdev=$(mapdevfs "$(cat $dev/device)")
if pv_on_device "$realdev"; then
-    bail_out pv_on_device
+ # Ask for mermission to erase LVM volumes + db_set partman-auto-lvm/purge_lvm_from_device "false"

I think db_set will override preseedings (as Geert said)...
+        db_input critical partman-auto-lvm/purge_lvm_from_device
+        db_go

should be db_go || exit 1?

if you say so :)

+        db_get partman-auto-lvm/purge_lvm_from_device
+ if [ "$RET" = "true" ] ;then + + targetvg=
+        #what volume groups is on any of the the disk partitions.
+        all_volume_groups=$(vg_list)
+        #we only care about vg's on the selected disk
+        for vg in $all_volume_groups
+ do + if [ "$(vg_list_pvs "$vg" | grep -c "$realdev")" != "0" ] ; then

this looks fishy...will it actually return zero? Shouldn't this be:
if $(vg_list_pvs "$vg" | grep -q "$realdev"); then

grep -c returns a number of lines matching. 0 if none do. but -q will work as well and probably be a cleaner way to do it

+                targetvg=${targetvg}" $vg"

weird quotation marks.....targetvg="$targetvg $vg" would be expected here

+            fi
+        done
+        for vg in $targetvg
+        do
+ #make sure the volume groups on the target disk don't span any other disks.

very good idea

+            if [ "$(lvm_get_info vgs pv_count "$vg")" != "1" ] ; then
+                log-output -t partman-auto-lvs vgs
+                bail_out pv_on_device
+            fi
+        done
+ + #it should now be safe to remove the vg's on the target disks
+        #remove lv's  from the target vg's.
+        for vg in $targetvg
+        do
+            for lv in $(vg_list_lvs $vg)
+            do
+                #remove the logical volumes on the volume group
+                   lv_delete $vg $lv
+            done
+            #remove the volume group
+            vg_delete $vg
+        done
+    else
+        bail_out pv_on_device
+    fi

in addition to lv_delete and vg_delete, a pv_delete should also be done. It needs to be added to lvm-tools.sh, and should call pvremove, I've just added it myself :)


choose_recipe "$free_size" lvm || exit $?

After having looked at this, I'm wondering whether this shouldn't be added to wipe_disk() in partman-auto/auto-shared.sh instead. If it was, that would mean that all methods would be able to warn that they are about to remove LVM VG/LV/PV's before actually doing so.

Of course, it would need to include a check whether the lvm-tools.sh script is present before doing so, but I think it would be a good idea in general.

I've attached a patch which details what this could look like...

Thanks for your hard work on this and sorry that I've jumped in with my comments so late :)


As long as it happens, it's good :)
thanks for reviewing and fixing, hope removal of lvm volume will be included in next partman :)


Reply to: