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

Fabian Grünbichler f.gruenbichler at proxmox.com
Wed Apr 15 11:28:34 CEST 2020


some style/design nits below

On March 11, 2020 2:05 pm, 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
>  - Adding tests
> 
> Relies on the corresponding patch in pve-manager.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> v2->v3:
>  - Relative instead of full paths
>  - Use run_command for system calls
>  - Die when a device is invalid instead of skipping
>  - More concise syntax
>  - More tests (with fake blockdevice path)
> v1->v2:
>  - Fix typo
>  - Add improvements
> 
> @Dominik: Thank you for the feedback!
> 
>  PVE/Diskmanage.pm     |  58 ++++++++++++++++++++
>  test/disklist_test.pm | 125 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 183 insertions(+)
> 
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index abb90a7..448f753 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -761,4 +761,62 @@ sub append_partition {
>      return $partition;
>  }
>  
> +# Return undef on error, the size of the blockdev otherwise

not so sure about this - why not make it 'die' for now, if we use it 
otherwise we can add $noerr with the usual semantics later? we die at 
the only call site if this returns undef ;)

> +sub get_blockdev_size {
> +    my ($dev) = @_;
> +    if (!defined($dev)) {
> +	warn "Undefined parameter";

die "undefined blockdev\n" if !defined...

> +	return undef;
> +    }
> +    if (!assert_blockdev($dev, 1)) {
> +	warn "$dev is not a blockdevice";
> +	return undef;

same here. just call assert_blockdev without $noerr.

> +    }
> +    my $size;
> +    my $exitcode = eval { run_command(

$exitcode is unused

> +	['blockdev', '--getsize64', $dev],
> +	(outfunc => sub { $size = shift })
> +    )};
> +    warn "run_command for \'blockdev\' syscall failed: $@" if $@;

nit: this is not a syscall ;) also, don't eval, just let it die.

> +    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.

200MB?

