From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 0AEAA1FF146 for ; Tue, 07 Jul 2026 15:04:35 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 45BE321475; Tue, 07 Jul 2026 15:04:29 +0200 (CEST) Message-ID: Date: Tue, 7 Jul 2026 15:04:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH v2 storage 03/13] api: multipath: add cluster-wide configuration endpoints To: Thomas Lamprecht , pve-devel@lists.proxmox.com References: <20260703124707.1172980-2-t.lamprecht@proxmox.com> <20260703124707.1172980-5-t.lamprecht@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260703124707.1172980-5-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783429454440 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.088 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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: IMY53UQ7OFBGPBTA7YH35EEPFD4UZVGB X-Message-ID-Hash: IMY53UQ7OFBGPBTA7YH35EEPFD4UZVGB X-MailFrom: d.csapak@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: high level comment: I find it a bit unusual that only the 'edit' wwid api path allows setting properties, don't we usually allow that directly on the add path too? some nits inline On 7/3/26 5:31 PM, Thomas Lamprecht wrote: > Add a CRUD API over the cluster-wide multipath.cfg, mounted by > pve-manager at /cluster/multipath: read the effective configuration, > manage the WWID allow-list, set the global defaults and the per-LUN > knobs and aliases, and replace the verbatim override sections. > > Updates take the settable knobs straight from the section schema, with > the schema 'default' stripped so omitting a knob leaves it unchanged > rather than resetting it; 'delete' is guarded to the settable > properties so it cannot strip a section's type and corrupt the file. > Applying the result on each node is handled separately by pvestatd via > the generator. > > Signed-off-by: Thomas Lamprecht > --- > > Changes in v2: > - guard the config writes with a digest against concurrent > modifications > - move the free-form overrides to their own GET/PUT endpoint with a > digest and cfs lock of their own (the edit window could never save) > - require the alias parameter on the alias POST instead of clearing > the alias when it is omitted > - add a wwid index GET, the per-LUN options were write-only > - drop the defaults section again when its last knob is deleted, an > empty leftover would keep the generator engaged forever > - reserve the 'defaults' id; de-duplicate the write paths > > src/PVE/API2/Makefile | 1 + > src/PVE/API2/Multipath.pm | 516 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 517 insertions(+) > create mode 100644 src/PVE/API2/Multipath.pm > > diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile > index fe316c5..002a3bd 100644 > --- a/src/PVE/API2/Makefile > +++ b/src/PVE/API2/Makefile > @@ -3,5 +3,6 @@ > .PHONY: install > install: > install -D -m 0644 Disks.pm ${DESTDIR}${PERLDIR}/PVE/API2/Disks.pm > + install -D -m 0644 Multipath.pm ${DESTDIR}${PERLDIR}/PVE/API2/Multipath.pm > make -C Storage install > make -C Disks install > diff --git a/src/PVE/API2/Multipath.pm b/src/PVE/API2/Multipath.pm > new file mode 100644 > index 0000000..cb138f6 > --- /dev/null > +++ b/src/PVE/API2/Multipath.pm > @@ -0,0 +1,516 @@ > +package PVE::API2::Multipath; > + > +use strict; > +use warnings; > + > +use PVE::Exception qw(raise_param_exc); > +use PVE::JSONSchema qw(get_standard_option); > +use PVE::Tools qw(extract_param); > + > +use PVE::Multipath; > +use PVE::Multipath::Config; > +use PVE::Multipath::ClusterConfig; > + > +use PVE::RESTHandler; > + > +use base qw(PVE::RESTHandler); > + > +# WWID as exported by /dev/disk/by-id/dm-uuid-mpath-; keep validation lenient, multipathd is > +# the authority on what is valid. > +my $WWID_RE = qr/^[a-zA-Z0-9._:-]+\z/; > + > +# multipathd resolves an alias to a map name, so two WWIDs sharing one alias makes it drop a map > +# (the loser is order-dependent and only logged at level 1). Reject a collision up front. > +my sub assert_alias_free { > + my ($cfg, $wwid, $alias) = @_; > + > + for my $other (keys $cfg->{ids}->%*) { > + next if $other eq $wwid; > + my $section = $cfg->{ids}->{$other}; > + next if ($section->{type} // '') ne 'wwid'; > + die "alias '$alias' is already assigned to WWID '$other'\n" > + if defined($section->{alias}) && $section->{alias} eq $alias; > + } > +} > + > +# Apply a section update: set the given properties, then unset those named in $delete. Guard $delete > +# to the settable options and never set and delete the same key, so a stray 'delete=type' cannot > +# strip the section type and corrupt the file (as PVE::API2::Storage::Config guards it too). > +my sub apply_section_update { > + my ($section, $param, $delete, $settable) = @_; > + > + my @delete = split(/,/, $delete // ''); > + for my $key (@delete) { > + raise_param_exc({ delete => "'$key' is not a settable property" }) if !$settable->{$key}; > + raise_param_exc({ $key => "cannot set and delete a property at the same time" }) > + if defined($param->{$key}); > + } > + $section->{$_} = $param->{$_} for keys %$param; > + delete $section->{$_} for @delete; > +} > + > +# Read the cluster config inside the lock and reject the write if it changed since the caller read > +# the digest, so a concurrent edit is not silently overwritten. > +my sub read_locked_config { > + my ($digest) = @_; > + > + my $cfg = PVE::Multipath::ClusterConfig::read_config(); > + PVE::Tools::assert_if_modified($cfg->{digest}, $digest); > + return $cfg; > +} > + > +# Read the locked config (asserting the digest), run $code to mutate it, then write it back; wraps > +# the read-modify-write envelope so each endpoint carries only its own mutation. > +my sub edit_locked_config { > + my ($digest, $errmsg, $code) = @_; > + > + PVE::Multipath::ClusterConfig::lock_config( > + sub { > + my $cfg = read_locked_config($digest); > + $code->($cfg); > + PVE::Multipath::ClusterConfig::write_config($cfg); > + }, > + $errmsg, > + ); > +} nit: both methods have the same naming scheme but are working differently (one needs to be called locked, one locks itself) my suggestion: rename 'read_locked_config' to 'read_config_unlocked' and 'edit_locked_config' to 'edit_config_locked' wdyt ? > + > +# Validate a WWID from the API and reject the reserved 'defaults' id (the global section), so a WWID > +# endpoint can never read, overwrite, or delete it. > +my sub assert_valid_wwid { > + my ($wwid) = @_; > + > + raise_param_exc({ wwid => "does not look like a valid WWID" }) > + if !defined($wwid) || $wwid !~ $WWID_RE || $wwid eq 'defaults'; > +} > + > +__PACKAGE__->register_method({ > + name => 'read', > + path => '', > + method => 'GET', > + description => "Read the cluster-wide multipath configuration.", > + permissions => { > + check => ['perm', '/', ['Sys.Audit']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => {}, > + }, > + returns => { > + type => 'object', > + properties => { > + defaults => { > + type => 'object', > + description => 'Effective global multipathd knobs.', > + additionalProperties => 1, > + }, > + wwids => { > + type => 'array', > + description => 'The multipath WWID allow-list.', > + items => { type => 'string' }, > + }, > + aliases => { > + type => 'object', > + description => 'Per-WWID human-readable aliases.', > + additionalProperties => { type => 'string' }, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + code => sub { > + my $cfg = PVE::Multipath::ClusterConfig::read_config(); > + return { > + defaults => PVE::Multipath::Config::effective_defaults($cfg), > + wwids => PVE::Multipath::Config::wwid_list($cfg), > + aliases => PVE::Multipath::Config::aliases($cfg), > + digest => $cfg->{digest}, > + }; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'read_overrides', > + path => 'overrides', > + method => 'GET', > + description => "Read the verbatim multipath.conf override sections.", > + permissions => { > + check => ['perm', '/', ['Sys.Audit']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => {}, > + }, > + returns => { > + type => 'object', > + properties => { > + overrides => { > + type => 'string', > + description => 'Verbatim override sections.', > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + code => sub { > + my $overrides = PVE::Multipath::ClusterConfig::read_overrides(); > + return { > + digest => PVE::Multipath::ClusterConfig::overrides_digest($overrides), > + defined($overrides) && length($overrides) ? (overrides => $overrides) : (), > + }; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'set_overrides', > + path => 'overrides', > + method => 'PUT', > + protected => 1, > + description => "Set the verbatim multipath.conf override sections, such as " > + . "hardware-specific 'device {}' or 'overrides {}' blocks.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + overrides => { > + type => 'string', > + description => 'Verbatim override sections. Omit to clear them.', > + maxLength => 128 * 1024, > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $overrides = $param->{overrides}; > + my $digest = extract_param($param, 'digest'); > + eval { PVE::Multipath::Config::check_overrides($overrides) }; > + raise_param_exc({ overrides => $@ }) if $@; > + > + PVE::Multipath::ClusterConfig::lock_overrides( > + sub { > + my $current = PVE::Multipath::ClusterConfig::read_overrides(); > + PVE::Tools::assert_if_modified( > + PVE::Multipath::ClusterConfig::overrides_digest($current), $digest, > + ); > + PVE::Multipath::ClusterConfig::write_overrides($overrides // ''); > + }, > + "updating multipath overrides failed", > + ); > + return undef; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'set_defaults', > + path => 'defaults', > + method => 'PUT', > + protected => 1, > + description => "Set the global multipathd defaults applied on every node.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + PVE::Multipath::Config::defaults_api_schema()->%*, > + delete => { > + type => 'string', > + description => 'A list of settings to reset to their default, comma-separated.', > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $delete = extract_param($param, 'delete'); > + my $digest = extract_param($param, 'digest'); > + my $settable = PVE::Multipath::Config::defaults_api_schema(); > + > + edit_locked_config( > + $digest, > + "updating multipath defaults failed", > + sub { > + my ($cfg) = @_; > + my $section = $cfg->{ids}->{defaults} //= { type => 'defaults' }; > + apply_section_update($section, $param, $delete, $settable); > + # drop the section when its last knob was deleted: the reserved id has no remove > + # endpoint, and an empty section would keep the generator engaged forever > + delete $cfg->{ids}->{defaults} if !grep { $_ ne 'type' } keys %$section; > + }, > + ); > + return undef; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'wwid_index', > + path => 'wwid', > + method => 'GET', > + description => "List the allow-listed WWIDs with their alias and per-LUN multipathd knobs.", > + permissions => { > + check => ['perm', '/', ['Sys.Audit']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => {}, > + }, > + returns => { > + type => 'array', > + items => { > + type => 'object', > + properties => { > + wwid => { type => 'string', description => 'The LUN WWID.' }, > + PVE::Multipath::Config::wwid_api_schema()->%*, > + }, > + }, > + links => [{ rel => 'child', href => '{wwid}' }], > + }, > + code => sub { > + my $cfg = PVE::Multipath::ClusterConfig::read_config(); > + my $opts = PVE::Multipath::Config::wwid_opts($cfg); > + return [map { { wwid => $_, $opts->{$_}->%* } } sort keys %$opts]; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'add_wwid', > + path => 'wwid', > + method => 'POST', > + protected => 1, > + description => "Add a WWID to the multipath allow-list.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + wwid => { > + type => 'string', > + description => 'The WWID of the LUN to manage via multipath.', > + maxLength => 128, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $wwid = $param->{wwid}; > + my $digest = extract_param($param, 'digest'); > + assert_valid_wwid($wwid); > + > + edit_locked_config( > + $digest, > + "adding WWID '$wwid' failed", > + sub { > + my ($cfg) = @_; > + $cfg->{ids}->{$wwid} //= { type => 'wwid' }; > + }, > + ); > + return undef; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'update_wwid', > + path => 'wwid/{wwid}', > + method => 'PUT', > + protected => 1, > + description => "Set the alias and per-LUN multipathd knobs for an allow-listed WWID.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + wwid => { > + type => 'string', > + description => 'The WWID to update.', > + maxLength => 128, > + }, > + PVE::Multipath::Config::wwid_api_schema()->%*, > + delete => { > + type => 'string', > + description => 'A list of settings to unset, comma-separated.', > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $wwid = extract_param($param, 'wwid'); > + my $delete = extract_param($param, 'delete'); > + my $digest = extract_param($param, 'digest'); > + my $settable = PVE::Multipath::Config::wwid_api_schema(); > + assert_valid_wwid($wwid); > + > + edit_locked_config( > + $digest, > + "updating WWID '$wwid' failed", > + sub { > + my ($cfg) = @_; > + my $section = $cfg->{ids}->{$wwid}; > + die "WWID '$wwid' is not on the allow-list, add it first\n" if !$section; > + > + assert_alias_free($cfg, $wwid, $param->{alias}) if defined($param->{alias}); > + apply_section_update($section, $param, $delete, $settable); > + }, > + ); > + return undef; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'remove_wwid', > + path => 'wwid/{wwid}', > + method => 'DELETE', > + protected => 1, > + description => "Remove a WWID from the multipath allow-list.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + wwid => { > + type => 'string', > + description => 'The WWID to remove.', > + maxLength => 128, > + }, > + force => { > + type => 'boolean', > + description => 'Remove even while a storage still consumes the LUN.', > + optional => 1, > + default => 0, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $wwid = $param->{wwid}; > + my $digest = extract_param($param, 'digest'); > + assert_valid_wwid($wwid); > + > + # dropping a consumed WWID flushes its map on the next sync, so the backing storage loses > + # the device; refuse unless explicitly forced > + if (!$param->{force}) { > + my $expectations = PVE::Multipath::cluster_storage_expectations(); > + my $storeid = $expectations->{consumers}->{$wwid}; > + die "WWID '$wwid' is still used by storage '$storeid', remove that" > + . " storage first or pass 'force' to override\n" > + if defined($storeid); > + } > + > + edit_locked_config( > + $digest, > + "removing WWID '$wwid' failed", > + sub { > + my ($cfg) = @_; > + delete $cfg->{ids}->{$wwid}; > + }, > + ); > + return undef; > + }, > +}); > + > +__PACKAGE__->register_method({ > + name => 'set_alias', > + path => 'alias', > + method => 'POST', > + protected => 1, > + description => "Set or replace the human-readable alias for an allow-listed WWID." > + . " The alias becomes the dm-multipath map name on every node.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + wwid => { > + type => 'string', > + description => 'The WWID to label.', > + maxLength => 128, > + }, > + alias => { PVE::Multipath::Config::wwid_api_schema()->{alias}->%*, optional => 0 }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $wwid = $param->{wwid}; > + my $alias = $param->{alias}; > + my $digest = extract_param($param, 'digest'); > + assert_valid_wwid($wwid); > + > + edit_locked_config( > + $digest, > + "setting alias for WWID '$wwid' failed", > + sub { > + my ($cfg) = @_; > + die "WWID '$wwid' is not on the allow-list, add it first\n" > + if !$cfg->{ids}->{$wwid}; > + assert_alias_free($cfg, $wwid, $alias); > + $cfg->{ids}->{$wwid}->{alias} = $alias; > + }, > + ); > + return undef; > + }, > +}); how does this api call differ from setting the alias with the update wwid call? > + > +__PACKAGE__->register_method({ > + name => 'remove_alias', > + path => 'alias/{wwid}', > + method => 'DELETE', > + protected => 1, > + description => "Remove the alias for a multipath WWID.", > + permissions => { > + check => ['perm', '/', ['Sys.Modify']], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + wwid => { > + type => 'string', > + description => 'The WWID whose alias to remove.', > + maxLength => 128, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'null' }, > + code => sub { > + my ($param) = @_; > + > + my $wwid = $param->{wwid}; > + my $digest = extract_param($param, 'digest'); > + assert_valid_wwid($wwid); > + > + edit_locked_config( > + $digest, > + "removing alias for WWID '$wwid' failed", > + sub { > + my ($cfg) = @_; > + delete $cfg->{ids}->{$wwid}->{alias} if $cfg->{ids}->{$wwid}; > + }, > + ); > + return undef; > + }, > +}); same here? can't i just update the wwid with 'delete: alias' ? > + > +1;