* [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting
@ 2023-09-07 9:49 Maximiliano Sandoval
2023-09-07 9:49 ` [pve-devel] [PATCH manager 2/2] ceph: api: use snake_case when setting options Maximiliano Sandoval
2023-09-20 12:10 ` [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Dominik Csapak
0 siblings, 2 replies; 6+ messages in thread
From: Maximiliano Sandoval @ 2023-09-07 9:49 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH manager 2/2] ceph: api: use snake_case when setting options
2023-09-07 9:49 [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Maximiliano Sandoval
@ 2023-09-07 9:49 ` Maximiliano Sandoval
2023-09-20 12:10 ` [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Dominik Csapak
1 sibling, 0 replies; 6+ messages in thread
From: Maximiliano Sandoval @ 2023-09-07 9:49 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting
2023-09-07 9:49 [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Maximiliano Sandoval
2023-09-07 9:49 ` [pve-devel] [PATCH manager 2/2] ceph: api: use snake_case when setting options Maximiliano Sandoval
@ 2023-09-20 12:10 ` Dominik Csapak
2023-09-20 13:21 ` Thomas Lamprecht
1 sibling, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2023-09-20 12:10 UTC (permalink / raw)
To: Proxmox VE development discussion, Maximiliano Sandoval
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};
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting
2023-09-20 12:10 ` [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Dominik Csapak
@ 2023-09-20 13:21 ` Thomas Lamprecht
2023-09-20 13:42 ` Maximiliano Sandoval
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2023-09-20 13:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Maximiliano Sandoval
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)..
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting
2023-09-20 13:21 ` Thomas Lamprecht
@ 2023-09-20 13:42 ` Maximiliano Sandoval
2023-09-20 14:27 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Maximiliano Sandoval @ 2023-09-20 13:42 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion, Dominik Csapak
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting
2023-09-20 13:42 ` Maximiliano Sandoval
@ 2023-09-20 14:27 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2023-09-20 14:27 UTC (permalink / raw)
To: Maximiliano Sandoval; +Cc: Proxmox VE development discussion, Dominik Csapak
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-20 14:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-07 9:49 [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Maximiliano Sandoval
2023-09-07 9:49 ` [pve-devel] [PATCH manager 2/2] ceph: api: use snake_case when setting options Maximiliano Sandoval
2023-09-20 12:10 ` [pve-devel] [PATCH manager 1/2] ceph: remove global pg bits setting Dominik Csapak
2023-09-20 13:21 ` Thomas Lamprecht
2023-09-20 13:42 ` Maximiliano Sandoval
2023-09-20 14:27 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox