[pve-devel] [PATCH installer v2 5/6] sys: command: add option to not print process output to stdout
If $noprint is set, the output of the command won't be printed to stdout
of the parent process.
Fully backwards-compatible again, only takes effect if the new argument
is actually specified.
Signed-off-by: Christoph Heiss
---
Changes since v1:
* added parameter documentation
Proxmox/Sys/Command.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index 36e99c1..d145483 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -85,8 +85,9 @@ sub syscmd {
# to exit early and ignore the rest of the process output.
# * $input - Stdin contents for the spawned subprocess
# * $noout - Whether to append any process output to the return value
+# * $noprint - Whether to print any process output to the parents stdout
sub run_command {
-my ($cmd, $func, $input, $noout) = @_;
+my ($cmd, $func, $input, $noout, $noprint) = @_;
my $cmdstr;
if (!ref($cmd)) {
@@ -178,7 +179,7 @@ sub run_command {
} elsif ($h eq $error) {
$ostream .= $buf if !($noout || $func);
}
- print $buf;
+ print $buf if !$noprint;
STDOUT->flush();
log_info($buf);
}
--
2.43.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 4/6] sys: command: allow terminating the process early from log subroutine
If the logging subroutine $func returns CMD_FINISHED after processing a
line, the running subprocess is killed early.
This mechanism can be used when e.g. only a certain part of the output
of a (long-running) command is needed, avoiding the extra time it would
take the command to finish properly.
This is done in a entirely backwards-compatible way, i.e. existing
usages don't need any modification.
Signed-off-by: Christoph Heiss
---
Changes since v1:
* introduced constant for return value (thanks Thomas!)
* added documentation
Proxmox/Sys/Command.pm | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index 6389b17..36e99c1 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -15,7 +15,9 @@ use Proxmox::Log;
use Proxmox::UI;
use base qw(Exporter);
-our @EXPORT_OK = qw(run_command syscmd);
+our @EXPORT_OK = qw(run_command syscmd CMD_FINISHED);
+
+use constant CMD_FINISHED => 1;
my sub shellquote {
my $str = shift;
@@ -79,7 +81,8 @@ sub syscmd {
#
# Arguments:
# * $cmd - The command to run, either a single string or array with individual
arguments
-# * $func - Logging subroutine to call, receives both stdout and stderr
+# * $func - Logging subroutine to call, receives both stdout and stderr. Might
return CMD_FINISHED
+# to exit early and ignore the rest of the process output.
# * $input - Stdin contents for the spawned subprocess
# * $noout - Whether to append any process output to the return value
sub run_command {
@@ -163,7 +166,13 @@ sub run_command {
$logout .= $buf;
while ($logout =~ s/^([^\010\r\n]*)(\r|\n|(\010)+|\r\n)//s) {
my $line = $1;
- $func->($line) if $func;
+ if ($func) {
+ my $ret = $func->($line);
+ if (defined($ret) && $ret == CMD_FINISHED) {
+ wait_for_process($pid, kill => 1);
+ return $ostream;
+ }
+ };
}
} elsif ($h eq $error) {
--
2.43.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 1/6] low-level: initialize UI backend for 'dump-env' subcommand too
Some detection routines might try to log things and call some
Proxmox::Ui functions all the way down, so just initialize it with the
stdio backend to avoid errors.
Signed-off-by: Christoph Heiss
---
Changes since v1:
* no changes
proxmox-low-level-installer | 1 +
1 file changed, 1 insertion(+)
diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer
index d127a40..2848295 100755
--- a/proxmox-low-level-installer
+++ b/proxmox-low-level-installer
@@ -91,6 +91,7 @@ Proxmox::Log::init("/tmp/install-low-level-${cmd}.log");
my $env = Proxmox::Install::ISOEnv::get();
if ($cmd eq 'dump-env') {
+Proxmox::UI::init_stdio({}, $env);
my $out_dir = $env->{locations}->{run};
make_path($out_dir);
--
2.43.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 6/6] fix #4872: run env: use run_command() for country detection
This fixes a rather longstanding issue [0][1] with the country
detection, in that it might get completely stuck and thus hangs the
installation.
This is due how Perl, signals and line reading interacts.
A minimal reproducer, how the installer currently works, looks like
this:
```
#!/usr/bin/env perl
use strict;
use warnings;
open (my $fh, '-|', 'sleep', '1000') or die;
my $prev = alarm(2);
eval {
local $SIG{ALRM} = sub { die "timed out!\n" };
my $line;
while (defined ($line = <$fh>)) {
print "line: $line";
}
};
alarm($prev);
close($fh);
```
One might expect that this times out after 2 seconds, as specified in
`alarm(2)`. The thruth is that `$line = <$fh>` apparently prevents the
signal to go through. This then causes the installer to hang there
indefinitely, if `traceroute` never progresses - which seems to happen
on lots of (weird) networks, as evidently can be seen in the forum [1].
Proxmox::Sys::Command::run_command() handles of these weird cases, takes
care of the nitty-gritty details and - most importantly - interacts
properly with SIGALRM, so just use that instead.
This _should_ really fix that issue, but reproducing it 1:1 as part of
the installation process is _very_ hard, basically pure luck. But
rewriting the reproducer using run_command (in the exact same way that
this patch rewrites detect_country_tracing_to()) fixes the issue there,
so it's the best we can probably do.
NB: This causes that the traceroute command is now printed to the log
(as run_command() logs that by default), which we could also hide e.g.
through another parameter if wanted.
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4872
[1]
https://forum.proxmox.com/threads/proxmox-installation-trying-to-detect-country.134301/
Signed-off-by: Christoph Heiss
---
Changes since v1:
* use new CMD_FINISHED constant
Proxmox/Install/RunEnv.pm | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index c393f67..d7ccea6 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -7,6 +7,7 @@ use Carp;
use JSON qw(from_json to_json);
use Proxmox::Log;
+use Proxmox::Sys::Command qw(run_command CMD_FINISHED);
use Proxmox::Sys::File qw(file_read_firstline);
use Proxmox::Sys::Block;
use Proxmox::Sys::Net;
@@ -188,35 +189,36 @@ my sub detect_country_tracing_to : prototype($$) {
my ($ipver, $destination) = @_;
print STDERR "trying to detect country...\n";
-open(my $TRACEROUTE_FH, '-|', 'traceroute', "-$ipver", '-N', '1', '-q',
'1', '-n', $destination)
- or return undef;
+my $traceroute_cmd = ['traceroute', "-$ipver", '-N', '1', '-q', '1', '-n',
$destination];
my $geoip_bin = ($ipver == 6) ? 'geoiplookup6' : 'geoiplookup';
my $country;
-
-my $previous_alarm = alarm (10);
+my $previous_alarm;
eval {
local $SIG{ALRM} = sub { die "timed out!\n" };
- my $line;
- while (defined ($line = <$TRACEROUTE_FH>)) {
+ $previous_alarm = alarm (10);
+
+ run_command($traceroute_cmd, sub {
+ my $line = shift;
+
log_debug("DC TRACEROUTE: $line");
if ($line =~ m/^\s*\d+\s+(\S+)\s/) {
my $geoip = qx/$geoip_bin $1/;
log_debug("DC GEOIP: $geoip");
+
if ($geoip =~ m/GeoIP Country Edition:\s*([A-Z]+),/) {
$country = lc ($1);
log_info("DC FOUND: $country\n");
- last;
+ return CMD_FINISHED;
}
}
- }
+ }, undef, undef, 1);
+
};
my $err = $@;
alarm ($previous_alarm);
-close($TRACEROUTE_FH);
-
if ($err) {
die "unable to detect country - $err\n";
} elsif ($country) {
--
2.43.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 2/6] sys: command: factor out kill() + waitpid() from run_command()
This moves the kill() + waitpid() combo into a separate subroutine,
avoiding open-coding that sequence. wait_for_process() also handles
properly unkillable process (e.g. in D-state) and avoids completely
locking up the installer in such cases. See [0].
For the latter case, a timeout exists (with a default of 5 seconds) in
which to wait for the process to exit after sending an optional
TERM/KILL signal.
Also while at it, add a few basic tests for run_command().
[0] https://lists.proxmox.com/pipermail/pve-devel/2024-February/061697.html
Signed-off-by: Christoph Heiss
---
Changes since v1:
* new patch
Proxmox/Sys/Command.pm | 60 +-
test/Makefile | 5 +++-
test/run-command.pl| 35
3 files changed, 92 insertions(+), 8 deletions(-)
create mode 100755 test/run-command.pl
diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index c3e24b3..e64e0ee 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -33,12 +33,55 @@ my sub cmd2string {
return join (' ', $quoted_args->@*);
}
+# Safely for the (sub-)process specified by $pid to exit, using a timeout.
+#
+# When kill => 1 is set, at first a TERM-signal is sent to the process before
+# checking if it exited.
+# If that fails, KILL is sent to process and then up to timeout => $timeout
+# seconds (default: 5) are waited for the process to exit.
+#
+# On sucess, the exitcode of the process is returned, otherwise `undef` (aka.
+# the process was unkillable).
+my sub wait_for_process {
+my ($pid, %params) = @_;
+
+kill('TERM', $pid) if $params{kill};
+
+my $terminated = waitpid($pid, WNOHANG);
+return $? if $terminated > 0;
+
+kill('KILL', $pid) if $params{kill};
+
+my $timeout = $params{timeout} // 5;
+for (1 .. $timeout) {
+ $terminated = waitpid($pid, WNOHANG);
+ return $? if $terminated > 0;
+ sleep(1);
+}
+
+log_warn("failed to kill child pid $pid, probably stuck in D-state?\n");
+
+# We tried our best, better let the child hang in the back then completely
+# blocking installer progress .. it's a rather short-lived environment
anyway
+}
+
sub syscmd {
my ($cmd) = @_;
return run_command($cmd, undef, undef, 1);
}
+# Runs a command an a subprocess, properly handling IO via piping, cleaning up
and passing back the
+# exit code.
+#
+# If $cmd contains a pipe |, the command will be executed inside a bash shell.
+# If $cmd contains 'chpasswd', the input will be specially quoted for that
purpose.
+#
+# Arguments:
+# * $cmd - The command to run, either a single string or array with individual
arguments
+# * $func - Logging subroutine to call, receives both stdout and stderr
+# * $input - Stdin contents for the spawned subprocess
+# * $noout - Whether to append any process output to the return value
sub run_command {
my ($cmd, $func, $input, $noout) = @_;
@@ -104,8 +147,7 @@ sub run_command {
my $count = sysread ($h, $buf, 4096);
if (!defined ($count)) {
my $err = $!;
- kill (9, $pid);
- waitpid ($pid, 0);
+ wait_for_process($pid, kill => 1);
die "command '$cmd' failed: $err";
}
$select->remove($h) if !$count;
@@ -128,15 +170,19 @@ sub run_command {
&$func($logout) if $func;
-my $rv = waitpid ($pid, 0);
+my $ec = wait_for_process($pid);
-return $? if $noout; # behave like standard system();
+# behave like standard system(); returns -1 in case of errors too
+return ($ec // -1) if $noout;
-if ($? == -1) {
+if (!defined($ec)) {
+ # Don't fail completely here to let the install continue
+ warn "command '$cmdstr' failed to exit properly\n";
+} elsif ($ec == -1) {
croak "command '$cmdstr' failed to execute\n";
-} elsif (my $sig = ($? & 127)) {
+} elsif (my $sig = ($ec & 127)) {
croak "command '$cmdstr' failed - got signal $sig\n";
-} elsif (my $exitcode = ($? >> 8)) {
+} elsif (my $exitcode = ($ec >> 8)) {
croak "command '$cmdstr' failed with exit code $exitcode";
}
diff --git a/test/Makefile b/test/Makefile
index fb80fc4..ae80a94 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,8 +3,11 @@ all:
export PERLLIB=..
.PHONY: check
-check: test-zfs-arc-max
+check: test-zfs-arc-max test-run-command
.PHONY: test-zfs-arc-max
test-zfs-arc-max:
./zfs-arc-max.pl
+
+test-run-command:
+ ./run-command.pl
diff --git a/test/run-command.pl b/test/run-command.pl
new file mode 100755
index 000..7d5805e
--- /dev/null
+++ b/test/run-command.pl
@@ -0,0 +1,35 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use File::Temp;
+use Test::More;
+
+use Proxmox::Sys::Command qw(run_command CMD_FINISHED);
+use Proxmox::Sys::File qw(file_read_all);
+use Proxmox::UI;
+
+my $log_file = File::Temp->new();
+Proxmox::Log::init($log_file->filename);
+
[pve-devel] [PATCH installer v2 3/6] sys: command: handle EINTR in run_command()
Previously, the I/O loop would continue endlessly until the subprocess
exited.
This explicit handling allows run_command() to be used with e.g.
alarm().
Signed-off-by: Christoph Heiss
---
Changes since v1:
* new patch
Proxmox/Sys/Command.pm | 9 -
test/run-command.pl| 11 +++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index e64e0ee..6389b17 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -134,10 +134,17 @@ sub run_command {
$select->add($error);
my ($ostream, $logout) = ('', '', '');
+my $caught_sig;
while ($select->count) {
my @handles = $select->can_read (0.2);
+ # If we catch a signal, stop processing & clean up
+ if ($!{EINTR}) {
+ $caught_sig = 1;
+ last;
+ }
+
Proxmox::UI::process_events();
next if !scalar (@handles); # timeout
@@ -170,7 +177,7 @@ sub run_command {
&$func($logout) if $func;
-my $ec = wait_for_process($pid);
+my $ec = wait_for_process($pid, kill => $caught_sig);
# behave like standard system(); returns -1 in case of errors too
return ($ec // -1) if $noout;
diff --git a/test/run-command.pl b/test/run-command.pl
index 7d5805e..19bdce0 100755
--- a/test/run-command.pl
+++ b/test/run-command.pl
@@ -27,6 +27,17 @@ my $ret = run_command('bash -c "echo test; sleep 1000; echo
test"', sub {
});
is($ret, '', 'using CMD_FINISHED');
+# https://bugzilla.proxmox.com/show_bug.cgi?id=4872
+my $prev;
+eval {
+local $SIG{ALRM} = sub { die "timed out!\n" };
+$prev = alarm(1);
+$ret = run_command('sleep 5');
+};
+alarm($prev);
+
+is($@, "timed out!\n", 'SIGALRM interaction');
+
# Check the log for errors/warnings
my $log = file_read_all($log_file->filename);
ok($log !~ m/(WARN|ERROR): /, 'no warnings or errors logged');
--
2.43.0
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH installer v2 0/6] fix #4872: properly timeout `traceroute` command in country detection
For all the details, see patch #6. TL;DR: SIGALRM does not interrupt line reading using <>, causing the installer to hang on country detection. Fix it by using Proxmox::Sys::Command::run_command(), which properly interacts with SIGALRM. Patch #1 is a rather mundane fix for some niche cases, #2 is a small refactoring as proposed by Thomas and #3 might warrant some more thought. #4 & #5 are preparatory and to not alter existing behaviour. v1: https://lists.proxmox.com/pipermail/pve-devel/2024-February/061677.html Changes since v1: * new patches #2 (refactoring) and #3 (EINTR handling) * introduce CMD_FINISHED constant for run_command() * added function documentation Christoph Heiss (6): low-level: initialize UI backend for 'dump-env' subcommand too sys: command: factor out kill() + waitpid() from run_command() sys: command: handle EINTR in run_command() sys: command: allow terminating the process early from log subroutine sys: command: add option to not print process output to stdout fix #4872: run env: use run_command() for country detection Proxmox/Install/RunEnv.pm | 22 +- Proxmox/Sys/Command.pm | 85 - proxmox-low-level-installer | 1 + test/Makefile | 5 ++- test/run-command.pl | 46 5 files changed, 137 insertions(+), 22 deletions(-) create mode 100755 test/run-command.pl -- 2.43.0 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v8 4/7] feature #1027: virtio-fs support
Am 13.02.24 um 12:52 schrieb Markus Frank: > Thanks, > > I already moved most of the code into a new PVE/QemuServer/Virtiofs.pm > module. > Great! :) > Just an clarification & question concerning the queue-size: > > On 2024-01-31 16:02, Fiona Ebner wrote: >>> + push @$devices, '-chardev', >>> "socket,id=virtfs$i,path=/var/run/virtiofsd/vm$vmid-fs$i"; >>> + push @$devices, '-device', 'vhost-user-fs-pci,queue-size=1024' >> >> Any specific reason for queue-size=1024? Better performance than the >> default 128? > > There was problem with Windows Guests: > https://bugzilla.redhat.com/show_bug.cgi?id=1873088 > https://github.com/virtio-win/kvm-guest-drivers-windows/issues/764 > > queue-size=1024 is still in every documentation and every qemu command > for virtiofs I have seen used 1024. Good to know! Please add a comment referencing this, so people looking at the code will be aware of it and why the value was chosen. > Would it better to add a parameter to configure this queue-size? We can add a parameter if enough users with valid use cases request it. But to start out, I don't see much need for it. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v8 4/7] feature #1027: virtio-fs support
Thanks, I already moved most of the code into a new PVE/QemuServer/Virtiofs.pm module. Just an clarification & question concerning the queue-size: On 2024-01-31 16:02, Fiona Ebner wrote: + push @$devices, '-chardev', "socket,id=virtfs$i,path=/var/run/virtiofsd/vm$vmid-fs$i"; + push @$devices, '-device', 'vhost-user-fs-pci,queue-size=1024' Any specific reason for queue-size=1024? Better performance than the default 128? There was problem with Windows Guests: https://bugzilla.redhat.com/show_bug.cgi?id=1873088 https://github.com/virtio-win/kvm-guest-drivers-windows/issues/764 queue-size=1024 is still in every documentation and every qemu command for virtiofs I have seen used 1024. Would it better to add a parameter to configure this queue-size? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager] sdn: evpn: allow empty primary exit node in zone form
It's broken since
https://git.proxmox.com/?p=pve-network.git;a=commit;h=3e3cafabaf955d53c4c2d4e346bf5c3a5c6d1852
Signed-off-by: Alexandre Derumier
---
www/manager6/sdn/zones/EvpnEdit.js | 5 +
1 file changed, 5 insertions(+)
diff --git a/www/manager6/sdn/zones/EvpnEdit.js
b/www/manager6/sdn/zones/EvpnEdit.js
index a08faef2..2e792322 100644
--- a/www/manager6/sdn/zones/EvpnEdit.js
+++ b/www/manager6/sdn/zones/EvpnEdit.js
@@ -10,6 +10,11 @@ Ext.define('PVE.sdn.zones.EvpnInputPanel', {
values.type = me.type;
}
+ if (values['exitnodes-primary'] === '') {
+ delete values['exitnodes-primary'];
+ values.delete.push('exitnodes-primary');
+ }
+
return values;
},
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH pmg_docs 1/1] Consistency of GB and GiB pmg
Thanks for the catch! pushed a fix-up On Tue, 6 Feb 2024 13:20:51 +0100 Thomas Lamprecht wrote: > Am 22/01/2024 um 18:58 schrieb Stoiko Ivanov: > > applied this one to pmg-docs - huge thanks! > > > > > > On Mon, Jul 10, 2023 at 03:49:49PM +0200, Noel Ullreich wrote: > >> Since the actual system-checks are done in GiB and to stay consistent > >> with the other docs, change all GB units to GiB > >> > >> Signed-off-by: Noel Ullreich > >> --- > >> pmg-planning-deployment.adoc | 8 > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/pmg-planning-deployment.adoc b/pmg-planning-deployment.adoc > >> index 9287574..6a0083b 100644 > >> --- a/pmg-planning-deployment.adoc > >> +++ b/pmg-planning-deployment.adoc > >> @@ -110,13 +110,13 @@ Minimum System Requirements > >> > >> * CPU: 64bit (Intel EMT64 or AMD64) > >> > >> -* 2 GB RAM > >> +* 2 GiB RAM > >> > >> * Bootable CD-ROM-drive or USB boot support > >> > >> * Monitor with a minimum resolution of 1024x768 for the installation > >> > >> -* Hard disk with at least 8 GB of disk space > >> +* Hard disk with at least 8 GiB of disk space > > this is using the wrong unit though, we use the SI based GB for > disk space, as most storage vendors do, GiB is fine for memory though. > > >> > >> * Ethernet network interface card (NIC) > >> > >> @@ -127,7 +127,7 @@ Recommended System Requirements > >> * Multi-core CPU: 64bit (Intel EMT64 or AMD64), + > >> ** for use in a virtual machine, activate Intel VT/AMD-V CPU flag > >> > >> -* 4 GB RAM > >> +* 4 GiB RAM > >> > >> * Bootable CD-ROM-drive or USB boot support > >> > >> @@ -135,7 +135,7 @@ Recommended System Requirements > >> > >> * 1 Gbps Ethernet network interface card (NIC) > >> > >> -* Storage: at least 8 GB free disk space, best set up with redundancy, > >> +* Storage: at least 8 GiB free disk space, best set up with redundancy, > > same here > > >>using a hardware RAID controller with battery backed write cache > >> (``BBU'') or > >>ZFS. ZFS is not compatible with hardware RAID controllers. For best > >>performance, use enterprise-class SSDs with power loss protection. > >> -- > >> 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key
On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This commit adds the `set_ceph_crash_conf` function, which dynamically
>> adapts the host's Ceph configuration in order to allow the Ceph crash
>> module's daemon to run without elevated privileges.
>>
>> This adaptation is only performed if:
>> * Ceph is installed
>> * Ceph is configured ('/etc/pve/ceph.conf' exists)
>> * Connection to RADOS is successful
>>
>> If the above conditions are met, the function will ensure that:
>> * Ceph possesses a key named 'client.crash'
>> * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>> * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>> * The 'client.crash' section has a key named 'keyring' which
>>references '/etc/pve/ceph/ceph.client.crash.keyring'
>>
>> Furthermore, if a key named 'client.crash' already exists within the
>> cluster, it shall be reused and not regenerated. Also, the
>> configuration is not altered if the conditions above are already met.
>>
>> This way the keyring file is available as read-only in
>> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
>> Because the `ceph` user has been made part of said `www-data` group
>> [0], it may access the file without requiring any additional
>> privileges.
>>
>> Thus, the configuration for the Ceph crash daemon is safely adapted as
>> expected by PVE tooling and also shared via pmxcfs across one's
>> cluster.
>
> I still don't think this is a good idea, even a simple perl -e '..'
> invocation or two (or a small helper script that doesn't live in $PATH)
> for doing the two steps we want (initialize key if missing, lock+modify
> config if key was missing) would be better (although compared to the
> "hidden" or regular command approach, it has the downside that somebody
> might miss the calls here when refactoring),
>
> among other things the code below
> - doesn't lock /etc/pve/ceph.conf but modifies it
> - implements yet another broken parser for ceph.conf (e.g., it doesn't
> handle the stuff you fix in the perl variant in this series!)
> - duplicates constants from the perl code that risk running out of sync,
> like paths or the key profile
> - still has issues that you fixed in the perl code between v1 and v2
> (restarting services)
>
> I haven't reviewed the bash code in detail for that reason!
>
> another issue - IMHO this should be version-guarded, since any new setup
> would already gain it when setting up a monitor, and we avoid access to
> pmxcfs in the upgrade hot path which can cause problems (cluster
> non-quorate, ..).
I hadn't considered the points you mentioned above - I agree with all of them,
actually. I'll see if I can rewrite all this in Perl (would probably be easier
than BASH anyway).
As discussed off-list, instead of writing an inline Perl script, a separate
helper script (as an executable) would probably be better - a sensible place
for this script would be in '/usr/share/pve-manager/helpers'. That way we
can also point users to this script if something goes wrong during the update.
>
>>
>> [0]:
>> https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
>>
>> Signed-off-by: Max Carrara
>> ---
>> Changes v1 --> v2:
>> * fix 'keyring' key being appended to 'client.crash' section even
>> if it already exists and configured correctly
>>
>> debian/postinst | 113
>> 1 file changed, 113 insertions(+)
>>
>> diff --git a/debian/postinst b/debian/postinst
>> index 6138ef6d..267a62ae 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
>> fi
>> }
>>
>> +set_ceph_crash_conf() {
>> +PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
>> +PVE_CEPH_CONFDIR='/etc/pve/ceph'
>> +PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
>> +PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
>> +
>> +# ceph isn't installed -> nothing to do
>> +if ! which ceph > /dev/null 2>&1; then
>> +return 0
>> +fi
>> +
>> +# ceph isn't configured -> nothing to do
>> +if test ! -f "${PVE_CEPH_CONFFILE}"; then
>> +return 0
>> +fi
>> +
>> +CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile
>> crash' mgr 'profile crash' 2>&1 || true)"
>> +
>> +# ceph is installed and possibly configured, but no connection to RADOS
>> +# -> assume no monitor was created, nothing to do
>> +if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
>> +return 0
>> +fi
>> +
>> +SECTION_RE='^\[\S+\]$'
>> +CRASH_SECTION_RE='^\[client\.crash\]$'
>> +
>> +if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
>> +DO_RESTART_UNIT=0
>> +CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E
>> 's/^\s+key\s+=\s+//')"
>> +
>> +if test ! -d
Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
On 2/12/24 14:34, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> when creating the cluster's first monitor.
>>
>> Signed-off-by: Max Carrara
>> ---
>> Changes v1 --> v2:
>> * do not enable/restart `ceph-crash` anymore when creating first mon
>> * drop changes to function `ceph_service_cmd` as they are no longer
>> needed
>> * create keyring for `ceph-crash` before modifying 'ceph.conf'
>> * always set keyring for 'client.crash' section instead of only
>> if section doesn't exist already
>> * only modify the keyring file in `get_or_create_crash_keyring()`
>> if the content differs from the output of `ceph auth get-or-create`
>
> you kinda ignored my comment about this adding yet another create keyring
> helper ;)
Saw your other reply - will disregard this as noted ;)
>
>> PVE/API2/Ceph/MON.pm | 17 -
>> PVE/Ceph/Tools.pm| 39 +++
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
>> index 1e959ef3..ae12a2d3 100644
>> --- a/PVE/API2/Ceph/MON.pm
>> +++ b/PVE/API2/Ceph/MON.pm
>> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>> });
>> die $@ if $@;
>> # automatically create manager after the first monitor is created
>> +# and set up keyring and config for ceph-crash.service
>> if ($is_first_monitor) {
>> PVE::API2::Ceph::MGR->createmgr({
>> node => $param->{node},
>> id => $param->{node}
>> -})
>> +});
>> +
>> +eval {
>> +PVE::Ceph::Tools::get_or_create_crash_keyring();
>
> this is called get_or_create, but it actually returns the path to, not
> the key(ring).. nothing uses the return value anyway, so it could also
> be called differently I guess and not return anything, but just from the
> name, I'd expect the key to be returned.
I agree; I initially wanted this helper to be similar to
`get_or_create_ceph_admin_keyring`,
but in this case I'll just unify the helper(s) once and for all in v3.
No need to beat around the bush if I'm gonna unify them anyway, so might
as well do it sooner than later ;)
>
>> +};
>> +warn "Unable to configure keyring for ceph-crash.service: $@"
>> if $@;
>> +
>> +PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
>> +my $cfg = cfs_read_file('ceph.conf');
>> +
>> +$cfg->{'client.crash'}->{keyring} =
>> '/etc/pve/ceph/$cluster.$name.keyring';
>
> I guess this doesn't make anything worse (since we starting from
> "ceph-crash is totally broken"), but if querying or creating the keyring
> failed, does it make sense to reference it in the config?
I guess in this case it doesn't, but we would need some kind of way to
re-init this part of the configuration in case the 'keyring' key doesn't
get set here.
Originally I planned to do this in a separate series to keep the scope of
this series focused on bug #4759, so perhaps it's best (at least for the
time being) to just not set the 'keyring' at all at the moment if querying
or creating the crash key fails.
So, I would change it to that in v3 and then later on supply a separate
series for the 're-init' part, if that's alright.
>
>> +
>> +cfs_write_file('ceph.conf', $cfg);
>> +});
>> +die $@ if $@;
>
> we could move this whole part up to where we do the monitor changes, and
> only lock and read and write the config once..
Good point, will be done in v3.
Thanks again for the thorough reviews! :)
>
>> }
>> };
>>
>> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
>> index 273a3eb6..02a932e3 100644
>> --- a/PVE/Ceph/Tools.pm
>> +++ b/PVE/Ceph/Tools.pm
>> @@ -18,7 +18,9 @@ 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_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";
>> @@ -37,12 +39,14 @@ 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,
>> };
>>
>> 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,
>> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>> return
Re: [pve-devel] [PATCH v2 pve-storage 07/11] amend! cephconfig: allow writing arbitrary sections
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> cephconfig: allow writing arbitrary sections
>>
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Sections associated with 'mds', 'mon', 'osd' and 'mgr' are also
>> written directly after their associated section.
>>
>> Signed-off-by: Max Carrara
>> ---
>> NOTE: This amend really just changes the order of the sections below and
>> may be dropped if not desired.
>
> does the order matter at all? the docs would imply that it doesn't,
> since there is a clear precedence between [global], [$type], and
> [$type.$id] (e.g., global -> client -> client.crash)..
No, it doesn't - I perhaps should've worded this a little more clearly;
this change is purely cosmetic and just makes it so each '[$type]' is followed
by its respective '[$type.$id]'.
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/#configuration-sections
>
>>
>> Changes v1 --> v2:
>> * new
>>
>> src/PVE/CephConfig.pm | 11 +++
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 86d3079..00b1f35 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -82,17 +82,20 @@ sub write_ceph_config {
>> };
>>
>> &$cond_write_sec('global');
>> +
>> &$cond_write_sec('client');
>> &$cond_write_sec('client\..*');
>>
>> &$cond_write_sec('mds');
>> -&$cond_write_sec('mon');
>> -&$cond_write_sec('osd');
>> -&$cond_write_sec('mgr');
>> -
>> &$cond_write_sec('mds\..*');
>> +
>> +&$cond_write_sec('mon');
>> &$cond_write_sec('mon\..*');
>> +
>> +&$cond_write_sec('osd');
>> &$cond_write_sec('osd\..*');
>> +
>> +&$cond_write_sec('mgr');
>> &$cond_write_sec('mgr\..*');
>>
>> &$cond_write_sec('.*');
>> --
>> 2.39.2
>>
>>
>>
>> ___
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 4/6] ipam: phpipam: fix get_ip_from_mac
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index f3f22b5..bb9f322 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -215,8 +215,11 @@ sub get_ips_from_mac {
my $ip4 = undef;
my $ip6 = undef;
-
-my $ips = PVE::Network::SDN::api_request("GET",
"$url/addresses/search_mac/$mac", $headers);
+my $ips = undef;
+eval {
+ $ips = PVE::Network::SDN::api_request("GET",
"$url/addresses/search_mac/$mac", $headers);
+};
+return if $@;
#fixme
die "parsing of result not yet implemented";
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 1/6] ipams : add_next_freeip : return ip not cidr
we want same result than add_next_free_range
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 13 -
src/PVE/Network/SDN/Ipams/PVEPlugin.pm | 2 +-
src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 2 +-
3 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
index 91010bb..14a69d9 100644
--- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
@@ -151,17 +151,15 @@ sub add_next_freeip {
my $params = { dns_name => $hostname, description => $description };
-my $ip = undef;
eval {
my $result = PVE::Network::SDN::api_request("POST",
"$url/ipam/prefixes/$internalid/available-ips/", $headers, $params);
- $ip = $result->{address};
+ my ($ip, undef) = split(/\//, $result->{address});
+ return $ip;
};
if ($@) {
die "can't find free ip in subnet $cidr: $@" if !$noerr;
}
-
-return $ip;
}
sub add_range_next_freeip {
@@ -176,19 +174,16 @@ sub add_range_next_freeip {
my $params = { dns_name => $data->{hostname}, description => $description
};
-my $ip = undef;
eval {
my $result = PVE::Network::SDN::api_request("POST",
"$url/ipam/ip-ranges/$internalid/available-ips/", $headers, $params);
- $ip = $result->{address};
+ my ($ip, undef) = split(/\//, $result->{address});
print "found ip free $ip in range
$range->{'start-address'}-$range->{'end-address'}\n" if $ip;
+ return $ip;
};
if ($@) {
die "can't find free ip in range
$range->{'start-address'}-$range->{'end-address'}: $@" if !$noerr;
}
-
-return $ip;
-
}
sub del_ip {
diff --git a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
index 270fb04..651acfb 100644
--- a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
@@ -176,7 +176,7 @@ sub add_next_freeip {
});
die "$@" if $@;
-return "$freeip/$mask";
+return $freeip;
}
sub add_range_next_freeip {
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index 1b7b666..7b3168d 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -181,7 +181,7 @@ sub add_next_freeip {
die "can't find free ip in subnet $cidr: $@" if !$noerr;
}
-return "$ip/$mask" if $ip && $mask;
+return $ip;
}
sub del_ip {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 0/6] external ipams fixes
multiples ipam fixes v2: add netbox ipam ip_is_gateway fix Alexandre Derumier (6): ipams : add_next_freeip : return ip not cidr sdn: add proxy support for api calls ipam: phpipam: fix subnet create ipam: phpipam: fix get_ip_from_mac ipam: phpipam: add_range_next_freeip ipam: netbox : fix ip_is_gateway src/PVE/Network/SDN.pm | 7 +++ src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 15 +- src/PVE/Network/SDN/Ipams/PVEPlugin.pm | 2 +- src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 24 ++ 4 files changed, 29 insertions(+), 19 deletions(-) -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 6/6] ipam: netbox : fix ip_is_gateway
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
index 14a69d9..d923269 100644
--- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
@@ -283,7 +283,7 @@ sub get_ip_id {
sub is_ip_gateway {
my ($url, $ip, $headers) = @_;
-my $result = PVE::Network::SDN::api_request("GET",
"$url/addresses/search/$ip", $headers);
+my $result = PVE::Network::SDN::api_request("GET",
"$url/ipam/ip-addresses/?q=$ip", $headers);
my $data = @{$result->{data}}[0];
my $description = $data->{description};
my $is_gateway = 1 if $description eq 'gateway';
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 5/6] ipam: phpipam: add_range_next_freeip
Currently is not possible in phpipam to search in specific range,
fallback to full subnet search
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 12
1 file changed, 12 insertions(+)
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index bb9f322..1c6159c 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -185,6 +185,18 @@ sub add_next_freeip {
return $ip;
}
+sub add_range_next_freeip {
+my ($class, $plugin_config, $subnet, $range, $data, $noerr) = @_;
+
+#not implemented in phpipam, we search in the full subnet
+
+my $vmid = $data->{vmid};
+my $mac = $data->{mac};
+my $hostname = $data->{hostname};
+
+return $class->add_next_freeip($plugin_config, undef, $subnet, $hostname,
$mac, $vmid);
+}
+
sub del_ip {
my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 3/6] ipam: phpipam: fix subnet create
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index 7b3168d..f3f22b5 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -51,7 +51,8 @@ sub add_subnet {
my $headers = ['Content-Type' => 'application/json; charset=UTF-8',
'Token' => $token];
#search subnet
-my $internalid = get_prefix_id($url, $cidr, $headers);
+my $internalid;
+eval { $internalid = get_prefix_id($url, $cidr, $headers) };
#create subnet
if (!$internalid) {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-network 2/6] sdn: add proxy support for api calls
Signed-off-by: Alexandre Derumier
---
src/PVE/Network/SDN.pm | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
index 3af09b5..b8f27d9 100644
--- a/src/PVE/Network/SDN.pm
+++ b/src/PVE/Network/SDN.pm
@@ -264,10 +264,9 @@ sub api_request {
my $req = HTTP::Request->new($method,$url, $headers, $encoded_data);
my $ua = LWP::UserAgent->new(protocols_allowed => ['http', 'https'],
timeout => 30);
-my $proxy = undef;
-
-if ($proxy) {
-$ua->proxy(['http', 'https'], $proxy);
+my $dccfg = PVE::Cluster::cfs_read_file('datacenter.cfg');
+if ($dccfg->{http_proxy}) {
+ $ua->proxy(['http', 'https'], $dccfg->{http_proxy});
} else {
$ua->env_proxy;
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> This adds support for writing arbitrary sections to 'ceph.conf' while
>> ensuring that already written sections are not duplicated.
>>
>> Sections that are associated with the client, for example
>> '[client.foo]', are written directly after the '[client]' section.
>>
>> Signed-off-by: Max Carrara
>
> Reviewed-by: Fabian Grünbichler
>
> would have been easier to parse if the style cleanup and actual
> behaviour change would have been split..
Fair point, acknowledged!
>
> style change:
>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..34c3107 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -65,10 +65,10 @@ 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";
>
> actual change (small nits inline!):
>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 34c3107..24bc78c 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -60,23 +60,30 @@ my $parse_ceph_file = sub {
>> sub write_ceph_config {
>> my ($filename, $cfg) = @_;
>>
>> +my $written_sections = {};
>> my $out = '';
>>
>> my $cond_write_sec = sub {
>> my $re = shift;
>>
>> for my $section (sort keys %$cfg) {
>> -next if $section !~ m/^$re$/;
>> +if ($section !~ m/^$re$/ || exists($written_sections->{$section})) {
>> +next;
>> +}
>
> these two could be more clearly written as
>
> next if $written_sections->{section};
> next if $section !~ m/$re$/;
>
> since the two checks are not related in any way, other than both having
> the same effect if true (skipping that section). the second line is then
> unchanged, and both the diff and the resulting code is easier to parse
> IMHO.
>
>> +
>> $out .= "[$section]\n";
>> for my $key (sort keys %{$cfg->{$section}}) {
>> -$out .= "\t $key = $cfg->{$section}->{$key}\n";
>> +$out .= "\t$key = $cfg->{$section}->{$key}\n";
>
> this part here is not mentioned at all, and I might have missed it if I
> hadn't split the diffs ;)
Mea culpa!
Will clean this up in v3 and split the changes accordingly.
>
>> }
>> $out .= "\n";
>> +
>> +$written_sections->{$section} = 1;
>> }
>> };
>>
>> &$cond_write_sec('global');
>> &$cond_write_sec('client');
>> +&$cond_write_sec('client\..*');
>>
>> &$cond_write_sec('mds');
>> &$cond_write_sec('mon');
>> @@ -88,6 +95,8 @@ sub write_ceph_config {
>> &$cond_write_sec('osd\..*');
>> &$cond_write_sec('mgr\..*');
>>
>> +&$cond_write_sec('.*');
>> +
>> return $out;
>> }
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser
On 2/12/24 14:33, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> 1. Comments, irrespective of whether they start with '#' or ';' are
>> now treated the same. Otherwise, sections and key-value pairs with
>> a trailing comment starting with ';' are still parsed. Consider
>> this example:
>>
>> [some.section] # inline comment after section
>> foo = bar ; inline comment after value
>>
>> The '[some.section]' section in the example above would otherwise
>> not be parsed at all, while in the key-value definition 'foo'
>> parses as the key, which is correct, but 'bar ; inline comment
>> after value' parses as value, which is incorrect according to
>> Ceph's grammar [0][1].
>>
>> 2. Sections may now contain any character, including whitespace, but
>> not '\n' or a comment literal '#' or ';'. The case for comment
>> literals is handled in 1. above.
>
> these seem sensible - what about line continuations? ;)
Yeah, those get swallowed. Will be corrected in v3, thanks for pointing
this out!
>
>>
>> 3. Instead of treating '-', '_' and ' ' as the same, only '_' and ' '
>> are treated the same, like in Ceph's parser [2].
>
> the ceph docs state something else - which is wrong? ;)
>
> https://docs.ceph.com/en/latest/rados/configuration/ceph-conf/ says:
>
>> Each of the Ceph configuration options has a unique name that consists
>> of words formed with lowercase characters and connected with
>> underscore characters (_).
>
> this would seem to agree
>
>> When option names are specified on the command line, underscore (_)
>> and dash (-) characters can be used interchangeably (for example,
>> --mon-host is equivalent to --mon_host).
>
> okay, this is CLI which might just have its own mapping
>
>> When option names appear in configuration files, spaces can also be
>> used in place of underscores or dashes. However, for the sake of
>> clarity and convenience, we suggest that you consistently use
>> underscores, as we do throughout this documentation.
>
> but this now says that dash in config files is OK?
The docs and the code don't seem to agree in that regard; at least the
INI parser in C++ does *not* treat `-` the same [0]:
> /* Normalize a key name.
> *
> * Normalized key names have no leading or trailing whitespace, and all
> * whitespace is stored as underscores. The main reason for selecting this
> * normal form is so that in common/config.cc, we can use a macro to stringify
> * the field names of md_config_t and get a key in normal form.
> */
(The code also actually does what it says on the tin. ;) )
So, I think it would be sensible to stick to how the Code does it, in this
case. Or was the original treatment of dashes in our code included in order
to guard against user errors, perhaps?
[0]:
https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>
>> [0]:
>> https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l178
>> [1]:
>> https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l194
>> [2]:
>> https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master#l294
>>
>> Signed-off-by: Max Carrara
>> ---
>> Changes v1 --> v2:
>> * new
>>
>> src/PVE/CephConfig.pm | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..77b745f 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -10,6 +10,8 @@ cfs_register_file('ceph.conf',
>>\_ceph_config,
>>\_ceph_config);
>>
>> +# For more details on how Ceph's config parser works, see:
>> +#
>> https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/common/ConfUtils.cc;h=2f78fd02bf9e27467275752e6f3bca0c5e3946ce;hb=refs/heads/master
>> sub parse_ceph_config {
>> my ($filename, $raw) = @_;
>>
>> @@ -20,14 +22,13 @@ sub parse_ceph_config {
>>
>> my $section;
>>
>> -foreach my $line (@lines) {
>> -$line =~ s/#.*$//;
>> +for my $line (@lines) {
>> +$line =~ s/(#|;).*$//;
>> $line =~ s/^\s+//;
>> -$line =~ s/^;.*$//;
>> $line =~ s/\s+$//;
>> next if !$line;
>>
>> -$section = $1 if $line =~ m/^\[(\S+)\]$/;
>> +$section = $1 if $line =~ m/^\[(.+)\]$/;
>> if (!$section) {
>> warn "no section - skip: $line\n";
>> next;
>> @@ -35,11 +36,10 @@ sub parse_ceph_config {
>>
>> if ($line =~ m/^(.*?\S)\s*=\s*(\S.*)$/) {
>> my ($key, $val) = ($1, $2);
>> -# ceph treats ' ', '_' and '-' in keys the same, so lets do too
>> -$key =~ s/[-\ ]/_/g;
>> +# ceph treats ' ' and '_' in keys
Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook
On 2/12/24 14:32, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
>> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>>
>> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>>
>> Signed-off-by: Max Carrara
>> ---
>> Changes v1 --> v2:
>> * use `find` instead of for-loop
>>
>> ...rmissions-of-subdirectories-of-var-l.patch | 50 +++
>> patches/series| 1 +
>> 2 files changed, 51 insertions(+)
>> create mode 100644
>> patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>>
>> diff --git
>> a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> new file mode 100644
>> index 0..7445f3945
>> --- /dev/null
>> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> @@ -0,0 +1,50 @@
>> +From Mon Sep 17 00:00:00 2001
>> +From: Max Carrara
>> +Date: Thu, 1 Feb 2024 18:43:36 +0100
>> +Subject: [PATCH] debian: adjust permissions of subdirectories of
>> /var/lib/ceph
>> +
>> +A rather recent PR made ceph-crash run as "ceph" user instead of
>> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
>> +ceph-crash cannot actually post any crash logs now.
>> +
>> +This commit fixes this by also updating the permissions of
>> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
>> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
>> +
>> +[0]: https://github.com/ceph/ceph/pull/48713
>> +
>> +Signed-off-by: Max Carrara
>> +---
>> + debian/ceph-base.postinst | 16 +---
>> + 1 file changed, 9 insertions(+), 7 deletions(-)
>> +
>> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
>> +index 75eeb59c624..70d07977f82 100644
>> +--- a/debian/ceph-base.postinst
>> b/debian/ceph-base.postinst
>> +@@ -33,13 +33,15 @@ case "$1" in
>> +rm -f /etc/init/ceph.conf
>> +[ -x /sbin/start ] && start ceph-all || :
>> +
>> +-# adjust file and directory permissions
>> +- for DIR in /var/lib/ceph/* ; do
>> +- if ! dpkg-statoverride --list $DIR >/dev/null
>> +- then
>> +- chown $SERVER_USER:$SERVER_GROUP $DIR
>> +- fi
>> +- done
>> ++ PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown
>> ${SERVER_USER}:${SERVER_GROUP} {}"
>
> this doesn't quote {} properly, files with spaces or other things in
> them can cause issues including shell command injection as root when
> this is executed by dpkg..
Noted, will be corrected in v3!
>
>> ++
>> ++ # adjust directory permissions
>> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
>> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> ++
>> ++ # adjust file permissions
>> ++ find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
>> ++ | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> + ;;
>
> do we need the depth stuff or the split by type? we want to make
> everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
> specifically overriden a particular path.
I added `-mindepth 1` because the '/var/lib/ceph' dir would be included in
`find`'s results otherwise, which isn't the case in the original code.
`-maxdepth 2` is to limit it to the depth that I had originally intended in
v1 - files and subdirectories, as well as sub-subdirectories and files in
those.
I agree that everything (in those two levels?) could just be `chown`ed
instead though - would make the above a little less verbose and only
need one invocation.
>
> a simple
>
> find /var/lib/ceph -print0 | ...
>
> should work and be much simpler (or if we want to limit to dirs and
> files, that can also be simply done in one go by ORing the two checks)
ORing the two checks didn't work in my case - turns out I just held `find`
wrongly.
Will correct all the above in v3 - thanks for your feedback!
>
>> + abort-upgrade|abort-remove|abort-deconfigure)
>> +:
>> +--
>> +2.39.2
>> +
>> diff --git a/patches/series b/patches/series
>> index 865caf23d..cf8f1ea31 100644
>> --- a/patches/series
>> +++ b/patches/series
>> @@ -12,3 +12,4 @@
>> 0012-backport-mgr-dashboard-simplify-authentication-proto.patch
>> 0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>> 0014-rocksb-inherit-parent-cmake-cxx-flags.patch
>> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> --
>> 2.39.2
>>
>>
>>
>> ___
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
>
