Re: [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set

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

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.ebner@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.carrara@proxmox.com>
Tested-by: Max R. Carrara <m.carrara@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