* [pve-devel] [PATCH v5 pve-storage 01/11] cephconfig: change code style inside config writer
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 02/11] test: add tests for 'ceph.conf' parser and writer Max Carrara
` (12 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 02/11] test: add tests for 'ceph.conf' parser and writer
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 01/11] cephconfig: change code style inside config writer Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 03/11] test: cephconfig: add regression test for Ceph config parser & writer Max Carrara
` (11 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 03/11] test: cephconfig: add regression test for Ceph config parser & writer
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 01/11] cephconfig: change code style inside config writer Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 02/11] test: add tests for 'ceph.conf' parser and writer Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 04/11] cephconfig: allow writing arbitrary sections Max Carrara
` (10 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 04/11] cephconfig: allow writing arbitrary sections
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (2 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 03/11] test: cephconfig: add regression test for Ceph config parser & writer Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 05/11] cephconfig: change order of written sections Max Carrara
` (9 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 05/11] cephconfig: change order of written sections
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (3 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 04/11] cephconfig: allow writing arbitrary sections Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 06/11] cephconfig: align written key-value pairs by tab Max Carrara
` (8 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 06/11] cephconfig: align written key-value pairs by tab
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (4 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 05/11] cephconfig: change order of written sections Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 07/11] cephconfig: escape un-escaped comment literals on write Max Carrara
` (7 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 07/11] cephconfig: escape un-escaped comment literals on write
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (5 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 06/11] cephconfig: align written key-value pairs by tab Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 08/11] cephconfig: align our parser with Ceph's parser Max Carrara
` (6 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-storage 08/11] cephconfig: align our parser with Ceph's parser
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (6 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 07/11] cephconfig: escape un-escaped comment literals on write Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 09/11] ceph: introduce '/etc/pve/ceph' Max Carrara
` (5 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-manager 09/11] ceph: introduce '/etc/pve/ceph'
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (7 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-storage 08/11] cephconfig: align our parser with Ceph's parser Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 10/11] fix #4759: ceph: configure ceph-crash.service and its key Max Carrara
` (4 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-manager 10/11] fix #4759: ceph: configure ceph-crash.service and its key
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (8 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 09/11] ceph: introduce '/etc/pve/ceph' Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 11/11] bin/make: gather helper scripts in separate variable Max Carrara
` (3 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH v5 pve-manager 11/11] bin/make: gather helper scripts in separate variable
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (9 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 10/11] fix #4759: ceph: configure ceph-crash.service and its key Max Carrara
@ 2024-04-02 14:55 ` Max Carrara
2024-04-09 9:48 ` [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Maximiliano Sandoval
` (2 subsequent siblings)
13 siblings, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-02 14:55 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (10 preceding siblings ...)
2024-04-02 14:55 ` [pve-devel] [PATCH v5 pve-manager 11/11] bin/make: gather helper scripts in separate variable Max Carrara
@ 2024-04-09 9:48 ` Maximiliano Sandoval
2024-04-09 9:55 ` Maximiliano Sandoval
2024-04-09 10:28 ` Max Carrara
2024-04-10 11:45 ` Friedrich Weber
2024-04-11 12:59 ` [pve-devel] applied-series: " Fabian Grünbichler
13 siblings, 2 replies; 17+ messages in thread
From: Maximiliano Sandoval @ 2024-04-09 9:48 UTC (permalink / raw)
To: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
2024-04-09 9:48 ` [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Maximiliano Sandoval
@ 2024-04-09 9:55 ` Maximiliano Sandoval
2024-04-09 10:28 ` Max Carrara
1 sibling, 0 replies; 17+ messages in thread
From: Maximiliano Sandoval @ 2024-04-09 9:55 UTC (permalink / raw)
To: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
2024-04-09 9:48 ` [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Maximiliano Sandoval
2024-04-09 9:55 ` Maximiliano Sandoval
@ 2024-04-09 10:28 ` Max Carrara
1 sibling, 0 replies; 17+ messages in thread
From: Max Carrara @ 2024-04-09 10:28 UTC (permalink / raw)
To: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (11 preceding siblings ...)
2024-04-09 9:48 ` [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Maximiliano Sandoval
@ 2024-04-10 11:45 ` Friedrich Weber
2024-04-11 12:59 ` [pve-devel] applied-series: " Fabian Grünbichler
13 siblings, 0 replies; 17+ messages in thread
From: Friedrich Weber @ 2024-04-10 11:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Max Carrara
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] applied-series: [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
2024-04-02 14:55 [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service Max Carrara
` (12 preceding siblings ...)
2024-04-10 11:45 ` Friedrich Weber
@ 2024-04-11 12:59 ` Fabian Grünbichler
13 siblings, 0 replies; 17+ messages in thread
From: Fabian Grünbichler @ 2024-04-11 12:59 UTC (permalink / raw)
To: Proxmox VE development discussion
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
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread