[pve-devel] [PATCH manager] Fix #2051: sub wipe_disks wipes partitions too

[Thomas Lamprecht]

For the commit message try to avoid mentioning perl specifics if it's not
addressing a perl specific issue.. I see that you touch wipe_disks in the
diff, for the commit message itself OK, but not in the subject.
Maybe something like:

Fix #2051: preserve DB/WAL possible still in use when destroying OSDs

On 1/21/19 1:44 PM, Alwin Antreich wrote:
> 'pveceph osd destroy <num> --cleanup'
> 
> When executing the command above, all disks associated with the OSD are
> at the moment wiped with dd (incl. separate disks with DB/WAL).
> 
> The patch adds the ability to 'wipe_disks' to wipe the partition instead
> of the whole disk. This preserves the sparate DB/WAL disks.

s/sparate/separate/

> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm |  8 +++-----
>  PVE/Ceph/Tools.pm    | 20 ++++++++++++++++----
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> index b4dc277e..939958d9 100644
> --- a/PVE/API2/Ceph/OSD.pm
> +++ b/PVE/API2/Ceph/OSD.pm
> @@ -1,5 +1,6 @@
>  package PVE::API2::Ceph::OSD;
>  
> +
>  use strict;
>  use warnings;
>  
> @@ -394,19 +395,18 @@ __PACKAGE__->register_method ({
>  	    # try to unmount from standard mount point
>  	    my $mountpoint = "/var/lib/ceph/osd/ceph-$osdid";
>  
> -	    my $disks_to_wipe = {};
>  	    my $remove_partition = sub {
>  		my ($part) = @_;
>  
>  		return if !$part || (! -b $part );
> +		($part) = $part =~ m|^(/.+)|; # untaint $part

maybe to the untaint below where we itterate over proc/mounts and check already for
/dev:
next if $dev !~ m|^/dev/|;
(one could add a capture group and just set $dev to $1 afterwards.)

>  		my $partnum = PVE::Diskmanage::get_partnum($part);
>  		my $devpath = PVE::Diskmanage::get_blockdev($part);
>  
> +		PVE::Ceph::Tools::wipe_disks($part);
>  		print "remove partition $part (disk '${devpath}', partnum $partnum)\n";
>  		eval { run_command(['/sbin/sgdisk', '-d', $partnum, "${devpath}"]); };
>  		warn $@ if $@;
> -
> -		$disks_to_wipe->{$devpath} = 1;
>  	    };
>  
>  	    my $partitions_to_remove = [];
> @@ -443,8 +443,6 @@ __PACKAGE__->register_method ({
>  		foreach my $part (@$partitions_to_remove) {
>  		    $remove_partition->($part);
>  		}
> -
> -		PVE::Ceph::Tools::wipe_disks(keys %$disks_to_wipe);
>  	    }
>  	};
>  
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 0ada98cf..f0a3cb54 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -232,11 +232,23 @@ sub systemd_managed {
>  sub wipe_disks {
>      my (@devs) = @_;
>  
> -    my @wipe_cmd = qw(/bin/dd if=/dev/zero bs=1M count=200 conv=fdatasync);
> +    my @wipe_cmd = qw(/bin/dd if=/dev/zero bs=1M conv=fdatasync);
> +    my $count = 200;
> +    my @blockdev = qw(/sbin/blockdev --getsize64);
> +    my $dev_size = 0;
> +
>      foreach my $devpath (@devs) {
> -        print "wipe disk: $devpath\n";
> -        eval { run_command([@wipe_cmd, "of=${devpath}"]) };
> -        warn $@ if $@;
> +	eval { run_command([@blockdev, "$devpath"],
> +	    outfunc => sub { $dev_size = shift; })};

this is really hard to read now... first break up the eval body into a newline,
as a starter, then I'd keep it a single line even if it needs more than 80 chars,
or at least keep "outfunc => sub {" on the first line.

Does reading "/sys/class/block/PARTITION/size" works too? Would sound a bit
saner/faster than spawing multiple processes for this...

> +	warn $@ if $@;
> +
> +	my $size = ($dev_size / 1024 / 1024);
> +	$count = $size if ($size lt $count);

why lt? we never use this, use <
Also, you do not reset the $count variable to 200 at loop start...
So if you have multiple @devs passed where the first is 100M big and the
second 150M you will wipe the full first one but only 100M from the second
one..

maybe do $count away completely and just do something like:
$size = ($size < 200) ? $size : 200; # we only need to wipe the first 200M

> +	push @wipe_cmd, "count=$count";

you add "count=$count" to the @wipe_cmd list _each_ iteration of this loop,
meaning that you'll get a failure from dd if more than one @devs is passed.

need to think a bit more about the general approach...

> +
> +	print "wipe disk/partition: $devpath\n";
> +	eval { run_command([@wipe_cmd, "of=${devpath}"]) };
> +	warn $@ if $@;
>      }
>  };
>  
>