[pve-devel] [v2 storage 4/4] rbd: implement pveceph flag handling

[Thomas Lamprecht]

On 08/29/2017 01:04 PM, Fabian Grünbichler wrote:
> add /etc/pve/ceph.conf to commands / option strings instead
> of the monitor list provided via the 'monhost' option.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> new in v2
> 
> note: rbd and rados silently ignore all but the last '-c' parameter, so we
> can't support a custom per storage config and the general pveceph config at the
> same time. I am pretty sure that people that use this feature (Alexandre ;)) do
> not rely on pveceph to manage their storage configuration..
> 
>   PVE/Storage/RBDPlugin.pm | 47 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 55fd252..d209e9e 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -10,6 +10,8 @@ use PVE::JSONSchema qw(get_standard_option);
>   
>   use base qw(PVE::Storage::Plugin);
>   
> +my $pveceph_config = '/etc/pve/ceph.conf';
> +
>   my $rbd_unittobytes = {
>       "k"  => 1024,
>       "M"  => 1024*1024,
> @@ -37,16 +39,29 @@ my $hostlist = sub {
>       } @monhostlist);
>   };
>   
> +my $check_monhost_pveceph = sub {
> +    my ($scfg, $storeid) = @_;
> +
> +    die "'monhost' and 'pveceph' options set on storage '$storeid'\n"

This error message does not states the problem but just the fact that I set
both params, maybe something like:

"'monhost' and 'pveceph' must not be set at the same time (storage: $storage)"

or

"mutual exclusive options 'monhost' and 'pveceph' both set on storage: $storage"

would make it more clear what the actual problem is?

> +	if $scfg->{monhost} && $scfg->{pveceph};
> +
> +    die "one of 'monhost' or 'pveceph' must be set on storage '$storeid'\n"
> +	if !($scfg->{monhost} || $scfg->{pveceph});
> +};
> +
>   my $build_cmd = sub {
>       my ($binary, $scfg, $storeid, $op, @options) = @_;
>   
> -    my $monhost = &$hostlist($scfg->{monhost}, ',');
> +    $check_monhost_pveceph->($scfg, $storeid);
>   
>       my $keyring = "/etc/pve/priv/ceph/${storeid}.keyring";
>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>       my $username =  $scfg->{username} ? $scfg->{username} : 'admin';
>   
> -    my $cmd = [$binary, '-p', $pool, '-m', $monhost];
> +    my $cmd = [$binary, '-p', $pool];
> +
> +    push @$cmd, '-m', $hostlist->($scfg->{monhost}, ',') if $scfg->{monhost};
> +    push @$cmd, '-c', $pveceph_config if $scfg->{pveceph};
>   
>       if (-e $keyring) {
>   	push @$cmd, '-n', "client.$username";
> @@ -59,7 +74,11 @@ my $build_cmd = sub {
>       my $cephconfig = "/etc/pve/priv/ceph/${storeid}.conf";
>   
>       if (-e $cephconfig) {
> -	push @$cmd, '-c', $cephconfig;
> +	if ($scfg->{pveceph}) {
> +	    warn "ignoring custom ceph config for storage '$storeid', 'pveceph' is set!\n";
> +	} else {
> +	    push @$cmd, '-c', $cephconfig;
> +	}
>       }
>   
>       push @$cmd, $op;
> @@ -295,15 +314,23 @@ sub path {
>       my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>       $name .= '@'.$snapname if $snapname;
>   
> +    $check_monhost_pveceph->($scfg, $storeid);
> +
>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>       return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd};
>   
> -    my $monhost = &$hostlist($scfg->{monhost}, ';');
> -    $monhost =~ s/:/\\:/g;
> -
>       my $username =  $scfg->{username} ? $scfg->{username} : 'admin';
>   
> -    my $path = "rbd:$pool/$name:mon_host=$monhost";
> +    my $path = "rbd:$pool/$name";
> +
> +    if ($scfg->{monhost}) {
> +	my $monhost = $hostlist->($scfg->{monhost}, ';');
> +	$monhost =~ s/:/\\:/g;
> +	$path .= ":mon_host=$monhost";
> +    }
> +
> +    $path .= ":conf=$pveceph_config" if $scfg->{pveceph};
> +
>       my $keyring = "/etc/pve/priv/ceph/${storeid}.keyring";
>   
>       if (-e $keyring) {
> @@ -315,7 +342,11 @@ sub path {
>       my $cephconfig = "/etc/pve/priv/ceph/${storeid}.conf";
>   
>       if (-e $cephconfig) {
> -	$path .= ":conf=$cephconfig";
> +	if ($scfg->{pveceph}) {
> +	    warn "ignoring custom ceph config for storage '$storeid', 'pveceph' is set!\n";
> +	} else {
> +	    $path .= ":conf=$cephconfig";
> +	}
>       }
>   
>       return ($path, $vmid, $vtype);
> 


rest looks OK.