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 4B75390DAA for ; Tue, 13 Feb 2024 10:10:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 252B63308E for ; Tue, 13 Feb 2024 10:09:33 +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 ; Tue, 13 Feb 2024 10:09:29 +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 5CCE647B7F for ; Tue, 13 Feb 2024 10:09:29 +0100 (CET) Message-ID: <6810f952-fd8f-4828-ac0f-d5534c6aa200@proxmox.com> Date: Tue, 13 Feb 2024 10:09:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pve-devel@lists.proxmox.com References: <20240205175419.1271680-1-m.carrara@proxmox.com> <20240205175419.1271680-10-m.carrara@proxmox.com> <1707743502.yuxc2lm198.astroid@yuna.none> From: Max Carrara In-Reply-To: <1707743502.yuxc2lm198.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.418 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [tools.pm, proxmox.com, mon.pm] Subject: Re: [pve-devel] [PATCH v2 pve-manager 09/11] 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: Tue, 13 Feb 2024 09:10:03 -0000 On 2/12/24 14:34, Fabian Grünbichler wrote: > On February 5, 2024 6:54 pm, Max Carrara wrote: >> when creating the cluster's first monitor. >> >> Signed-off-by: Max Carrara >> --- >> Changes v1 --> v2: >> * do not enable/restart `ceph-crash` anymore when creating first mon >> * drop changes to function `ceph_service_cmd` as they are no longer >> needed >> * create keyring for `ceph-crash` before modifying 'ceph.conf' >> * always set keyring for 'client.crash' section instead of only >> if section doesn't exist already >> * only modify the keyring file in `get_or_create_crash_keyring()` >> if the content differs from the output of `ceph auth get-or-create` > > you kinda ignored my comment about this adding yet another create keyring helper ;) Saw your other reply - will disregard this as noted ;) > >> PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++- >> PVE/Ceph/Tools.pm | 39 +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 55 insertions(+), 1 deletion(-) >> >> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm >> index 1e959ef3..ae12a2d3 100644 >> --- a/PVE/API2/Ceph/MON.pm >> +++ b/PVE/API2/Ceph/MON.pm >> @@ -459,11 +459,26 @@ __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} >> - }) >> + }); >> + >> + eval { >> + PVE::Ceph::Tools::get_or_create_crash_keyring(); > > this is called get_or_create, but it actually returns the path to, not > the key(ring).. nothing uses the return value anyway, so it could also > be called differently I guess and not return anything, but just from the > name, I'd expect the key to be returned. I agree; I initially wanted this helper to be similar to `get_or_create_ceph_admin_keyring`, but in this case I'll just unify the helper(s) once and for all in v3. No need to beat around the bush if I'm gonna unify them anyway, so might as well do it sooner than later ;) > >> + }; >> + warn "Unable to configure keyring for ceph-crash.service: $@" if $@; >> + >> + PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub { >> + my $cfg = cfs_read_file('ceph.conf'); >> + >> + $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring'; > > I guess this doesn't make anything worse (since we starting from > "ceph-crash is totally broken"), but if querying or creating the keyring > failed, does it make sense to reference it in the config? I guess in this case it doesn't, but we would need some kind of way to re-init this part of the configuration in case the 'keyring' key doesn't get set here. Originally I planned to do this in a separate series to keep the scope of this series focused on bug #4759, so perhaps it's best (at least for the time being) to just not set the 'keyring' at all at the moment if querying or creating the crash key fails. So, I would change it to that in v3 and then later on supply a separate series for the 're-init' part, if that's alright. > >> + >> + cfs_write_file('ceph.conf', $cfg); >> + }); >> + die $@ if $@; > > we could move this whole part up to where we do the monitor changes, and > only lock and read and write the config once.. Good point, will be done in v3. Thanks again for the thorough reviews! :) > >> } >> }; >> >> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm >> index 273a3eb6..02a932e3 100644 >> --- a/PVE/Ceph/Tools.pm >> +++ b/PVE/Ceph/Tools.pm >> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name >> my $ceph_cfgdir = "/etc/ceph"; >> my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf"; >> my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf"; >> +my $pve_ceph_cfgdir = "/etc/pve/ceph"; >> >> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring"; >> my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring"; >> my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring"; >> my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring"; >> @@ -37,12 +39,14 @@ my $ceph_service = { >> >> my $config_values = { >> ccname => $ccname, >> + pve_ceph_cfgdir => $pve_ceph_cfgdir, >> ceph_mds_data_dir => $ceph_mds_data_dir, >> long_rados_timeout => 60, >> }; >> >> my $config_files = { >> pve_ceph_cfgpath => $pve_ceph_cfgpath, >> + pve_ceph_crash_key_path => $pve_ceph_crash_key_path, >> pve_mon_key_path => $pve_mon_key_path, >> pve_ckeyring_path => $pve_ckeyring_path, >> ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring, >> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring { >> return $pve_ckeyring_path; >> } >> >> +# requires connection to existing monitor >> +sub get_or_create_crash_keyring { >> + my ($rados) = @_; >> + >> + if (!defined($rados)) { >> + $rados = PVE::RADOS->new(); >> + } >> + >> + 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) { >> + File::Path::make_path($pve_ceph_cfgdir); >> + } >> + >> + if (-f $pve_ceph_crash_key_path) { >> + my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path); >> + >> + if ($contents ne $output) { >> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output); >> + } >> + } else { >> + PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output); >> + } >> + >> + return $pve_ceph_crash_key_path; >> +} >> + >> # get ceph-volume managed osds >> sub ceph_volume_list { >> my $result = {}; >> -- >> 2.39.2 >> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > >