[pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

Fix #4759: Configure Permissions for ceph-crash.service - Version 5
===================================================================

Notable changes since v4
------------------------

  * The patches regarding Ceph Reef and Ceph Quincy have been applied in
    v4 and are thus dropped from this series (thanks Thomas!)
  * The tests for both the parser and the writer are rewritten, making
    them independent of one another
  * Many additional test cases
  * New test subroutine that compares each test case against Ceph's
    `ceph-conf` CLI in order to preemptively detect changes / regressions
    in Ceph's config file format
    - In total, there are now 37 test cases, each tested against 3
      testing subroutines, making 111 tests in total
  * The Ceph config writer now correctly escapes all un-escaped comment
    literals
  * The Ceph config parser is completely rewritten, making its
    implementation almost fully equivalent to Ceph's original parser
  * Suppress warning messages while running `postinst` of 'pve-manager'
    (thanks Friedrich!)
  * Prevent `deb-systemd-invoke` from failing (thanks Fabian!)

For a detailed list of changes, please see the comments in the in the
individual patches.

Post-Installation Version Guard
-------------------------------

The call to the new function in the `postinst` hook, which is introduced
in patch 09, now uses '0.0.0' as placeholder in its version guard.

When applying this series, this placeholder must be replaced with the
next expected version of the 'pve-manager' package in order to run the
Ceph configuration helper during users' next update (patches 09, 10).

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
v3: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061802.html
v4: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062093.html

Summary of Changes
------------------

pve-storage:

Max Carrara (8):
  cephconfig: change code style inside config writer
  test: add tests for 'ceph.conf' parser and writer
  test: cephconfig: add regression test for Ceph config parser & writer
  cephconfig: allow writing arbitrary sections
  cephconfig: change order of written sections
  cephconfig: align written key-value pairs by tab
  cephconfig: escape un-escaped comment literals on write
  cephconfig: align our parser with Ceph's parser

 debian/control                             |    1 +
 src/Makefile                               |    1 +
 src/PVE/CephConfig.pm                      |  282 +++++-
 src/PVE/Makefile                           |    4 +
 src/PVE/test/Makefile                      |    9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 1035 ++++++++++++++++++++
 6 files changed, 1305 insertions(+), 27 deletions(-)
 create mode 100644 src/PVE/test/Makefile
 create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl


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 | 142 ++++++++++++++++++++++++++++++++++++++++
 debian/postinst         |  25 +++++++
 6 files changed, 230 insertions(+), 3 deletions(-)
 create mode 100755 bin/pve-init-ceph-crash

-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 01/11] 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

Changes v3 --> v4:
  * use postfix deref ($hashref->%* instead of %$hashref)
  * rebased due to previous changes

Changes v4 --> v5:
  * reorder commit (makes rebasing etc. in remaining patches easier)

 src/PVE/CephConfig.pm | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b10d46..9934fd5 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -65,28 +65,35 @@ 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$/;
+
 	    $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";
 	}
     };
 
-    &$cond_write_sec('global');
-    &$cond_write_sec('client');
+    my @rexprs = (
+	qr/global/,
+	qr/client/,
+
+	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('mds\..*');
-    &$cond_write_sec('mon\..*');
-    &$cond_write_sec('osd\..*');
-    &$cond_write_sec('mgr\..*');
+    for my $re (@rexprs) {
+	$cond_write_sec->($re);
+    }
 
     return $out;
 }
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 02/11] test: add tests for 'ceph.conf' parser and writer

[Max Carrara]

These tests attempt to cover most of Ceph's config parser's grammar,
including all of its syntax quirks [0].

Each case is tested against two testing subroutines:

  1. The parser's output is compared with the expected output.

  2. The writer's output is parsed again ant then compared with the
     expected output.

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=e9fe820e7fffd1b7cde143a9f77653b73fcec748#l144

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

Changes v4 --> v5:
  * refactor loop over test cases
    - each test is now represented by its own subroutine
    - put loop into new `main` subroutine
    - make it easier to add new testing subroutines by looping over
      them and calling them by reference on each case
  * add many new test cases
  * improve diagnostic output on test failure
  * reword commit message, omitting paragraph about quoted / unquoted
    string handling
  * reorder commit (was previously added after parser / writer changes)

 src/Makefile                               |   1 +
 src/PVE/Makefile                           |   4 +
 src/PVE/test/Makefile                      |   9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 794 +++++++++++++++++++++
 4 files changed, 808 insertions(+)
 create mode 100644 src/PVE/test/Makefile
 create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl

diff --git a/src/Makefile b/src/Makefile
index 1eba51e..a322f46 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -15,6 +15,7 @@ install: PVE bin udev-rbd
 test:
 	perl -I. -T -e "use PVE::CLI::pvesm; PVE::CLI::pvesm->verify_api();"
 	$(MAKE) -C test
+	$(MAKE) -C PVE test
 
 .PHONY: clean
 clean:
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index 5fe4a0a..d438804 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -9,4 +9,8 @@ install:
 	make -C API2 install
 	make -C CLI install
 
+.PHONY: test
+test:
+	$(MAKE) -C test test
+
 clean:
