Re: [pve-devel] [PATCH manager v4 3/6] added Config for Shared Filesystem Directories

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

see cover letter for high level aspects!

On April 25, 2023 12:21 pm, Markus Frank wrote:
> and made an API Endpoint for getting, adding and removing
> directories to the config.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  PVE/API2/DirConfig.pm | 129 +++++++++++++++++++++++++++++++++++
>  PVE/API2/Makefile     |   1 +
>  PVE/API2/Nodes.pm     |   6 ++
>  PVE/DirConfig.pm      | 155 ++++++++++++++++++++++++++++++++++++++++++
>  PVE/Makefile          |   1 +
>  5 files changed, 292 insertions(+)
>  create mode 100644 PVE/API2/DirConfig.pm
>  create mode 100644 PVE/DirConfig.pm
> 
> diff --git a/PVE/API2/DirConfig.pm b/PVE/API2/DirConfig.pm
> new file mode 100644
> index 00000000..0cbc6f96
> --- /dev/null
> +++ b/PVE/API2/DirConfig.pm
> @@ -0,0 +1,129 @@
> +package PVE::API2::DirConfig;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::DirConfig;
> +use PVE::Tools qw(extract_param);
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    name => 'get_config',
> +    path => '',
> +    method => 'GET',
> +    description => "Get Directories for Host Directory Sharing.",
> +    permissions => {
> +	check => ['perm', '/map/dirs', [ 'Map.Audit' ]],
> +    },
> +    proxyto => 'node',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'object',
> +	    properties => {
> +		dirid => {
> +		    type => 'string',
> +		    description => 'Directory ID',
> +		},
> +		path => {
> +		    type => 'string',
> +		    description => 'Host Directory Path',
> +		},
> +	    },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $config = PVE::DirConfig::load_config($param->{node});
> +	delete $config->{description};
> +	my $result = [];
> +	foreach my $key (keys %{$config}) {
> +	    push @$result, {
> +		dirid => $key,
> +		path => $config->{$key},
> +	    };
> +	}
> +
> +	return $result;
> +    }
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'add_dir',
> +    path => '',
> +    method => 'POST',
> +    description => "Add Directories for Host Directory Sharing.",
> +    permissions => {
> +	check => ['perm', '/map/dirs', [ 'Map.Modify' ]],
> +    },
> +    protected => 1,
> +    proxyto => 'node',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    dirid => {
> +		type => 'string',
> +		pattern => '[a-zA-Z0-9\-]+',
> +	    },
> +	    path => {
> +		type => 'string',
> +		maxLength => 4096,
> +		format => 'pve-storage-path',

there's probably a few more options in the future ;) e.g. at least read
only would be expected, I am not sure which of the other things should
go here and which should go to the usage side in qemu-server:
- xattr (support)
- acls (support)
- recursion (i.e., cross mountpoint boundaries)
- caching
- direct IO

the first three might be configurable here (to signify that the underlying
storage supports this) and at the usage side (to control whether we want
to pass an enabled feature on to the guest).

caching and direct IO might be candidates for only being configurable at
usage time. I also haven't checked whether 9P and (either C or rust)
virtiofsd support the same things.

> +	    },
> +	},
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +	my $node = extract_param($param, 'node');
> +	my $dirid = extract_param($param, 'dirid');
> +	my $path = extract_param($param, 'path');
> +	PVE::DirConfig::add_dir_config($node, $dirid, $path);
> +	return undef;
> +    },
> +});
> +
> +
> +__PACKAGE__->register_method({
> +    name => 'del_dir',
> +    path => '',
> +    method => 'DELETE',
> +    description => "Remove Directory from Host Directory Sharing.",
> +    permissions => {
> +	check => ['perm', '/map/dirs', [ 'Map.Modify' ]],
> +    },
> +    protected => 1,
> +    proxyto => 'node',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    dirid => {
> +		type => 'string',
> +		pattern => '[a-zA-Z0-9\-]+',
> +	    },
> +	},
> +    },
> +    returns => { type => "null" },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $node = extract_param($param, 'node');
> +	my $dirid = extract_param($param, 'dirid');
> +	PVE::DirConfig::del_dir_config($node, $dirid);
> +	return undef;
> +    },
> +});
> +
> +
> +1;
> diff --git a/PVE/API2/Makefile b/PVE/API2/Makefile
> index 5c08ebe0..1b96223c 100644
> --- a/PVE/API2/Makefile
> +++ b/PVE/API2/Makefile
> @@ -12,6 +12,7 @@ PERLSOURCE = 			\
>  	Ceph.pm			\
>  	Certificates.pm		\
>  	Cluster.pm		\
> +	DirConfig.pm		\
>  	HAConfig.pm		\
>  	Hardware.pm		\
>  	Network.pm		\
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index bfe5c40a..b4e2992f 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -48,6 +48,7 @@ use PVE::API2::LXC::Status;
>  use PVE::API2::LXC;
>  use PVE::API2::Network;
>  use PVE::API2::NodeConfig;
> +use PVE::API2::DirConfig;
>  use PVE::API2::Qemu::CPU;
>  use PVE::API2::Qemu;
>  use PVE::API2::Replication;
> @@ -199,6 +200,11 @@ __PACKAGE__->register_method ({
>      path => 'config',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::DirConfig",
> +    path => 'dirs',
> +});
> +
>  if ($have_sdn) {
>      __PACKAGE__->register_method ({
>  	subclass => "PVE::API2::Network::SDN::Zones::Status",
> diff --git a/PVE/DirConfig.pm b/PVE/DirConfig.pm
> new file mode 100644
> index 00000000..56796029
> --- /dev/null
> +++ b/PVE/DirConfig.pm
> @@ -0,0 +1,155 @@
> +package PVE::DirConfig;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Tools qw(file_get_contents file_set_contents lock_file);
> +
> +my $dir_config_lock = '/var/lock/dirs.lock';

if this becomes cluster wide it needs to change. it should also probably
be a cfs_registered file.

> +
> +sub config_file {
> +    my ($node) = @_;
> +
> +    return "/etc/pve/nodes/${node}/dirs";
> +}
> +
> +sub load_config {
> +    my ($node) = @_;
> +
> +    my $filename = config_file($node);
> +    my $raw = eval { PVE::Tools::file_get_contents($filename); };
> +    return {} if !$raw;
> +
> +    return parse_file_config($raw, $filename);
> +}
> +
> +sub write_config {
> +    my ($node, $conf) = @_;
> +
> +    my $filename = config_file($node);
> +
> +    my $raw = write_file_config($conf);
> +
> +    PVE::Tools::file_set_contents($filename, $raw);
> +}
> +
> +sub lock_config {
> +    my ($node, $realcode, @param) = @_;
> +
> +    # make sure configuration file is up-to-date
> +    my $code = sub {
> +	PVE::Cluster::cfs_update();
> +	$realcode->(@_);
> +    };
> +
> +    my $res = lock_file($dir_config_lock, 10, $code, @param);
> +
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
> +my $descr = " Description for Shared Files Directory Config.\n"
> +    ." Add Directories with:\n dirid: /path/to/share";
> +
> +my $dir_desc = {
> +    path => {
> +	type => 'string',
> +	format_description => 'path',
> +	description => 'path of Directory ID',
> +	default_key => 1,
> +    },
> +};

this is too restrictive with regards to future extensions IMHO. see
above for some ideas. also see the discussions surrounding the hardware
map format.

> [..]

left out the rest since I expect the format to change ;)