Bug#375491: yet another lvm_removal_on_demand patch suggestion

[Ronny Aasen <ronny@skolelinux.org>]

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
>                                                                    
> ^^^^^^^^^
>                                     existing
> > + 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
> > fi
> >
> > +
> > # 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 :)
>
> > fi
> >
> > 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 :)
>
> Regards,
> David 

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

Regards
Ronny