[pve-devel] [PATCH storage 3/3] disks: allow add_storage for already configured local storage
Dominik Csapak
d.csapak at proxmox.com
Thu Jul 14 13:23:19 CEST 2022
comments inline
On 7/13/22 12:47, Aaron Lauterer wrote:
> One of the smaller annoyances, especially for less experienced users, is
> the fact, that when creating a local storage (ZFS, LVM (thin), dir) in a
> cluster, one can only leave the "Add Storage" option enabled the first
> time.
>
> On any following node, this option needed to be disabled and the new
> node manually added to the list of nodes for that storage.
>
> This patch changes the behavior. If a storage of the same name already
> exists, it will verify that necessary parameters match the already
> existing one.
> Then, if the 'nodes' parameter is set, it adds the current node and
> updates the storage config.
> In case there is no nodes list, nothing else needs to be done, and the
> GUI will stop showing the question mark for the configured, but until
> then, not existing local storage.
>
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>
> I decided to make the storage type a parameter of the
> 'assert_sid_usable_on_node' function and not part of the $verify_params,
> so that it cannot be forgotten when calling it.
>
> PVE/API2/Disks/Directory.pm | 32 ++++++++++++++++++-----------
> PVE/API2/Disks/LVM.pm | 30 +++++++++++++++++----------
> PVE/API2/Disks/LVMThin.pm | 30 +++++++++++++++++----------
> PVE/API2/Disks/ZFS.pm | 29 +++++++++++++++++---------
> PVE/Storage.pm | 41 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 118 insertions(+), 44 deletions(-)
>
> diff --git a/PVE/API2/Disks/Directory.pm b/PVE/API2/Disks/Directory.pm
> index 8e03229..00b9a37 100644
> --- a/PVE/API2/Disks/Directory.pm
> +++ b/PVE/API2/Disks/Directory.pm
> @@ -203,10 +203,14 @@ __PACKAGE__->register_method ({
> my $dev = $param->{device};
> my $node = $param->{node};
> my $type = $param->{filesystem} // 'ext4';
> + my $path = "/mnt/pve/$name";
>
> $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> PVE::Diskmanage::assert_disk_unused($dev);
> - PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
> +
> + my $verify_params = { path => $path };
> + PVE::Storage::assert_sid_usable_on_node($name, $node, 'dir', $verify_params)
> + if $param->{add_storage};
>
> my $mounted = PVE::Diskmanage::mounted_paths();
> if ($mounted->{$path} =~ /^(\/dev\/.+)$/ ) {
> @@ -214,7 +218,6 @@ __PACKAGE__->register_method ({
> }
>
> my $worker = sub {
> - my $path = "/mnt/pve/$name";
> my $mountunitname = PVE::Systemd::escape_unit($path, 1) . ".mount";
> my $mountunitpath = "/etc/systemd/system/$mountunitname";
>
> @@ -287,16 +290,21 @@ __PACKAGE__->register_method ({
> run_command(['systemctl', 'start', $mountunitname]);
>
> if ($param->{add_storage}) {
> - my $storage_params = {
> - type => 'dir',
> - storage => $name,
> - content => 'rootdir,images,iso,backup,vztmpl,snippets',
> - is_mountpoint => 1,
> - path => $path,
> - nodes => $node,
> - };
> -
> - PVE::API2::Storage::Config->create($storage_params);
> + my $status = PVE::Storage::create_or_update_storage($name, $node);
> + if ($status->{create}) {
> + my $storage_params = {
> + type => 'dir',
> + storage => $name,
> + content => 'rootdir,images,iso,backup,vztmpl,snippets',
> + is_mountpoint => 1,
> + path => $path,
> + nodes => $node,
> + };
> + PVE::API2::Storage::Config->create($storage_params);
> + } elsif ($status->{update_params}) {
> + print "Adding '${node}' to nodes for storage '${name}'\n";
> + PVE::API2::Storage::Config->update($status->{update_params});
> + } # no action needed if storage exists, but not limited to nodes
> }
> });
> };
> diff --git a/PVE/API2/Disks/LVM.pm b/PVE/API2/Disks/LVM.pm
> index a27afe2..88b8d0b 100644
> --- a/PVE/API2/Disks/LVM.pm
> +++ b/PVE/API2/Disks/LVM.pm
> @@ -150,7 +150,10 @@ __PACKAGE__->register_method ({
>
> $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> PVE::Diskmanage::assert_disk_unused($dev);
> - PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
> +
> + my $verify_params = { vgname => $name };
> + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvm', $verify_params)
> + if $param->{add_storage};
>
> die "volume group with name '${name}' already exists\n"
> if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
> @@ -169,16 +172,21 @@ __PACKAGE__->register_method ({
> PVE::Diskmanage::udevadm_trigger($dev);
>
> if ($param->{add_storage}) {
> - my $storage_params = {
> - type => 'lvm',
> - vgname => $name,
> - storage => $name,
> - content => 'rootdir,images',
> - shared => 0,
> - nodes => $node,
> - };
> -
> - PVE::API2::Storage::Config->create($storage_params);
> + my $status = PVE::Storage::create_or_update_storage($name, $node);
> + if ($status->{create}) {
> + my $storage_params = {
> + type => 'lvm',
> + vgname => $name,
> + storage => $name,
> + content => 'rootdir,images',
> + shared => 0,
> + nodes => $node,
> + };
> + PVE::API2::Storage::Config->create($storage_params);
> + } elsif ($status->{update_params}) {
> + print "Adding '${node}' to nodes for storage '${name}'\n";
> + PVE::API2::Storage::Config->update($status->{update_params});
> + } # no action needed if storage exists, but not limited to nodes
> }
> });
> };
> diff --git a/PVE/API2/Disks/LVMThin.pm b/PVE/API2/Disks/LVMThin.pm
> index 690c183..d880154 100644
> --- a/PVE/API2/Disks/LVMThin.pm
> +++ b/PVE/API2/Disks/LVMThin.pm
> @@ -108,7 +108,10 @@ __PACKAGE__->register_method ({
>
> $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> PVE::Diskmanage::assert_disk_unused($dev);
> - PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
> +
> + my $verify_params = { vgname => $name, thinpool => $name };
> + PVE::Storage::assert_sid_usable_on_node($name, $node, 'lvmthin', $verify_params)
> + if $param->{add_storage};
>
> die "volume group with name '${name}' already exists\n"
> if PVE::Storage::LVMPlugin::lvm_vgs()->{$name};
> @@ -147,16 +150,21 @@ __PACKAGE__->register_method ({
> PVE::Diskmanage::udevadm_trigger($dev);
>
> if ($param->{add_storage}) {
> - my $storage_params = {
> - type => 'lvmthin',
> - vgname => $name,
> - thinpool => $name,
> - storage => $name,
> - content => 'rootdir,images',
> - nodes => $node,
> - };
> -
> - PVE::API2::Storage::Config->create($storage_params);
> + my $status = PVE::Storage::create_or_update_storage($name, $node);
> + if ($status->{create}) {
> + my $storage_params = {
> + type => 'lvmthin',
> + vgname => $name,
> + thinpool => $name,
> + storage => $name,
> + content => 'rootdir,images',
> + nodes => $node,
> + };
> + PVE::API2::Storage::Config->create($storage_params);
> + } elsif ($status->{update_params}) {
> + print "Adding '${node}' to nodes for storage '${name}'\n";
> + PVE::API2::Storage::Config->update($status->{update_params});
> + } # no action needed if storage exists, but not limited to nodes
> }
> });
> };
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index ceb0212..793bc14 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -337,6 +337,7 @@ __PACKAGE__->register_method ({
>
> my $name = $param->{name};
> my $devs = [PVE::Tools::split_list($param->{devices})];
> + my $node = $param->{node};
> my $raidlevel = $param->{raidlevel};
> my $compression = $param->{compression} // 'on';
>
> @@ -344,7 +345,10 @@ __PACKAGE__->register_method ({
> $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> PVE::Diskmanage::assert_disk_unused($dev);
> }
> - PVE::Storage::assert_sid_unused($name) if $param->{add_storage};
> +
> + my $verify_params = { pool => $name };
> + PVE::Storage::assert_sid_usable_on_node($name, $node, 'zfspool', $verify_params)
> + if $param->{add_storage};
>
> my $pools = PVE::API2::Disks::ZFS->index({ node => $param->{node} });
> my $poollist = { map { $_->{name} => 1 } @{$pools} };
> @@ -427,15 +431,20 @@ __PACKAGE__->register_method ({
> PVE::Diskmanage::udevadm_trigger($devs->@*);
>
> if ($param->{add_storage}) {
> - my $storage_params = {
> - type => 'zfspool',
> - pool => $name,
> - storage => $name,
> - content => 'rootdir,images',
> - nodes => $param->{node},
> - };
> -
> - PVE::API2::Storage::Config->create($storage_params);
> + my $status = PVE::Storage::create_or_update_storage($name, $node);
> + if ($status->{create}) {
> + my $storage_params = {
> + type => 'zfspool',
> + pool => $name,
> + storage => $name,
> + content => 'rootdir,images',
> + nodes => $param->{node},
> + };
> + PVE::API2::Storage::Config->create($storage_params);
> + } elsif ($status->{update_params}) {
> + print "Adding '${node}' to nodes for storage '${name}'\n";
> + PVE::API2::Storage::Config->update($status->{update_params});
> + } # no action needed if storage exists, but not limited to nodes
i've read that chunk 4 times now (apart from the params assembly ofc),
and imho i think this could belong into the 'create_or_update_storage'
function itself, at least we wouldn't have the same
if (create) {
create()
} else {
update()
}
code multiple times
if you're concerned with dependency of PVE::Storage and
PVE::API2::Storage::Config, id just move it there, then there is no real
problem
> }
> };
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index b9c53a1..6dfcc3c 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -2127,6 +2127,47 @@ sub assert_sid_unused {
> return undef;
> }
>
> +# Checks if storage id can be used on the node, intended for local storage types
> +# Compares the type and verify_params hash against a potentially configured
> +# storage. Use it for storage parameters that need to be the same throughout
> +# the cluster. For example, pool for ZFS, vg_name for LVM, ...
> +sub assert_sid_usable_on_node {
> + my ($sid, $node, $type, $verify_params) = @_;
> +
> + my $cfg = config();
> + if (my $scfg = storage_config($cfg, $sid, 1)) {
> + $node = PVE::INotify::nodename() if !$node || ($node eq 'localhost');
> + die "storage ID '${sid}' already exists on node ${node}\n"
> + if defined($scfg->{nodes}) && $scfg->{nodes}->{$node};
> +
> + $verify_params->{type} = $type;
> + for my $key (keys %$verify_params) {
> + die "option '${key}' ($verify_params->{$key}) does not match ".
> + "existing storage configuration: $scfg->{$key}\n"
> + if $verify_params->{$key} ne $scfg->{$key};
that'll log a warning if $scfg->{$key} is undef so a "// ''" could help
here (or explicit definedness check)
> + }
> + }
> +}
> +
> +# returns if storage needs to be created or updated. In the update case it
> +# checks for a node list and returns the needed parameters to update the
> +# storage with the new node list
> +sub create_or_update_storage {
> + my ($sid, $node) = @_;
> +
> + my $cfg = config();
> + my $status = { create => 1 };
> + if (my $scfg = storage_config($cfg, $sid, 1)) {
> + $status->{create} = 0;
> + if ($scfg->{nodes}) {
> + $scfg->{nodes}->{$node} = 1;
> + $status->{update_params}->{nodes} = join(',', sort keys $scfg->{nodes}->%*),
> + $status->{update_params}->{storage} = $sid;
> + }
> + }
> + return $status;
> +}
> +
> # removes leading/trailing spaces and (back)slashes completely
> # substitutes every non-ASCII-alphanumerical char with '_', except '_.-'
> sub normalize_content_filename {
More information about the pve-devel
mailing list