From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 39EBA6D7BD for ; Thu, 1 Apr 2021 16:48:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2DB4E1E0AA for ; Thu, 1 Apr 2021 16:48:12 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 648B41E09D for ; Thu, 1 Apr 2021 16:48:11 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 294AA42F71 for ; Thu, 1 Apr 2021 16:48:11 +0200 (CEST) Message-ID: <5f42111c-488d-a702-69a6-7deebec46b92@proxmox.com> Date: Thu, 1 Apr 2021 16:48:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:88.0) Gecko/20100101 Thunderbird/88.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20210401134057.26305-1-a.lauterer@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210401134057.26305-1-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.044 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rbdplugin.pm] Subject: Re: [pve-devel] [PATCH storage 1/2] rbd: fix #3286 add namespace support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Apr 2021 14:48:12 -0000 On 01.04.21 15:40, Aaron Lauterer wrote: > This patch introduces support for Cephs RBD namespaces. > > A new storage config parameter 'namespace' defines the namespace to be > used for the RBD storage. > > The namespace must already exist in the Ceph cluster as it is not > automatically created. > > The main intention is to use this for external Ceph clusters. With > namespaces, each PVE cluster can get its own namespace and will not > conflict with other PVE clusters. > > Signed-off-by: Aaron Lauterer > > remove just style changes > --- > Changes from RFC: > > add --namespace parameter centrally in sub $build_cmd. All commands > except one (rbd unmap) support it. To handle commands that don't support > it, a hash with them was introduced. > > In a few places paths (FS, Ceph hierarchy) are needed. These are the > places scattered throughout the plugin where the namespace is inserted > if it is configured. > > PVE/Storage/RBDPlugin.pm | 34 ++++++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm > index fab6d57..2d78f40 100644 > --- a/PVE/Storage/RBDPlugin.pm > +++ b/PVE/Storage/RBDPlugin.pm > @@ -27,7 +27,9 @@ my $add_pool_to_disk = sub { > > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > > - return "$pool/$disk"; > + my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : ""; can I have "0" as namespace? as then above check may break. Maybe do: my $namespace = '/' . ($scfg->{namespace} // ''); return "${pool}${namespace}${disk}"; > + > + return "${pool}${namespace}/${disk}"; > }; > > my $build_cmd = sub { > @@ -38,6 +40,14 @@ my $build_cmd = sub { > > my $cmd = [$binary, '-p', $pool]; > > + # some subcommands will fail if the --namespace parameter is present > + my $no_namespace_parameter = { > + unmap => 1, > + }; > + > + push @$cmd, '--namespace', $scfg->{namespace} > + if ($scfg->{namespace} && !$no_namespace_parameter->{$op}); > + > push @$cmd, '-c', $cmd_option->{ceph_conf} if ($cmd_option->{ceph_conf}); > push @$cmd, '-m', $cmd_option->{mon_host} if ($cmd_option->{mon_host}); > push @$cmd, '--auth_supported', $cmd_option->{auth_supported} if ($cmd_option->{auth_supported}); > @@ -154,6 +164,7 @@ sub rbd_ls { > > my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l', '--format', 'json'); > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > + $pool .= "/$scfg->{namespace}" if $scfg->{namespace}; > > my $raw = ''; > my $parser = sub { $raw .= shift }; > @@ -281,6 +292,10 @@ sub properties { > description => "Pool.", > type => 'string', > }, > + namespace=> { > + description => "RBD Namespace.", > + type => 'string', > + }, > username => { > description => "RBD Id.", > type => 'string', > @@ -302,6 +317,7 @@ sub options { > disable => { optional => 1 }, > monhost => { optional => 1}, > pool => { optional => 1 }, > + namespace => { optional => 1 }, > username => { optional => 1 }, > content => { optional => 1 }, > krbd => { optional => 1 }, > @@ -349,9 +365,10 @@ sub path { > $name .= '@'.$snapname if $snapname; > > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > - return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd}; > + my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : ""; same as above, a for perl falsy $ns value "breaks" this. maybe we could use a `get_pool_ns_path($scfg)` (possible better name possible, should be relatively short and not too misguiding) sub returning both of above? Or actually: maybe even a `get_rbd_path($scfg, $image)` returning "${pool}${namespace}${image}" (with $ns omitted/added depending on config) There's seems to be more use for that one when looking at the changes made here. > + return ("/dev/rbd/${pool}${namespace}/${name}", $vmid, $vtype) if $scfg->{krbd}; > > - my $path = "rbd:$pool/$name"; > + my $path = "rbd:${pool}${namespace}/${name}"; > > $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf}; > if (defined($scfg->{monhost})) { > @@ -370,6 +387,7 @@ sub find_free_diskname { > my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_; > > my $cmd = &$rbd_cmd($scfg, $storeid, 'ls'); > + unrelated new line added? (albeit this one may improve code readability, so fine for me) > my $disk_list = []; > > my $parser = sub { > @@ -487,6 +505,7 @@ sub free_image { > my ($vtype, $name, $vmid, undef, undef, undef) = > $class->parse_volname($volname); > > + bogus new lien added? > my $snaps = rbd_ls_snap($scfg, $storeid, $name); > foreach my $snap (keys %$snaps) { > if ($snaps->{$snap}->{protected}) { > @@ -511,6 +530,7 @@ sub list_images { > > $cache->{rbd} = rbd_ls($scfg, $storeid) if !$cache->{rbd}; > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > + $pool .= "/$scfg->{namespace}" if $scfg->{namespace}; > > my $res = []; > > @@ -575,7 +595,9 @@ sub deactivate_storage { > } > > my $get_kernel_device_name = sub { > - my ($pool, $name) = @_; > + my ($pool, $name, $namespace) = @_; > + > + return "/dev/rbd/${pool}/${namespace}/${name}" if $namespace; > > return "/dev/rbd/$pool/$name"; > }; > @@ -590,7 +612,7 @@ sub map_volume { > > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > > - my $kerneldev = $get_kernel_device_name->($pool, $name); > + my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace}); > > return $kerneldev if -b $kerneldev; # already mapped > > @@ -611,7 +633,7 @@ sub unmap_volume { > > my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd'; > > - my $kerneldev = $get_kernel_device_name->($pool, $name); > + my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace}); > > if (-b $kerneldev) { > my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev); >