[pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

This marks version 03 of the patch series "Fix #4759: Configure
Permissions for ceph-crash.service". Older versions can be found below.

Notable changes since v2
------------------------

  * The 'ceph.conf' parser in pve-storage is now equivalent to Ceph's
    and even supports continued lines
  * The addition of the '/etc/pve/ceph' directory has been moved into a
    separate patch in order to preserve the context of its purpose in
    the git history
  * The debian `postinst` hook for pve-manager is now version-guarded
    and uses a separate Perl helper script instead of doing everything
    in BASH


Older Versions
--------------

v1: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061646.html



ceph (master):

Max Carrara (2):
  debian: add patch to fix ceph crash dir permissions in postinst hook
  patches: add patch that reorders clients used by ceph-crash

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 86 insertions(+)
 create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
 create mode 100644 patches/0017-ceph-crash-change-order-of-client-names.patch


ceph (quincy-stable-8):

Max Carrara (2):
  debian: add patch to fix ceph crash dir permissions in postinst hook
  patches: add patch that reorders clients used by ceph-crash

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
 patches/series                                |  2 +
 3 files changed, 86 insertions(+)
 create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
 create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch


pve-storage:

Max Carrara (6):
  cephconfig: align our parser more with Ceph's parser
  cephconfig: support line-continuations in parser
  cephconfig: allow writing arbitrary sections
  cephconfig: change code style inside config writer
  cephconfig: change order of written sections
  cephconfig: remove leading whitespace on write to Ceph config

 src/PVE/CephConfig.pm | 80 +++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 22 deletions(-)


pve-manager:

Max Carrara (3):
  ceph: introduce '/etc/pve/ceph'
  fix #4759: ceph: configure ceph-crash.service and its key
  bin/make: gather helper scripts in separate variable

 PVE/API2/Ceph.pm        |   5 ++
 PVE/API2/Ceph/MON.pm    |   8 ++++
 PVE/Ceph/Tools.pm       |  47 +++++++++++++++++-
 bin/Makefile            |   6 ++-
 bin/pve-init-ceph-crash | 104 ++++++++++++++++++++++++++++++++++++++++
 debian/postinst         |  16 +++++++
 6 files changed, 183 insertions(+), 3 deletions(-)
 create mode 100755 bin/pve-init-ceph-crash

-- 
2.39.2

[pve-devel] [PATCH v3 master ceph 01/13] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara]

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on the contents of
'/var/lib/ceph/crash'.

This patch therefore also recursively updates the permissions of
'/var/lib/ceph/crash'.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop
Changes v2 --> v3:
  * rebased on master
  * `chown` all kinds of entries, not just files and directories
    (as discussed off-list) 
  * instead of `chown`-ing '/var/lib/ceph/**/*', recusively call `chown`
    on the contents of `/var/lib/ceph/crash` (as discussed off-list)

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch

diff --git a/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch b/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
new file mode 100644
index 000000000..36f4df3aa
--- /dev/null
+++ b/patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
@@ -0,0 +1,54 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: recursively adjust permissions of /var/lib/ceph/crash
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by recursively updating the permissions of
+'/var/lib/ceph/crash', which ensures that all files and directories
+used by 'ceph-crash.service' are actually owned by the user configured
+for Ceph.
+
+The previously existing loop has also been replaced by an invocation
+of `find | xargs`.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..424c2c889d5 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ 	rm -f /etc/init/ceph.conf
+ 	[ -x /sbin/start ] && start ceph-all || :
+ 
+-        # adjust file and directory permissions
+-	for DIR in /var/lib/ceph/* ; do
+-	    if ! dpkg-statoverride --list $DIR >/dev/null
+-	    then
+-		chown $SERVER_USER:$SERVER_GROUP $DIR
+-	    fi
+-	done
++	PERM_COMMAND="dpkg-statoverride --list '{}' > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} '{}'"
++
++	# adjust file and directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 1 -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust permissions so ceph-crash.service can post reports
++	find /var/lib/ceph/crash -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 6ad754713..83a168ec9 100644
--- a/patches/series
+++ b/patches/series
@@ -13,3 +13,4 @@
 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
 0015-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
+0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
-- 
2.39.2

[pve-devel] [PATCH v3 master ceph 02/13] patches: add patch that reorders clients used by ceph-crash

[Max Carrara]

This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on master

Note: I preseved Fabian's 'Reviewed-by' trailer as no changes have been
made to the patch this commit adds. I hope that's okay.

 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 patches/0017-ceph-crash-change-order-of-client-names.patch

diff --git a/patches/0017-ceph-crash-change-order-of-client-names.patch b/patches/0017-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..8131fced5
--- /dev/null
+++ b/patches/0017-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 0e02837fadd..713080a4dc1 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+ 
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+-              'client.crash',
++auth_names = ['client.crash',
++              'client.crash.%s' % socket.gethostname(),
+               'client.admin']
+ 
+ 
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 83a168ec9..9bde2a241 100644
--- a/patches/series
+++ b/patches/series
@@ -14,3 +14,4 @@
 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
 0015-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
 0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
+0017-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v3 quincy-stable-8 ceph 03/13] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara]

Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
to ceph:ceph (in our case), but misses out on the contents of
'/var/lib/ceph/crash'.

This patch therefore also recursively updates the permissions of
'/var/lib/ceph/crash'.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * use `find` instead of for-loop
Changes v2 --> v3:
  * rebased on quincy-stable-8
  * `chown` all kinds of entries, not just files and directories
    (as discussed off-list) 
  * instead of `chown`-ing '/var/lib/ceph/**/*', recusively call `chown`
    on the contents of `/var/lib/ceph/crash` (as discussed off-list)

 ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch

