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

[David Limbeck]

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.
>>   		}
>> +		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 $@;
>> +		}
>>   	    }
>>   	};
>>   
>>