Re: [PATCH v2 storage 03/13] api: multipath: add cluster-wide configuration endpoints

[Dominik Csapak <d.csapak@proxmox.com>]

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 <t.lamprecht@proxmox.com>
> ---
> 
> 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-<wwid>; 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;