Re: [pve-devel] [PATCH manager v2 1/6] api: ceph: add rbd namespace management endpoints

[Friedrich Weber <f.weber@proxmox.com>]

Hi, I have some minor comments inline:

On 23/12/2024 17:00, Aaron Lauterer wrote:
> RBD supports namespaces. To make the management easier and possible via
> the web UI, we need to add API endpoints to:
> * list
> * create
> * delete
> namespaces.
> 
> We only allow creatng namespaces for pools that have the RBD application
> set.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since v1:
> * integrated recommendations:
>   * code style in many places
>   * added check on removal if a storage config that uses the namespace
>     exists. I can be overriden with the new optional force flag
> * on create we trim the namespace parameter and check if it has any
>   length. Is there an option in the API that would do that for us
>   already?
> * added TODO note as we might want to rethink the ACL privileges
> 
>  PVE/API2/Ceph/Pool.pm | 205 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 203 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Ceph/Pool.pm b/PVE/API2/Ceph/Pool.pm
> index 5ee982f4..94121dea 100644
> --- a/PVE/API2/Ceph/Pool.pm
> +++ b/PVE/API2/Ceph/Pool.pm
> @@ -3,6 +3,8 @@ package PVE::API2::Ceph::Pool;
>  use strict;
>  use warnings;
>  
> +use JSON qw(decode_json);
> +
>  use PVE::Ceph::Tools;
>  use PVE::Ceph::Services;
>  use PVE::JSONSchema qw(get_standard_option parse_property_string);
> @@ -10,7 +12,7 @@ use PVE::RADOS;
>  use PVE::RESTHandler;
>  use PVE::RPCEnvironment;
>  use PVE::Storage;
> -use PVE::Tools qw(extract_param);
> +use PVE::Tools qw(extract_param run_command);
>  
>  use PVE::API2::Storage::Config;
>  
> @@ -32,6 +34,7 @@ my $get_autoscale_status = sub {
>      return $data;
>  };
>  
> +# TODO: think about adding dedicated Ceph ACL privileges as the currently used ones don't match well
>  
>  __PACKAGE__->register_method ({
>      name => 'lspools',
> @@ -302,7 +305,7 @@ my $ceph_pool_common_options = sub {
>  
>  
>  my $add_storage = sub {
> -    my ($pool, $storeid, $ec_data_pool) = @_;
> +    my ($pool, $storeid, $ec_data_pool, $namespace) = @_;
>  
>      my $storage_params = {
>  	type => 'rbd',
> @@ -312,6 +315,8 @@ my $add_storage = sub {
>  	content => 'rootdir,images',
>      };
>  
> +    $storage_params->{namespace} = $namespace if $namespace;
> +
>      $storage_params->{'data-pool'} = $ec_data_pool if $ec_data_pool;
>  
>      PVE::API2::Storage::Config->create($storage_params);
> @@ -798,4 +803,200 @@ __PACKAGE__->register_method ({
>      }});
>  
>  
> +my $get_rbd_namespaces = sub {
> +    my ($pool) = @_;
> +
> +    my $raw = '';
> +    run_command(
> +	['/usr/bin/rbd', 'namespace', 'list', $pool, '--format', 'json'],
> +	outfunc => sub { $raw .= shift },
> +	errmsg => "rbd error",
> +	errfunc => sub {},
> +    );
> +    return [] if $raw eq '[]';
> +
> +    my ($untainted_raw) = $raw =~ /^(.+)$/; # untaint
> +    my $namespaces = eval { decode_json($untainted_raw) };
> +    die "failed to parse as JSON - $@\n" if $@;
> +
> +    return [
> +	map { { name => $_->{name}  } } $namespaces->@*
> +    ];
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'listnamespaces',
> +    path => '{name}/namespace',
> +    method => 'GET',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit', 'Datastore.Audit' ], any => 1],
> +    },
> +    description => "Get pool RBD namespaces.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',

I was a bit confused about this default -- seeing that {name} is part of
the path, is there a situation where we can fallback to 'rbd'?

> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		name => { type => 'string', title => "Namespace" }
> +	    },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';

Do we need this fallback?

> +	return $get_rbd_namespaces->($pool);
> +    }});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'createnamespace',
> +    path => '{name}/namespace',
> +    method => 'POST',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    description => "Create new RBD namespace.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',

