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 <f.eb...@proxmox.com>
> ---
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 <m.carr...@proxmox.com>
Tested-by: Max R. Carrara <m.carr...@proxmox.com>
> 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