> +#   - {wipefs} additionally erase all available signatures from all devices.
> +#	Default: false
> +sub wipe_blockdevices {
> +    my ($devices, $options) = @_;
> +    my @dd_cmd = qw(dd if=/dev/zero iflag=count_bytes conv=fdatasync);
> +    my $max_amount = $options->{amount} // 209715200;

// 200 * 1024 * 1024;

makes it readable at a glance.

> +    die "$max_amount is not a natural number and thus an invalid value for max_amount."

die "invalid max_amount '$max_amount', must be a positive integer\n"

> +	if ($max_amount !~ /^[0-9]+$/);

\d instead of [0-9]

> +
> +    my %toDelete = ();

we could simple call this dev_sizes, and just store the retrieved sizes 
inside.

> +    foreach my $dev (@$devices) {
> +	my $device_size = get_blockdev_size($dev);

add an eval here, and set $dev_sizes->{$dev} directly

> +	die "Not wiping anything because could not get size of $dev." if !defined($device_size);

and add ' - $@' to the message here. but note - you probably want to 
check $dev for definedness at the start of the loop, else this triggers 
a warning for printing undef ;)

or, if you want to make it shorter with something like:

my $dev;
my $dev_sizes = { eval { map { $dev = $_ // ''; $_ => get_blockdev_size($_) } @$devices } };
die "... $dev ... - $@\n" if $@;

(could be made shorter if we don't care about including $dev in the 
error message, or make get_blockdev_size include it).

> +	my $amount = ($device_size < $max_amount) ? $device_size : $max_amount;

move this to the actual wiping below where it gets used

> +	$toDelete{$dev} = $amount;

can be dropped

> +    }
> +
> +    foreach my $dev (keys %toDelete) {
> +	print "Wiping device: $dev\n";
> +	if ($options->{wipefs}) {
> +	    eval { run_command(['wipefs', '-a', $dev]) };
> +	    warn $@ if $@;
> +	}

limit amount by $dev_sizes->{$dev} here

> +	# 1M seems reasonable for this case
> +	eval { run_command([@dd_cmd, "count=$toDelete{$dev}", "bs=1M", "of=$dev"]) };
> +	warn $@ if $@;
> +    }
> +}
> +
>  1;
> diff --git a/test/disklist_test.pm b/test/disklist_test.pm
> index 9cb6763..e5d339b 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,128 @@ while (readdir $dh) {
>      test_disk_list($dir);
>  }
>  
> +subtest 'get_blockdev_size' => sub {
> +    plan tests => 8;
> +
> +    my $blockdevice = '/fake/block/device';
> +    my $msg = "Test";
> +    $diskmanage_module->mock('assert_blockdev' => sub { return 1 });
> +    {
> +	$diskmanage_module->mock('run_command' => sub { die });
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture { PVE::Diskmanage::get_blockdev_size(undef) };
> +	is ($exitcode, undef, 'Return undef if parameter is undefined.');
> +	like ($stderr, qr!Undefined parameter!, 'Print warning for undefined parameter.');
> +    }
> +    {
> +	$diskmanage_module->mock('assert_blockdev' => sub { return undef });
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture { PVE::Diskmanage::get_blockdev_size($blockdevice) };
> +	is ($exitcode, undef, 'Return undef if device is not a blockdevice.');
> +	like ($stderr, qr!not a blockdev!, 'Print warning for invalid blockdevices.');
> +	$diskmanage_module->mock('assert_blockdev' => sub { return 1 });
> +    }
> +    {
> +	$diskmanage_module->mock('run_command' => sub { return 0 });
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture { PVE::Diskmanage::get_blockdev_size($blockdevice) };
> +	is ($exitcode, undef, 'Return undef when reading size from stdout failed.');
> +    }
> +    {
> +	$diskmanage_module->mock('run_command' => sub { die });
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture { PVE::Diskmanage::get_blockdev_size($blockdevice) };
> +	is ($exitcode, undef, 'Return undef on error without message.');
> +    }
> +    {
> +	$diskmanage_module->mock('run_command' => sub { die $msg});
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture { PVE::Diskmanage::get_blockdev_size($blockdevice) };
> +	is ($exitcode, undef, 'Return undef on error with message.');
> +	like ($stderr, qr!${msg}!, 'Print error message from run_command as warning.');
> +    }
> +    $testcount++;
> +};
> +
> +subtest 'wipe_blockdevices' => sub {
> +    plan tests => 12;
> +
> +    our $blockdevice = '/fake/block/device';
> +    sub wipe_helper {
> +	my ($options, $devices) = @_;
> +	$devices //= [$blockdevice];
> +	return Capture::Tiny::capture {
> +	    PVE::Diskmanage::wipe_blockdevices($devices, $options);
> +	};
> +    }
> +
> +    my $device_size = 0;
> +    $diskmanage_module->mock('run_command' => sub { });
> +    $diskmanage_module->mock('get_blockdev_size' => sub { return $device_size });
> +    {
> +	eval { PVE::Diskmanage::wipe_blockdevices([$blockdevice], {amount=>-3})};
> +	like ($@, qr/-3 is not/, 'Die with negative amounts');
> +    }
> +    {
> +	$diskmanage_module->mock('get_blockdev_size' => sub { return undef });
> +	eval { PVE::Diskmanage::wipe_blockdevices([$blockdevice])};
> +	like ($@, qr!Not wiping anything!, 'Die when helper returns undef');
> +	$diskmanage_module->mock('get_blockdev_size' => sub { return $device_size });
> +    }
> +    {
> +	$diskmanage_module->unmock('get_blockdev_size');
> +	$diskmanage_module->mock('assert_blockdev' => sub { return undef });
> +	my (undef, $stderr, $exitcode) = Capture::Tiny::capture {
> +	    eval { PVE::Diskmanage::wipe_blockdevices([$blockdevice]) }
> +	};
> +	like ($@, qr!Not wiping anything!, 'Die if helper finds an invalid device');
> +	like ($stderr, qr!is not a blockdev!, 'Helper prints a reason for dying');
> +	$diskmanage_module->mock('get_blockdev_size' => sub { return $device_size });
> +    }
> +    {
> +	$diskmanage_module->mock('get_blockdev_size' => sub { return undef });
> +	eval { PVE::Diskmanage::wipe_blockdevices([$blockdevice])};
> +	like ($@, qr!Not wiping anything!, 'Die with invalid devices');
> +	$diskmanage_module->mock('get_blockdev_size' => sub { return $device_size });
> +    }
> +    {
> +	my ($stdout, undef, undef) = wipe_helper();
> +	like ($stdout, qr!Wiping device: $blockdevice\n!, 'Invoking with a disk');
> +    }
> +    {
> +	my ($stdout, undef, undef) = wipe_helper(undef, ["${blockdevice}1"]);
> +	like ($stdout, qr!Wiping device: ${blockdevice}1\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);
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 




More information about the pve-devel mailing list