From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 5F7E81FF185 for ; Mon, 21 Jul 2025 13:23:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 849C3B45B; Mon, 21 Jul 2025 13:25:00 +0200 (CEST) Mime-Version: 1.0 Date: Mon, 21 Jul 2025 13:24:56 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max R. Carrara" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20250716124716.104765-1-f.ebner@proxmox.com> In-Reply-To: <20250716124716.104765-1-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753097089519 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Wed Jul 16, 2025 at 2:46 PM CEST, Fiona Ebner wrote: > With the switch from QEMU's -drive to -blockdev, it is not possible > anymore to pass along the Ceph 'keyring' option via the QEMU > commandline anymore, as was previously done for externally managed RBD > storages. For such storages, it is now necessary that the 'keyring' > option is correctly configured in the storage's Ceph configuration. > > For newly created external RBD storages in Proxmox VE 9, the Ceph > configuration with the 'keyring' option is automatically added, as > well as for existing storages that do not have a Ceph configuration > at all yet. But storages that already got an existing Ceph > configuration are not automatically handled by the storage layer in > Proxmox VE 9, the check and script here covers those as well. > > Signed-off-by: Fiona Ebner > --- Code Review ----------- Patch doesn't apply 100% cleanly via `git am -3`, but the merge conflict in PVE::CLI::pve8to9 is trivial (--> choose all); I don't think a rebase is necessary here. The code is pretty straightforward and easy to follow; I don't have any criticisms in that regard. Testing ------- Tested this by adding one of my local Ceph clusters as external RBD storage to my development VM. The missing `keyring` option gets added to the storage-specific .conf file via the script; the file is created beforehand if it doesn't exist. When running pve8to9 on one of my local (hyper-converged) Ceph clusters, the check correctly detects that the pool is managed by PVE. When running the migration script, nothing is done (as expected). All the other cases I had tested also worked as expected: - Existing $storeid.conf file but no `[global]` section --> section + `keyring` option get added - Existing $storeid.conf file, no `[global]` section, other sections present --> section + `keyring` option get added - Existing $storeid.conf file with `[global]` section, but no `keyring` --> `keyring` option gets added - Nonexistent $storeid.keyring file --> ignored (as expected) - Path of `keyring` differs from location of $storeid.keyring --> warning emitted (as expected) Summary ------- LGTM & works as expected; nice work! Consider: Reviewed-by: Max R. Carrara Tested-by: Max R. Carrara > PVE/CLI/pve8to9.pm | 103 ++++++++++++++++++++++++++ > bin/Makefile | 3 +- > bin/pve-rbd-storage-configure-keyring | 23 ++++++ > 3 files changed, 128 insertions(+), 1 deletion(-) > create mode 100755 bin/pve-rbd-storage-configure-keyring > > diff --git a/PVE/CLI/pve8to9.pm b/PVE/CLI/pve8to9.pm > index 91b50cd9..af8d4acc 100644 > --- a/PVE/CLI/pve8to9.pm > +++ b/PVE/CLI/pve8to9.pm > @@ -268,6 +268,107 @@ sub check_pve_packages { > } > } > > +sub check_rbd_storage_keyring { > + my ($cfg, $dry_run) = @_; > + > + my $pve_managed = []; > + my $already_good = []; > + my $update = []; > + > + log_info("Checking whether all external RBD storages have the 'keyring' option configured"); > + > + for my $storeid (sort keys $cfg->{ids}->%*) { > + eval { > + my $scfg = PVE::Storage::storage_config($cfg, $storeid); > + > + return if $scfg->{type} ne 'rbd'; > + > + if (!defined($scfg->{monhost})) { > + push $pve_managed->@*, $storeid; > + return; > + } > + > + my $ceph_storage_keyring = "/etc/pve/priv/ceph/${storeid}.keyring"; > + my $ceph_storage_config = "/etc/pve/priv/ceph/${storeid}.conf"; > + > + my $ceph_config = {}; > + > + if (-e $ceph_storage_config) { > + my $content = PVE::Tools::file_get_contents($ceph_storage_config); > + $ceph_config = > + PVE::CephConfig::parse_ceph_config($ceph_storage_config, $content); > + > + if (my $keyring_path = $ceph_config->{global}->{keyring}) { > + if ($keyring_path eq $ceph_storage_keyring) { > + push $already_good->@*, $storeid; > + } else { > + log_warn( > + "storage $storeid: keyring option configured ($keyring_path), but" > + . " different from the expected value ($ceph_storage_keyring)," > + . " check manually!"); > + } > + > + return; > + } > + } > + > + if (!-e $ceph_storage_keyring) { > + log_info("skipping storage $storeid: keyring file $ceph_storage_keyring does" > + . " not exist"); > + return; > + } > + > + if ($dry_run) { > + push $update->@*, $storeid; > + return; > + } > + > + $ceph_config->{global}->{keyring} = $ceph_storage_keyring; > + > + my $contents = > + PVE::CephConfig::write_ceph_config($ceph_storage_config, $ceph_config); > + PVE::Tools::file_set_contents($ceph_storage_config, $contents, 0600); > + > + push $update->@*, $storeid; > + }; > + my $err = $@; > + if ($err) { > + log_fail("could not ensure that 'keyring' option is set for storage '$storeid': $err"); > + } > + } > + > + if (scalar($pve_managed->@*)) { > + my $storeid_txt = join(',', $pve_managed->@*); > + log_info("The following RBD storages are PVE-managed, so nothing to do: $storeid_txt"); > + } > + > + if (scalar($already_good->@*)) { > + my $storeid_txt = join(',', $already_good->@*); > + log_pass( > + "The following externally managed RBD storages already have the 'keyring' option" > + . " configured correctly: $storeid_txt"); > + } > + > + if (scalar($update->@*)) { > + my $storeid_txt = join(',', $update->@*); > + if ($dry_run) { > + log_notice( > + "Starting with PVE 9, externally managed RBD storages require that the 'keyring'" > + . " option is configured in the storage's Ceph configuration.\nYou can run the" > + . " following command to automatically set the option:\n\n" > + . "\t/usr/share/pve-manager/migrations/pve-rbd-storage-configure-keyring\n"); > + log_fail( > + "The Ceph configuration of the following externally managed RBD storages needs to" > + . " be updated: $storeid_txt"); > + > + } else { > + log_pass( > + "The Ceph configuration of the following externally managed RBD storages has" > + . " been updated: $storeid_txt"); > + } > + } > +} > + > sub check_storage_health { > print_header("CHECKING CONFIGURED STORAGES"); > my $cfg = PVE::Storage::config(); > @@ -292,6 +393,8 @@ sub check_storage_health { > check_storage_content(); > eval { check_storage_content_dirs() }; > log_fail("failed to check storage content directories - $@") if $@; > + > + check_rbd_storage_keyring($cfg, 1); > } > > sub check_cluster_corosync { > diff --git a/bin/Makefile b/bin/Makefile > index 51683596..e290ccbd 100644 > --- a/bin/Makefile > +++ b/bin/Makefile > @@ -31,7 +31,8 @@ HELPERS = \ > pve-init-ceph-crash > > MIGRATIONS = \ > - pve-lvm-disable-autoactivation > + pve-lvm-disable-autoactivation \ > + pve-rbd-storage-configure-keyring > > SERVICE_MANS = $(addsuffix .8, $(SERVICES)) > > diff --git a/bin/pve-rbd-storage-configure-keyring b/bin/pve-rbd-storage-configure-keyring > new file mode 100755 > index 00000000..1e9c61d3 > --- /dev/null > +++ b/bin/pve-rbd-storage-configure-keyring > @@ -0,0 +1,23 @@ > +#!/usr/bin/perl > + > +use strict; > +use warnings; > + > +use PVE::RPCEnvironment; > +use PVE::Storage; > + > +use PVE::CLI::pve8to9; > + > +sub main { > + PVE::RPCEnvironment->setup_default_cli_env(); > + > + my $cfg = PVE::Storage::config(); > + > + print "INFO: Starting with PVE 9, externally managed RBD storages require that the 'keyring'" > + . " option is configured in the storage's Ceph configuration. This script creates and" > + . " updates the storage's Ceph configurations.\n"; > + > + PVE::CLI::pve8to9::check_rbd_storage_keyring($cfg, 0); > +} > + > +main(); _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel