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

[Thomas Lamprecht]

Am 2/5/19 um 11:31 AM schrieb Alwin Antreich:
> '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>
> ---
> Added suggestions by Wolfgang B.

I'd prefer to either link to suggestions or list them here shortly inline,
this makes it easier for people looking at this and maybe also for you to
ensure that you did not forgot or actively chose to not address a point,
thanks!

> 
>  PVE/API2/Ceph/OSD.pm | 18 +++++++-----------
>  PVE/Ceph/Tools.pm    | 19 +++++++++++++++----
>  2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> index b4dc277e..8affca9a 100644
> --- a/PVE/API2/Ceph/OSD.pm
> +++ b/PVE/API2/Ceph/OSD.pm
> @@ -17,6 +17,7 @@ use PVE::RADOS;
>  use PVE::RESTHandler;
>  use PVE::RPCEnvironment;
>  use PVE::Tools qw(run_command file_set_contents);
> +use PVE::ProcFSTools;
>  
>  use base qw(PVE::RESTHandler);
>  
> @@ -394,7 +395,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,32 +402,30 @@ __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 = [];
>  
>  	    if ($param->{cleanup}) {
> -		if (my $fd = IO::File->new("/proc/mounts", "r")) {
> -		    while (defined(my $line = <$fd>)) {
> -			my ($dev, $path, $fstype) = split(/\s+/, $line);
> +		if (my $mp = PVE::ProcFSTools::parse_proc_mounts()) {
> +		    foreach my $line (@$mp) {
> +			my ($dev, $path, $fstype) = @$line;

Thanks for doing this, but as Wolfgang requested, this should be a followup
patch, i.e., separate. Has nothing to do with bug #2051 and the resulting
change...


>  			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 $part

I guess that Wolfgang's comment regarding dangling/unchecked untainting
was meant for all such constructs :-) Something like the following
(untested) could be a bit more readable:

my $dev_path = abs_path($dev);
$dev_path =~ m|^(/.+)| or die "invalid path: '$dev_path'\n";
my $data_part = $1;

>  			    push @$partitions_to_remove, $data_part;
>  			    last;
>  			}
>  		    }
> -		    close($fd);
>  		}
>  
>  		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..582e227a 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -4,6 +4,7 @@ use strict;
>  use warnings;
>  
>  use File::Path;
> +use File::Basename;
>  use IO::File;
>  
>  use PVE::Tools qw(run_command dir_glob_foreach);
> @@ -232,11 +233,21 @@ 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 $@;
> +	my $devname = File::Basename::fileparse($devpath);
 
I'd  just use basename, while we do not really depend on its bug/feature parity
with BSD's basename here, it'd be a bit clearer what it does by just reading.
Also, as explained not long ago[0] File::Basename exports a few things directly,
so the module part can be omitted (from both fileparse or basename).

[0]: https://pve.proxmox.com/pipermail/pve-devel/2019-January/035414.html

> +	my $dev_size = PVE::Tools::file_get_contents("/sys/class/block/$devname/size");
> +
> +	($dev_size) = $dev_size =~ m|(\d+)|; # untaint $dev_size
> +	die "Coulnd't get the size of the device $devname\n" if (!defined($dev_size));
> +
> +	my $size = ($dev_size * 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 $@;
>      }
>  };
>  
>