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.jae...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
