[pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[Maximiliano Sandoval]

This setting was removed in [1] as part of the v13.0.2 tag.

[1] https://github.com/ceph/ceph/commit/e6acf2d1d528a2395947d446a57bec04a3a002dc

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

I did a grep search across multiple projects and I was not able to find
more uses of this option. A second pair of eyes might help here.

 PVE/API2/Ceph.pm | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 7e0763cf..a4101dee 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -158,16 +158,6 @@ __PACKAGE__->register_method ({
 		minimum => 1,
 		maximum => 7,
 	    },
-	    pg_bits => {
-		description => "Placement group bits, used to specify the " .
-		    "default number of placement groups.\n\nNOTE: 'osd pool " .
-		    "default pg num' does not work for default pools.",
-		type => 'integer',
-		default => 6,
-		optional => 1,
-		minimum => 6,
-		maximum => 14,
-	    },
 	    disable_cephx => {
 		description => "Disable cephx authentication.\n\n" .
 		    "WARNING: cephx is a security feature protecting against " .
@@ -224,11 +214,6 @@ __PACKAGE__->register_method ({
 		$cfg->{client}->{keyring} = '/etc/pve/priv/$cluster.$name.keyring';
 	    }
 
-	    if ($param->{pg_bits}) {
-		$cfg->{global}->{'osd pg bits'} = $param->{pg_bits};
-		$cfg->{global}->{'osd pgp bits'} = $param->{pg_bits};
-	    }
-
 	    if ($param->{network}) {
 		$cfg->{global}->{'public network'} = $param->{network};
 		$cfg->{global}->{'cluster network'} = $param->{network};
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] ceph: api: use snake_case when setting options

[Maximiliano Sandoval]

Continuation of ab70343982f36a5343d3fcf4a1a6489bd3f52a66. Discussion at
https://lists.proxmox.com/pipermail/pve-devel/2023-September/059013.html.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 PVE/API2/Ceph.pm | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index a4101dee..afc9b9e2 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -197,12 +197,12 @@ __PACKAGE__->register_method ({
 
 		$cfg->{global} = {
 		    'fsid' => $fsid,
-		    'auth cluster required' => $auth,
-		    'auth service required' => $auth,
-		    'auth client required' => $auth,
-		    'osd pool default size' => $param->{size} // 3,
-		    'osd pool default min size' => $param->{min_size} // 2,
-		    'mon allow pool delete' => 'true',
+		    'auth_cluster_required' => $auth,
+		    'auth_service_required' => $auth,
+		    'auth_client_required' => $auth,
+		    'osd_pool_default_size' => $param->{size} // 3,
+		    'osd_pool_default_min_size' => $param->{min_size} // 2,
+		    'mon_allow_pool_delete' => 'true',
 		};
 
 		# this does not work for default pools
@@ -215,12 +215,12 @@ __PACKAGE__->register_method ({
 	    }
 
 	    if ($param->{network}) {
-		$cfg->{global}->{'public network'} = $param->{network};
-		$cfg->{global}->{'cluster network'} = $param->{network};
+		$cfg->{global}->{'public_network'} = $param->{network};
+		$cfg->{global}->{'cluster_network'} = $param->{network};
 	    }
 
 	    if ($param->{'cluster-network'}) {
-		$cfg->{global}->{'cluster network'} = $param->{'cluster-network'};
+		$cfg->{global}->{'cluster_network'} = $param->{'cluster-network'};
 	    }
 
 	    cfs_write_file('ceph.conf', $cfg);
-- 
2.39.2

Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[Dominik Csapak]

sadly removing these parameters would be a breaking change

i'd rather mark it deprecatedand non-functional in the description
and remove the functionality

then on the next major release, we can announce the breaking change and
remove the parameter

On 9/7/23 11:49, Maximiliano Sandoval wrote:
> This setting was removed in [1] as part of the v13.0.2 tag.
> 
> [1] https://github.com/ceph/ceph/commit/e6acf2d1d528a2395947d446a57bec04a3a002dc
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> I did a grep search across multiple projects and I was not able to find
> more uses of this option. A second pair of eyes might help here.
> 
>   PVE/API2/Ceph.pm | 15 ---------------
>   1 file changed, 15 deletions(-)
> 
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 7e0763cf..a4101dee 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -158,16 +158,6 @@ __PACKAGE__->register_method ({
>   		minimum => 1,
>   		maximum => 7,
>   	    },
> -	    pg_bits => {
> -		description => "Placement group bits, used to specify the " .
> -		    "default number of placement groups.\n\nNOTE: 'osd pool " .
> -		    "default pg num' does not work for default pools.",
> -		type => 'integer',
> -		default => 6,
> -		optional => 1,
> -		minimum => 6,
> -		maximum => 14,
> -	    },
>   	    disable_cephx => {
>   		description => "Disable cephx authentication.\n\n" .
>   		    "WARNING: cephx is a security feature protecting against " .
> @@ -224,11 +214,6 @@ __PACKAGE__->register_method ({
>   		$cfg->{client}->{keyring} = '/etc/pve/priv/$cluster.$name.keyring';
>   	    }
>   
> -	    if ($param->{pg_bits}) {
> -		$cfg->{global}->{'osd pg bits'} = $param->{pg_bits};
> -		$cfg->{global}->{'osd pgp bits'} = $param->{pg_bits};
> -	    }
> -
>   	    if ($param->{network}) {
>   		$cfg->{global}->{'public network'} = $param->{network};
>   		$cfg->{global}->{'cluster network'} = $param->{network};

Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[Thomas Lamprecht]

Am 20/09/2023 um 14:10 schrieb Dominik Csapak:
> sadly removing these parameters would be a breaking change
> 

@maximiliano: how does ceph react if one sets this currently?
I.e., does it silently accepts unknown config keys, or does it break
something?

Because if it's the latter we can assume that nobody uses this
anyway and simply remove it in any release.

Otherwise, yes, dropping the usage but keeping the API parameter
with reducing the description to something like:
"depreacted, will be removed with Proxmox VE 9" would be slightly
better, not that I think it will make a difference in practice
(this is very niche stuff)..

Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> @maximiliano: how does ceph react if one sets this currently?
> I.e., does it silently accepts unknown config keys, or does it break
> something?

Running

    ceph config set global osd_pg_bits 42

results in

    Error EINVAL: unrecognized config option 'osd_pg_bits'

Having them set on /etc/pve/ceph.conf on the other hand does not throw
any kind of warning as far as I can tell, but I suspect it does not do
anything either.

Please tell if I am missing something or if I should do a new version of
the patch.

--
Maximiliano

Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting

[Thomas Lamprecht]

Am 20/09/2023 um 15:42 schrieb Maximiliano Sandoval:
> Thomas Lamprecht <t.lamprecht@proxmox.com> writes:
> 
>> @maximiliano: how does ceph react if oe sets this currently?
>> I.e., does it silently accepts unknown config keys, or does it break
>> something?
> 
> Running
> 
>     ceph config set global osd_pg_bits 42
> 
> results in
> 
>     Error EINVAL: unrecognized config option 'osd_pg_bits'
> 
> Having them set on /etc/pve/ceph.conf on the other hand does not throw
> any kind of warning as far as I can tell, but I suspect it does not do
> anything either.

Meh, as we do the latter it means that cephs will eat that value and
just ignore it, so this is a question about how strict we want to keep
the API "unbroken".

Dominik is def. right that just removing the parameter from our API
would be a breaking change in the technically correct sense.

But, AFAICT, this was never exposed via the pveceph CLI command nor the
web UI, so it's not that far-fetched to assume that it has close to zero
use in the wild, but we surely cannot be certain about that (some user
might have added it to their automatic provision script and us remove it
the param now would suddenly cause that to fail after the upgrade).

> Please tell if I am missing something or if I should do a new version of
> the patch.

While I'm not a favor of keeping things around that (Very Highly
Probably™) nobody will miss anyway, here just keeping the parameter is
simply way too cheap to not do that. So, let's not go for my normal
route of ripping it out and see if somebody complains once it hits
no-subscription, to then react accordingly.

In summary, yes, please send a new version of this patch that keeps a
(reduced) "pg_bits" API parameter, drops setting the value (as that is a
NO-OP anyway) and maybe even adds a warning like:
"the 'pg_bits' is deprecated and will be removed in the future"