diff --git a/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch b/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
new file mode 100644
index 000000000..36f4df3aa
--- /dev/null
+++ b/patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
@@ -0,0 +1,54 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Thu, 1 Feb 2024 18:43:36 +0100
+Subject: [PATCH] debian: recursively adjust permissions of /var/lib/ceph/crash
+
+A rather recent PR made ceph-crash run as "ceph" user instead of
+root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
+ceph-crash cannot actually post any crash logs now.
+
+This commit fixes this by recursively updating the permissions of
+'/var/lib/ceph/crash', which ensures that all files and directories
+used by 'ceph-crash.service' are actually owned by the user configured
+for Ceph.
+
+The previously existing loop has also been replaced by an invocation
+of `find | xargs`.
+
+[0]: https://github.com/ceph/ceph/pull/48713
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ debian/ceph-base.postinst | 16 +++++++++-------
+ 1 file changed, 9 insertions(+), 7 deletions(-)
+
+diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
+index 75eeb59c624..424c2c889d5 100644
+--- a/debian/ceph-base.postinst
++++ b/debian/ceph-base.postinst
+@@ -33,13 +33,15 @@ case "$1" in
+ 	rm -f /etc/init/ceph.conf
+ 	[ -x /sbin/start ] && start ceph-all || :
+ 
+-        # adjust file and directory permissions
+-	for DIR in /var/lib/ceph/* ; do
+-	    if ! dpkg-statoverride --list $DIR >/dev/null
+-	    then
+-		chown $SERVER_USER:$SERVER_GROUP $DIR
+-	    fi
+-	done
++	PERM_COMMAND="dpkg-statoverride --list '{}' > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} '{}'"
++
++	# adjust file and directory permissions
++	find /var/lib/ceph -mindepth 1 -maxdepth 1 -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
++
++	# adjust permissions so ceph-crash.service can post reports
++	find /var/lib/ceph/crash -print0 \
++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
+     ;;
+     abort-upgrade|abort-remove|abort-deconfigure)
+ 	:
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 30fc83ec0..132bfa739 100644
--- a/patches/series
+++ b/patches/series
@@ -17,3 +17,4 @@
 0022-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
 0024-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
+0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
-- 
2.39.2

[pve-devel] [PATCH v3 quincy-stable-8 ceph 04/13] patches: add patch that reorders clients used by ceph-crash

[Max Carrara]

This patch makes it so that `ceph-crash` attempts to use the
non-host-specific keyring before anything else, which avoids
unnecessary error messages landing in the systemd-journal in our case.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on quincy-stable-8

Note: I preseved Fabian's 'Reviewed-by' trailer as no changes have been
made to the patch this commit adds. I hope that's okay.

 ...h-crash-change-order-of-client-names.patch | 30 +++++++++++++++++++
 patches/series                                |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch

diff --git a/patches/0026-ceph-crash-change-order-of-client-names.patch b/patches/0026-ceph-crash-change-order-of-client-names.patch
new file mode 100644
index 000000000..8131fced5
--- /dev/null
+++ b/patches/0026-ceph-crash-change-order-of-client-names.patch
@@ -0,0 +1,30 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Max Carrara <m.carrara@proxmox.com>
+Date: Mon, 5 Feb 2024 11:44:14 +0100
+Subject: [PATCH] ceph-crash: change order of client names
+
+This simply puts 'client.crash' before 'client.crash.${HOSTNAME}'.
+
+Signed-off-by: Max Carrara <m.carrara@proxmox.com>
+---
+ src/ceph-crash.in | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/ceph-crash.in b/src/ceph-crash.in
+index 0e02837fadd..713080a4dc1 100755
+--- a/src/ceph-crash.in
++++ b/src/ceph-crash.in
+@@ -16,8 +16,8 @@ import time
+ logging.basicConfig(level=logging.INFO)
+ log = logging.getLogger('ceph-crash')
+ 
+-auth_names = ['client.crash.%s' % socket.gethostname(),
+-              'client.crash',
++auth_names = ['client.crash',
++              'client.crash.%s' % socket.gethostname(),
+               'client.admin']
+ 
+ 
+-- 
+2.39.2
+
diff --git a/patches/series b/patches/series
index 132bfa739..406d4624a 100644
--- a/patches/series
+++ b/patches/series
@@ -18,3 +18,4 @@
 0023-rocksb-inherit-parent-cmake-cxx-flags.patch
 0024-ceph-osd-postinst-avoid-reloading-all-sysctl-setting.patch
 0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
+0026-ceph-crash-change-order-of-client-names.patch
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 05/13] cephconfig: align our parser more with Ceph's parser

[Max Carrara]

 1. Comments, irrespective of whether they start with '#' or ';' are
    now treated the same. Otherwise, sections and key-value pairs with
    a trailing comment starting with ';' are still parsed. Consider
    this example:

      [some.section] # inline comment after section
      foo = bar ; inline comment after value

     The '[some.section]' section in the example above would otherwise
     not be parsed at all, while in the key-value definition 'foo'
     parses as the key, which is correct, but 'bar ; inline comment
     after value' parses as value, which is incorrect according to
     Ceph's grammar [0][1].

 2. Sections may now contain any character, including whitespace, but
    not '\n' or a comment literal '#' or ';'. The case for comment
    literals is handled in 1. above.

 3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
    are treated the same, like in Ceph's parser [2].

 4. Comment literals ('#', ';') now are preserved when escaped via a
    backslash '\' [3].

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
[2]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
[3]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l179

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * support comment literals (4.)

 src/PVE/CephConfig.pm | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..bbb9d39 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
 		  \&parse_ceph_config,
 		  \&write_ceph_config);
 
+# For more details on how Ceph's config parser works, see:
+# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
 sub parse_ceph_config {
     my ($filename, $raw) = @_;
 
@@ -20,14 +22,13 @@ sub parse_ceph_config {
 
     my $section;
 
-    foreach my $line (@lines) {
-	$line =~ s/#.*$//;
+    for my $line (@lines) {
+	$line =~ s/(?<!\\)(#|;).*$//;
 	$line =~ s/^\s+//;
-	$line =~ s/^;.*$//;
 	$line =~ s/\s+$//;
 	next if !$line;
 
-	$section = $1 if $line =~ m/^\[(\S+)\]$/;
+	$section = $1 if $line =~ m/^\[(.+)\]$/;
 	if (!$section) {
 	    warn "no section - skip: $line\n";
 	    next;
@@ -35,11 +36,10 @@ sub parse_ceph_config {
 
 	if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
 	    my ($key, $val) = ($1, $2);
-	    # ceph treats ' ', '_' and '-' in keys the same, so lets do too
-	    $key =~ s/[-\ ]/_/g;
+	    # ceph treats ' ' and '_' in keys the same, so lets do too
+	    $key =~ s/ /_/g;
 	    $cfg->{$section}->{$key} = $val;
 	}
-
     }
 
     return $cfg;
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 06/13] cephconfig: support line-continuations in parser

[Max Carrara]

Ceph's docs state the following [0]:
> The backslash character `\` is used as the line-continuation marker
> that combines the next line with the current one.

This commit implements the support of such line-continuations in our
parser.

The line following a line ending with a '\' has its whitespace
preserved, which matches the behaviour in Ceph's original
implementation [1]. In other words, leading and trailing whitespace is
not stripped from a continued line.

[0]: https://docs.ceph.com/en/reef/rados/configuration/ceph-conf/#changes-introduced-in-octopus
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l262

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new

 src/PVE/CephConfig.pm | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index bbb9d39..2c4a086 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -19,13 +19,32 @@ sub parse_ceph_config {
     return $cfg if !defined($raw);
 
     my @lines = split /\n/, $raw;
+    my @lines_normalized;
+
+    my $re_comment_not_escaped = qr/(?<!\\)(#|;).*$/;
+    my $re_leading_ws = qr/^\s+/;
+    my $re_trailing_ws = qr/\s+$/;
+
+    while (scalar(@lines)) {
+	my $line = shift(@lines);
+	$line =~ s/$re_comment_not_escaped//;
+	$line =~ s/$re_leading_ws//;
+	$line =~ s/$re_trailing_ws//;
+	next if !$line;
+
+	# merge lines ending with continuation character '\'
+	while ($line =~ s/\\$//) {
+	    my $next_line = shift(@lines);
+	    $next_line =~ s/$re_comment_not_escaped//;
+	    $line .= $next_line;
+	}
+
+	push(@lines_normalized, $line);
+    }
 
     my $section;
 
-    for my $line (@lines) {
-	$line =~ s/(?<!\\)(#|;).*$//;
-	$line =~ s/^\s+//;
-	$line =~ s/\s+$//;
+    for my $line (@lines_normalized) {
 	next if !$line;
 
 	$section = $1 if $line =~ m/^\[(.+)\]$/;
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 07/13] cephconfig: allow writing arbitrary sections

[Max Carrara]

This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.

Sections that are associated with the client, for example
'[client.foo]', are written directly after the '[client]' section.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * Instead of just adding 'client.crash' as a separate section,
    also allow writing arbitrary sections
Changes v2 --> v3:
  * this patch now only contains the changes actually mentioned in the
    commit message; the other changes have been put into separate
    patches

 src/PVE/CephConfig.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 2c4a086..e78209e 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -79,6 +79,7 @@ my $parse_ceph_file = sub {
 sub write_ceph_config {
     my ($filename, $cfg) = @_;
 
+    my $written_sections = {};
     my $out = '';
 
     my $cond_write_sec = sub {
@@ -86,16 +87,21 @@ sub write_ceph_config {
 
 	foreach my $section (sort keys %$cfg) {
 	    next if $section !~ m/^$re$/;
+	    next if exists($written_sections->{$section});
+
 	    $out .= "[$section]\n";
 	    foreach my $key (sort keys %{$cfg->{$section}}) {
 		$out .= "\t $key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
+
+	    $written_sections->{$section} = 1;
 	}
     };
 
     &$cond_write_sec('global');
     &$cond_write_sec('client');
+    &$cond_write_sec('client\..*');
 
     &$cond_write_sec('mds');
     &$cond_write_sec('mon');
@@ -107,6 +113,8 @@ sub write_ceph_config {
     &$cond_write_sec('osd\..*');
     &$cond_write_sec('mgr\..*');
 
+    &$cond_write_sec('.*');
+
     return $out;
 }
 
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 08/13] cephconfig: change code style inside config writer

[Max Carrara]

This commit changes the code style of subroutine `write_ceph_config`
to match our style guide [0] more.

Furthermore, the repeated calls to the inner subroutine are replaced
with a loop, while the regular expressions used by the inner sub are
now quoted with `qr` to prevent any accidental mis-quotings in the
future.

[0]: https://pve.proxmox.com/wiki/Perl_Style_Guide

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; initial style changes moved from patch 06 of series v2
  * `qr` operator for regexes to avoid character escaping issues
  * call `$cond_write_sec` in loop

 src/PVE/CephConfig.pm | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index e78209e..16ad061 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -85,12 +85,12 @@ sub write_ceph_config {
     my $cond_write_sec = sub {
 	my $re = shift;
 
-	foreach my $section (sort keys %$cfg) {
+	for my $section (sort keys %$cfg) {
 	    next if $section !~ m/^$re$/;
 	    next if exists($written_sections->{$section});
 
 	    $out .= "[$section]\n";
-	    foreach my $key (sort keys %{$cfg->{$section}}) {
+	    for my $key (sort keys %{$cfg->{$section}}) {
 		$out .= "\t $key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
@@ -99,21 +99,27 @@ sub write_ceph_config {
 	}
     };
 
-    &$cond_write_sec('global');
-    &$cond_write_sec('client');
-    &$cond_write_sec('client\..*');
+    my @rexprs = (
+	qr/global/,
+	qr/client/,
+	qr/client\..*/,
 