same as above

> +	    },
> +	    namespace => {
> +		description => 'The name of the new namespace',
> +		type => 'string',
> +	    },
> +	    'add-storage' => {
> +		description => "Configure VM and CT storage using the new namespace.",
> +		type => 'boolean',
> +		optional => 1,
> +		default => "0",
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';

same as above

> +	my $namespace = PVE::Tools::trim(extract_param($param, 'namespace'));
> +	my $add_storages = extract_param($param, 'add-storage');
> +
> +	die "namespace cannot be empty" if length($namespace) < 1; # in case it was just whitespace

missing a \n at the end

> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +	if ($add_storages) {
> +	    $rpcenv->check($user, '/storage', ['Datastore.Allocate']);
> +	    die "namespace name contains characters which are illegal for storage naming\n"
> +		if !PVE::JSONSchema::parse_storage_id("${pool}-${namespace}");
> +	}
> +
> +	my $rados = PVE::RADOS->new();
> +	my $apps = $rados->mon_command({ prefix => "osd pool application get", pool => "$pool", });
> +	die "the pool does not have application 'rbd' enabled" if !defined($apps->{rbd});

missing a \n at the end

> +
> +	my $current_namespaces = { map { $_->{name} => 1 } $get_rbd_namespaces->($pool)->@*};
> +	die "namespace already exists" if $current_namespaces->{$namespace};

missing a \n at the end

> +
> +	my $worker = sub {
> +	    run_command(
> +		['/usr/bin/rbd', 'namespace', 'create', "${pool}/${namespace}"],
> +		errmsg => "rbd create namespace error",
> +		errfunc => sub {},
> +		outfunc => sub {},
> +	    );
> +	    if ($add_storages) {
> +		eval { $add_storage->($pool, "${pool}-${namespace}", 0, $namespace) };
> +		die "adding PVE storage for ceph rbd namespace failed: pool: ${pool}, namespace: ${namespace}: $@\n" if $@;
> +	    }
> +	};
> +
> +	return $rpcenv->fork_worker('cephcreaterbdnamespace', $pool,  $user, $worker);
> +    }});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'destroynamespace',
> +    path => '{name}/namespace',
> +    method => 'DELETE',
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Modify' ]],
> +    },
> +    description => "Delete RBD namespace.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    name => {
> +		description => 'The name of the pool.',
> +		type => 'string',
> +		default => 'rbd',
> +	    },
> +	    namespace => {
> +		description => 'The name of the namespace',
> +		type => 'string',
> +	    },
> +	    force => {
> +		description => 'Force removal of the namespace',
> +		type => 'boolean',
> +		optional => 1,
> +	    },
> +	},
> +    },
> +    returns => { type => 'string' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $pool = extract_param($param, 'name') // 'rbd';
> +	my $namespace = extract_param($param, 'namespace');
> +	my $force = extract_param($param, 'force');
> +
> +	if (!$force) {
> +	    my $storages = $get_storages->($pool);
> +	    for my $storage (keys $storages->%*) {
> +		if (
> +		    defined($storages->{$storage}->{namespace})
> +		    && $storages->{$storage}->{namespace} eq $namespace
> +		) {
> +		    die "namespace '${namespace}' is configured in storage '${storage}', remove it first";

missing a \n at the end

> +		}
> +	    }
> +	}
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +	my $worker = sub {
> +	    run_command(
> +		['/usr/bin/rbd', 'namespace', 'remove', "${pool}/${namespace}"],
> +		errmsg => "rbd create namespace error",

*remove

> +		errfunc => sub {},
> +		outfunc => sub {},
> +	    );
> +	};
> +
> +	return $rpcenv->fork_worker('cephdestroyrbdnamespace', $pool,  $user, $worker);
> +    }});
> +
>  1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel