* [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set
@ 2025-07-16 12:46 Fiona Ebner
2025-07-21 11:24 ` Max R. Carrara
2025-07-29 12:57 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 2 replies; 3+ messages in thread
From: Fiona Ebner @ 2025-07-16 12:46 UTC (permalink / raw)
To: pve-devel
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>
---
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();
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set
2025-07-16 12:46 [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set Fiona Ebner
@ 2025-07-21 11:24 ` Max R. Carrara
2025-07-29 12:57 ` [pve-devel] applied: " Thomas Lamprecht
1 sibling, 0 replies; 3+ messages in thread
From: Max R. Carrara @ 2025-07-21 11:24 UTC (permalink / raw)
To: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pve-devel] applied: [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set
2025-07-16 12:46 [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set Fiona Ebner
2025-07-21 11:24 ` Max R. Carrara
@ 2025-07-29 12:57 ` Thomas Lamprecht
1 sibling, 0 replies; 3+ messages in thread
From: Thomas Lamprecht @ 2025-07-29 12:57 UTC (permalink / raw)
To: pve-devel, Fiona Ebner
On Wed, 16 Jul 2025 14:46:59 +0200, 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.
>
> [...]
Applied, with a bunch of smaller follow-ups but only for some opinionated
polishing, thanks!
[1/1] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set
commit: 7b2ac8d295d12bb332673fa0e233da61f7ec4fdb
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-07-29 12:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-16 12:46 [pve-devel] [PATCH manager stable-8+master] pve8to9: add check and script to ensure that 'keyring' option for external RBD storages is set Fiona Ebner
2025-07-21 11:24 ` Max R. Carrara
2025-07-29 12:57 ` [pve-devel] applied: " Thomas Lamprecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.