[pve-devel] [PATCH v2 manager] add wipe_disk option when destroying ceph disk

[Thomas Lamprecht]

On 10/24/18 10:43 AM, David Limbeck wrote:
> On 10/24/18 10:31 AM, Thomas Lamprecht wrote:
>> On 10/24/18 9:32 AM, David Limbeck wrote:
>>> this allows the disk to be reused as ceph disk by zeroing the first 200M
>>> of the destroyed disk
>>>
>>> Signed-off-by: David Limbeck <d.limbeck at proxmox.com>
>>> ---
>>> since v1:
>>>      wipe is always done after remove_partition
>>>      fdatasync is used to make sure data is synced on some disks
>>>      (as proposed by Alwin)
>>>
>>>   PVE/API2/Ceph.pm | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
>>> index 69489a70..a0b5042d 100644
>>> --- a/PVE/API2/Ceph.pm
>>> +++ b/PVE/API2/Ceph.pm
>>> @@ -434,6 +434,13 @@ __PACKAGE__->register_method ({
>>>           }
>>>           }
>>>   +        my $disks_to_wipe = {};
>>> +        foreach my $part (@$partitions_to_remove) {
>>> +        next if !$part || (! -b $part );
>>> +        my $devpath = PVE::Diskmanage::get_blockdev($part);
>>> +        $disks_to_wipe->{$devpath} = 1;
>>> +        }
>> Could be done in single line with:
>>
>> my $disks_to_wipe = {
>>      map {  PVE::Diskmanage::get_blockdev($_) => 1 } grep { -b } @partitions_to_remove
>> };
>>
>> I find it much nicer, as more concise and using what perl provides,
>> but I don't think a lot other here think the same way, so take with
>> caution ;-)
> 
> Same style as $remove_partition.
> 
>>
>> But I have another question....
>>
>>> +
>>>           print "Unmount OSD $osdsection from  $mountpoint\n";
>>>           eval { run_command(['/bin/umount', $mountpoint]); };
>>>           if (my $err = $@) {
>>> @@ -443,6 +450,11 @@ __PACKAGE__->register_method ({
>>>           foreach my $part (@$partitions_to_remove) {
>>>               $remove_partition->($part);
>> ...why don't you do the wiping in the $remove_partition closure?
>>
>> Because your code seems to introduce two additional and unnecessary for
>> loops over  $partitions_to_remove
>>
>> Just doing the checks in the aforementioned closure, if those checks are
>> needed at all, and run the command there? Should be much shorter (almost
>> a one-line only change)?
>>
> Only 1 additional loop over $partitions_to_remove to create a hash set for the disks. This was done so partitions could be deleted first before wiping everything as well as only wiping once per disk.

Even then the 'add to hash' functionality could be done in the closure,
removes one forerach entirely and the PVE::Diskmanage::get_blockdev is used
there already.

it also may be worth mentioning such thoughts in a short statement in the
commit message, the "why" is always nice to know for review :-)

>>>           }
>>> +        foreach my $devpath (keys %$disks_to_wipe) {
>>> +            print "wipe disk: $devpath\n";
>>> +            eval { run_command(['/bin/dd', 'if=/dev/zero', "of=${devpath}", 'bs=1M', 'count=200', 'conv=fdatasync']); };
>>> +            warn $@ if $@;
>>> +        }
>>>           }
>>>       };
>>>