[pve-devel] [PATCH manager v2] Fix #2051: preserve DB/WAL disk on destroy

[Wolfgang Bumiller]

On Wed, Jan 30, 2019 at 10:55:37AM +0100, 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.
> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm | 10 +++-------
>  PVE/Ceph/Tools.pm    | 16 ++++++++++++----
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> index b4dc277e..3214498a 100644
> --- a/PVE/API2/Ceph/OSD.pm
> +++ b/PVE/API2/Ceph/OSD.pm
> @@ -394,7 +394,6 @@ __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) = @_;
>  
> @@ -402,11 +401,10 @@ __PACKAGE__->register_method ({
>  		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 = [];
> @@ -418,7 +416,7 @@ __PACKAGE__->register_method ({

I noticed this loop parses /proc/mounts "quick & dirty" - would you be
willing to send a followup patch changing it to use
PVE::ProcFSTools::parse_proc_mounts()? It also takes care of potentially
required decoding of entries.

>  			next if !($dev && $path && $fstype);
>  			next if $dev !~ m|^/dev/|;
>  			if ($path eq $mountpoint) {
> -			    my $data_part = abs_path($dev);
> +			    my ($data_part) = abs_path($dev) =~ m|^(/.+)|; #untaint $data_part
>  			    push @$partitions_to_remove, $data_part;
>  			    last;
>  			}
> @@ -427,7 +425,7 @@ __PACKAGE__->register_method ({
>  		}
>  
>  		foreach my $path (qw(journal block block.db block.wal)) {
> -		    my $part = abs_path("$mountpoint/$path");
> +		    my ($part) = abs_path("$mountpoint/$path") =~ m|^(/.+)|; # untaint $part
>  		    if ($part) {
>  			push @$partitions_to_remove, $part;
>  		    }
> @@ -443,8 +441,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..0de72483 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -232,11 +232,19 @@ 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);
> +
>      foreach my $devpath (@devs) {
> -        print "wipe disk: $devpath\n";
> -        eval { run_command([@wipe_cmd, "of=${devpath}"]) };
> -        warn $@ if $@;
> +	$devpath =~ m|([^/]*)$|;

This looks like `basename()` from File::Basename (which is in the
perl-modules package and already imported in API2/APT), so you could use
that. One less dangling regex match.

> +	my $dev_size = PVE::Tools::file_get_contents("/sys/class/block/$1/size");
> +
> +	$dev_size =~ m|(\d+)|; # untaint $dev_size

I'm not a big fan of having those matches dangle around like that
unchecked. While we know this comes from sysfs, it's "weird" not to at
least `or die "..."`.

> +	my $size = ($1 * 512 / 1024 / 1024);
> +	my $count = ($size < 200) ? $size : 200;
> +
> +	print "wipe disk/partition: $devpath\n";
> +	eval { run_command([@wipe_cmd, "count=$count", "of=${devpath}"]) };
> +	warn $@ if $@;
>      }
>  };
>  
> -- 
> 2.11.0