From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 99DDE1FF0AF for ; Fri, 06 Nov 2026 01:59:05 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7EDAE21E9A; Fri, 06 Nov 2026 01:51:22 +0100 (CET) From: Thomas Lamprecht To: pve-devel@lists.proxmox.com Subject: [PATCH v3 storage 31/76] multipath: apply the configuration on every node Date: Fri, 6 Nov 2026 00:31:06 +0100 Message-ID: <20261105233336.2178064-32-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261105233336.2178064-1-t.lamprecht@proxmox.com> References: <20261105233336.2178064-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793926179388 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.297 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: J7NOLSV6Z3NWHQPFH7CCT6RZUZGG7BMJ X-Message-ID-Hash: J7NOLSV6Z3NWHQPFH7CCT6RZUZGG7BMJ X-MailFrom: t.lamprecht@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Each node needs local multipath-tools configuration and allow-list entries for its explicitly managed and storage-derived LUNs. Add a generator for periodic use by the status daemon. It writes drop-ins, updates the allow-list through multipath, and reloads multipathd, leaving nodes that do not use the feature unchanged. Track which entries Proxmox VE added and why. Preserve entries added by hand or for boot from SAN. Remove managed entries only when no configuration needs them, not when a target is temporarily unreachable. Keep maps with local users allow-listed to avoid falling back to single paths after reboot. Read signatures from the device, since udev may not know about volume groups created by other nodes. Unreadable maps and maps without usable paths count as in use, as do maps held open by a process, even without signatures or holders. The paths can fail during the read, so it runs as a bounded status command that cannot stall the status daemon if the map queues I/O. Record a pending reload before changing anything so failures are retried even if the configuration files are already up to date. Signed-off-by: Thomas Lamprecht --- src/PVE/Makefile | 1 + src/PVE/Multipath.pm | 145 +++++++++++++- src/PVE/Multipath/Generator.pm | 294 ++++++++++++++++++++++++++++ src/test/Makefile | 5 +- src/test/run_lun_identity_tests.pl | 1 + src/test/run_multipath_api_tests.pl | 192 ++++++++++++++++++ src/test/run_multipath_tests.pl | 247 +++++++++++++++++++++++ src/test/run_san_library_tests.pl | 6 + src/test/run_san_taint_tests.pl | 80 ++++++++ 9 files changed, 968 insertions(+), 3 deletions(-) create mode 100644 src/PVE/Multipath/Generator.pm create mode 100755 src/test/run_san_taint_tests.pl diff --git a/src/PVE/Makefile b/src/PVE/Makefile index 29d0146d..4edc609d 100644 --- a/src/PVE/Makefile +++ b/src/PVE/Makefile @@ -7,6 +7,7 @@ install: install -D -m 0644 Multipath.pm ${DESTDIR}${PERLDIR}/PVE/Multipath.pm install -D -m 0644 Multipath/Config.pm ${DESTDIR}${PERLDIR}/PVE/Multipath/Config.pm install -D -m 0644 Multipath/ClusterConfig.pm ${DESTDIR}${PERLDIR}/PVE/Multipath/ClusterConfig.pm + install -D -m 0644 Multipath/Generator.pm ${DESTDIR}${PERLDIR}/PVE/Multipath/Generator.pm install -D -m 0644 Multipath/Identity.pm ${DESTDIR}${PERLDIR}/PVE/Multipath/Identity.pm install -D -m 0644 Multipath/Status.pm ${DESTDIR}${PERLDIR}/PVE/Multipath/Status.pm install -D -m 0644 CephConfig.pm ${DESTDIR}${PERLDIR}/PVE/CephConfig.pm diff --git a/src/PVE/Multipath.pm b/src/PVE/Multipath.pm index e2f7ea40..96f1e84d 100644 --- a/src/PVE/Multipath.pm +++ b/src/PVE/Multipath.pm @@ -2,11 +2,13 @@ package PVE::Multipath; use v5.36; -use JSON; +use PVE::Diskmanage; -use PVE::Tools qw(run_command file_read_firstline); +use PVE::Tools qw(run_command file_read_firstline file_get_contents file_set_contents); +use PVE::Multipath::Config; use PVE::RS::Multipath; +use PVE::Storage::StatusCommand; my $MULTIPATH = '/sbin/multipath'; my $MULTIPATHD = '/sbin/multipathd'; @@ -122,6 +124,14 @@ my sub dm_size_bytes($sysfs) { return int($sectors) * 512; } +my sub dir_has_entries($dir) { + return 0 if !-d $dir; + opendir(my $dh, $dir) or return 0; + my @entries = grep { $_ ne '.' && $_ ne '..' } readdir($dh); + closedir($dh); + return scalar(@entries) ? 1 : 0; +} + my sub path_transport($dev) { return undef if !$dev; my $link = readlink("/sys/block/$dev"); @@ -201,4 +211,135 @@ sub wait_for_map($wwid, $timeout = undef) { } } +my $WWIDS_FILE = '/etc/multipath/wwids'; + +# The WWIDs in /etc/multipath/wwids, the on-disk allow-list that multipathd assembles from with +# 'find_multipaths strict'. Distinct from the cluster config's desired set (Config::wwid_list). +sub list_etc_multipath_wwids() { + return [] if !-e $WWIDS_FILE; + return PVE::Multipath::Config::parse_wwids(file_get_contents($WWIDS_FILE)); +} + +# multipath reads other arguments as options or device numbers, see Config::WWID_PATTERN. +my sub assert_wwid($wwid) { + die "invalid WWID '$wwid'\n" if $wwid !~ $PVE::Multipath::Config::WWID_RE; +} + +sub add_wwid($wwid) { + assert_supported(); + assert_wwid($wwid); + run_command([$MULTIPATH, '-a', '--', $wwid], timeout => $TOOL_TIMEOUT); +} + +sub remove_wwid($wwid) { + assert_supported(); + assert_wwid($wwid); + run_command([$MULTIPATH, '-w', '--', $wwid], timeout => $TOOL_TIMEOUT); +} + +# Read the device: udev can miss another node's writes. Without active paths, I/O can queue +# indefinitely; only a successful empty probe proves non-use. Removing an in-use map's allow-list +# entry could expose its PV through a single path after reboot. The paths can still fail during the +# read, so it runs as a status command, which leaves a reader stuck in I/O behind instead of +# waiting for it. Probes sharing $cache share its time budget. +sub map_user($map, $cache = undef) { + my ($dev) = ($map->{sysfs} // '') =~ m/\A(dm-\d+)\z/; + return 'unknown' if !defined($dev); + return 'Device Mapper' if dir_has_entries("/sys/block/$dev/holders"); + return 'unknown' if !$map->{'paths-active'}; + + my $open = ''; + eval { + run_command( + ['dmsetup', 'info', '-c', '--noheadings', '-o', 'open', "/dev/$dev"], + timeout => $QUERY_TIMEOUT, + outfunc => sub($line) { $open .= "$line\n" }, + errfunc => sub($line) { }, + ); + }; + return 'unknown' if $@ || $open !~ m/^\s*(\d+)\s*$/; + return 'open device' if $1 > 0; + + my $output = ''; + eval { + local $PVE::Storage::StatusCommand::BUDGET = $QUERY_TIMEOUT; + PVE::Storage::StatusCommand::run( + $cache // {}, + ['/sbin/wipefs', '--no-act', '--json', "/dev/$dev"], + outfunc => sub($line) { $output .= "$line\n" }, + ); + }; + return 'unknown' if $@; + my $signatures = eval { PVE::Diskmanage::parse_wipefs_signatures($output) }; + return 'unknown' if $@; + + my %types = map { $_->{type} => 1 } $signatures->@*; + return undef if !%types; + return 'LVM' if $types{LVM2_member}; + return 'ZFS' if $types{zfs_member}; + return 'partitions' if grep { $types{$_} } qw(gpt PMBR dos); + return (sort keys %types)[0]; +} + +# Avoid repeatedly reading the SAN while removal of an in-use map is pending. +our $IN_USE_RECHECK_SECONDS = 60; +my $in_use_seen = {}; + +# { wwid => user } for those of the given WWIDs whose map is assembled on this node and in use, see +# map_user(). Every WWID counts as in use when the maps cannot be queried. +sub maps_in_use($wwids) { + my $maps = eval { get_maps() }; + if (my $err = $@) { + warn "multipath: querying the maps failed - $err"; + return { map { $_ => 'unknown' } $wwids->@* }; + } + my %map_of = map { $_->{wwid} => $_ } grep { defined($_->{wwid}) } $maps->@*; + + my $now = time(); + my $in_use = {}; + # one time budget for all probes, so a pass does not grow with the number of maps + my $probes = { 'status-command-budget' => $QUERY_TIMEOUT }; + for my $wwid ($wwids->@*) { + my $map = $map_of{$wwid} // next; + my $seen = $in_use_seen->{$wwid}; + my $user; + if ($seen && $now - $seen->{time} < $IN_USE_RECHECK_SECONDS) { + $user = $seen->{user}; + } else { + $user = map_user($map, $probes); + $in_use_seen->{$wwid} = { time => $now, user => $user } if defined($user); + } + next if !defined($user); + $in_use->{$wwid} = $user; + } + delete $in_use_seen->{$_} for grep { !$in_use->{$_} } keys %$in_use_seen; + return $in_use; +} + +# Track only PVE-added entries so pruning never removes hand-managed or boot-from-SAN WWIDs. +# Removal waits until every ownership reason disappears. +my $MANAGED_WWIDS_FILE = '/etc/multipath/wwids.pve'; + +# The ownership record, { wwid => [sources] } (empty if Proxmox VE added nothing yet). +sub managed_wwid_record() { + return {} if !-e $MANAGED_WWIDS_FILE; + return PVE::Multipath::Config::parse_managed_wwids(file_get_contents($MANAGED_WWIDS_FILE)); +} + +sub set_managed_wwid_record($record) { + if (!%$record) { + if (-e $MANAGED_WWIDS_FILE) { + unlink($MANAGED_WWIDS_FILE) + or die "could not remove '$MANAGED_WWIDS_FILE' - $!\n"; + } + return; + } + file_set_contents($MANAGED_WWIDS_FILE, PVE::Multipath::Config::format_managed_wwids($record)); +} + +sub reconfigure() { + assert_supported(); + run_command([$MULTIPATHD, 'reconfigure'], timeout => $RECONFIGURE_TIMEOUT); +} + 1; diff --git a/src/PVE/Multipath/Generator.pm b/src/PVE/Multipath/Generator.pm new file mode 100644 index 00000000..b7aec227 --- /dev/null +++ b/src/PVE/Multipath/Generator.pm @@ -0,0 +1,294 @@ +package PVE::Multipath::Generator; + +use v5.36; + +use File::Path qw(make_path); + +use PVE::Tools qw(file_get_contents file_set_contents); + +use PVE::Multipath; +use PVE::Multipath::Config; +use PVE::Multipath::ClusterConfig; + +# Keep rendered files local so multipathd can start before pmxcfs, using the last successful render. +# multipath rejects duplicate section keywords within one file. Separate baseline, overrides and +# aliases so they merge across files; overrides must sort after the baseline. +our $DEFAULTS_DROPIN = '/etc/multipath/conf.d/pve-defaults.conf'; +our $OVERRIDES_DROPIN = '/etc/multipath/conf.d/pve-overrides.conf'; +our $ALIASES_DROPIN = '/etc/multipath/conf.d/pve-aliases.conf'; + +# Set before changing files and clear only after a successful reload, so partial passes and +# failed reloads retry even when files already match. Reboot loads the files without this marker. +our $RECONFIGURE_PENDING = '/run/pve-multipath.reconfigure-pending'; + +my sub mark_reconfigure_pending() { + file_set_contents($RECONFIGURE_PENDING, '') if !-e $RECONFIGURE_PENDING; +} + +my sub write_if_changed($path, $content) { + my $old = -e $path ? eval { file_get_contents($path) } : undef; + return 0 if defined($old) && $old eq $content; + + mark_reconfigure_pending(); + my $dir = $path =~ s!/[^/]+$!!r; + make_path($dir) if !-d $dir; + file_set_contents($path, $content); + return 1; +} + +my sub remove_if_present($path) { + return 0 if !-e $path; + mark_reconfigure_pending(); + unlink($path) or die "could not remove '$path' - $!\n"; + return 1; +} + +# Derived storages need the baseline even without explicit sections; remove unused generated files +# so hand-managed configuration takes over. +my sub config_in_use($cfg, $overrides, $derived, $node) { + return + scalar(PVE::Multipath::Config::wwids_for_node($cfg, $node)->@*) + || (defined($overrides) && length($overrides)) + || $cfg->{ids}->{defaults} + || scalar(keys(($derived // {})->%*)); +} + +# Retain owned storage entries while their storage is enabled here: an outage or unsettled scan +# is not evidence that the LUN was withdrawn. Only configuration changes remove ownership reasons. +sub plan_wwid_changes($explicit, $derived, $current, $managed) { + my %desired; + push $desired{$_}->@*, 'config' for keys %$explicit; + for my $storeid (sort keys %$derived) { + push $desired{$_}->@*, "storage:$storeid" for keys $derived->{$storeid}->{wwids}->%*; + } + for my $wwid (keys %$managed) { + my %have = map { $_ => 1 } ($desired{$wwid} // [])->@*; + for my $source ($managed->{$wwid}->@*) { + next if $have{$source}; + my ($storeid) = $source =~ m/^storage:(.+)$/; + push $desired{$wwid}->@*, $source if defined($storeid) && $derived->{$storeid}; + } + } + $desired{$_} = [sort $desired{$_}->@*] for keys %desired; + + my @to_add = sort grep { !$current->{$_} } keys %desired; + my @to_remove = sort grep { !$desired{$_} && $current->{$_} } keys %$managed; + return (\@to_add, \@to_remove, \%desired); +} + +# Do not claim pre-existing foreign entries: removing a storage must not prune hand-managed WWIDs. +# Pending or failed removals must retain their recorded reasons for retry. +sub owned_wwid_record($desired, $managed, $current, $keep) { + my %owned = map { $_ => $managed->{$_} } grep { defined($managed->{$_}) } keys %$keep; + for my $wwid (keys %$desired) { + $owned{$wwid} = $desired->{$wwid} if $managed->{$wwid} || !$current->{$wwid}; + } + return \%owned; +} + +sub wwid_records_differ($a, $b) { + my $flat = sub($record) { + join("\0", map { "$_=" . join(',', sort $record->{$_}->@*) } sort keys $record->%*); + }; + return $flat->($a) ne $flat->($b); +} + +# Storages can expose a LUN through different portal sets, so judge map health against the largest +# expected path count. +sub desired_wwid_info($desired, $derived) { + my $wwids = {}; + for my $wwid (keys %$desired) { + my $info = {}; + for my $source ($desired->{$wwid}->@*) { + my ($storeid) = $source =~ m/^storage:(.+)$/ or next; + push $info->{storages}->@*, $storeid; + my $luns = ($derived->{$storeid} // {})->{wwids} // {}; + my $expected = ($luns->{$wwid} // {})->{'paths-expected'}; + $info->{'paths-expected'} = $expected + if defined($expected) && $expected > ($info->{'paths-expected'} // 0); + } + $wwids->{$wwid} = $info; + } + return $wwids; +} + +# Collect per-operation errors instead of losing the WWID state needed by health reports. +sub regenerate($cfg = undef, $overrides = undef, $derived = undef) { + $cfg //= PVE::Multipath::ClusterConfig::read_config(); + $overrides //= PVE::Multipath::ClusterConfig::read_overrides(); + $derived //= {}; + + require PVE::INotify; + my $node = PVE::INotify::nodename(); + + my $changed = 0; + + if (config_in_use($cfg, $overrides, $derived, $node)) { + my $defaults = PVE::Multipath::Config::effective_defaults($cfg); + $changed = 1 + if write_if_changed( + $DEFAULTS_DROPIN, + PVE::Multipath::Config::generate_managed_conf($defaults), + ); + } else { + $changed = 1 if remove_if_present($DEFAULTS_DROPIN); + } + + if (defined($overrides) && length($overrides)) { + my $content = + "# Managed by Proxmox VE - edit overrides in /etc/pve/multipath-overrides.conf.\n\n" + . "$overrides\n"; + $changed = 1 if write_if_changed($OVERRIDES_DROPIN, $content); + } else { + $changed = 1 if remove_if_present($OVERRIDES_DROPIN); + } + + my $block = + PVE::Multipath::Config::build_multipaths_block(PVE::Multipath::Config::wwid_opts($cfg)); + if (length($block)) { + my $content = + "# Managed by Proxmox VE - edit aliases and per-LUN options in /etc/pve/multipath.cfg.\n\n" + . $block; + $changed = 1 if write_if_changed($ALIASES_DROPIN, $content); + } else { + $changed = 1 if remove_if_present($ALIASES_DROPIN); + } + + # Use multipath's own allow-list writer. Isolate operations so a failing WWID cannot prevent + # the others from converging; failed operations are retried on the next pass. + my %explicit = map { $_ => 1 } PVE::Multipath::Config::wwids_for_node($cfg, $node)->@*; + my %current = map { $_ => 1 } PVE::Multipath::list_etc_multipath_wwids()->@*; + my $managed = PVE::Multipath::managed_wwid_record(); + my ($to_add, $to_remove, $desired) = + plan_wwid_changes(\%explicit, $derived, \%current, $managed); + + my @errors; + for my $storeid (sort keys %$derived) { + my $err = $derived->{$storeid}->{error} // next; + push @errors, "identifying the LUN WWIDs of storage '$storeid' failed - $err"; + } + + # Keep ownership while a map is in use, so removal can be retried after release. + my $in_use = scalar($to_remove->@*) ? PVE::Multipath::maps_in_use($to_remove) : {}; + my @removable = grep { !$in_use->{$_} } $to_remove->@*; + + # Record ownership before adding: a successful add followed by a failed record write would + # leak an unprunable entry. Keep pending removals owned until the entries are actually gone. + my %pending = map { $_ => 1 } $to_remove->@*; + my $record = owned_wwid_record($desired, $managed, \%current, \%pending); + my $record_ok = 1; + if (wwid_records_differ($record, $managed)) { + eval { PVE::Multipath::set_managed_wwid_record($record); }; + if (my $err = $@) { + push @errors, "recording managed WWIDs failed - $err"; + $record_ok = 0; + $record = $managed; + } + } + + mark_reconfigure_pending() if scalar($to_add->@*) || scalar(@removable); + if ($record_ok) { + for my $wwid ($to_add->@*) { + eval { PVE::Multipath::add_wwid($wwid); }; + if (my $err = $@) { + push @errors, "adding WWID '$wwid' failed - $err"; + } else { + $changed = 1; + } + } + } + my %kept = map { $_ => 1 } keys %$in_use; + for my $wwid (@removable) { + eval { PVE::Multipath::remove_wwid($wwid); }; + if (my $err = $@) { + push @errors, "removing WWID '$wwid' failed - $err"; + $kept{$wwid} = 1; + } else { + $changed = 1; + } + } + + # A failed ownership release is safe: absent entries leave the record on the next pass. + if (scalar(@removable)) { + my $final = owned_wwid_record($desired, $managed, \%current, \%kept); + if (wwid_records_differ($final, $record)) { + eval { PVE::Multipath::set_managed_wwid_record($final); }; + push @errors, "recording managed WWIDs failed - $@" if $@; + } + } + + # Reload successful changes even if other operations failed; retain the marker until success. + if (-e $RECONFIGURE_PENDING && PVE::Multipath::is_running()) { + eval { PVE::Multipath::reconfigure(); }; + if (my $err = $@) { + push @errors, "reconfigure failed - $err"; + } else { + unlink($RECONFIGURE_PENDING); + } + } + + my $wwids = desired_wwid_info($desired, $derived); + $wwids->{$_} = { kept => $in_use->{$_} } for keys %$in_use; + + my $error = join('', map { $_ =~ m/\n\z/ ? $_ : "$_\n" } @errors); + + return { + changed => $changed, + wwids => $wwids, + length($error) ? (error => $error) : (), + }; +} + +# On config-read failure, retain WWIDs from the ownership record so managed LUNs remain visible. +sub sync() { + my $record = eval { PVE::Multipath::managed_wwid_record() }; + return { wwids => {}, error => "reading managed WWIDs failed - $@" } if $@; + my $state = { wwids => desired_wwid_info($record, {}) }; + + my $cfg = eval { PVE::Multipath::ClusterConfig::read_config() }; + return { $state->%*, error => "reading cluster config failed - $@" } if $@; + + require PVE::INotify; + my $node = PVE::INotify::nodename(); + my %explicit = map { $_ => 1 } PVE::Multipath::Config::wwids_for_node($cfg, $node)->@*; + $state->{wwids}->{$_} //= {} for keys %explicit; + + # Without storage.cfg, an empty derived set could incorrectly prune storage-owned WWIDs. + my $derived = eval { + require PVE::Storage; + PVE::Storage::derived_multipath_wwids(PVE::Storage::config()); + }; + return { $state->%*, error => "reading the storage configuration failed - $@" } if $@; + + my (undef, undef, $desired) = + plan_wwid_changes(\%explicit, $derived, {}, $record); + $state->{wwids} = desired_wwid_info($desired, $derived); + my $overrides = eval { PVE::Multipath::ClusterConfig::read_overrides() }; + return { $state->%*, error => "reading cluster overrides failed - $@" } if $@; + + my $in_use = config_in_use($cfg, $overrides, $derived, $node); + + if (!PVE::Multipath::is_supported()) { + return $state if !scalar(keys %$derived) && !%explicit; + return { $state->%*, error => "multipath-tools is not installed\n" }; + } + + # Leftover generated files or ownership records require another teardown pass. + return $state + if !$in_use + && !-e $DEFAULTS_DROPIN + && !-e $OVERRIDES_DROPIN + && !-e $ALIASES_DROPIN + && !-e $RECONFIGURE_PENDING + && !%$record; + + my $res = eval { regenerate($cfg, $overrides, $derived) }; + return { $state->%*, error => "$@" } if $@; + + return { + wwids => $res->{wwids}, + defined($res->{error}) ? (error => $res->{error}) : (), + }; +} + +1; diff --git a/src/test/Makefile b/src/test/Makefile index 97d9db4d..9ecd59d4 100644 --- a/src/test/Makefile +++ b/src/test/Makefile @@ -1,6 +1,6 @@ all: test -test: test_zfspoolplugin test_lvmplugin test_lvm_create test_lvm_lock_retry test_lvm_base_volume test_lvm_hints test_lvm_base_activation test_lvm_status test_status_command test_disklist test_disk_api test_bwlimit test_plugin test_ovf test_volume_access test_multipath test_iscsi test_lun_identity test_multipath_api test_san_api test_san_libraries +test: test_zfspoolplugin test_lvmplugin test_lvm_create test_lvm_lock_retry test_lvm_base_volume test_lvm_hints test_lvm_base_activation test_lvm_status test_status_command test_disklist test_disk_api test_bwlimit test_plugin test_ovf test_volume_access test_multipath test_iscsi test_lun_identity test_multipath_api test_san_api test_san_libraries test_san_taint test_zfspoolplugin: run_test_zfspoolplugin.pl ./run_test_zfspoolplugin.pl @@ -75,3 +75,6 @@ test_san_api: run_san_api_tests.pl test_san_libraries: run_san_library_tests.pl ./run_san_library_tests.pl + +test_san_taint: run_san_taint_tests.pl + perl -T ./run_san_taint_tests.pl diff --git a/src/test/run_lun_identity_tests.pl b/src/test/run_lun_identity_tests.pl index df248b38..33b92d7c 100755 --- a/src/test/run_lun_identity_tests.pl +++ b/src/test/run_lun_identity_tests.pl @@ -16,6 +16,7 @@ use Test::More; use PVE::Multipath; use PVE::Multipath::Identity; +use PVE::Multipath::Generator; use PVE::Storage; use PVE::Storage::ISCSIPlugin; use PVE::Multipath::Status; diff --git a/src/test/run_multipath_api_tests.pl b/src/test/run_multipath_api_tests.pl index c6fdfb7c..b013ba38 100755 --- a/src/test/run_multipath_api_tests.pl +++ b/src/test/run_multipath_api_tests.pl @@ -3,11 +3,21 @@ use strict; use warnings; +# a fixed clock for the boundaries of the report ages, the real one otherwise; installed before the +# modules are compiled, so their time() binds to it +our $NOW; + +BEGIN { + *CORE::GLOBAL::time = sub { return $NOW // CORE::time() }; +} + use lib ('.', '..'); +use Storable qw(dclone); use Test::More; use Test::MockModule; use PVE::Multipath; +use PVE::Multipath::Generator; use PVE::Storage; use PVE::Storage::ISCSIPlugin; use PVE::Multipath::Status; @@ -89,4 +99,186 @@ subtest 'enabling the multipath option needs every node to know it' => sub { ); }; +subtest 'an entry stays allow-listed while its map is in use' => sub { + my $mp = Test::MockModule->new('PVE::Multipath'); + my $map = { wwid => 'w1', sysfs => 'dm-999999', 'paths-active' => 2 }; + $mp->mock(get_maps => sub { [$map, { wwid => 'w2', sysfs => 'dm-999998' }] }); + my ($wipefs, $ran, @caches) = ('', 0); + $mp->mock( + run_command => sub { + my ($cmd, %param) = @_; + $ran++; + die "unexpected command @$cmd\n" if $cmd->[0] ne 'dmsetup'; + return $param{outfunc}->('0'); + }, + ); + my $status_command = Test::MockModule->new('PVE::Storage::StatusCommand'); + $status_command->mock( + run => sub { + my ($cache, $cmd, %param) = @_; + $ran++; + push @caches, $cache; + is($cmd->[0], '/sbin/wipefs', 'the device is read as a bounded status command'); + die "status command 'wipefs' failed: exit code 1\n" if $wipefs eq 'unreadable'; + die "another status command is still running\n" if $wipefs eq 'contended'; + $param{outfunc}->($wipefs); + }, + ); + my $signatures = sub { + return '{"signatures":[' . join(',', map { qq({"type":"$_"}) } @_) . ']}'; + }; + local $PVE::Multipath::IN_USE_RECHECK_SECONDS = 0; + + $wipefs = $signatures->('LVM2_member'); + is_deeply( + PVE::Multipath::maps_in_use(['w1', 'w3']), + { w1 => 'LVM' }, + 'a physical volume keeps the entry, a LUN without a map here does not', + ); + $wipefs = $signatures->(); + is_deeply(PVE::Multipath::maps_in_use(['w1']), {}, + 'a map without signatures is not in use'); + $wipefs = $signatures->('gpt', 'PMBR'); + is_deeply(PVE::Multipath::maps_in_use(['w1']), { w1 => 'partitions' }, 'a partition table'); + $wipefs = 'unreadable'; + is_deeply(PVE::Multipath::maps_in_use(['w1']), { w1 => 'unknown' }, 'an unreadable map'); + $wipefs = 'contended'; + is_deeply(PVE::Multipath::maps_in_use(['w1']), { w1 => 'unknown' }, 'a contended probe'); + + # the probes of one pass share a time budget + $mp->mock( + get_maps => sub { + [map { { wwid => $_, sysfs => 'dm-999999', 'paths-active' => 1 } } qw(w1 w4)]; + }, + ); + ($wipefs, @caches) = ($signatures->()); + is_deeply(PVE::Multipath::maps_in_use(['w1', 'w4']), {}, 'two unused maps'); + is(scalar(@caches), 2, 'both maps were probed'); + ok($caches[0] == $caches[1], 'with one shared budget'); + is($caches[0]->{'status-command-budget'}, 10, 'of ten seconds'); + $mp->mock(get_maps => sub { [$map, { wwid => 'w2', sysfs => 'dm-999998' }] }); + + # an unused map leaves no remembered result behind + $wipefs = $signatures->(); + PVE::Multipath::maps_in_use(['w1']); + $PVE::Multipath::IN_USE_RECHECK_SECONDS = 60; + $wipefs = $signatures->('LVM2_member'); + PVE::Multipath::maps_in_use(['w1']); + $ran = 0; + $wipefs = $signatures->(); + is_deeply( + PVE::Multipath::maps_in_use(['w1']), + { w1 => 'LVM' }, + 'a map in use is not reread', + ); + is($ran, 0, 'until the recheck interval passed'); + $PVE::Multipath::IN_USE_RECHECK_SECONDS = 0; + + $ran = 0; + is_deeply( + PVE::Multipath::maps_in_use(['w2']), + { w2 => 'unknown' }, + 'a map without a usable path counts as in use', + ); + is($ran, 0, 'without being probed, as its I/O could block'); + + $mp->mock(get_maps => sub { die "daemon gone\n" }); + { + local $SIG{__WARN__} = sub { }; + is_deeply( + PVE::Multipath::maps_in_use(['w1', 'w3']), + { w1 => 'unknown', w3 => 'unknown' }, + 'nothing is pruned while the maps cannot be queried', + ); + } +}; + +subtest 'regeneration prunes only conclusively closed blank maps' => sub { + require File::Temp; + my $dir = File::Temp::tempdir(CLEANUP => 1); + local $PVE::Multipath::Generator::DEFAULTS_DROPIN = "$dir/defaults"; + local $PVE::Multipath::Generator::OVERRIDES_DROPIN = "$dir/overrides"; + local $PVE::Multipath::Generator::ALIASES_DROPIN = "$dir/aliases"; + local $PVE::Multipath::Generator::RECONFIGURE_PENDING = "$dir/pending"; + local $PVE::Multipath::IN_USE_RECHECK_SECONDS = 60; + local $NOW = 100_000; + my $mp = Test::MockModule->new('PVE::Multipath'); + my $inotify = Test::MockModule->new('PVE::INotify'); + $inotify->mock(nodename => sub { 'n1' }); + my $cfg = PVE::Multipath::Config->parse_config('multipath.cfg', "wwid: w2\n"); + my ($opens, $signatures, $failed) = ('1', '{"signatures":[]}', ''); + my ($record, $current, @removed); + $mp->mock( + get_maps => sub { [{ wwid => 'w1', sysfs => 'dm-999999', 'paths-active' => 2 }] }, + is_running => sub { 0 }, + managed_wwid_record => sub { dclone($record) }, + set_managed_wwid_record => sub { $record = dclone($_[0]) }, + list_etc_multipath_wwids => sub { [keys %$current] }, + assert_supported => sub { 1 }, + run_command => sub { + my ($cmd, %param) = @_; + if ($cmd->[0] eq 'dmsetup') { + is($param{timeout}, 10, 'probe has a bounded timeout'); + die "probe failed\n" if $failed eq $cmd->[0]; + $param{outfunc}->($opens); + } elsif ($cmd->[0] eq '/sbin/multipath' && $cmd->[1] eq '-w') { + is($cmd->[2], '--', 'the WWID is no option'); + push @removed, $cmd->[3]; + delete $current->{ $cmd->[3] }; + } else { + die "unexpected command @$cmd\n"; + } + }, + ); + my $status_command = Test::MockModule->new('PVE::Storage::StatusCommand'); + $status_command->mock( + run => sub { + my ($cache, $cmd, %param) = @_; + is($cmd->[0], '/sbin/wipefs', 'only the device read is a status command'); + die "status command 'wipefs' failed: exit code 1\n" if $failed eq $cmd->[0]; + $param{outfunc}->($signatures); + }, + ); + for my $case ( + ['raw opener', '1', '{"signatures":[]}', ''], + ['missing open count', '', '{"signatures":[]}', ''], + ['malformed open count', 'zero', '{"signatures":[]}', ''], + ['multiple open counts', "0\n0", '{"signatures":[]}', ''], + ['failed open probe', '0', '{"signatures":[]}', 'dmsetup'], + ['malformed signature', '0', '{"signatures":[{}]}', ''], + ['missing signatures', '0', '{}', ''], + ['failed signature probe', '0', '{"signatures":[]}', '/sbin/wipefs'], + ) { + my $name; + ($name, $opens, $signatures, $failed) = @$case; + $record = { w1 => ['config'], w2 => ['config'] }; + $current = { w1 => 1, w2 => 1 }; + @removed = (); + $NOW += 61; + my $state = PVE::Multipath::Generator::regenerate($cfg, '', {}); + is_deeply(\@removed, [], "$name never runs multipath -w"); + ok($record->{w1}, "$name retains ownership"); + ok($state->{wwids}->{w1}->{kept}, "$name reports retention"); + ($opens, $signatures, $failed) = ('0', '{"signatures":[]}', ''); + PVE::Multipath::Generator::regenerate($cfg, '', {}); + is_deeply(\@removed, [], 'positive in-use result remains cached'); + $NOW += 61; + PVE::Multipath::Generator::regenerate($cfg, '', {}); + is_deeply(\@removed, ['w1'], 'closed and blank map is eventually pruned'); + ok(!$record->{w1}, 'pruning releases ownership'); + ok($record->{w2}, 'other managed entry is retained'); + } +}; + +subtest 'unreadable configuration keeps the known LUNs reported' => sub { + my $mp = Test::MockModule->new('PVE::Multipath'); + $mp->mock(managed_wwid_record => sub { +{ '3600abc' => ['storage:san'] } }); + my $cc = Test::MockModule->new('PVE::Multipath::ClusterConfig'); + $cc->mock(read_config => sub { die "config unavailable\n" }); + my $state = PVE::Multipath::Generator::sync(); + like($state->{error}, qr/config unavailable/, 'the failure is reported'); + is_deeply($state->{wwids}->{'3600abc'}->{storages}, ['san'], + 'the recorded LUN stays known'); +}; + done_testing(); diff --git a/src/test/run_multipath_tests.pl b/src/test/run_multipath_tests.pl index 0e247284..992dd3dd 100755 --- a/src/test/run_multipath_tests.pl +++ b/src/test/run_multipath_tests.pl @@ -3,11 +3,14 @@ use strict; use warnings; +use Test::MockModule; use Test::More; use lib ('.', '..'); use PVE::Multipath; use PVE::Multipath::Config; +use PVE::Multipath::Generator; +use PVE::Multipath::Status; # A recorded `multipathd show maps json` reply with three maps exercising each # health state: an all-active map, a partially-failed map and an all-failed map. @@ -360,6 +363,230 @@ is( . "3600abc config\n3600def config,storage:san1\n", 'format_managed_wwids sorts and round-trips', ); +is_deeply( + PVE::Multipath::Generator::desired_wwid_info( + { w1 => ['config', 'storage:a', 'storage:b'], w2 => ['config'] }, + { + a => { wwids => { w1 => { 'paths-expected' => 2 } } }, + b => { wwids => { w1 => { 'paths-expected' => 4 } } }, + }, + ), + { w1 => { storages => ['a', 'b'], 'paths-expected' => 4 }, w2 => {} }, + 'every deriving storage is named, the largest path expectation wins', +); + +# --- WWID allow-list reconcile plan: only prune what PVE itself added, and only once every --- +# --- reason it was added for is gone from the configuration --- +# 'f' is a foreign, hand-added WWID present in the local allow-list but never managed by PVE. +{ + my @cases = ( + { + name => 'explicit-only config adds missing WWIDs, foreign entries stay untouched', + explicit => { a => 1, b => 1 }, + current => { b => 1, f => 1 }, + managed => { b => ['config'] }, + add => ['a'], + remove => [], + desired => { a => ['config'], b => ['config'] }, + }, + { + name => 'an owned WWID dropped from an explicit-only config is pruned', + explicit => { a => 1 }, + current => { a => 1, b => 1, f => 1 }, + managed => { a => ['config'], b => ['config'] }, + add => [], + remove => ['b'], + }, + { + name => 'derived WWIDs are added like explicit ones, sourced to their storage', + derived => { san1 => { wwids => { d1 => {}, d2 => {} } } }, + current => { d1 => 1 }, + managed => { d1 => ['storage:san1'] }, + add => ['d2'], + remove => [], + desired => { d1 => ['storage:san1'], d2 => ['storage:san1'] }, + }, + { + name => 'a LUN gone from the target stays while its storage is still enabled', + derived => { san1 => { wwids => { d1 => {} } } }, + current => { d1 => 1, gone => 1, f => 1 }, + managed => { d1 => ['storage:san1'], gone => ['storage:san1'] }, + add => [], + remove => [], + desired => { d1 => ['storage:san1'], gone => ['storage:san1'] }, + }, + { + name => 'a storage no longer enabled on the node prunes its owned WWIDs', + derived => {}, + current => { d1 => 1, f => 1 }, + managed => { d1 => ['storage:san1'] }, + add => [], + remove => ['d1'], + }, + { + name => 'an outage (nothing visible) keeps every owned WWID of the storage', + derived => { san1 => { wwids => {} } }, + current => { d1 => 1, f => 1 }, + managed => { d1 => ['storage:san1'] }, + add => [], + remove => [], + desired => { d1 => ['storage:san1'] }, + }, + { + name => 'a failing derivation of one storage does not touch the others', + derived => { + san1 => { wwids => { d1 => {} } }, + san2 => { wwids => {}, error => 'boom' }, + }, + current => { d1 => 1, d2 => 1 }, + managed => { d1 => ['storage:san1'], d2 => ['storage:san2'] }, + add => [], + remove => [], + }, + { + name => 'an explicit section on a derived WWID adds a reason, it replaces none', + explicit => { p => 1 }, + derived => { san1 => { wwids => { p => {} } } }, + current => { p => 1 }, + managed => { p => ['storage:san1'] }, + add => [], + remove => [], + desired => { p => ['config', 'storage:san1'] }, + }, + { + name => 'unpinning during an outage keeps the WWID for its storage', + explicit => {}, + derived => { san1 => { wwids => {} } }, + current => { p => 1 }, + managed => { p => ['config', 'storage:san1'] }, + add => [], + remove => [], + desired => { p => ['storage:san1'] }, + }, + { + name => 'unpinning a WWID whose storage is gone prunes it', + explicit => {}, + derived => {}, + current => { p => 1 }, + managed => { p => ['config', 'storage:san1'] }, + add => [], + remove => ['p'], + }, + { + name => 'a WWID handed from one storage to another follows the deriving storage', + derived => { san2 => { wwids => { d => {} } } }, + current => { d => 1 }, + managed => { d => ['storage:san1'] }, + add => [], + remove => [], + desired => { d => ['storage:san2'] }, + }, + { + name => 'an owned config WWID missing from the allow-list is restored', + explicit => { a => 1 }, + current => {}, + managed => { a => ['config'] }, + add => ['a'], + remove => [], + }, + { + name => + 'an owned derived WWID missing from the allow-list is restored during an outage', + derived => { san1 => { wwids => {} } }, + current => {}, + managed => { d1 => ['storage:san1'] }, + add => ['d1'], + remove => [], + }, + { + name => + 'an owned WWID neither in the allow-list nor wanted anymore is just forgotten', + explicit => {}, + current => { f => 1 }, + managed => { stale => ['config'] }, + add => [], + remove => [], + desired => {}, + }, + ); + for my $case (@cases) { + my ($add, $remove, $desired) = PVE::Multipath::Generator::plan_wwid_changes( + $case->{explicit} // {}, + $case->{derived} // {}, + $case->{current} // {}, + $case->{managed} // {}, + ); + is_deeply($add, $case->{add}, "$case->{name}: additions"); + is_deeply($remove, $case->{remove}, "$case->{name}: removals"); + is_deeply($desired, $case->{desired}, "$case->{name}: desired reasons") + if exists $case->{desired}; + } +} + +# --- ownership record: never adopt a pre-existing hand-made allow-list entry --- +{ + # 'h' was already in the allow-list when it appeared in the cluster config, so PVE never added + # it and it must stay foreign even while desired; dropping it from the config later must not + # prune the hand-made entry (think a boot-from-SAN LUN added just for health monitoring) + my $owned = PVE::Multipath::Generator::owned_wwid_record( + { a => ['config'], h => ['config'] }, # desired + { a => ['config'] }, # managed before + { a => 1, h => 1 }, # both already in the allow-list + {}, + ); + is_deeply($owned, { a => ['config'] }, 'a desired but hand-added WWID is not adopted'); + + $owned = PVE::Multipath::Generator::owned_wwid_record( + { a => ['config'], n => ['storage:san1'] }, # desired + { a => ['config'] }, # managed before + { a => 1 }, # 'n' is absent, so PVE is about to add it + {}, + ); + is_deeply( + $owned, + { a => ['config'], n => ['storage:san1'] }, + 'a WWID PVE is about to add becomes owned before the add, with its reasons', + ); + + $owned = PVE::Multipath::Generator::owned_wwid_record( + { a => ['config'] }, # desired + { a => ['config'], gone => ['storage:san1'] }, # managed; 'gone' is no longer wanted + { a => 1, gone => 1 }, + { gone => 1 }, # but its removal is pending or failed + ); + is_deeply( + $owned, + { a => ['config'], gone => ['storage:san1'] }, + 'a pending or failed prune keeps ownership with the recorded reasons', + ); + + $owned = PVE::Multipath::Generator::owned_wwid_record( + { d => ['config', 'storage:san1'] }, # now also pinned by an explicit section + { d => ['storage:san1'] }, + { d => 1 }, + {}, + ); + is_deeply($owned, { d => ['config', 'storage:san1'] }, 'an owned WWID takes over new reasons'); + + is_deeply( + PVE::Multipath::Generator::owned_wwid_record({}, {}, {}, {}), + {}, + 'empty in, empty out', + ); + + ok( + PVE::Multipath::Generator::wwid_records_differ({ a => ['config'] }, { a => ['storage:x'] }), + 'records with different reasons differ', + ); + ok( + !PVE::Multipath::Generator::wwid_records_differ( + { a => ['config', 'storage:x'], b => ['storage:x'] }, + { b => ['storage:x'], a => ['storage:x', 'config'] }, + ), + 'records with the same reasons do not differ', + ); +} + # --- override guard --- eval { PVE::Multipath::Config::check_overrides("devices {\n\tdevice {\n\t\tvendor X\n\t}\n}\n") }; is($@, '', 'a well-formed devices{} block passes the guard'); @@ -385,4 +612,24 @@ is( 'the overrides writer trims trailing whitespace', ); +{ + my $mp = Test::MockModule->new('PVE::Multipath'); + my @commands; + $mp->mock(assert_supported => sub { 1 }, run_command => sub { push @commands, $_[0] }); + for my $wwid ('-W', '-F', '8:0') { + eval { PVE::Multipath::add_wwid($wwid) }; + like($@, qr/^invalid WWID/, "allow-list addition refuses '$wwid'"); + eval { PVE::Multipath::remove_wwid($wwid) }; + like($@, qr/^invalid WWID/, "allow-list removal refuses '$wwid'"); + } + is_deeply(\@commands, [], 'multipath never ran for them'); + PVE::Multipath::add_wwid('3600abc'); + PVE::Multipath::remove_wwid('3600abc'); + is_deeply( + \@commands, + [['/sbin/multipath', '-a', '--', '3600abc'], ['/sbin/multipath', '-w', '--', '3600abc']], + 'a WWID is passed after the end of the options', + ); +} + done_testing(); diff --git a/src/test/run_san_library_tests.pl b/src/test/run_san_library_tests.pl index 84a8539c..e1564deb 100755 --- a/src/test/run_san_library_tests.pl +++ b/src/test/run_san_library_tests.pl @@ -15,6 +15,7 @@ my %registered; my $cluster = Test::MockModule->new('PVE::Cluster'); $cluster->mock(cfs_register_file => sub { $registered{ $_[0] } = [@_[1, 2]] }); require PVE::Multipath::ClusterConfig; +require PVE::Multipath::Generator; subtest 'CFS callbacks preserve filenames, data and method receivers' => sub { my ($parse, $write) = $registered{'multipath.cfg'}->@*; @@ -33,6 +34,11 @@ subtest 'CFS callbacks preserve filenames, data and method receivers' => sub { subtest 'omitted optional parameters retain explicit undef behavior' => sub { my $config = Test::MockModule->new('PVE::Multipath::ClusterConfig'); + $config->mock(read_config => sub { die "configuration unavailable\n" }); + for my $args ([], [undef, undef, undef]) { + eval { PVE::Multipath::Generator::regenerate(@$args) }; + is($@, "configuration unavailable\n", 'regeneration reads the default configuration'); + } $config->mock(cfs_lock_file => sub { $@ = "lock unavailable\n" }); for my $lock ( \&PVE::Multipath::ClusterConfig::lock_config, diff --git a/src/test/run_san_taint_tests.pl b/src/test/run_san_taint_tests.pl new file mode 100755 index 00000000..fd992171 --- /dev/null +++ b/src/test/run_san_taint_tests.pl @@ -0,0 +1,80 @@ +#!/usr/bin/perl -T + +use strict; +use warnings; + +use File::Temp qw(tempdir); +use JSON qw(decode_json encode_json); +use Scalar::Util qw(tainted); +use Test::MockModule; +use Test::More; + +use lib ('.', '..'); +use PVE::Multipath; +use PVE::Tools; + +die "run this test with perl -T\n" if !${^TAINT}; +$ENV{PATH} = '/usr/sbin:/usr/bin:/sbin:/bin'; +delete @ENV{qw(IFS CDPATH ENV BASH_ENV)}; + +my $root = tempdir(CLEANUP => 1); +my sub from_file { + my ($data) = @_; + my $file = "$root/reply.json"; + open(my $fh, '>', $file) or die $!; + print $fh encode_json($data); + close($fh) or die $!; + return decode_json(PVE::Tools::file_get_contents($file)); +} + +my $mp = Test::MockModule->new('PVE::Multipath'); +my @commands; +my $signature; +$mp->mock( + dir_has_entries => sub { 0 }, + run_command => sub { + my ($cmd, %opts) = @_; + push @commands, [@$cmd]; + PVE::Tools::run_command(['/usr/bin/true', @$cmd]); + die "unexpected command @$cmd\n" if $cmd->[0] ne 'dmsetup'; + $opts{outfunc}->('0'); + }, +); +my $status_command = Test::MockModule->new('PVE::Storage::StatusCommand'); +$status_command->mock( + run => sub { + my ($cache, $cmd, %opts) = @_; + push @commands, [@$cmd]; + PVE::Tools::run_command(['/usr/bin/true', @$cmd]); + die "unexpected command @$cmd\n" if $cmd->[0] ne '/sbin/wipefs'; + $opts{outfunc} + ->(encode_json({ signatures => $signature ? [{ type => $signature }] : [] })); + }, +); + +for my $type (undef, 'LVM2_member') { + @commands = (); + $signature = $type; + my $map = from_file({ sysfs => 'dm-900', 'paths-active' => 1 }); + ok(tainted($map->{sysfs}), 'map name from JSON file is tainted'); + is(PVE::Multipath::map_user($map), $type ? 'LVM' : undef, + 'taint does not make map use unknown'); + is_deeply( + \@commands, + [ + ['dmsetup', 'info', '-c', '--noheadings', '-o', 'open', '/dev/dm-900'], + ['/sbin/wipefs', '--no-act', '--json', '/dev/dm-900'], + ], + 'both map probes cross a real benign exec boundary', + ); + ok(!tainted($_->[-1]), 'map probe uses captured device name') for @commands; +} + +for my $name (undef, '', 'dm-x', 'sda', '../dm-900', "dm-900\n", 'dm-900 extra') { + @commands = (); + my $map = from_file({ sysfs => $name, 'paths-active' => 1 }); + is(PVE::Multipath::map_user($map), 'unknown', 'invalid map name stays inconclusive'); + is_deeply(\@commands, [], 'invalid map name reaches no command'); +} + +done_testing(); -- 2.47.3