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 3BC6593496 for ; Mon, 5 Feb 2024 12:57:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1AF461608D for ; Mon, 5 Feb 2024 12:57:20 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (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 for ; Mon, 5 Feb 2024 12:57:19 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 18AF04431D for ; Mon, 5 Feb 2024 12:57:19 +0100 (CET) Message-ID: <7f34dbe6-c2c0-4604-b7ac-4590e227a025@proxmox.com> Date: Mon, 5 Feb 2024 12:57:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pve-devel@lists.proxmox.com References: <20240130184041.1125674-1-m.carrara@proxmox.com> <20240130184041.1125674-6-m.carrara@proxmox.com> <1706704585.2yd7g33cz4.astroid@yuna.none> From: Max Carrara In-Reply-To: <1706704585.2yd7g33cz4.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.031 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH pve-manager 5/8] fix #4759: ceph: configure keyring for ceph-crash.service 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: Mon, 05 Feb 2024 11:57:20 -0000 On 1/31/24 14:17, Fabian Grünbichler wrote: > On January 30, 2024 7:40 pm, Max Carrara wrote: >> when creating the cluster's first monitor. >> >> Signed-off-by: Max Carrara >> --- >> PVE/API2/Ceph/MON.pm | 28 +++++++++++++++++++++++++++- >> PVE/Ceph/Services.pm | 12 ++++++++++-- >> PVE/Ceph/Tools.pm | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 75 insertions(+), 3 deletions(-) >> >> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm >> index 1e959ef3..8d75f5d1 100644 >> --- a/PVE/API2/Ceph/MON.pm >> +++ b/PVE/API2/Ceph/MON.pm >> @@ -459,11 +459,37 @@ __PACKAGE__->register_method ({ >> }); >> die $@ if $@; >> # automatically create manager after the first monitor is created >> + # and set up keyring and config for ceph-crash.service >> if ($is_first_monitor) { >> PVE::API2::Ceph::MGR->createmgr({ >> node => $param->{node}, >> id => $param->{node} >> - }) >> + }); >> + >> + PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub { >> + my $cfg = cfs_read_file('ceph.conf'); >> + >> + if ($cfg->{'client.crash'}) { >> + return undef; >> + } >> + >> + $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring'; >> + >> + cfs_write_file('ceph.conf', $cfg); >> + }); >> + die $@ if $@; >> + >> + eval { >> + PVE::Ceph::Tools::get_or_create_crash_keyring(); >> + }; >> + warn "Unable to configure keyring for ceph-crash.service: $@" if $@; > > the order here should maybe be switched around? first handle the > keyring, then put it in the config? > >> + >> + print "enabling service 'ceph-crash.service'\n"; >> + PVE::Ceph::Services::ceph_service_cmd('enable', 'crash'); > > shouldn't this already be handled by default? > >> + print "starting service 'ceph-crash.service'\n"; >> + # ceph-crash already runs by default, >> + # this makes sure the keyring is used >> + PVE::Ceph::Services::ceph_service_cmd('restart', 'crash'); > > this should probably be a try-restart to avoid starting it if the admin > explicitly disabled and/or stopped it.. > > but - AFAICT, the ceph-crash script that is executed by the service > boils down to (as forked process!) "ceph -n XXX ..." where XXX is (in sequence) > client.crash.$HOST, client.crash, client.admin, so a service restart > shouldn't even be needed, since a fresh ceph (client) process will pick > up the config changes anyway? Good point - I've found the respective code and will just change the order there as well as drop the calls here to enable/restart `ceph-crash`. Thanks! > >> } >> }; >> >> diff --git a/PVE/Ceph/Services.pm b/PVE/Ceph/Services.pm >> index e0f31e8e..5f5986f9 100644 >> --- a/PVE/Ceph/Services.pm >> +++ b/PVE/Ceph/Services.pm > >> [..] > >> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm >> index 3acef11b..cf9f2ed4 100644 >> --- a/PVE/Ceph/Tools.pm >> +++ b/PVE/Ceph/Tools.pm > >> [..] > >> + my $output = $rados->mon_command({ >> + prefix => 'auth get-or-create', >> + entity => 'client.crash', >> + caps => [ >> + mon => 'profile crash', >> + mgr => 'profile crash', >> + ], >> + format => 'plain', >> + }); >> + >> + if (! -d $pve_ceph_cfgdir) { >> + mkdir $pve_ceph_cfgdir; >> + } >> + >> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output); >> + >> + return $pve_ceph_crash_key_path; >> +} >> + > > we have another helper for creating a keyring (and another inline call > to ceph-authtool when creating a monitor), should we unify them? In this case it's better not to, in my opinion - the function for `ceph-crash` specifically uses `ceph auth get-or-create` as that's quite a bit easier to use in this scenario, as the key will automatically be generated if it doesn't exist. This does require a connection to RADOS, but that will exist once the first mon is set up anyway. Otherwise we'd have to use `ceph-authtool` and then also import the key to cephx if it doesn't exist already, like we do in other places. Ultimately it ends up achieving the same, but the former just seemed more straightforward IMO. I did however notice that there are several `run_command` calls to `ceph-authtool` floating around that could maaaybe benefit from a helper function, but I would rather implement that in a different patch series, as that's not really relevant for this one. > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > >