[pve-devel] [PATCH storage 1/3] Diskmanage: change parttype uuid detection
Fabian Grünbichler
f.gruenbichler at proxmox.com
Thu Jul 11 16:32:43 CEST 2019
some style/naming stuff inline, with one exception. rest of the series
looks good.
On Thu, Jul 11, 2019 at 12:49:16PM +0200, Dominik Csapak wrote:
> previously ceph included a udev rule to populate
> /dev/disk/by-parttypeuuid/
>
> but not anymore, so we now use 'lsblk --json -o path,parttype' to
> get a mapping between parttype uuid and partition
>
> fix the test by simulating empty lsblk output
>
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> PVE/Diskmanage.pm | 108 ++++++++++++++++++++++++++++--------------
> test/disklist_test.pm | 9 +++-
> 2 files changed, 79 insertions(+), 38 deletions(-)
>
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 0deb1a6..8230aad 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -6,6 +6,7 @@ use PVE::ProcFSTools;
> use Data::Dumper;
> use Cwd qw(abs_path);
> use Fcntl ':mode';
> +use JSON;
>
> use PVE::Tools qw(extract_param run_command file_get_contents file_read_firstline dir_glob_regex dir_glob_foreach trim);
>
> @@ -15,6 +16,7 @@ my $SGDISK = "/sbin/sgdisk";
> my $PVS = "/sbin/pvs";
> my $LVS = "/sbin/lvs";
> my $UDEVADM = "/bin/udevadm";
> +my $LSBLK = "/bin/lsblk";
>
> sub verify_blockdev_path {
> my ($rel_path) = @_;
> @@ -154,7 +156,38 @@ sub get_smart_data {
> return $smartdata;
> }
>
> +sub get_lsblk_info() {
I'd name this less general, unless you plan on including other
information as well?
> + my $cmd = [$LSBLK, '--json', '-o', 'path,parttype'];
> + my $output = "";
> + my $res = {};
> + eval {
> + run_command($cmd, outfunc => sub {
> + $output .= "$_\n";
> + });
> + };
> + warn "$@\n" if $@;
at this point $output might be an empty string, in which case we can
just return an empty list. not very likely to happen, but for certain
errors lsblk just prints an error message on stderr and nothing on stdout.
this is the only thing that IMHO must really be cleared up, as it can cause
unnecessary and confusing warnings from decode_json.
> + my $list = [];
> + eval {
> + $list = decode_json($output);
> + $list = $list->{blockdevices}; # pull out the device list
> + };
> + warn "$@\n" if $@;
now $list is either
[] (decode_json failed or no blockdevices found)
undef (json does not contain blockdevices key)
a list of blockdevices
since you don't use $list much, why not something like
my $parsed = eval { json_decode($output) };
warn "$@\n" if $@;
my $list = $parsed->{blockdevices} // [];
much more readable IMHO.
> +
> + for my $dev (@$list) {
s/for/foreach
here and more below..
(according to our style guide, although this file in particular already
has lots of 'for's :-P)
> + next if !($dev->{parttype});
> + my $type = $dev->{parttype};
> + my $path = $dev->{path};
> + if (!$res->{$type}) {
> + $res->{$type} = [];
> + }
> + push @{$res->{$type}}, $path;
this could also be shortened somewhat:
next if !($dev->{parttype});
my $type = $dev->{parttype};
$res->{$type} = [] if !defined($res->{type});
push @{$res->{$type}}, $dev->{path};
> + }
> +
> + return $res;
> +}
> +
> sub get_zfs_devices {
> + my ($lsblk) = @_;
I'd rename this parameter - it's not important where this information
comes from, but what it is - a mapping from (part)uuids to device paths
> my $list = {};
>
> return {} if ! -x $ZPOOL;
> @@ -176,19 +209,24 @@ sub get_zfs_devices {
> # because maybe zfs tools are not installed
> warn "$@\n" if $@;
>
> - my $applezfsuuid = "6a898cc3-1dd2-11b2-99a6-080020736631";
> - my $bsdzfsuuid = "516e7cba-6ecf-11d6-8ff8-00022d09712b";
> + my $uuids = [
> + "6a898cc3-1dd2-11b2-99a6-080020736631", # apple
> + "516e7cba-6ecf-11d6-8ff8-00022d09712b", # bsd
> + ];
>
> - dir_glob_foreach('/dev/disk/by-parttypeuuid', "($applezfsuuid|$bsdzfsuuid)\..+", sub {
> - my ($entry) = @_;
> - my $real_dev = abs_path("/dev/disk/by-parttypeuuid/$entry");
> - $list->{$real_dev} = 1;
> - });
> + for my $uuid (@$uuids) {
> + if ($lsblk->{$uuid}) {
> + for my $dev (@{$lsblk->{$uuid}}) {
> + $list->{$dev} = 1;
> + }
> + }
> + }
this code is a prime candidate for refactoring into its own sub, e.g.
(untested, and not very pretty variable names):
my $get_devices_by_partuuid = sub {
my ($uuid_device_map, $partuuids, $res) = @_;
my $res = {} if !defined($res);
foreach my $uuid (@$partuuids) {
map { $res->{$_} = 1 } @{$uuid_device_map->{$uuid}};
}
return $res;
}
and now all the get_foo subs can just call this with their list of
target uuids (and partially filled $res).
>
> return $list;
also, the return variable is called $list here (while it
is a hash), but $___hash or $___list on the caller side depending on
storage..
> }
>
> sub get_lvm_devices {
> + my ($lsblk) = @_;
> my $list = {};
> eval {
> run_command([$PVS, '--noheadings', '--readonly', '-o', 'pv_name'], outfunc => sub{
> @@ -205,39 +243,35 @@ sub get_lvm_devices {
> warn "$@\n" if $@;
>
> my $lvmuuid = "e6d6d379-f507-44c2-a23c-238f2a3df928";
> -
> - dir_glob_foreach('/dev/disk/by-parttypeuuid', "$lvmuuid\..+", sub {
> - my ($entry) = @_;
> - my $real_dev = abs_path("/dev/disk/by-parttypeuuid/$entry");
> - $list->{$real_dev} = 1;
> - });
> + if ($lsblk->{$lvmuuid}) {
> + for my $dev (@{$lsblk->{$lvmuuid}}) {
> + $list->{$dev} = 1;
> + }
> + }
see above
>
> return $list;
as well ;)
> }
>
> sub get_ceph_journals {
> - my $journalhash = {};
> -
> - my $journal_uuid = '45b0969e-9b03-4f30-b4c6-b4b80ceff106';
> - my $db_uuid = '30cd0809-c2b2-499c-8879-2d6b78529876';
> - my $wal_uuid = '5ce17fce-4087-4169-b7ff-056cc58473f9';
> - my $block_uuid = 'cafecafe-9b03-4f30-b4c6-b4b80ceff106';
> -
> - dir_glob_foreach('/dev/disk/by-parttypeuuid', "($journal_uuid|$db_uuid|$wal_uuid|$block_uuid)\..+", sub {
> - my ($entry, $type) = @_;
> - my $real_dev = abs_path("/dev/disk/by-parttypeuuid/$entry");
> - if ($type eq $journal_uuid) {
> - $journalhash->{$real_dev} = 1;
> - } elsif ($type eq $db_uuid) {
> - $journalhash->{$real_dev} = 2;
> - } elsif ($type eq $wal_uuid) {
> - $journalhash->{$real_dev} = 3;
> - } elsif ($type eq $block_uuid) {
> - $journalhash->{$real_dev} = 4;
> + my ($lsblk) = @_;
> + my $list = {};
> +
> + my $uuids = [
> + '45b0969e-9b03-4f30-b4c6-b4b80ceff106', # journal
> + '30cd0809-c2b2-499c-8879-2d6b78529876', # db
> + '5ce17fce-4087-4169-b7ff-056cc58473f9', # wal
> + 'cafecafe-9b03-4f30-b4c6-b4b80ceff106', # block
> + ];
> +
> + for my $uuid (@$uuids) {
> + if ($lsblk->{$uuid}) {
> + for my $dev (@{$lsblk->{$uuid}}) {
> + $list->{$dev} = 1;
> + }
> }
> - });
> + }
same here (this is basically only the helper anyway ;))
>
> - return $journalhash;
> + return $list;
and same here
> }
>
> # reads the lv_tags and matches them with the devices
> @@ -442,12 +476,14 @@ sub get_disks {
> return $mounted->{$dev};
> };
>
> - my $journalhash = get_ceph_journals();
> + my $lsblkinfo = get_lsblk_info();
same comment applies here - more specific name, unless you plan on
adding and using more properties determined via lsblk in the near
future.
> +
> + my $journalhash = get_ceph_journals($lsblkinfo);
> my $ceph_volume_infos = get_ceph_volume_infos();
>
> - my $zfslist = get_zfs_devices();
> + my $zfslist = get_zfs_devices($lsblkinfo);
>
> - my $lvmlist = get_lvm_devices();
> + my $lvmlist = get_lvm_devices($lsblkinfo);
>
> my $disk_regex = ".*";
> if (defined($disks)) {
> diff --git a/test/disklist_test.pm b/test/disklist_test.pm
> index 527e882..9cb6763 100644
> --- a/test/disklist_test.pm
> +++ b/test/disklist_test.pm
> @@ -54,9 +54,14 @@ sub mocked_run_command {
> @$outputlines = split(/\n/, read_test_file('pvs'));
> } elsif ($cmd->[0] =~ m/lvs/i) {
> @$outputlines = split(/\n/, read_test_file('lvs'));
> + } elsif ($cmd->[0] =~ m/lsblk/i) {
> + my $content = read_test_file('lsblk');
> + if ($content eq '') {
> + $content = '{}';
> + }
> + @$outputlines = split(/\n/, $content);
> } else {
> - print "unexpected run_command call: '@$cmd', aborting\n";
> - die;
> + die "unexpected run_command call: '@$cmd', aborting\n";
> }
> } else {
> print "unexpected run_command call: '@$cmd', aborting\n";
> --
> 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