[pve-devel] [v2 manager 06/27] ceph: add add_storage helper

[Thomas Lamprecht]

On 08/29/2017 01:04 PM, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> changes since v1:
> - drop $monhash parameter
> - don't generate and set monhost storage parameter
> 
>   PVE/API2/Ceph.pm | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 7f709f55..04a19524 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -12,6 +12,7 @@ use PVE::INotify;
>   use PVE::Cluster qw(cfs_lock_file cfs_read_file cfs_write_file);
>   use PVE::AccessControl;
>   use PVE::Storage;
> +use PVE::API2::Storage::Config;
>   use PVE::RESTHandler;
>   use PVE::RPCEnvironment;
>   use PVE::JSONSchema qw(get_standard_option);
> @@ -699,6 +700,43 @@ __PACKAGE__->register_method ({
>   
>       }});
>   
> +my $add_storage = sub {
> +    my ($pool, $storeid, $krbd) = @_;
> +
> +    my $pve_ceph_keydir = PVE::CephTools::get_config('pve_ceph_keydir');
> +    mkdir $pve_ceph_keydir;
> +
> +    my $pve_ckeyring_path = PVE::CephTools::get_config('pve_ckeyring_path');
> +    my $pve_ceph_keyring_path = "$pve_ceph_keydir/$storeid.keyring";

I'd rename this variable, its a bit confusing in combination of the $pve_ckeyring_path
variable. $ceph_storage_keyring_fn (or ending with _path) could be more fitting?
"pve_ceph_keyring" just sounds like a general keyring for ceph but then a $storeid
specific path gets assigned.
Also the unlink below seems then a bit harsh with that in mind, one could think
"why unlink the whole ceph key for pve here?" at the first glance.
Its clear what happens after looking but I had to look quite closely.

> +
> +    die "ceph authx keyring file for storage '$storeid' already exists!\n"
> +	if -e $pve_ceph_keyring_path;
> +
> +    eval {
> +	my $ckeyring_data = PVE::Tools::file_get_contents($pve_ckeyring_path);
> +	PVE::Tools::file_set_contents($pve_ceph_keyring_path, $ckeyring_data);

We have PVE::Tools::file_copy which does exactly that.

> +    };
> +    if (my $err = $@) {
> +	unlink $pve_ceph_keyring_path;
> +	die "failed to copy ceph authx keyring for storage '$storeid': $err\n";
> +    }
> +
> +    my $storage_params = {
> +	type => 'rbd',
> +	pool => $pool,
> +	storage => $storeid,
> +	krbd => $krbd // 0,
> +	content => $krbd ? 'rootdir' : 'images',
> +	pveceph => 1,
> +    };
> +
> +    eval { PVE::API2::Storage::Config->create($storage_params); };
> +    if (my $err = $@) {
> +	unlink $pve_ceph_keyring_path;
> +	die "failed adding storage to storage.cfg: $err\n";
> +    }
> +};
> +
>   __PACKAGE__->register_method ({
>       name => 'listmon',
>       path => 'mon',
> 

looks OK, besides the nitpicks above.