Re: [pve-devel] [PATCH v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

["Max Carrara" <m.carrara@proxmox.com>]

On Wed Mar 20, 2024 at 9:05 AM CET, Fabian Grünbichler wrote:
> > Max Carrara <m.carrara@proxmox.com> hat am 19.03.2024 18:41 CET geschrieben:
> > On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> > > On March 5, 2024 4:07 pm, Max Carrara wrote:
> > > > diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> > > > new file mode 100755
> > > > index 00000000..c8e9217d
> > > > --- /dev/null
> > > > +++ b/bin/pve-init-ceph-crash
> > > > @@ -0,0 +1,129 @@
> > > > +#!/usr/bin/perl
> > > > +
> > > > +use strict;
> > > > +use warnings;
> > > > +
> > > > +use List::Util qw(first);
> > > > +
> > > > +use PVE::Ceph::Tools;
> > > > +use PVE::Cluster;
> > > > +use PVE::RADOS;
> > > > +use PVE::RPCEnvironment;
> > > > +
> > > > +my $ceph_cfg_file = 'ceph.conf';
> > > > +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> > > > +
> > > > +my $entity = 'client.crash';
> > >
> > > nit: this could be inlined?
> > 
> > Inlined as in use 'client.crash' as the key directly? Eh, I'm more a fan
> > of using a variable here, as constantly spelling it out while changing a
> > bunch of things gets a little painful ...
> > 
> > If you meant that I should put it in `try_adapt_cfg` - sure, I missed
> > that that's the only `sub` in which it's being used, woops!
>
> both would be fine for me :)

ACK!

>
> > > > +
> > > > +
>
> [..]
>
> > > > +
> > > > +sub main {
> > > > +    my $rpcenv = PVE::RPCEnvironment->init('cli');
> > > > +
> > > > +    $rpcenv->init_request();
> > > > +    $rpcenv->set_language($ENV{LANG});
> > > > +    $rpcenv->set_user('root@pam');
> > >
> > > why do we need an rpcenv here?
> > 
> > Double-checked, just to be sure: `librados-perl` requires an
> > `RPCEnvironment` to do some handling regarding forks -
> > `RPCEnvironment::get()` will die if no env was initialized.
>
> good to know. maybe a comment makes sense - the rpcenv is not used by anything else here it seems, and the fact that librados uses it because it forks itself and wants to clean up in case it is called in an API server process is not obvious ;)
>
> you could replace those lines with a call to setup_default_cli_env that does all of the above and the "am I root" check in one ;)

Also ACK! Thanks again for your input!