[pve-devel] [PATCH v2 storage 2/3] Incorporate wipe_disks from PVE::Ceph::Tools

[Dominik Csapak]

Comments inline

On 2/25/20 11:28 AM, Dominic Jäger wrote:
> Move wipe_disks from PVE::Ceph::Tools to PVE::Diskmanage and improve it by
>   - Handling invalid parameters
>   - Adding options for wiping
>   - Making names clearer, especially the sub name to indicate that it works for
>     partitions, too.
>   - Use `blockdev` system call because it is easier to understand
>   - Adding tests
> 
> Relies on the corresponding patch in pve-manager.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> v1->v2:
>   - Fix typo
>   - Not only move the function but also improve it
> 
>   PVE/Diskmanage.pm     | 63 ++++++++++++++++++++++++++++++++++++++
>   test/disklist_test.pm | 71 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 134 insertions(+)
> 
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index abb90a7..09d89a0 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -761,4 +761,67 @@ sub append_partition {
>       return $partition;
>   }
>   
> +sub get_blockdev_size {
> +    my ($dev) = @_;
> +    my $size = `blockdev --getsize64 $dev`;

this is prone to command injection, if somehow someone manages to
call this with $dev set to e.g., 'foo; rm -rf /*'

i'd rather leave that a read from sysfs or use run_command
if you really want to use blockdev

> +    chomp $size;
> +    return $size
> +}
> +
> +# Wipe the first part of a device
> +#
> +# Calls dd and optionally wipefs. This is useful to clear off leftovers from
> +# previous use as it avoids failing OSD and directory creation.
> +#
> +# $devices array of blockdevices (including partitions), e.g. ['/dev/sda3']
> +# $options hash with
> +#   - {amount} number of bytes that will be deleted. Will wipe at most the
> +#	device size. Default: 209715200.
> +#   - {wipefs} additionally erase all available signatures from all devices.
> +#	Default: false
> +sub wipe_blockdevices {
> +    my ($devices, $options) = @_;
> +
> +    my @dd_cmd = qw(/bin/dd if=/dev/zero iflag=count_bytes conv=fdatasync);
> +
> +    my $max_amount = $options->{amount};
> +    if (!defined $max_amount) {
> +	$max_amount = 209715200;
> +    } elsif ($max_amount !~ /^\d+$/) {
> +	die "Dying because $max_amount is not a natural number ".
> +	    "and thus not a valid value for max_amount.";
> +    }

nitpick: probably better to write as:

$max_amount = $options->{amount} // 209...;
die "[..]" if $max_amount !~ m/^[0-9]+$/;

> +
> +    foreach my $dev (@$devices) {
> +	eval { assert_blockdev($dev) };
> +	if ($@) {
> +	    warn "Skipping $dev because it is not a blockdevice";
> +	    next;
> +	}

does it really make sense to skip 'invalid' devices?
we want to give a list of things to wipe and just skip one
if it is not a blockdev?

would it not be better to assert all devs first, and error out
if any of them is not a valid blockdev before actually wiping anything?

> +
> +	my $device_size = get_blockdev_size($dev);
> +	my $amount;
> +	if (defined $device_size) {

nit: please use brackets: defined($foo)

> +	    $amount = ($device_size < $max_amount) ? $device_size : $max_amount;
> +	} else {
> +	    warn "Could not get size of $dev. Wiping nonetheless but dd might throw errors.";
> +	    $amount = $max_amount;

when can this happen? are there blockdevs (asserted above) that have no 
size?
if yes, does it really makes sense to write something to it?

> +	}
> +
> +	print "Wiping device: $dev\n";
> +	if ($options->{wipefs}) {
> +	    # wipefs location:
> +	    #  - Debian  9: /sbin/wipefs
> +	    #  - Debian 10: /usr/sbin/wipefs
> +	    # The symlink from /sbin to /usr/sbin makes this work for fresh PVE

the comment is odd:
pve6 here:

# which wipefs
/sbin/wipefs

# dpkg -S /sbin/wipefs
util-linux

# dpkg -L util-linux
/sbin/wipefs
/usr/share/bash[...]
/usr/share/man[...]

> +	    # 6 as well as upgraded ones
> +	    eval { run_command(['/sbin/wipefs', '-a', $dev]) };
> +	    warn $@ if $@;
> +	}
> +	# 1M seems reasonable for this case
> +	eval { run_command([@dd_cmd, "count=$amount", "bs=1M", "of=${dev}"]) };
> +	warn $@ if $@;
> +    }
> +}
> +
>   1;
> diff --git a/test/disklist_test.pm b/test/disklist_test.pm
> index 9cb6763..280ea41 100644
> --- a/test/disklist_test.pm
> +++ b/test/disklist_test.pm
> @@ -10,6 +10,7 @@ use PVE::Tools;
>   
>   use Test::MockModule;
>   use Test::More;
> +use Capture::Tiny;
>   use JSON;
>   use Data::Dumper;
>   
> @@ -248,4 +249,74 @@ while (readdir $dh) {
>       test_disk_list($dir);
>   }
>   
> +
> +subtest 'wipe_blockdevices' => sub {
> +
> +    plan tests => 9;
> +    sub wipe_helper {
> +	my ($options, $devices) = @_;
> +	$devices //= ['/dev/sdb'];
> +	return Capture::Tiny::capture {
> +	    PVE::Diskmanage::wipe_blockdevices($devices, $options);
> +	};
> +    }
> +
> +    $diskmanage_module->mock('run_command' => sub { });
> +    my $device_size = 0;
> +    $diskmanage_module->mock('get_blockdev_size' => sub { return $device_size });
> +    {
> +	eval { PVE::Diskmanage::wipe_blockdevices(['/dev/sdb'], {amount=>-3})};
> +	like ($@, qr/-3 is not/, 'Die with negative amounts');
> +    }
> +    {
> +	$diskmanage_module->mock('assert_blockdev' => sub { die });
> +	my (undef, $stderr, undef) = wipe_helper();
> +	like ($stderr, qr!Skipping /dev/sdb!, 'Skip invalid devices');
> +	$diskmanage_module->mock('assert_blockdev' => sub { });
> +    }
> +    {
> +	my ($stdout, undef, undef) = wipe_helper();
> +	like ($stdout, qr!Wiping device: /dev/sdb\n!, 'Invoking with a disk');
> +    }
> +    {
> +	my ($stdout, undef, undef) = wipe_helper(undef, ['/dev/sdb1']);
> +	like ($stdout, qr!Wiping device: /dev/sdb1\n!, 'Invoking with a partition');
> +    }
> +
> +    $diskmanage_module->mock('run_command' => sub { print Dumper(@_) });
> +    my $default_max_wipe_amount = 209715200;
> +    {
> +	$device_size = $default_max_wipe_amount-1;
> +	my ($stdout, undef, undef) = wipe_helper();
> +	like ($stdout, qr!count=$device_size!,
> +		'Device size is smaller than default maximum wipe amount');
> +    }
> +    {
> +	$device_size = $default_max_wipe_amount+1;
> +	my ($stdout, undef, undef) = wipe_helper();
> +	like ($stdout, qr!count=$default_max_wipe_amount!,
> +		'Device size is bigger than default maximum wipe amount');
> +    }
> +
> +    my $changed_max_wipe_amount = 50000;
> +    {
> +	$device_size = $changed_max_wipe_amount-1;
> +	my ($stdout, undef, undef) = wipe_helper({amount=>$changed_max_wipe_amount});
> +	like ($stdout, qr!count=$device_size!,
> +		'Device size is smaller than changed maximum wipe amount');
> +    }
> +    {
> +	$device_size = $changed_max_wipe_amount+1;
> +	my ($stdout, undef, undef) = wipe_helper({amount=>$changed_max_wipe_amount});
> +	like ($stdout, qr!count=$changed_max_wipe_amount!,
> +		'Device size is bigger than changed maximum wipe amount');
> +    }
> +    {
> +	$device_size = 0;
> +	my ($stdout, $stderr, undef) = wipe_helper({wipefs=>1});
> +	like ($stdout, qr!/wipefs!, 'Call wipefs');
> +    }
> +    $testcount++;
> +};
> +
>   done_testing($testcount);
> 

good that you included tests :)

maybe it is my paranoia, but i would prefer to use a path that
is not a real blockdev e.g., /tmp/pve/fakeblock
just in case that run command is not actually mocked?

also i guess those tests only work if there is actually a /dev/sdb since 
i do not see an 'assert_blockdev' mock at the beginning so the first 
test would fail?