diff --git a/src/PVE/test/Makefile b/src/PVE/test/Makefile
new file mode 100644
index 0000000..f9c6c79
--- /dev/null
+++ b/src/PVE/test/Makefile
@@ -0,0 +1,9 @@
+all:
+
+.PHONY: test
+test: test_ceph_conf_parse_write
+
+.PHONY: test_ceph_conf_parse_write
+test_ceph_conf_parse_write: ceph_conf_parse_write_test.pl
+	./ceph_conf_parse_write_test.pl
+
diff --git a/src/PVE/test/ceph_conf_parse_write_test.pl b/src/PVE/test/ceph_conf_parse_write_test.pl
new file mode 100755
index 0000000..1d5e506
--- /dev/null
+++ b/src/PVE/test/ceph_conf_parse_write_test.pl
@@ -0,0 +1,794 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(../..);
+
+use Test::More;
+
+use PVE::CephConfig;
+
+
+# An array of test cases.
+# Each test case is comprised of the following keys:
+#   description	  => to identify a single test
+#   expected_cfg  => the hash that parse_ceph_config should return
+#   raw	          => the raw content of the file to test
+my $tests = [
+    {
+	description => 'empty file',
+	expected_cfg => {},
+	raw => <<~EOF,
+	EOF
+    },
+    {
+	description => 'file without section',
+	expected_cfg => {},
+	raw => <<~EOF,
+	While Ceph's format doesn't allow this, we do, because it makes things simpler
+	foo = bar
+	arbitrary text can go here
+
+	Rust is better than Perl
+	EOF
+    },
+    {
+	description => 'single section',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, no key-value pairs',
+	expected_cfg => {
+	    foo => {},
+	},
+	raw => <<~EOF,
+	[foo]
+	EOF
+    },
+    {
+	description => 'single section, whitespace before key',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	     \t     bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header with preceding whitespace',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	  \t    [foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header with comment',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo] # some comment
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, section header ' .
+	    'with preceding whitespace and comment',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	  \t  [foo] ; some comment
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, arbitrary text before section',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	Rust is better than Perl
+
+	This text is ignored by our parser, because it makes things simpler
+	[foo]
+	bar = baz
+	EOF
+    },
+    {
+	description => 'single section, invalid key-value pairs',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	this here will cause a warning and is ignored
+	bar = baz
+	as well as this
+	EOF
+    },
+    {
+	description => 'single section, multiple key-value pairs',
+	expected_cfg => {
+	    foo => {
+		one => 1,
+		two => 2,
+		three => 3,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one = 1
+	two = 2
+
+	three = 3
+	EOF
+    },
+    {
+	description => 'multiple sections with whitespace in section headers',
+	expected_cfg => {
+	    'foo bar' => {},
+	    ' quo  qux ' => {},
+	},
+	raw => <<~EOF,
+	[foo bar]
+	[ quo  qux ]
+	EOF
+    },
+    {
+	description => 'single section with whitespace in section header '
+	    . 'and multiple key-value pair',
+	expected_cfg => {
+	    'foo bar' => {
+		one => 1,
+		two => 2,
+	    },
+	    '  quo  ' => {
+		three => 3,
+	    },
+	},
+	raw => <<~EOF,
+	[foo bar]
+	one = 1
+	two = 2
+
+	[  quo  ]
+	three = 3
+	EOF
+    },
+    {
+	description => 'single section, numeric key-value pairs',
+	expected_cfg => {
+	    foo => {
+		'0' => 0,
+		'1' => 1,
+		'2' => 0,
+		'3' => 1,
+		'3.14' => 1.414,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	0 = 0
+	1 = 1
+	2 = 0
+	3 = 1
+	3.14 = 1.414
+	EOF
+    },
+    {
+	description => 'single section, keys with single-quoted values',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = 'baz'
+	quo = 'qux'
+	EOF
+    },
+    {
+	description => 'single section, keys with double-quoted values',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = "baz"
+	quo = "qux"
+	EOF
+    },
+    {
+	description => 'single section, keys with quoted values, '
+	    . 'comment literals within quotes',
+	expected_cfg => {
+	    foo => {},
+	},
+	raw => <<~EOF,
+	[foo]
+	one = "1;1"
+	two = "2#2"
+	three = '3;3'
+	four = '4#4'
+	EOF
+    },
+    {
+	description => 'single section, keys with quoted values, '
+	    . 'escaped comment literals within quotes',
+	expected_cfg => {
+	    foo => {
+		one => '1;1',
+		two => '2#2',
+		three => '3;3',
+		four => '4#4',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one = "1\\;1"
+	two = "2\\#2"
+	three = '3\\;3'
+	four = '4\\#4'
+	EOF
+    },
+    {
+	description => 'single section, keys with quoted values, '
+	    . 'comments after values',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = "baz" ; some comment
+	quo = 'qux'	# another comment
+	EOF
+    },
+    {
+	description => 'single section, keys with quoted values, '
+	    . 'continued lines after quotes',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = "baz"\\
+
+	quo = 'qux'\\
+
+	EOF
+    },
+    {
+	description => 'single section, keys with quoted values, '
+	    . 'continued lines with comments after quotes',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = "baz"\\
+	# believe it or not, this is valid syntax
+
+	quo = 'qux'\\
+	\\
+	\\
+	\\
+	    \t \t ; and it "supports" trailing whitespace for some reason
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with whitespace',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	       \t  bar  \t   =\t \tbaz\t
+
+	quo\t=\tqux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs without whitespace',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar=baz
+	quo=qux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with repeated whitespace '
+	    . 'in key names',
+	expected_cfg => {
+	    foo => {
+		'one_space' => 1,
+		'two_spaces' => 2,
+		'three_spaces' => 3,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one space = 1
+	two  spaces = 2
+	three  spaces = 3
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with whitespace and '
+	    . 'complex whitespace in key names',
+	expected_cfg => {
+	    foo => {
+		'one_two' => 'foo',
+		'three_four' => 'bar',
+		'five_six' => 'baz',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	       \t  one two  \t   =\t \tfoo\t
+
+	three   \t  four\t=\tbar\t
+	five     six=baz
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with repeated whitespace '
+	    . 'and underlines in key names',
+	expected_cfg => {
+	    foo => {
+		'one_ul' => 2,
+		'two_ul' => 0,
+		'two__ul' => 0,
+		'odd___name' => 4,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	# these are equivalent
+	one ul = 0
+	one_ul = 1
+	one             ul = 2
+
+	# these are not
+	two  ul = 0
+	two__ul = 0
+
+	# these are equivalent
+	odd _ name = 0
+	odd      _     name = 1
+	odd__ name = 2
+	odd __name = 3
+	odd___name = 4
+
+	EOF
+    },
+    {
+	description => 'single section with line continuations, multiple key-value pairs',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[\\
+	f\\
+	o\\
+	o\\
+	]\\
+
+	bar = baz
+	quo = qux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with continued lines in keys',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar\\
+	= baz
+	\\
+	quo\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	= qux
+	EOF
+    },
+    {
+	description => 'multiple sections with escaped comment literals, '
+	    . 'multiple key-value pairs',
+	expected_cfg => {
+	    'f;oo' => {
+		one => 1,
+		two => 2,
+	    },
+	    'b#ar' => {
+		three => 3,
+		four => 4,
+	    },
+	    '###' => {
+		five => 5,
+	    },
+	    ';;;' => {
+		six => 6,
+	    },
+	},
+	raw => <<~EOF,
+	[f\\;oo]
+	one = 1
+	two = 2
+
+	[b\\#ar]
+	three = 3
+	four = 4
+
+	[\\#\\#\\#]
+	five = 5
+
+	[\\;\\;\\;]
+	six = 6
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with comments',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz',
+		quo => 'qux',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	; preceding comment
+	bar = baz # some comment
+	### comment inbetween
+	;; another one for good measure
+	quo = qux ; another comment
+	# trailing comment
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with continued lines',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz continued baz',
+		quo => "qux continued \tqux",
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz \\
+	continued baz
+
+	quo =\\
+	qux \\
+	continued \\
+	\tqux
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'continued lines and comments',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz continued baz',
+		quo => 'qux continued qux',
+		key => 'value',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz \\
+	continued baz # comments are allowed here
+
+	quo =\\
+	qux \\
+	continued \\
+	qux # but this continuation will be ignored, because it's in a comment: \\
+	key = value\\
+	# really weird comment
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'escaped commment literals in values',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz#escaped',
+		quo => 'qux;escaped',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz\\#escaped
+	quo = qux\\;escaped
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with ' .
+	    'continued lines and escaped commment literals in values',
+	expected_cfg => {
+	    foo => {
+		bar => 'baz#escaped',
+		quo => 'qux;escaped continued# escaped done',
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	bar = baz\\#escaped
+
+	quo = qux\\;escaped\\
+	 continued\\# escaped \\
+	done
+	EOF
+    },
+    {
+	description => 'single section, key-value pairs with escaped comment '
+	    . 'literals in key names',
+	expected_cfg => {
+	    foo => {
+		'b#a#r' => 'baz',
+		';q;uo' => 'qux',
+		'#' => 1,
+		'##' => 2,
+		'###' => 3,
+		';' => 1,
+		';;' => 2,
+		';;;' => 3,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	b\\#a\\#r = baz
+	\\;q\\;uo = qux
+
+	\\# = 1
+	\\#\\# = 2
+	\\#\\#\\# = 3
+
+	\\; = 1
+	\\;\\; = 2
+	\\;\\;\\; = 3
+	EOF
+    },
+    {
+	description => 'multiple sections, multiple key-value pairs',
+	expected_cfg => {
+	    foo => {
+		one => 1,
+		two => 2,
+	    },
+	    bar => {
+		three => 3,
+		four => 4,
+	    },
+	},
+	raw => <<~EOF,
+	[foo]
+	one = 1
+	two = 2
+	[bar]
+	three = 3
+	four = 4
+	EOF
+    },
+    {
+	description => 'multiple sections, multiple key-value pairs, '
+	    . 'comments inline and inbetween, escaped comment literals, '
+	    . 'continued lines, arbitrary whitespace',
+	# NOTE: We don't use '/etc/pve/priv/$cluster.$name.keyring' as value for
+	# 'keyring' below, because `ceph-conf` will actually substitute those.
+	# Because we don't care for that (not the parser's or writer's job) we
+	# just omit the dollar signs.
+	expected_cfg => {
+	    global => {
+		auth_client_required => 'cephx',
+		auth_cluster_required => 'cephx',
+		auth_service_required => 'cephx',
+		cluster_network => '172.16.65.0/24',
+		fsid => '0e2f72eb-ffff-ffff-ffff-f480790a5b07',
+		mon_allow_pool_delete => 'true',
+		mon_host => '172.16.65.12 172.16.65.13 172.16.65.11',
+		ms_bind_ipv4 => 'true',
+		osd_pool_default_min_size => '2',
+		osd_pool_default_size => '3',
+		public_network => '172.16.65.0/24',
+	    },
+	    client => {
+		keyring => '/etc/pve/priv/cluster.name.keyring',
+	    },
+	    'mon.ceph-01' => {
+		public_addr => '172.16.65.11',
+	    },
+	    'mon.ceph-02' => {
+		public_addr => '172.16.65.12',
+	    },
+	    'mon.ceph-03' => {
+		public_addr => '172.16.65.13',
+	    },
+	    'some arbitrary section' => {
+		some_key => 'foo;bar;baz',
+	    },
+	},
+	raw => <<~EOF,
+	[global]
+	auth_client_required = cephx
+		auth_cluster_required = \\
+	cephx
+	auth_service_required =           cephx
+			cluster_network =        172.16.65.0/24
+	fsid = 0e2f72eb-ffff-ffff-ffff-f480790a5b07
+	    mon_allow_pool_delete = true
+	mon_host = \\
+	172.16.65.12 \\
+	172.16.65.13 \\
+	172.16.65.11 # why is this one last? nobody knows for sure!
+
+	ms_bind_ipv4 = true
+	osd_pool_default_min_size =\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	\\
+	2
+	osd_pool_default_size =\\
+					    3
+	public_network = 172.16.65.0/24 # some comment
+
+	[client] ### another comment
+	keyring = /etc/pve/priv/cluster.name.keyring# cheeky trailing comment
+
+	## mon config ##
+	[mon.ceph-01]
+	public_addr = 172.16.65.11 ; foo
+
+	;; another arbitrary comment ;;
+	[mon.ceph-02] ;; very important comment here
+	public_addr = "172.16.65.12" # bar
+
+	[mon.ceph-03]
+	public_addr = '172.16.65.13'			# baz
+
+	[some arbitrary section]
+	some key = "foo\\;bar\\;baz"    # I am a comment
+	EOF
+    },
+];
+
+sub test_parse_ceph_config {
+    my ($case) = @_;
+
+    my $desc = "parse_ceph_config: $case->{description}";
+
+    my $parse_result = eval { PVE::CephConfig::parse_ceph_config(undef, $case->{raw}) };
+    if ($@) {
+	fail($desc);
+	diag('Failed to parse config:');
+	diag($@);
+	return;
+    }
+
+    if (!is_deeply($parse_result, $case->{expected_cfg}, $desc)) {
+	diag("=== Expected ===");
+	diag(explain($case->{expected_cfg}));
+	diag("=== Got ===");
+	diag(explain($parse_result));
+    }
+}
+
+sub test_write_ceph_config {
+    my ($case) = @_;
+
+    my $desc = "write_ceph_config: $case->{description}";
+
+    my $write_result = eval { PVE::CephConfig::write_ceph_config(undef, $case->{expected_cfg}) };
+    if ($@) {
+	fail($desc);
+	diag('Failed to write config:');
+	diag($@);
+	return;
+    }
+
+    my $parse_result = eval { PVE::CephConfig::parse_ceph_config(undef, $write_result) };
+    if ($@) {
+	fail($desc);
+	diag('Failed to parse previously written config:');
+	diag($@);
+	return;
+    }
+
+    if (!is_deeply($parse_result, $case->{expected_cfg}, $desc)) {
+	diag("=== Expected ===");
+	diag(explain($case->{expected_cfg}));
+	diag("=== Got ===");
+	diag(explain($parse_result));
+	diag("=== Write Output ===");
+	diag($write_result);
+    }
+}
+
+sub main {
+    my $test_subs = [
+	\&test_parse_ceph_config,
+	\&test_write_ceph_config,
+    ];
+
+    plan(tests => scalar($tests->@*) * scalar($test_subs->@*));
+
+    for my $case ($tests->@*) {
+	for my $test_sub ($test_subs->@*) {
+	    eval {
+		# suppress warnings here to make output less noisy for certain tests
+		local $SIG{__WARN__} = sub {};
+		$test_sub->($case);
+	    };
+	    warn "$@\n" if $@;
+	};
+    }
+
+    done_testing();
+}
+
+main();
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 03/11] test: cephconfig: add regression test for Ceph config parser & writer

[Max Carrara]

Building on the previously declared cases, this test constructs a
config hash akin to the one the parser returns by invoking the
`ceph-conf` command.

The cases which `ceph-conf` cannot handle are marked with a special
key. If the key is provided, invocatinos to `ceph-conf` are expected
to fail. Furthermore, our parser is expected to behave differently for
those cases for simplicity's sake.

Should `ceph-conf` suddenly succeed when it parses a case where it is
expected to fail, or should it fail to parse a case where it is
expected to succeed, the test itself will fail. This way we can detect
any upstream changes to Ceph's config format and act accordingly.

Naturally, this requires the "ceph-common" package to be available
during builds, which is added to "debian/control" as build dep.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Changes v4 --> v5:
  * new

 debian/control                             |   1 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 241 +++++++++++++++++++++
 2 files changed, 242 insertions(+)

diff --git a/debian/control b/debian/control
index d0b1d95..90a895e 100644
--- a/debian/control
+++ b/debian/control
@@ -3,6 +3,7 @@ Section: perl
 Priority: optional
 Maintainer: Proxmox Support Team <support@proxmox.com>
 Build-Depends: debhelper-compat (= 13),
+               ceph-common (>= 12.2~),
                libfile-chdir-perl,
                libposix-strptime-perl,
                libpve-access-control,
diff --git a/src/PVE/test/ceph_conf_parse_write_test.pl b/src/PVE/test/ceph_conf_parse_write_test.pl
index 1d5e506..e3116c6 100755
--- a/src/PVE/test/ceph_conf_parse_write_test.pl
+++ b/src/PVE/test/ceph_conf_parse_write_test.pl
@@ -8,13 +8,182 @@ use lib qw(../..);
 use Test::More;
 
 use PVE::CephConfig;
+use PVE::Tools qw(run_command file_set_contents);
 
 
+sub with_tmp_ceph_conf_file {
+    my ($raw_content, $func) = @_;
+
+    my $cfg_filename = "/tmp/ceph-test-$$.conf";
+    my $perm = 0600;
+
+    my $rval = eval {
+	file_set_contents($cfg_filename, $raw_content, $perm);
+	return $func->($cfg_filename);
+    };
+
+    my $err = $@;
+
+    unlink $cfg_filename or warn "Failed to unlink temporary file '$cfg_filename' - $!\n";
+
+    die $err if $err;
+
+    return $rval;
+}
+
+# In an ideal world, `ceph-conf` would be able to just generate some JSON output
+# via the '--format JSON' flag, but that unfortunately doesn't do anything.
+# Whe therefore have to invoke `ceph-conf` for each key in each section here.
+#
+# The returned hash contains two keys:
+#   cfg => The Ceph config hash that is constructed by invoking the CLI
+#   cmd_infos => Information of the command invoked for each key of each
+#		 section, in the form of:
+#	{
+#	    section => {
+#		key => {
+#		    cmd => The invoked command,
+#		    exit_code => The command's exit code,
+#		    stdout => Its stdout,
+#		    stderr => Its stderr,
+#		},
+#		[...]
+#	    },
+#	    [...]
+#	}
+sub query_cfg_via_ceph_conf_cli {
+    my ($cfg_filename, $expected_cfg) = @_;
+
+    my $queried_cfg = {};
+
+    my $section_cmd_info = invoke_ceph_conf_cli_for_sections($cfg_filename);
+    for my $section (split(/\n/, $section_cmd_info->{stdout})) {
+	$queried_cfg->{$section} = {};
+    }
+
+    my $key_cmd_infos = {};
+
+    for my $section (sort keys $expected_cfg->%*) {
+	for my $key (sort keys $expected_cfg->{$section}->%*) {
+	    my $value = $expected_cfg->{$section}->{$key};
+
+	    my $output = invoke_ceph_conf_cli_for_key($cfg_filename, $section, $key);
+
+	    $queried_cfg->{$section}->{$key} = $output->{stdout};
+	    $key_cmd_infos->{$section}->{$key} = $output;
+	}
+    }
+
+    return {
+	cfg => $queried_cfg,
+	section_cmd_info => $section_cmd_info,
+	key_cmd_infos => $key_cmd_infos,
+    };
+}
+
+sub invoke_ceph_conf_cli_for_sections {
+    my ($cfg_filename) = @_;
+
+    my $cmd_info = {
+	cmd => ['ceph-conf', '-c', $cfg_filename, '--list-all-sections'],
+	exit_code => -1,
+	stdout => '',
+	stderr => '',
+    };
+
+    return invoke_ceph_conf_cli($cmd_info);
+}
+
+sub invoke_ceph_conf_cli_for_key {
+    my ($cfg_filename, $section, $key) = @_;
+
+    # Need to escape (not escaped) comment literals here, otherwise the CLI
+    # will fail to look up the key
+    $key =~ s/(?<!\\)([;#])/\\$1/g;
+
+    my $cmd_info = {
+	cmd => ['ceph-conf', '-c', $cfg_filename, '-s', $section, '--lookup', $key],
+	exit_code => -1,
+	stdout => '',
+	stderr => '',
+    };
+
+    return invoke_ceph_conf_cli($cmd_info);
+}
+
+sub invoke_ceph_conf_cli {
+    my ($cmd_info) = @_;
+
+    my $exit_code = eval {
+	run_command(
+	    $cmd_info->{cmd},
+	    noerr => 1,
+	    outfunc => sub { $cmd_info->{stdout} .= "$_[0]\n"; },
+	    errfunc => sub { $cmd_info->{stderr} .= "$_[0]\n"; },
+	)
+    };
+    $cmd_info->{exit_code} = $exit_code;
+
+    if ($@) {
+	my $errmsg = "Fatal error while invoking 'ceph-conf' (exit code: $exit_code) - $@\n\n";
+
+	if ($cmd_info->{stdout}) {
+	    $errmsg .= "stdout:\n" . $cmd_info->{stdout};
+	}
+
+	if ($cmd_info->{stderr}) {
+	    $errmsg .= "stderr:\n" . $cmd_info->{stderr};
+	}
+
+	die "$errmsg\n";
+    }
+
+    # remove trailing newlines for easier comparison later
+    $cmd_info->{stdout} =~ s/\n$//;
+    $cmd_info->{stderr} =~ s/\n$//;
+
+    return $cmd_info;
+}
+
+# Because `ceph-conf` doesn't have a separate exit code for parse failures,
+# this checks if all invocations failed and printed something to stderr.
+sub has_parse_failure {
+    my ($query_result) = @_;
+
+    my $section_cmd_info = $query_result->{section_cmd_info};
+
+    if ($section_cmd_info->{stderr} eq '' || $section_cmd_info->{exit_code} == 0) {
+	return 0;
+    }
+
+    my $key_cmd_infos = $query_result->{key_cmd_infos};
+
+    for my $section (keys $key_cmd_infos->%*) {
+	for my $key (keys $key_cmd_infos->{$section}->%*) {
+	    my $cmd_info = $key_cmd_infos->{$section}->{$key};
+	    my $failed = $cmd_info->{stderr} ne '' && $cmd_info->{exit_code} != 0;
+
+	    return 0 if !$failed;
+	}
+    }
+
+    return 1;
+}
+
 # An array of test cases.
 # Each test case is comprised of the following keys:
 #   description	  => to identify a single test
 #   expected_cfg  => the hash that parse_ceph_config should return
 #   raw	          => the raw content of the file to test
+#
+# Some cases also contain the optional 'expect_cli_parse_fail' key.
+# If supplied, this marks the case to be expected to fail for Ceph's
+# `ceph-conf` CLI interface that is used to check for regressions.
+#
+# Should a case that's expected to fail suddenly pass, or should
+# it fail when it's supposed to pass, the `ceph-conf` CLI test will
+# fail. This means that the upstream config grammar has most likely
+# changed.
 my $tests = [
     {
 	description => 'empty file',
@@ -24,6 +193,7 @@ my $tests = [
     },
     {
 	description => 'file without section',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {},
 	raw => <<~EOF,
 	While Ceph's format doesn't allow this, we do, because it makes things simpler
@@ -68,6 +238,7 @@ my $tests = [
     },
     {
 	description => 'single section, section header with preceding whitespace',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {
 	    foo => {
 		bar => 'baz',
@@ -93,6 +264,7 @@ my $tests = [
     {
 	description => 'single section, section header ' .
 	    'with preceding whitespace and comment',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {
 	    foo => {
 		bar => 'baz',
@@ -105,6 +277,7 @@ my $tests = [
     },
     {
 	description => 'single section, arbitrary text before section',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {
 	    foo => {
 		bar => 'baz',
@@ -120,6 +293,7 @@ my $tests = [
     },
     {
 	description => 'single section, invalid key-value pairs',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {
 	    foo => {
 		bar => 'baz',
@@ -232,6 +406,7 @@ my $tests = [
     {
 	description => 'single section, keys with quoted values, '
 	    . 'comment literals within quotes',
+	expect_cli_parse_fail => 1,
 	expected_cfg => {
 	    foo => {},
 	},
@@ -717,6 +892,71 @@ my $tests = [
     },
 ];
 
+sub test_ceph_cli {
+    my ($case) = @_;
+
+    my $desc = "ceph-conf cli: $case->{description}";
+
+    # subtest descriptions
+    my $desc_format = "ceph-conf cli (format): $case->{description}";
+    my $desc_comparison = "ceph-conf cli (comparison): $case->{description}";
+
+    my $query_result = eval {
+	return with_tmp_ceph_conf_file($case->{raw}, sub {
+	    my ($cfg_filename) = @_;
+	    return query_cfg_via_ceph_conf_cli($cfg_filename, $case->{expected_cfg});
+	});
+    };
+    die "failed to query test ceph.conf file: $@\n" if $@;
+
+    my $cli_may_fail = defined($case->{expect_cli_parse_fail}) && $case->{expect_cli_parse_fail};
+
+    subtest $desc => sub {
+	plan(tests => 2);
+
+	my $did_fail = has_parse_failure($query_result);
+
+	if ($cli_may_fail) {
+	    if ($did_fail) {
+		pass($desc_format);
+		pass($desc_comparison);
+	    } else {
+		fail($desc_format);
+
+		diag("=== Call to ceph-conf succeeded unexpectedly - did upstream change? ===");
+		diag(explain($query_result));
+
+		fail($desc_comparison);
+	    }
+
+	    return;
+	}
+
+	if ($did_fail) {
+	    fail($desc_format);
+
+	    diag("=== Call to ceph-conf failed unexpectedly - did upstream change? ===");
+	    diag(explain($query_result));
+
+	    fail($desc_comparison);
+
+	    return;
+	}
+
+	pass($desc_format);
+
+	if (!is_deeply($query_result->{cfg}, $case->{expected_cfg}, $desc_comparison)) {
+	    diag("=== Expected ===");
+	    diag(explain($case->{expected_cfg}));
+	    diag("=== Got ===");
+	    diag(explain($query_result->{cfg}));
+	    diag("=== Diagnostic Output ===");
+	    diag(explain($query_result->{section_cmd_info}));
+	    diag(explain($query_result->{key_cmd_infos}));
+	}
+    };
+}
+
 sub test_parse_ceph_config {
     my ($case) = @_;
 
@@ -771,6 +1011,7 @@ sub test_write_ceph_config {
 
 sub main {
     my $test_subs = [
+	\&test_ceph_cli,
 	\&test_parse_ceph_config,
 	\&test_write_ceph_config,
     ];
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 04/11] 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.

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

Changes v3 --> v4:
  * none

Changes v4 --> v5:
  * rebase onto changes of patch 01
  * move writing 'client.*' sections to patch 05 (of this series)

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 9934fd5..1fb467d 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -60,6 +60,7 @@ my $parse_ceph_file = sub {
 sub write_ceph_config {
     my ($filename, $cfg) = @_;
 
+    my $written_sections = {};
     my $out = '';
 
     my $cond_write_sec = sub {
@@ -67,12 +68,15 @@ sub write_ceph_config {
 
 	for my $section (sort keys $cfg->%*) {
 	    next if $section !~ m/^$re$/;
+	    next if exists($written_sections->{$section});
 
 	    $out .= "[$section]\n";
 	    for my $key (sort keys $cfg->{$section}->%*) {
 		$out .= "\t $key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
+
+	    $written_sections->{$section} = 1;
 	}
     };
 
@@ -89,6 +93,8 @@ sub write_ceph_config {
 	qr/mon\..*/,
 	qr/osd\..*/,
 	qr/mgr\..*/,
+
+	qr/.*/,
     );
 
     for my $re (@rexprs) {
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 05/11] cephconfig: change order of written sections

[Max Carrara]

in order to group related sections together.

Additionally, 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 v2 --> v3:
  * new; originally patch 07 of series v2, now adapted to this series 

Changes v3 --> v4:
  * none

Changes v4 --> v5:
  * rebase onto changes of patch 01
  * write 'client.*' sections after 'client' (moved from patch 04)

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 1fb467d..1b6e86c 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -82,16 +82,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 v5 pve-storage 06/11] cephconfig: align written key-value pairs by tab

[Max Carrara]

instead of tab + space.

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

Changes v3 --> v4:
  * rebased due to previous changes

Changes v4 --> v5: (originally patch 12)
  * rebase onto changes of patch 01
  * align by tab only instead of removing all leading whitespace
    - as it turns out, continued lines are rather rare anyways and
      aligning the keys by tab makes the configuration more readable
  * reword commit message and remove previous explanation

 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 1b6e86c..845b7d2 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -72,7 +72,7 @@ sub write_ceph_config {
 
 	    $out .= "[$section]\n";
 	    for my $key (sort keys $cfg->{$section}->%*) {
-		$out .= "\t $key = $cfg->{$section}->{$key}\n";
+		$out .= "\t$key = $cfg->{$section}->{$key}\n";
 	    }
 	    $out .= "\n";
 
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 07/11] cephconfig: escape un-escaped comment literals on write

[Max Carrara]

in order to prevent configuration errors or the configuration being
misinterpreted.

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

Changes v4 --> v5:
  * escape *all* comment literals (that are not escaped) instead of only
    escaping them within values

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 845b7d2..0def1f2 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -105,6 +105,9 @@ sub write_ceph_config {
 	$cond_write_sec->($re);
     }
 
+    # Escape comment literals that aren't escaped already
+    $out =~ s/(?<!\\)([;#])/\\$1/gm;
+
     return $out;
 }
 
-- 
2.39.2

[pve-devel] [PATCH v5 pve-storage 08/11] cephconfig: align our parser with Ceph's parser

[Max Carrara]

This commit rewrites the entire parser for ceph.conf, aligning its
behaviour as closely as possible with Ceph's parser grammar [0].

The most notable improvements are as follows:

  1. The characters '#' and ';' now both mark comments, instead of
     just the '#' character.

  2. Any character, including comment literals ('#' and ';'), may now
     be escaped.

  3. Quoted values (single and double) are now supported.

  4. Line continuations are now supported (lines ending with '\').

  5. Repeated whitespace characters in keys are now treated as a
     single space character.

  6. Dashes '-' are not treated the same as spaces and underscores
     anymore, as Ceph's grammar doesn't treat them that way.
     * Paired with 5., this means that repeated whitespace is now
       equivalent to a single underscore.

  7. Escaped comment literals are now un-escaped.

  8. Although not too crucial, the parser now also supports empty
     sections and will just initialize them with an empty hash.

Furthermore, the original grammar's more quirky behaviours are also
respected where sanely possible.

[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=e9fe820e7fffd1b7cde143a9f77653b73fcec748#l144

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

Changes v2 --> v3:
  * support comment literals

Changes v3 --> v4:
  * support empty sections
  * fix and move support for comment literals to separate patch

Changes v4 --> v5:
  * rewrite entire parsing logic in order to handle a lot more
    (edge) cases
  * support quoted values
  * support line continuations wherever Ceph's grammar supports them
    (in section headings, directly after the key, direcly after the
    equals sign of kv-pairs, within unquoted values, directly after
    quoted values, randomly within the file as long as the next line
    consists of whitespace)
  * support escaping any kind of character
  * treat repeated whitespace as single space character (like Ceph does)
  * use upfront defined regexes for readability
  * use inner `sub`s for readability
  * reword commit message

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 0def1f2..56e7989 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -10,36 +10,244 @@ cfs_register_file('ceph.conf',
 		  \&parse_ceph_config,
 		  \&write_ceph_config);
 
+# For more information on how the Ceph parser works and how its grammar is
+# defined, see:
+# https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=e9fe820e7fffd1b7cde143a9f77653b73fcec748#l144
 sub parse_ceph_config {
     my ($filename, $raw) = @_;
 
     my $cfg = {};
     return $cfg if !defined($raw);
 
-    my @lines = split /\n/, $raw;
+    # Note: According to Ceph's config grammar, a single key-value pair in a file
+    # (and nothing else!) is a valid config file and will be parsed by `ceph-conf`.
+    # We choose not to handle this case here because it doesn't seem to be used
+    # by Ceph at all (and otherwise doesn't really make sense anyway).
+
+    # Regexes ending with '_class' consist of only an extended character class
+    # each, which allows them to be interpolated into other ext. char classes.
+
+    my $re_leading_ws = qr/^\s+/;
+    my $re_trailing_ws = qr/\s+$/;
+
+    my $re_continue_marker = qr/\\/;
+    my $re_comment_class = qr/(?[ [ ; # ] ])/;
+    my $re_not_allowed_in_section_header_class = qr/(?[ [ \] ] + $re_comment_class ])/;
+
+    # Note: The Ceph config grammar defines keys the following way:
+    #
+    #     key %= raw[+(text_char - char_("=[ ")) % +blank];
+    #
+    # The ' - char_("=[ ")' expression might lure you into thinking that keys
+    # may *not* contain spaces, but they can, due to the "% +blank" at the end!
+    #
+    # See: https://www.boost.org/doc/libs/1_42_0/libs/spirit/doc/html/spirit/qi/reference/operator/list.html
+    #
+    # Allowing spaces in this class and later squeezing whitespace as well as
+    # removing any leading and trailing whitespace from keys is just so much
+    # easier in our case.
+    my $re_not_allowed_in_keys_class = qr/(?[ [  = \[  ] + $re_comment_class ])/;
+
+    my $re_not_allowed_in_single_quoted_text_class = qr/(?[ [  '  ] + $re_comment_class ])/;
+    my $re_not_allowed_in_double_quoted_text_class = qr/(?[ [  "  ] + $re_comment_class ])/;
+
+    my $re_text_char = qr/\\.|(?[ ! $re_comment_class ])/;
+    my $re_section_header_char = qr/\\.|(?[ ! $re_not_allowed_in_section_header_class ])/;
+
+    my $re_key_char = qr/\\.|(?[ ! $re_not_allowed_in_keys_class ])/;
+
+    my $re_single_quoted_text_char = qr/\\.|(?[ ! $re_not_allowed_in_single_quoted_text_class ])/;
+    my $re_double_quoted_text_char = qr/\\.|(?[ ! $re_not_allowed_in_double_quoted_text_class ])/;
+
+    my $re_single_quoted_value = qr/'(($re_single_quoted_text_char)*)'/;
+    my $re_double_quoted_value = qr/"(($re_double_quoted_text_char)*)"/;
+
+    my $re_key = qr/^(($re_key_char)+)/;
+    my $re_quoted_value = qr/$re_single_quoted_value|$re_double_quoted_value/;
+    my $re_unquoted_value = qr/(($re_text_char)*)/;
+    my $re_value = qr/($re_quoted_value|$re_unquoted_value)/;
+
+    my $re_kv_separator = qr/\s*(=)\s*/;
+
+    my $re_section_start = qr/\[/;
+    my $re_section_end = qr/\]/;
+    my $re_section_header = qr/$re_section_start(($re_section_header_char)+)$re_section_end/;
 
     my $section;
+    my @lines = split(/\n/, $raw);
+
+    my $parse_section_header = sub {
+	my ($section_line) = @_;
+
+	# continued lines in section headers are allowed
+	while ($section_line =~ s/$re_continue_marker$//) {
+	    $section_line .= shift(@lines);
+	}
+
+	my $remainder = $section_line;
+
+	$remainder =~ s/$re_section_header//;
+	my $parsed_header = $1;
+
+	# Un-escape comment literals
+	$parsed_header =~ s/\\($re_comment_class)/$1/g;
+
+	if (!$parsed_header) {
+	    die "failed to parse section - skip: $section_line\n";
+	}
+
+	# preserve Ceph's behaviour and disallow anything after the section header
+	# that's not whitespace or a comment
+	$remainder =~ s/$re_leading_ws//;
+	$remainder =~ s/^$re_comment_class.*$//;
+
+	if ($remainder) {
+	    die "unexpected remainder after section - skip: $section_line\n";
+	}
+
+	return $parsed_header;
+    };
+
+    my $parse_key = sub {
+	my ($line) = @_;
+
+	my $remainder = $line;
+
+	my $key = '';
+	while ($remainder =~ s/$re_key//) {
+	    $key .= $1;
+
+	    while ($key =~ s/$re_continue_marker$//) {
+		$remainder = shift(@lines);
+	    }
+	}
+
+	$key =~ s/$re_trailing_ws//;
+	$key =~ s/$re_leading_ws//;
+
+	$key =~ s/\s/ /;
+	while ($key =~ s/\s\s/ /) {}  # squeeze repeated whitespace
+
+	# Ceph treats *single* spaces in keys the same as underscores,
+	# but we'll just use underscores for readability
+	$key =~ s/ /_/g;
+
+	# Un-escape comment literals
+	$key =~ s/\\($re_comment_class)/$1/g;
+
+	if ($key eq '') {
+	    die "failed to parse key from line - skip: $line\n";
+	}
+
+	my $had_equals = $remainder =~ s/^$re_kv_separator//;
+
+	if (!$had_equals) {
+	    die "expected '=' after key - skip: $line\n";
+	}
+
+	while ($remainder =~ s/^$re_continue_marker$//) {
+	    # Whitespace and continuations after equals sign can be arbitrary
+	    $remainder = shift(@lines);
+	    $remainder =~ s/$re_leading_ws//;
+	}
+
+	return ($key, $remainder);
+    };
+
+    my $parse_value = sub {
+	my ($line, $remainder) = @_;
+
+	my $starts_with_quote = $remainder =~ m/^['"]/;
+	$remainder =~ s/$re_value//;
+	my $value = $1 // '';
+
+	if ($value eq '') {
+	    die "failed to parse value - skip: $line\n";
+	}
+
+	if ($starts_with_quote) {
+	    # If it started with a quote, the parsed value MUST end with a quote
+	    my $is_single_quoted = $value =~ m/$re_single_quoted_value/;
+	    $value = $1 if $is_single_quoted;
+	    my $is_double_quoted = !$is_single_quoted && $value =~ m/$re_double_quoted_value/;
+	    $value = $1 if $is_double_quoted;
 
-    foreach my $line (@lines) {
-	$line =~ s/#.*$//;
-	$line =~ s/^\s+//;
-	$line =~ s/^;.*$//;
-	$line =~ s/\s+$//;
+	    if (!($is_single_quoted || $is_double_quoted)) {
+		die "failed to parse quoted value - skip: $line\n";
+	    }
+
+	    # Optionally, *only* line continuations may *only* follow right after
+	    while ($remainder =~ s/^$re_continue_marker$//) {
+		$remainder .= shift(@lines);
+	    }
+
+	    # Nothing but whitespace or a comment may follow
+	    $remainder =~ s/$re_leading_ws//;
+	    $remainder =~ s/^$re_comment_class.*$//;
+
+	    if ($remainder) {
+		die "unexpected remainder after value - skip: $line\n";
+	    }
+
+	} else {
+	    while ($value =~ s/$re_continue_marker$//) {
+		my $next_line = shift(@lines);
+
+		$next_line =~ s/$re_unquoted_value//;
+		my $value_part = $1 // '';
+		$value .= $value_part;
+	    }
+
+	    $value =~ s/$re_trailing_ws//;
+	}
+
+	# Un-escape comment literals
+	$value =~ s/\\($re_comment_class)/$1/g;
+
+	return $value;
+    };
+
+    while (scalar(@lines)) {
+	my $line = shift(@lines);
+
+	$line =~ s/^\s*(?<!\\)$re_comment_class.*$//;
+	$line =~ s/^\s*$//;
 	next if !$line;
+	next if $line =~ m/^$re_continue_marker$/;
+
+	if ($line =~ m/$re_section_start/) {
+	    $section = undef;
+
+	    eval { $section = $parse_section_header->($line) };
+	    if ($@) {
+		warn "$@\n";
+	    }
 
-	$section = $1 if $line =~ m/^\[(\S+)\]$/;
-	if (!$section) {
-	    warn "no section - skip: $line\n";
+	    if (defined($section)) {
+		$cfg->{$section} = {} if !exists($cfg->{$section});
+	    }
+
+	    next;
+	}
+
+	if (!defined($section)) {
+	    warn "no section header - skip: $line\n";
 	    next;
 	}
 
-	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;
-	    $cfg->{$section}->{$key} = $val;
+	my ($key, $remainder) = eval { $parse_key->($line) };
+	if ($@) {
+	    warn "$@\n";
+	    next;
+	}
+
+	my $value = eval { $parse_value->($line, $remainder) };
+	if ($@) {
+	    warn "$@\n";
+	    next;
 	}
 
+	$cfg->{$section}->{$key} = $value;
     }
 
     return $cfg;
-- 
2.39.2

[pve-devel] [PATCH v5 pve-manager 09/11] 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 when updating.

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

Changes v3 --> v4:
  * none

Changes v4 --> v5:
  * use placeholder for version guard in `postinst` (see cover letter)
  * reword commit message

 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..58f2713f 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' '0.0.0'; 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 v5 pve-manager 10/11] 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'
 4. The 'client.crash' section has *no* key named 'key'

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

Changes v3 --> v4:
  * ensure key named 'key' does not exist for 'client.crash' section
    in helper script (thanks Friedrich!)
  * restart ceph-crash.service if it wasn't disabled (thanks Friedrich!)
  * add empty line before and after making changes in `update_ceph_conf`
    in 'postinst' hook
  * clean up the helper script; logic is easier to follow now
  * increase verbosity of output of helper script, showing what's
    exactly done when invoked

Changes v4 --> v5:
  * helper script: improve error handling (thanks Fabian!)
  * helper script: improve overall control flow (thanks Fabian!)
  * helper script: add comment regarding RPC env and RADOS object
  * helper script: improve overall command output and make it more
    obvious when and what something is modified
  * helper script: improve error output and differ between errors
    regarding pmxcfs and altering the configuration
  * helper script: remove unnecessary code
  * postinst: suppress output to stderr when checking whether
    'ceph-crash.service' is enabled (thanks Friedrich!)
  * postinst: ensure `deb-systemd-invoke` never fails (thanks Fabian!)

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

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 00b8f503..60c8d8dd 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -451,6 +451,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..89170b2e
--- /dev/null
+++ b/bin/pve-init-ceph-crash
@@ -0,0 +1,142 @@
+#!/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;
+
+my $ceph_cfg_file = 'ceph.conf';
+my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
+
+
+sub try_adapt_cfg {
+    my ($cfg) = @_;
+
+    my $entity = 'client.crash';
+    my $removed_key = 0;
+
+    print("Checking whether the configuration for '$entity' needs to be updated.\n");
+
+    my $add_keyring = sub {
+	print("Setting keyring path to '$keyring_value'.\n");
+	$cfg->{$entity}->{keyring} = $keyring_value;
+    };
+
+    if (!exists($cfg->{$entity})) {
+	print("Adding missing section for '$entity'.\n");
+	$add_keyring->();
+	return 1;
+    }
+
+    if (exists($cfg->{$entity}->{key})) {
+	print("Removing existing usage of key.\n");
+	delete($cfg->{$entity}->{key});
+	$removed_key = 1;
+    }
+
+    if (!exists($cfg->{$entity}->{keyring})) {
+	print("Keyring path is missing from configuration.\n");
+	$add_keyring->();
+	return 1;
+    }
+
+    my $current_keyring_value = $cfg->{$entity}->{keyring};
+    if ($current_keyring_value ne $keyring_value) {
+	print("Current keyring path differs from expected path.\n");
+	$add_keyring->();
+	return 1;
+    }
+
+    return $removed_key;
+}
+
+
+sub main {
+    # PVE::RADOS expects an active RPC Environment because it forks itself
+    # and may want to clean up after
+    my $rpcenv = PVE::RPCEnvironment->setup_default_cli_env();
+
+    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() };
+    my $ceph_crash_key_path = PVE::Ceph::Tools::get_config('pve_ceph_crash_key_path');
+
+    my $inner_err = '';
+
+    my $rval = PVE::Cluster::cfs_lock_file($ceph_cfg_file, undef, sub {
+	eval {
+	    my $cfg = PVE::Cluster::cfs_read_file($ceph_cfg_file);
+
+	    if (!defined($rados)) {
+		my $has_mon_host = defined($cfg->{global}) && defined($cfg->{global}->{mon_host});
+		if ($has_mon_host && $cfg->{global}->{mon_host} ne '') {
+		    die "Connection to RADOS failed even though a monitor is configured.\n" .
+			"Please verify whether your configuration in '$ceph_cfg_file' is correct.\n"
+		}
+
+		print(
+		    "Connection to RADOS failed and no monitor is configured in '$ceph_cfg_file'.\n".
+		    "Assuming that things are fine. No action required.\n"
+		);
+		return;
+	    }
+
+	    my $updated_keyring = PVE::Ceph::Tools::create_or_update_crash_keyring_file($rados);
+
+	    if ($updated_keyring) {
+		print("Keyring file '$ceph_crash_key_path' was updated.\n");
+	    }
+
+	    my $changed = try_adapt_cfg($cfg);
+
+	    if ($changed) {
+		print("Committing updated configuration to '$ceph_cfg_file'.\n");
+		PVE::Cluster::cfs_write_file($ceph_cfg_file, $cfg);
+		print("Successfully updated configuration for 'ceph-crash.service'.\n");
+	    } else {
+		print("Configuration in '$ceph_cfg_file' does not need to be updated.\n");
+	    }
+	};
+	$inner_err = $@;
+
+	return 1;
+    });
+
+    # cfs_lock_file sets $@ explicitly to undef
+    my $err = $@ // '';
+
+    my $has_err = !defined($rval) || $inner_err || $err;
+
+    if ($has_err) {
+	$err =~ s/\n*$//;
+	$inner_err =~ s/\n*$//;
+
+	if (!defined($rval)) {
+	    warn("Error while acquiring or releasing lock for '$ceph_cfg_file'.\n");
+	    warn("Error: $err\n") if $err ne '';
+	}
+
+	warn("Failed to configure keyring for 'ceph-crash.service'.\nError: $inner_err\n")
+	    if $inner_err ne '';
+
+	exit 1;
+    }
+}
+
+main();
diff --git a/debian/postinst b/debian/postinst
index 58f2713f..2bd71459 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -82,10 +82,23 @@ EOF
 
 update_ceph_conf() {
     CEPH_CONF_DIR='/etc/pve/ceph'
+    UNIT='ceph-crash.service'
+
+    echo ""
 
     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
+
+    if systemctl -q is-enabled "$UNIT" 2> /dev/null; then
+        deb-systemd-invoke restart "$UNIT" || true
+    fi
+
+    echo ""
 }
 
 migrate_apt_auth_conf() {
-- 
2.39.2

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

[Max Carrara]

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

Changes v3 --> v4:
  * none

Changes v4 --> v5:
  * none

 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 v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Maximiliano Sandoval]

Max Carrara <m.carrara@proxmox.com> writes:

> Fix #4759: Configure Permissions for ceph-crash.service - Version 5
> ===================================================================

I tested this patch series on a testing cluster updated to
no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing
I removed the version check against 0.0.0.

The following things were working as expected:

 - There are no more ceph-crash errors in the journal
 - /etc/pve/ceph.conf contains:
   ```
   [client.crash]
	keyring = /etc/pve/ceph/$cluster.$name.keyring
   ```
 - The new keyring is the right place at
   ```
   # ls /etc/pve/ceph
   ceph.client.crash.keyring
   ```
 - After a few minutes the crash reports at /var/lib/ceph/crash/ were
   moved to /var/lib/ceph/crash/posted.

One thing that was broken is running the ceph-crash binary directly:

```
# ceph-crash
INFO:ceph-crash:pinging cluster to exercise our key
2024-04-09T11:42:31.591+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
2024-04-09T11:42:31.595+0200 7009fca926c0 -1 monclient: keyring not found
[errno 13] RADOS permission denied (error connecting to the cluster)
INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s
```

--
Maximiliano

Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Maximiliano Sandoval]

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> Max Carrara <m.carrara@proxmox.com> writes:
>
>> Fix #4759: Configure Permissions for ceph-crash.service - Version 5
>> ===================================================================
>
> I tested this patch series on a testing cluster updated to
> no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing
> I removed the version check against 0.0.0.

I forgot to mention that `ceph status` now reports the crashes in its
health status:

```
# ceph status
  cluster:
    id:     bb91405d-a19b-49e7-b13f-a708d2e8e38c
    health: HEALTH_WARN
            20 mgr modules have recently crashed
```

and that the count gets bumped correctly with a few minutes of delay.

--
Maximiliano

Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Max Carrara]

On Tue Apr 9, 2024 at 11:48 AM CEST, Maximiliano Sandoval wrote:
>
> Max Carrara <m.carrara@proxmox.com> writes:
>
> > Fix #4759: Configure Permissions for ceph-crash.service - Version 5
> > ===================================================================
>
> I tested this patch series on a testing cluster updated to
> no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing
> I removed the version check against 0.0.0.
>
> The following things were working as expected:
>
>  - There are no more ceph-crash errors in the journal
>  - /etc/pve/ceph.conf contains:
>    ```
>    [client.crash]
> 	keyring = /etc/pve/ceph/$cluster.$name.keyring
>    ```
>  - The new keyring is the right place at
>    ```
>    # ls /etc/pve/ceph
>    ceph.client.crash.keyring
>    ```
>  - After a few minutes the crash reports at /var/lib/ceph/crash/ were
>    moved to /var/lib/ceph/crash/posted.

Thanks a lot for testing this, much appreciated!

>
> One thing that was broken is running the ceph-crash binary directly:
>
> ```
> # ceph-crash
> INFO:ceph-crash:pinging cluster to exercise our key
> 2024-04-09T11:42:31.591+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
> 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
> 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
> 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied
> 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 monclient: keyring not found
> [errno 13] RADOS permission denied (error connecting to the cluster)

That's not actually "broken" (even though it looks like it, tbh) -
that's just how Ceph rolls in this case ...

On startup `ceph-crash` will first check if the cluster is even
reachable [0]. I'm not sure why it resorts to looking up the admin
keyring first.

> INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s

Here it does actually then monitor the crash dir as expected, so it
works just fine.

The usual errors that appear every 10 minutes are otherwise silenced by
a patch on our side [1] (which were the most annoying kinds of errors
anyway).

> ```


[0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/ceph-crash.in;h=0e02837fadd4dde8abd66985b485836402e10a37;hb=HEAD#l131
[1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=patches/0017-ceph-crash-change-order-of-client-names.patch;h=8131fced55f3e4c757bd22c16539070f83480a19;hb=HEAD

>
> --
> Maximiliano
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Friedrich Weber]

On 02/04/2024 16:55, Max Carrara wrote:
> Fix #4759: Configure Permissions for ceph-crash.service - Version 5
> ===================================================================

Thanks for the v4! Consider this

Tested-by: Friedrich Weber <f.weber@proxmox.com>

Details:

- like Maximiliano, removed the version check against 0.0.0 for testing

- set up a fresh PVE 8.1 cluster (without Ceph yet) and installed the
patched pve-storage and pve-manager. Noticed that this creates
/etc/pve/ceph/ even though Ceph is not set up, but I suppose this is
intended (and the extra directory shouldn't hurt). Afterwards, set up a
Quincy cluster. Did not notice any issues. Keyring was created and Ceph
config was written correctly, ceph-crash was able to post crash reports.

- installed patched packages on top of my existing PVE 8.1 + Reef
cluster, did not notice any issues. As expected, ceph-crash was
restarted and started posting crash reports.

- did the same for an existing PVE 8.0 + Quincy cluster (as part of the
upgrade to PVE 8.1), did not notice any issues

- installed on top of a Ceph-less standalone node, did not notice any issues

[pve-devel] applied-series: [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service

[Fabian Grünbichler]

with a few small follow-ups in pve-manager

On April 2, 2024 4:55 pm, Max Carrara wrote:
> Fix #4759: Configure Permissions for ceph-crash.service - Version 5
> ===================================================================
> 
> Notable changes since v4
> ------------------------
> 
>   * The patches regarding Ceph Reef and Ceph Quincy have been applied in
>     v4 and are thus dropped from this series (thanks Thomas!)
>   * The tests for both the parser and the writer are rewritten, making
>     them independent of one another
>   * Many additional test cases
>   * New test subroutine that compares each test case against Ceph's
>     `ceph-conf` CLI in order to preemptively detect changes / regressions
>     in Ceph's config file format
>     - In total, there are now 37 test cases, each tested against 3
>       testing subroutines, making 111 tests in total
>   * The Ceph config writer now correctly escapes all un-escaped comment
>     literals
>   * The Ceph config parser is completely rewritten, making its
>     implementation almost fully equivalent to Ceph's original parser
>   * Suppress warning messages while running `postinst` of 'pve-manager'
>     (thanks Friedrich!)
>   * Prevent `deb-systemd-invoke` from failing (thanks Fabian!)
> 
> For a detailed list of changes, please see the comments in the in the
> individual patches.
> 
> Post-Installation Version Guard
> -------------------------------
> 
> The call to the new function in the `postinst` hook, which is introduced
> in patch 09, now uses '0.0.0' as placeholder in its version guard.
> 
> When applying this series, this placeholder must be replaced with the
> next expected version of the 'pve-manager' package in order to run the
> Ceph configuration helper during users' next update (patches 09, 10).
> 
> 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
> v3: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061802.html
> v4: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062093.html
> 
> Summary of Changes
> ------------------
> 
> pve-storage:
> 
> Max Carrara (8):
>   cephconfig: change code style inside config writer
>   test: add tests for 'ceph.conf' parser and writer
>   test: cephconfig: add regression test for Ceph config parser & writer
>   cephconfig: allow writing arbitrary sections
>   cephconfig: change order of written sections
>   cephconfig: align written key-value pairs by tab
>   cephconfig: escape un-escaped comment literals on write
>   cephconfig: align our parser with Ceph's parser
> 
>  debian/control                             |    1 +
>  src/Makefile                               |    1 +
>  src/PVE/CephConfig.pm                      |  282 +++++-
>  src/PVE/Makefile                           |    4 +
>  src/PVE/test/Makefile                      |    9 +
>  src/PVE/test/ceph_conf_parse_write_test.pl | 1035 ++++++++++++++++++++
>  6 files changed, 1305 insertions(+), 27 deletions(-)
>  create mode 100644 src/PVE/test/Makefile
>  create mode 100755 src/PVE/test/ceph_conf_parse_write_test.pl
> 
> 
> 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 | 142 ++++++++++++++++++++++++++++++++++++++++
>  debian/postinst         |  25 +++++++
>  6 files changed, 230 insertions(+), 3 deletions(-)
>  create mode 100755 bin/pve-init-ceph-crash
> 
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>