-    &$cond_write_sec('mds');
-    &$cond_write_sec('mon');
-    &$cond_write_sec('osd');
-    &$cond_write_sec('mgr');
+	qr/mds/,
+	qr/mon/,
+	qr/osd/,
+	qr/mgr/,
 
-    &$cond_write_sec('mds\..*');
-    &$cond_write_sec('mon\..*');
-    &$cond_write_sec('osd\..*');
-    &$cond_write_sec('mgr\..*');
+	qr/mds\..*/,
+	qr/mon\..*/,
+	qr/osd\..*/,
+	qr/mgr\..*/,
 
-    &$cond_write_sec('.*');
+	qr/.*/,
+    );
+
+    for my $re (@rexprs) {
+	$cond_write_sec->($re);
+    }
 
     return $out;
 }
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 09/13] cephconfig: change order of written sections

[Max Carrara]

in order to group related sections together.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; originally patch 07 of series v2, now adapted to this series 

 src/PVE/CephConfig.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 16ad061..cc991eb 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -101,17 +101,20 @@ sub write_ceph_config {
 
     my @rexprs = (
 	qr/global/,
+
 	qr/client/,
 	qr/client\..*/,
 
 	qr/mds/,
-	qr/mon/,
-	qr/osd/,
-	qr/mgr/,
-
 	qr/mds\..*/,
+
+	qr/mon/,
 	qr/mon\..*/,
+
+	qr/osd/,
 	qr/osd\..*/,
+
+	qr/mgr/,
 	qr/mgr\..*/,
 
 	qr/.*/,
-- 
2.39.2

[pve-devel] [PATCH v3 pve-storage 10/13] cephconfig: remove leading whitespace on write to Ceph config

[Max Carrara]

Because continued lines (lines following lines that end with '\')
preserve whitespace, this commit removes any leading whitespace that
is added by our config writer.

This is done in order to make continued lines look less "awkward" when
manually changing values in 'ceph.conf' after the file has been
modified by our tooling. Before this commit, continued lines would
have to be added as follows:

[some_section]
	 some_key = Lorem ipsum dolor sit amet, consectetur \
adipiscing elit, sed do eiusmod tempor incididunt ut labore et \
dolore magna aliqua.

Due to whitespace being preserved, continued lines cannot be on the
same indentation level as `some_key`. Furthermore, the indentation
level might lead some users to mistakenly believe that leading
whitespace is ignored.

Thus, this commit removes the leading whitespace that is added by our
config writer.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new

Note: This patch may be dropped if not desired.

 src/PVE/CephConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index cc991eb..7982fb6 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -91,7 +91,7 @@ sub write_ceph_config {
 
 	    $out .= "[$section]\n";
 	    for my $key (sort keys %{$cfg->{$section}}) {
-		$out .= "\t $key = $cfg->{$section}->{$key}\n";
+		$out .= "$key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
 
-- 
2.39.2

[pve-devel] [PATCH v3 pve-manager 11/13] ceph: introduce '/etc/pve/ceph'

[Max Carrara]

This commit adds the '/etc/pve/ceph' directory to our overall expected
Ceph configuration.

This directory is meant to store cluster-wide, non-private
configuration files used by Ceph applications and services that are
executed with lower privileges, such as 'ceph-crash.service'.

The existence of the directory is now also checked for when checking
whether Ceph is configured correctly. This makes it easier for our
other tooling to rely on the directory's existence, reducing the
number of otherwise needless frequent checking.

For new clusters: `pveceph init` now creates '/etc/pve/ceph' when
called.

For existing clusters: The 'postinst' hook this commit adds ensures
that '/etc/pve/ceph' is created upon update.

The 'postinst' hook is also version-guarded and does not run when
upgrading from pve-manager version 8.1.5 and above.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new; originally part of patches 09, 10 of series v2 - decided it's
    better to move all this into a separate patch to make context +
    intention clearer

 PVE/API2/Ceph.pm  |  5 +++++
 PVE/Ceph/Tools.pm | 12 ++++++++++--
 debian/postinst   | 12 ++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 81c17d6e..7fedb87a 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -192,6 +192,11 @@ __PACKAGE__->register_method ({
 	    PVE::Ceph::Tools::check_ceph_installed('ceph_bin');
 	}
 
+	my $pve_ceph_cfgdir = PVE::Ceph::Tools::get_config('pve_ceph_cfgdir');
+	if (! -d $pve_ceph_cfgdir) {
+	    File::Path::make_path($pve_ceph_cfgdir);
+	}
+
 	my $auth = $param->{disable_cephx} ? 'none' : 'cephx';
 
 	# simply load old config if it already exists
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index ee6c515c..735bb116 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -18,6 +18,7 @@ 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_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
 my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
@@ -37,6 +38,7 @@ 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,
 };
@@ -186,8 +188,14 @@ sub check_ceph_inited {
 
     return undef if !check_ceph_installed('ceph_mon', $noerr);
 
-    if (! -f $pve_ceph_cfgpath) {
-	die "pveceph configuration not initialized\n" if !$noerr;
+    my @errors;
+
+    push(@errors, "missing '$pve_ceph_cfgpath'") if ! -f $pve_ceph_cfgpath;
+    push(@errors, "missing '$pve_ceph_cfgdir'") if ! -d $pve_ceph_cfgdir;
+
+    if (@errors) {
+	my $err = 'pveceph configuration not initialized - ' . join(', ', @errors) . "\n";
+	die $err if !$noerr;
 	return undef;
     }
 
diff --git a/debian/postinst b/debian/postinst
index 6138ef6d..61b50f97 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -80,6 +80,14 @@ EOF
     fi
 }
 
+update_ceph_conf() {
+    CEPH_CONF_DIR='/etc/pve/ceph'
+
+    if test ! -d "${CEPH_CONF_DIR}"; then
+        mkdir -p "${CEPH_CONF_DIR}"
+    fi
+}
+
 migrate_apt_auth_conf() {
     output=""
     removed=""
@@ -190,6 +198,10 @@ case "$1" in
 
     set_lvm_conf
 
+    if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.1.5~'; then
+        update_ceph_conf
+    fi
+
     if test ! -e /proxmox_install_mode; then
         # modeled after code generated by dh_start
         for unit in ${UNITS}; do
-- 
2.39.2

[pve-devel] [PATCH v3 pve-manager 12/13] fix #4759: ceph: configure ceph-crash.service and its key

[Max Carrara]

Due to Ceph dropping privileges when running the 'ceph-crash' daemon
[0], it is necessary to allow the daemon to authenticate with its
cluster in a safe manner.

In order to avoid exposing sensitive keyrings or somehow escalating
its privileges again, 'ceph-crash' is therefore provided with its own
keyring in the '/etc/pve/ceph' directory. This directory, due to being
on 'pmxcfs', may be read by members of the 'www-data' group, which
'ceph-crash' is made part of [1].


Expected Configuration
----------------------

 1. A keyring file named '/etc/pve/ceph/ceph.client.crash.keyring'
    exists
 2. A section named 'client.crash' exists in '/etc/pve/ceph.conf'
 3. The 'client.crash' section has a key named 'keyring' which
    references the keyring file as '/etc/pve/ceph/$cluster.$name.keyring'


New Clusters
------------

The keyring file is created and the conf file is updated after the first
monitor has been created (when calling `pveceph mon create`).


Existing Clusters
-----------------

A new helper script creates and configures the 'client.crash' keyring in
`postinst`, if:
 * Ceph is installed
 * Ceph is initialized ('/etc/pve/ceph.conf' and '/etc/pve/ceph' exist)
 * Connection to RADOS is successful

If the above conditions are met, the helper script ensures that the
existing configuration matches the expected configuration mentioned
above.

The configuration is not changed if it is already as expected.

The helper script may be called again manually if the `postinst` hook
fails. It is installed to '/usr/share/pve-manager/helpers/pve-init-ceph-crash'.


Existing `client.crash` Key
---------------------------

If a key named 'client.crash' already exists within the cluster, it is
reused and not regenerated. 


[0]: https://github.com/ceph/ceph/pull/48713
[1]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * fix 'keyring' key being appended to 'client.crash' section even
    if it already exists and configured correctly
Changes v2 --> v3:
  * avoid needless second 'pmxcfs' lock on 'ceph.conf' when configuring
    'client.crash' keyring after first monitor was created
  * change name of helper sub that creates the ceph.crash keyring
  * the helper sub now implicitly expects the /etc/pve/ceph directory to
    exist (as the previous commit enforces its existence)
  * remove BASH part of postinst hook related to `ceph-crash`
  * add Perl helper script and call that instead in postinst hook
  * reword commit message

 PVE/API2/Ceph/MON.pm    |   8 ++++
 PVE/Ceph/Tools.pm       |  35 ++++++++++++++
 bin/Makefile            |   1 +
 bin/pve-init-ceph-crash | 104 ++++++++++++++++++++++++++++++++++++++++
 debian/postinst         |   4 ++
 5 files changed, 152 insertions(+)
 create mode 100755 bin/pve-init-ceph-crash

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..8b7ce376 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -450,6 +450,14 @@ __PACKAGE__->register_method ({
 			)
 		    };
 		    warn "$@" if $@;
+
+		    print "Configuring keyring for ceph-crash.service\n";
+		    eval {
+			PVE::Ceph::Tools::create_or_update_crash_keyring_file();
+			$cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';
+			cfs_write_file('ceph.conf', $cfg);
+		    };
+		    warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
 		}
 
 		eval { PVE::Ceph::Services::ceph_service_cmd('enable', $monsection) };
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 735bb116..9b97171e 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -20,6 +20,7 @@ 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";
@@ -45,6 +46,7 @@ my $config_values = {
 
 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,
@@ -420,6 +422,39 @@ sub get_or_create_admin_keyring {
     return $pve_ckeyring_path;
 }
 
+# is also used in `pve-init-ceph-crash` helper
+sub create_or_update_crash_keyring_file {
+    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 (-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);
+	    return 1;
+	}
+    } else {
+	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
+	return 1;
+    }
+
+    return 0;
+}
+
 # get ceph-volume managed osds
 sub ceph_volume_list {
     my $result = {};
diff --git a/bin/Makefile b/bin/Makefile
index 06d8148e..b221e4b1 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -83,6 +83,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
 	install -m 0755 $(SCRIPTS) $(BINDIR)
 	install -d $(USRSHARE)/helpers
 	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
+	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
 	install -d $(MAN1DIR)
 	install -m 0644 $(CLI_MANS) $(MAN1DIR)
 	install -d $(MAN8DIR)
diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
new file mode 100755
index 00000000..f7c2f6e4
--- /dev/null
+++ b/bin/pve-init-ceph-crash
@@ -0,0 +1,104 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use List::Util qw(first);
+
+use PVE::Ceph::Tools;
+use PVE::Cluster;
+use PVE::RADOS;
+use PVE::RPCEnvironment;
+
+
+sub main {
+    my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+    $rpcenv->init_request();
+    $rpcenv->set_language($ENV{LANG});
+    $rpcenv->set_user('root@pam');
+
+    die "Not running as root. Exiting.\n" if $> != 0;
+
+    print("Checking whether keyring for 'ceph-crash.service' needs to be configured...\n");
+
+    if (!PVE::Ceph::Tools::check_ceph_installed('ceph_bin', 1)) {
+        print("Ceph is not installed. No action required.\n");
+        exit 0;
+    }
+
+    eval {
+        PVE::Ceph::Tools::check_ceph_inited();
+    };
+    if ($@) {
+        print("Ceph is not initialized. No action required.\n");
+        exit 0;
+    }
+
+    my $rados = eval { PVE::RADOS->new() };
+    $@ = undef; # warnings are displayed regardless
+
+    my $ceph_cfg_file = 'ceph.conf';
+    my $ceph_crash_key_path = PVE::Ceph::Tools::get_config('pve_ceph_crash_key_path');
+
+    my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
+
+    my $changed = 0;
+
+    PVE::Cluster::cfs_lock_file($ceph_cfg_file, undef, sub {
+        my $cfg = PVE::Cluster::cfs_read_file($ceph_cfg_file);
+
+	if (!defined($rados)) {
+	    my $has_mon_config = defined(first { m/mon\..*/ } keys $cfg->%*);
+	    if ($has_mon_config) {
+		die "Connection to RADOS failed even though a monitor is configured.\n" .
+		    "Please verify whether your configuration is correct.\n"
+	    }
+
+	    print(
+		"Connection to RADOS failed and no monitor is configured. ".
+		"Assume that things are fine. No action required.\n"
+	    );
+	    return undef;
+	}
+
+	my $write_to_conf = sub {
+	    print("Updating Ceph configuration...\n");
+	    $cfg->{'client.crash'}->{keyring} = $keyring_value;
+	    PVE::Cluster::cfs_write_file($ceph_cfg_file, $cfg);
+	    $changed = 1;
+	};
+
+	$changed = PVE::Ceph::Tools::create_or_update_crash_keyring_file($rados);
+
+	if (!exists($cfg->{'client.crash'})) {
+	    $write_to_conf->();
+	    return undef;
+	}
+
+	if (!exists($cfg->{'client.crash'}->{keyring})) {
+	    $write_to_conf->();
+	    return undef;
+	}
+
+	my $current_keyring_value = $cfg->{'client.crash'}->{keyring};
+	if ($current_keyring_value ne $keyring_value) {
+	    $write_to_conf->();
+	    return undef;
+	}
+
+    });
+    if ($@) {
+	warn("Failed to configure keyring for 'ceph-crash.service'. Error:\n");
+	warn("$@");
+	exit 1;
+    }
+
+    if ($changed) {
+	print("Successfully updated configuration for 'ceph-crash.service'.\n");
+    }
+}
+
+main();
+
+exit 0;
diff --git a/debian/postinst b/debian/postinst
index 61b50f97..c989dac1 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -86,6 +86,10 @@ update_ceph_conf() {
     if test ! -d "${CEPH_CONF_DIR}"; then
         mkdir -p "${CEPH_CONF_DIR}"
     fi
+
+    # Don't fail in case user has "exotic" configuration where RADOS
+    # isn't available on all nodes for some reason
+    /usr/share/pve-manager/helpers/pve-init-ceph-crash || true
 }
 
 migrate_apt_auth_conf() {
-- 
2.39.2

[pve-devel] [PATCH v3 pve-manager 13/13] bin/make: gather helper scripts in separate variable

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v2 --> v3:
  * new

 bin/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/bin/Makefile b/bin/Makefile
index b221e4b1..180a91b5 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -25,6 +25,10 @@ SCRIPTS =  			\
 	pveperf			\
 	pvereport
 
+HELPERS =			\
+	pve-startall-delay	\
+	pve-init-ceph-crash
+
 SERVICE_MANS = $(addsuffix .8, $(SERVICES))
 
 CLI_MANS = 				\
@@ -82,8 +86,7 @@ install: $(SCRIPTS) $(CLI_MANS) $(SERVICE_MANS) $(BASH_COMPLETIONS) $(ZSH_COMPLE
 	install -d $(BINDIR)
 	install -m 0755 $(SCRIPTS) $(BINDIR)
 	install -d $(USRSHARE)/helpers
-	install -m 0755 pve-startall-delay $(USRSHARE)/helpers
-	install -m 0755 pve-init-ceph-crash $(USRSHARE)/helpers
+	install -m 0755 $(HELPERS) $(USRSHARE)/helpers
 	install -d $(MAN1DIR)
 	install -m 0644 $(CLI_MANS) $(MAN1DIR)
 	install -d $(MAN8DIR)
-- 
2.39.2

Re: [pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Friedrich Weber]

Quickly tested the patch series on my existing Ceph Quincy cluster, did
not encounter major issues -- the keyring was created and the Ceph
config was rewritten accordingly. After a restart of `ceph-crash`, it
correctly posts crashes (produced with `kill -n11 $(pidof ceph-osd)`)
again and does not write any potentially misleading messages to the
journal. Nice!

Didn't have time yet to test these patches when setting up a new
cluster, but I'll try to do so this week and report back.

Two minor things I've noticed so far:

- the `ceph-crash` service does not restart after installing the patched
ceph-base package, so the reordering done by patches 02+04 does not take
effect immediately: ceph-crash posts crash logs just fine, but logs to
the journal that it can't find a keyring. After a restart of ceph-crash,
the patch takes effect, so only a tiny inconvenience, but still: Not
sure if restarting the service is something we'd want to do in a
postinst -- is this an acceptable thing to do in a postinst?

- Might there be issues in a mixed-version cluster scenario, so if some
node A already has an updated pve-storage package (patches 05-10), but
node B doesn't yet? One thing I noticed is that node A will add the
[client.crash] section, but node B may remove it again when it needs to
rewrite the Ceph config (e.g. when creating a monitor). I don't find
this particular issue too concerning, as hopefully node B will be
updated eventually as well and reinstate the [client.crash] section. But
I wonder if there could be other more serious issues?

On 16/02/2024 15:56, Max Carrara wrote:
> This marks version 03 of the patch series "Fix #4759: Configure
> Permissions for ceph-crash.service". Older versions can be found below.
> 
> Notable changes since v2
> ------------------------
> 
>   * The 'ceph.conf' parser in pve-storage is now equivalent to Ceph's
>     and even supports continued lines
>   * The addition of the '/etc/pve/ceph' directory has been moved into a
>     separate patch in order to preserve the context of its purpose in
>     the git history
>   * The debian `postinst` hook for pve-manager is now version-guarded
>     and uses a separate Perl helper script instead of doing everything
>     in BASH
> 
> 
> Older Versions
> --------------
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061646.html
> 
> 
> 
> ceph (master):
> 
> Max Carrara (2):
>   debian: add patch to fix ceph crash dir permissions in postinst hook
>   patches: add patch that reorders clients used by ceph-crash
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0017-ceph-crash-change-order-of-client-names.patch
> 
> 
> ceph (quincy-stable-8):
> 
> Max Carrara (2):
>   debian: add patch to fix ceph crash dir permissions in postinst hook
>   patches: add patch that reorders clients used by ceph-crash
> 
>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>  patches/series                                |  2 +
>  3 files changed, 86 insertions(+)
>  create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>  create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch
> 
> 
> pve-storage:
> 
> Max Carrara (6):
>   cephconfig: align our parser more with Ceph's parser
>   cephconfig: support line-continuations in parser
>   cephconfig: allow writing arbitrary sections
>   cephconfig: change code style inside config writer
>   cephconfig: change order of written sections
>   cephconfig: remove leading whitespace on write to Ceph config
> 
>  src/PVE/CephConfig.pm | 80 +++++++++++++++++++++++++++++++------------
>  1 file changed, 58 insertions(+), 22 deletions(-)
> 
> 
> pve-manager:
> 
> Max Carrara (3):
>   ceph: introduce '/etc/pve/ceph'
>   fix #4759: ceph: configure ceph-crash.service and its key
>   bin/make: gather helper scripts in separate variable
> 
>  PVE/API2/Ceph.pm        |   5 ++
>  PVE/API2/Ceph/MON.pm    |   8 ++++
>  PVE/Ceph/Tools.pm       |  47 +++++++++++++++++-
>  bin/Makefile            |   6 ++-
>  bin/pve-init-ceph-crash | 104 ++++++++++++++++++++++++++++++++++++++++
>  debian/postinst         |  16 +++++++
>  6 files changed, 183 insertions(+), 3 deletions(-)
>  create mode 100755 bin/pve-init-ceph-crash
> 

Re: [pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

On 2/21/24 12:55, Friedrich Weber wrote:
> Quickly tested the patch series on my existing Ceph Quincy cluster, did
> not encounter major issues -- the keyring was created and the Ceph
> config was rewritten accordingly. After a restart of `ceph-crash`, it
> correctly posts crashes (produced with `kill -n11 $(pidof ceph-osd)`)
> again and does not write any potentially misleading messages to the
> journal. Nice!
> 
> Didn't have time yet to test these patches when setting up a new
> cluster, but I'll try to do so this week and report back.
> 

Thank you very much for the testing and the feedback!

> Two minor things I've noticed so far:
> 
> - the `ceph-crash` service does not restart after installing the patched
> ceph-base package, so the reordering done by patches 02+04 does not take
> effect immediately: ceph-crash posts crash logs just fine, but logs to
> the journal that it can't find a keyring. After a restart of ceph-crash,
> the patch takes effect, so only a tiny inconvenience, but still: Not
> sure if restarting the service is something we'd want to do in a
> postinst -- is this an acceptable thing to do in a postinst?

Initially the service was being restarted, but that's omitted in the new
hook, as Fabian and I had noticed that `ceph-crash` just checks for its
expected keys after its waiting period again anyways. I had unfortunately
forgotten to put that into the changelog of the postinst hook stuff -
mea culpa!

I think restarting the service would be necessary then in order to apply
the new sequence which keys are checked in, as that's hard-coded in
`ceph-crash`.

It certainly should be acceptable (as we already do it in some instances),
as long as we restart it only if the service is enabled. That was part
of the old BASH function anyway - I don't think there's any harm in adding
it back (either in BASH or Perl).

> 
> - Might there be issues in a mixed-version cluster scenario, so if some
> node A already has an updated pve-storage package (patches 05-10), but
> node B doesn't yet? One thing I noticed is that node A will add the
> [client.crash] section, but node B may remove it again when it needs to
> rewrite the Ceph config (e.g. when creating a monitor). I don't find
> this particular issue too concerning, as hopefully node B will be
> updated eventually as well and reinstate the [client.crash] section. But
> I wonder if there could be other more serious issues?

The scenario you mentioned might indeed happen somehow, but once all
nodes are updated - even if the config has been changed inbetween updates -
the '[client.crash]' section should definitely exist.

One issue that's been fixed by moving things to the Perl helper is that
simultaneous updates could potentially modify 'ceph.conf' at the same time
- the Perl helper now locks the file on pmxcfs, so that cannot happen anymore.

I cannot think of any other scenario at the moment.

In any case, even if *somehow* 'ceph.conf' ends up not containing the section
or the keyring file ends up missing, the helper script will be available
after the update has been performed, so it's possible to just run it again
manually to adapt the config.

That being said, this reminds me that the '[ceph.crash]' section, the location
of the keyring file, etc. should probably be in our docs as well, so I will
send in a follow-up series for that (unless this series ends up needing a v4,
then I'll include it there).

Thanks again for the feedback and the tests you ran!

> 
> On 16/02/2024 15:56, Max Carrara wrote:
>> This marks version 03 of the patch series "Fix #4759: Configure
>> Permissions for ceph-crash.service". Older versions can be found below.
>>
>> Notable changes since v2
>> ------------------------
>>
>>   * The 'ceph.conf' parser in pve-storage is now equivalent to Ceph's
>>     and even supports continued lines
>>   * The addition of the '/etc/pve/ceph' directory has been moved into a
>>     separate patch in order to preserve the context of its purpose in
>>     the git history
>>   * The debian `postinst` hook for pve-manager is now version-guarded
>>     and uses a separate Perl helper script instead of doing everything
>>     in BASH
>>
>>
>> Older Versions
>> --------------
>>
>> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-January/061546.html
>> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061646.html
>>
>>
>>
>> ceph (master):
>>
>> Max Carrara (2):
>>   debian: add patch to fix ceph crash dir permissions in postinst hook
>>   patches: add patch that reorders clients used by ceph-crash
>>
>>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>>  patches/series                                |  2 +
>>  3 files changed, 86 insertions(+)
>>  create mode 100644 patches/0016-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>>  create mode 100644 patches/0017-ceph-crash-change-order-of-client-names.patch
>>
>>
>> ceph (quincy-stable-8):
>>
>> Max Carrara (2):
>>   debian: add patch to fix ceph crash dir permissions in postinst hook
>>   patches: add patch that reorders clients used by ceph-crash
>>
>>  ...ly-adjust-permissions-of-var-lib-cep.patch | 54 +++++++++++++++++++
>>  ...h-crash-change-order-of-client-names.patch | 30 +++++++++++
>>  patches/series                                |  2 +
>>  3 files changed, 86 insertions(+)
>>  create mode 100644 patches/0025-debian-recursively-adjust-permissions-of-var-lib-cep.patch
>>  create mode 100644 patches/0026-ceph-crash-change-order-of-client-names.patch
>>
>>
>> pve-storage:
>>
>> Max Carrara (6):
>>   cephconfig: align our parser more with Ceph's parser
>>   cephconfig: support line-continuations in parser
>>   cephconfig: allow writing arbitrary sections
>>   cephconfig: change code style inside config writer
>>   cephconfig: change order of written sections
>>   cephconfig: remove leading whitespace on write to Ceph config
>>
>>  src/PVE/CephConfig.pm | 80 +++++++++++++++++++++++++++++++------------
>>  1 file changed, 58 insertions(+), 22 deletions(-)
>>
>>
>> pve-manager:
>>
>> Max Carrara (3):
>>   ceph: introduce '/etc/pve/ceph'
>>   fix #4759: ceph: configure ceph-crash.service and its key
>>   bin/make: gather helper scripts in separate variable
>>
>>  PVE/API2/Ceph.pm        |   5 ++
>>  PVE/API2/Ceph/MON.pm    |   8 ++++
>>  PVE/Ceph/Tools.pm       |  47 +++++++++++++++++-
>>  bin/Makefile            |   6 ++-
>>  bin/pve-init-ceph-crash | 104 ++++++++++++++++++++++++++++++++++++++++
>>  debian/postinst         |  16 +++++++
>>  6 files changed, 183 insertions(+), 3 deletions(-)
>>  create mode 100755 bin/pve-init-ceph-crash
>>

Re: [pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Friedrich Weber]

On 21/02/2024 14:15, Max Carrara wrote:
> On 2/21/24 12:55, Friedrich Weber wrote:
>> [...]
>>
>> - the `ceph-crash` service does not restart after installing the patched
>> ceph-base package, so the reordering done by patches 02+04 does not take
>> effect immediately: ceph-crash posts crash logs just fine, but logs to
>> the journal that it can't find a keyring. After a restart of ceph-crash,
>> the patch takes effect, so only a tiny inconvenience, but still: Not
>> sure if restarting the service is something we'd want to do in a
>> postinst -- is this an acceptable thing to do in a postinst?
> 
> Initially the service was being restarted, but that's omitted in the new
> hook, as Fabian and I had noticed that `ceph-crash` just checks for its
> expected keys after its waiting period again anyways. I had unfortunately
> forgotten to put that into the changelog of the postinst hook stuff -
> mea culpa>
> I think restarting the service would be necessary then in order to apply
> the new sequence which keys are checked in, as that's hard-coded in
> `ceph-crash`.
> 
> It certainly should be acceptable (as we already do it in some instances),
> as long as we restart it only if the service is enabled. That was part
> of the old BASH function anyway - I don't think there's any harm in adding
> it back (either in BASH or Perl).

If it's acceptable, I think it would be nice to restart ceph-crash (it
doesn't seem to be restarted that often).

>> - Might there be issues in a mixed-version cluster scenario, so if some
>> node A already has an updated pve-storage package (patches 05-10), but
>> node B doesn't yet? One thing I noticed is that node A will add the
>> [client.crash] section, but node B may remove it again when it needs to
>> rewrite the Ceph config (e.g. when creating a monitor). I don't find
>> this particular issue too concerning, as hopefully node B will be
>> updated eventually as well and reinstate the [client.crash] section. But
>> I wonder if there could be other more serious issues?
> 
> The scenario you mentioned might indeed happen somehow, but once all
> nodes are updated - even if the config has been changed inbetween updates -
> the '[client.crash]' section should definitely exist.
> 
> One issue that's been fixed by moving things to the Perl helper is that
> simultaneous updates could potentially modify 'ceph.conf' at the same time
> - the Perl helper now locks the file on pmxcfs, so that cannot happen anymore.

Nice!

> I cannot think of any other scenario at the moment.

Yeah, me neither.

> In any case, even if *somehow* 'ceph.conf' ends up not containing the section
> or the keyring file ends up missing, the helper script will be available
> after the update has been performed, so it's possible to just run it again
> manually to adapt the config.
> 
> That being said, this reminds me that the '[ceph.crash]' section, the location
> of the keyring file, etc. should probably be in our docs as well, so I will
> send in a follow-up series for that (unless this series ends up needing a v4,
> then I'll include it there).
> 
> Thanks again for the feedback and the tests you ran!

Sure! I ran some more tests installing a fresh Reef cluster with the
patched packages, and did not notice any major issues.

One minor thing I noticed: If a user has manually worked around the
issue by generating a client.crash keyring, and adding a [client.crash]
section, as described in [1]:

[client.crash]
    key = <yourbase64key>

... after the upgrade, this user will end up with the following
[client.crash] section:

[client.crash]
key = <yourbase64key>
keyring = /etc/pve/ceph/$cluster.$name.keyring

and the same keyring <yourbase64key> in
/etc/pve/ceph/ceph.client.crash.keyring.

In my test this is not a problem, though (probably since both keys are
the same).

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4759#c7

Re: [pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

On 2/23/24 17:19, Friedrich Weber wrote:
> On 21/02/2024 14:15, Max Carrara wrote:
>> On 2/21/24 12:55, Friedrich Weber wrote:
>>> [...]
>>>
>>> - the `ceph-crash` service does not restart after installing the patched
>>> ceph-base package, so the reordering done by patches 02+04 does not take
>>> effect immediately: ceph-crash posts crash logs just fine, but logs to
>>> the journal that it can't find a keyring. After a restart of ceph-crash,
>>> the patch takes effect, so only a tiny inconvenience, but still: Not
>>> sure if restarting the service is something we'd want to do in a
>>> postinst -- is this an acceptable thing to do in a postinst?
>>
>> Initially the service was being restarted, but that's omitted in the new
>> hook, as Fabian and I had noticed that `ceph-crash` just checks for its
>> expected keys after its waiting period again anyways. I had unfortunately
>> forgotten to put that into the changelog of the postinst hook stuff -
>> mea culpa>
>> I think restarting the service would be necessary then in order to apply
>> the new sequence which keys are checked in, as that's hard-coded in
>> `ceph-crash`.
>>
>> It certainly should be acceptable (as we already do it in some instances),
>> as long as we restart it only if the service is enabled. That was part
>> of the old BASH function anyway - I don't think there's any harm in adding
>> it back (either in BASH or Perl).
> 
> If it's acceptable, I think it would be nice to restart ceph-crash (it
> doesn't seem to be restarted that often).

I agree!

> 
>>> - Might there be issues in a mixed-version cluster scenario, so if some
>>> node A already has an updated pve-storage package (patches 05-10), but
>>> node B doesn't yet? One thing I noticed is that node A will add the
>>> [client.crash] section, but node B may remove it again when it needs to
>>> rewrite the Ceph config (e.g. when creating a monitor). I don't find
>>> this particular issue too concerning, as hopefully node B will be
>>> updated eventually as well and reinstate the [client.crash] section. But
>>> I wonder if there could be other more serious issues?
>>
>> The scenario you mentioned might indeed happen somehow, but once all
>> nodes are updated - even if the config has been changed inbetween updates -
>> the '[client.crash]' section should definitely exist.
>>
>> One issue that's been fixed by moving things to the Perl helper is that
>> simultaneous updates could potentially modify 'ceph.conf' at the same time
>> - the Perl helper now locks the file on pmxcfs, so that cannot happen anymore.
> 
> Nice!
> 
>> I cannot think of any other scenario at the moment.
> 
> Yeah, me neither.
> 
>> In any case, even if *somehow* 'ceph.conf' ends up not containing the section
>> or the keyring file ends up missing, the helper script will be available
>> after the update has been performed, so it's possible to just run it again
>> manually to adapt the config.
>>
>> That being said, this reminds me that the '[ceph.crash]' section, the location
>> of the keyring file, etc. should probably be in our docs as well, so I will
>> send in a follow-up series for that (unless this series ends up needing a v4,
>> then I'll include it there).
>>
>> Thanks again for the feedback and the tests you ran!
> 
> Sure! I ran some more tests installing a fresh Reef cluster with the
> patched packages, and did not notice any major issues.
> 
> One minor thing I noticed: If a user has manually worked around the
> issue by generating a client.crash keyring, and adding a [client.crash]
> section, as described in [1]:
> 
> [client.crash]
>     key = <yourbase64key>
> 
> ... after the upgrade, this user will end up with the following
> [client.crash] section:
> 
> [client.crash]
> key = <yourbase64key>
> keyring = /etc/pve/ceph/$cluster.$name.keyring
> 
> and the same keyring <yourbase64key> in
> /etc/pve/ceph/ceph.client.crash.keyring.
> 
> In my test this is not a problem, though (probably since both keys are
> the same).
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4759#c7

Oh, good catch! I'll correct this in a v4, I think. We want to ensure we're
only setting the keyring, in our case.

Thanks again for all the tests, much appreciated!