Re: [pve-devel] [PATCH docs 5/9] package-repos: align wording with pmg-docs

2024-03-05 Thread Gabriel Goller
On Mon Mar 4, 2024 at 2:22 PM CET, Christoph Heiss wrote:
>  The `root@pam` user is notified via email about available updates. Click the
>  'Changelog' button in the GUI to see more details about the selected update.
>  
> -You need a valid subscription key to access the `pve-enterprise` repository.
> -Different support levels are available. Further details can be found at
> -{pricing-url}.
> +As soon as updates are available, the `root@pam` user is notified via email
> +about the newly available packages. From the GUI, the 'Changelog' button in 
> the
> +GUI can be used to see more details about the selected update.
> +Thus, you will never miss important security fixes.

The section about the 'Changelog' button is duplicated, it is already in
the paragraph above.
Also `GUI` appears twice in the same sentence, I would propose something
like this (if we use the second sentence): 

The 'Changelog' button in the GUI can be used to see more 
details about the selected update.




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



Re: [pve-devel] [PATCH docs 6/9] installation: iso: improve & align wording with pmg-docs

2024-03-05 Thread Gabriel Goller
On Mon Mar 4, 2024 at 2:22 PM CET, Christoph Heiss wrote:
> +When copying and setting up the packages has finished, you can reboot the
> +server. This will be done automatically after a few seconds by default.

I would remove the `by default` so that the sentence becomes:

This will be done automatically after a few seconds.

> +. Log in using the `root` (realm 'PAM') username and the password chosen 
> during
> +installation.

I think you need to fix the indentation here...



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



Re: [pve-devel] [PATCH pve-manager] ui: lxc: add firewall log view filtering

2024-03-05 Thread Christian Ebner

ping, still applies!


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



Re: [pve-devel] [PATCH storage 1/1] storage/plugins: pass scfg to parse_volname

2024-03-05 Thread Roland Kammerer via pve-devel
--- Begin Message ---
On Fri, Mar 01, 2024 at 11:14:31AM +0100, Roland Kammerer wrote:
> On Fri, Mar 01, 2024 at 10:45:37AM +0100, Dietmar Maurer wrote:
> > 
> > > On 29.2.2024 16:09 CET Roland Kammerer via pve-devel 
> > >  wrote:
> > > All in all, yes, this is specific for our use case, otherwise
> > > parse_volname would already have that additional parameter as all the
> > > other plugin functions, but I don't see where this would hurt existing
> > > code, and it certainly helps us to enable reassigning disks to VMs.
> > > Passing in a param all other functions already get access to also does
> > > not sound too terrible to me.
> > > 
> > > If there are further questions please feel free to ask.
> > 
> > Are you aware that parse_volname() is sometimes called for
> > all volumes, i.e inside volume_is_base_and_used().
> > 
> > Would that be fast enough? IMHO its a bad idea to make a network query for 
> > each volume there...
> 
> Thanks for mentioning that, I saw that in my tests as well. We already
> have infrastructure for persistent caches on the local file system in
> the plugin for status(), which is also called often on all nodes and
> which would otherwise hammer down the central LINSTOR controller with
> too many requests.
> 
> From what I saw the calls to parse_volname() usually happen in bursts,
> my current implementation uses our existing cache infrastructure to
> implement a "burst cache" valid for some seconds. I'd assume that is
> then good enough, being one network request per burst.

Dear PVE-devs,

it has been some days without further feedback, can this get merged
then?

Regards, rck

--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


Re: [pve-devel] [PATCH storage 1/1] storage/plugins: pass scfg to parse_volname

2024-03-05 Thread Thomas Lamprecht
Am 23/02/2024 um 10:24 schrieb Roland Kammerer:
> This passes the well known $scfg to parse_volname and bumps the API
> versions accordingly. This allows plugins to access their configuration
> if necessary.

We discussed this another time here and effectively it can be fine, while
the need for it seems like a slight smell from our architecture POV, as
it basically always means that the VMID -> volume mapping is not encoded
in the name any more (worse UX), it still does not hurt our, or other
external, existing plug-ins.

So fine to add, but please also the parameter also to the base
"parse_volname" method including a comment that mentions that this
is in general not used and only required if the storage cannot
provide all required information in the volume name.
 > Signed-off-by: Roland Kammerer 
> ---
>  ApiChangeLog   |  7 +++
>  src/PVE/Storage.pm | 18 +-
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 98b5893..042e13b 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -6,6 +6,13 @@ without breaking anything unaware of it.)
>  
>  Future changes should be documented in here.
>  
> +##  Version 11:
> +
> +* `parse_volname` got additional `$scfg` parameter
> +
> +  This allows plugins to use information from their configuration.
> +  As this is the last, additional parameter APIAGE is not reset.
> +
>  ##  Version 10:
>  
>  * a new `rename_volume` method has been added
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 0beabbc..3ac4f1e 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -41,11 +41,11 @@ use PVE::Storage::PBSPlugin;
>  use PVE::Storage::BTRFSPlugin;
>  
>  # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> +use constant APIVER => 11;
>  # Age is the number of versions we're backward compatible with.
>  # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>  # see 
> https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 1;
> +use constant APIAGE => 2;
>  

unrelated, but wondering if we could rework the approach how we warn about
to old plugins somewhat, as currently this is IMO rather to noisy, especially
for a change like this one.


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



Re: [pve-devel] [PATCH docs 5/9] package-repos: align wording with pmg-docs

2024-03-05 Thread Christoph Heiss
On Tue, Mar 05, 2024 at 09:28:21AM +0100, Gabriel Goller wrote:
> On Mon Mar 4, 2024 at 2:22 PM CET, Christoph Heiss wrote:
> >  The `root@pam` user is notified via email about available updates. Click 
> > the
> >  'Changelog' button in the GUI to see more details about the selected 
> > update.
> >
> > -You need a valid subscription key to access the `pve-enterprise` 
> > repository.
> > -Different support levels are available. Further details can be found at
> > -{pricing-url}.
> > +As soon as updates are available, the `root@pam` user is notified via email
> > +about the newly available packages. From the GUI, the 'Changelog' button 
> > in the
> > +GUI can be used to see more details about the selected update.
> > +Thus, you will never miss important security fixes.
>
> The section about the 'Changelog' button is duplicated, it is already in
> the paragraph above.
> Also `GUI` appears twice in the same sentence, I would propose something
> like this (if we use the second sentence):
>
> The 'Changelog' button in the GUI can be used to see more
> details about the selected update.
>

Yep, both paragraphs can indeed be a bit de-duplicated, will do!

Thanks!


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



Re: [pve-devel] [PATCH docs 6/9] installation: iso: improve & align wording with pmg-docs

2024-03-05 Thread Christoph Heiss
Thanks for the review!

On Tue, Mar 05, 2024 at 09:38:02AM +0100, Gabriel Goller wrote:
> On Mon Mar 4, 2024 at 2:22 PM CET, Christoph Heiss wrote:
> > +When copying and setting up the packages has finished, you can reboot the
> > +server. This will be done automatically after a few seconds by default.
>
> I would remove the `by default` so that the sentence becomes:
>
> This will be done automatically after a few seconds.

Well, the option to automatically reboot can be changed and is on by
default, so it makes sense IMO to have that bit in there. Although the
sentence can probably be rephrased a bit to make it read easier - I'll
see what I can come up with for v2.

>
> > +. Log in using the `root` (realm 'PAM') username and the password chosen 
> > during
> > +installation.
>
> I think you need to fix the indentation here...
>

Will do, although we have (esp. in pmg-docs) a mix-match of both styles,
since it seems not not matter to asciidoc.
I will then probably go about and fix all the other instances too, if
I'm already it at ..

>
>
> ___
> 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 storage 1/1] storage/plugins: pass scfg to parse_volname

2024-03-05 Thread Fabian Grünbichler
Roland Kammerer wrote:
> On Thu, Feb 29, 2024 at 02:29:51PM +0100, Fiona Ebner wrote:
> > Am 23.02.24 um 10:24 schrieb Roland Kammerer:
> > > This passes the well known $scfg to parse_volname and bumps the API
> > > versions accordingly. This allows plugins to access their configuration
> > > if necessary.
> > > 
> > 
> > Hi,
> > can you please describe your use case in more detail? Why does parsing
> > the volume name depend on something in the storage configuration?
> 
> Hi,
> 
> I can try, hopefully not only repeating what I wrote in the cover
> letter:
> 
> - We want to implement reassigning disks to different VMs.
> - this calls the plugin's rename_volume.
> 
> Usually this renames a disk from vm-100-disk-1 to something like
> vm-101-disk-1. Great, no changes needed to parse_volume, the (new) VM ID
> can be parsed from the name with a regex and the actual disk.
> 
> For us this would mean that we would need to rename the resource in
> LINSTOR, the DRBD devices on all nodes, the backing device for the DRBD
> resources on all nodes,... For various reasons that is terribly hard,
> especially considering cluster wide rollbacks. If you want me to, I can
> fill some pages about that :).
> 
> So I came up with this:
> 
> - Let's not store the VM ID in the name of the resource/device, but
>   let's generate resource/"disk" names like pm-$UUID. The LINSTOR
>   resource is named pm-$UUID, the DRBD device is named pm-$UUID, the
>   backing devices on LVM/ZFS are named /dev/vg/pm-$UUID...
> - Great, we abstracted away the VM IDs. So where do we then store the VM
>   ID that for example parse_volname needs to return the actual VM ID?
> - We can store that in meta data that is associated with the
>   LINSTOR/DRBD resource. We have a central component, the LINSTOR
>   controller, the "brain of the SDS", which is needed anyways and it's
>   IP is already in the storage configuration. Just think of it as a
>   "data base".
> - So in that "data base" we store that pm-123... belongs to VM ID 100.
> - Reassigning a disk to a new VM (ID) is then trivial, we just update
>   the "data base" entry for pm-$UUID, now pointing to VM ID 101 (or
>   whatever). That is exactly what I do in rename_volume. I don't
>   actually rename the LINSTOR/DRBD/LVM objects/disks, I just update the
>   mapping. When getting called for "reassing disks to a new VM", the
>   plugin is free to generate a name. I return the old/existing name, but
>   the new VM ID. Yes, that is a bit if cheating, but works perfectly
>   fine for that scenario of reassinging disks to VMs.
> - Whenever we need the VM ID, we don't parse it from the pm-$UUID name,
>   we can't, but we ask our "data base".
> - In oder to ask the "data base", we need to know its IP. And for that
>   we need a pointer to the storage configuration that (already) contains
>   that IP.
> - all plugin functions already get a parameter to $scfg. With one
>   exception: parse_volname.
> 
> All in all, yes, this is specific for our use case, otherwise
> parse_volname would already have that additional parameter as all the
> other plugin functions, but I don't see where this would hurt existing
> code, and it certainly helps us to enable reassigning disks to VMs.
> Passing in a param all other functions already get access to also does
> not sound too terrible to me.
> 
> If there are further questions please feel free to ask.

thanks for spelling out your rationale!

I wonder whether the following wouldn't also work?

- keep the volume name on the PVE side like the other storage plugins (which 
means - encode vital information about the volume there so that it's possible 
to query *without* talking to the storage[0])
- use a separate, unknown to PVE identifier on the DRBD side
- store the association (either from volname to internal, or both ways) in 
linstore and potentially cache it on the PVE side if needed

that way, the interface can remain as it is. renaming is just a metadata update 
(change volname on the PVE side, update mapping in linstore), no need to change 
the internal DRBD identifier or any related entities. there is no chance of 
accidentally triggering a storm of requests or a return of outdated information 
just by calls to parse_volname. you only need to query the mapping between PVE 
volname and DRBD identifier when you actually need to do stuff with the volume 
on the DRBD end (e.g., things that already have $scfg at the moment).

the reason parse_volname doesn't get the storage config entry at the moment is 
exactly that we want to force plugins to encode the basic information in the 
volname, so that we can rely on it being cheap to call everywhere.

basically, instead of your proposed mapping of volname to "information usually 
encoded in volname" via linstore, the approach lined out above would be a 
mapping of volname "with all the basic information" to DRBD-internal, real 
volume ID via linstore. there wouldn't be a fundamental difference to other 
storages that also hav

[pve-devel] [PATCH docs] local-zfs: mention `zfs_arc_max` clamping by the installer

2024-03-05 Thread Christoph Heiss
This was forgotten to be updated when it changed it the installer and
now reported in the forum [0] that the docs are a bit outdated in this
regard.

[0] 
https://forum.proxmox.com/threads/hat-proxmox-8-neue-zfs_arc_max-settings.142754/

Signed-off-by: Christoph Heiss 
---
 local-zfs.adoc | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/local-zfs.adoc b/local-zfs.adoc
index a1a14e4..c977558 100644
--- a/local-zfs.adoc
+++ b/local-zfs.adoc
@@ -568,11 +568,16 @@ Limit ZFS Memory Usage
 ~~
 
 ZFS uses '50 %' of the host memory for the **A**daptive **R**eplacement
-**C**ache (ARC) by default. Allocating enough memory for the ARC is crucial for
-IO performance, so reduce it with caution. As a general rule of thumb, allocate
-at least +2 GiB Base + 1 GiB/TiB-Storage+. For example, if you have a pool with
-+8 TiB+ of available storage space then you should use +10 GiB+ of memory for
-the ARC.
+**C**ache (ARC) by default. For new installations starting with {pve} 8.1, the
+ARC usage limit will be set to '10 %' of the installed physical memory, clamped
+to a maximum of +16 GiB+. This value is written to `/etc/modprobe.d/zfs.conf`.
+
+Allocating enough memory for the ARC is crucial for IO performance, so reduce 
it
+with caution. As a general rule of thumb, allocate at least +2 GiB Base + 1
+GiB/TiB-Storage+. For example, if you have a pool with +8 TiB+ of available
+storage space then you should use +10 GiB+ of memory for the ARC.
+
+ZFS also enforces a minimum value of +64 MiB+.
 
 You can change the ARC usage limit for the current boot (a reboot resets this
 change again) by writing to the +zfs_arc_max+ module parameter directly:
@@ -581,8 +586,8 @@ change again) by writing to the +zfs_arc_max+ module 
parameter directly:
  echo "$[10 * 1024*1024*1024]" >/sys/module/zfs/parameters/zfs_arc_max
 
 
-To *permanently change* the ARC limits, add the following line to
-`/etc/modprobe.d/zfs.conf`:
+To *permanently change* the ARC limits, add (or change if already present) the
+following line to `/etc/modprobe.d/zfs.conf`:
 
 
 options zfs zfs_arc_max=8589934592
-- 
2.43.1



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



[pve-devel] [PATCH manager 1/3] fix #5255: node: wol: add optional bind interface

2024-03-05 Thread Christian Ebner
Allows to optionally configure a local interface name to which to
bind to when sending a wake on lan packet to wake a remote node.

Default behaviour remains to send the packet via the interface for
the default gateway.

Signed-off-by: Christian Ebner 
---
 PVE/API2/Nodes.pm | 13 -
 PVE/NodeConfig.pm |  6 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index cc5ee65e..620dac1c 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -682,9 +682,10 @@ __PACKAGE__->register_method({
my ($param) = @_;
 
my $node = $param->{node};
+   my $local_node = PVE::INotify::nodename();
 
die "'$node' is local node, cannot wake my self!\n"
-   if $node eq 'localhost' || $node eq PVE::INotify::nodename();
+   if $node eq 'localhost' || $node eq $local_node;
 
PVE::Cluster::check_node_exists($node);
 
@@ -694,6 +695,9 @@ __PACKAGE__->register_method({
die "No wake on LAN MAC address defined for '$node'!\n";
}
 
+   my $local_config = PVE::NodeConfig::load_config($local_node);
+   my $bind_iface = $local_config->{'wakeonlan-bind-interface'};
+
$mac_addr =~ s/://g;
my $packet = chr(0xff) x 6 . pack('H*', $mac_addr) x 16;
 
@@ -706,6 +710,13 @@ __PACKAGE__->register_method({
setsockopt($sock, Socket::SOL_SOCKET, Socket::SO_BROADCAST, 1)
|| die "Unable to set socket option: $!\n";
 
+   if (defined($bind_iface)) {
+   # Null terminated interface name
+   my $bind_iface_raw = pack('Z*', $bind_iface);
+   setsockopt($sock, Socket::SOL_SOCKET, Socket::SO_BINDTODEVICE, 
$bind_iface_raw)
+   || die "Unable to bind socket to interface '$bind_iface': $!\n";
+   }
+
send($sock, $packet, 0, $to)
|| die "Unable to send packet: $!\n";
 
diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
index 941e6009..5450ab2f 100644
--- a/PVE/NodeConfig.pm
+++ b/PVE/NodeConfig.pm
@@ -91,6 +91,12 @@ my $confdesc = {
format => 'mac-addr',
optional => 1,
 },
+'wakeonlan-bind-interface' => {
+   type => 'string',
+   description => 'Bind to this interface when sending wake on LAN packet',
+   format => 'pve-iface',
+   optional => 1,
+},
 'startall-onboot-delay' => {
description => 'Initial delay in seconds, before starting all the 
Virtual Guests with on-boot enabled.',
type => 'integer',
-- 
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 docs 3/3] pvenode/wake-on-lan: mention optional config options

2024-03-05 Thread Christian Ebner
Show how to configure the optional bind interface and broadcast address
options via `pvenode`.

Signed-off-by: Christian Ebner 
---
 pvenode.adoc | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/pvenode.adoc b/pvenode.adoc
index 59eeecb..8a1da08 100644
--- a/pvenode.adoc
+++ b/pvenode.adoc
@@ -87,6 +87,20 @@ of `` obtained from the `wakeonlan` property. The 
node-specific
 pvenode config set -wakeonlan XX:XX:XX:XX:XX:XX
 
 
+Optionally, the interface via which to send the WoL packet can be specified by
+setting the `wakeonlan-bind-interface` via the following command:
+
+
+pvenode config set -wakeonlan-bind-interface 
+
+
+The broadcast address used when sending the WoL packet can further be set by
+specifying the `wakeonlan-broadcast-address` using the following command:
+
+
+pvenode config set -wakeonlan-broadcast-address 
+
+
 Task History
 
 
-- 
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 manager 2/3] fix #5255: node: wol: configurable broadcast address

2024-03-05 Thread Christian Ebner
Allows to configure a custom broadcast address to use when sending a
wake on lan packet to wake a remote node.

Default behaviour remains to fallback to 255.255.255.255.

Signed-off-by: Christian Ebner 
---
 PVE/API2/Nodes.pm | 3 ++-
 PVE/NodeConfig.pm | 6 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 620dac1c..e73fc28f 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -697,11 +697,12 @@ __PACKAGE__->register_method({
 
my $local_config = PVE::NodeConfig::load_config($local_node);
my $bind_iface = $local_config->{'wakeonlan-bind-interface'};
+   my $broadcast_addr = $local_config->{'wakeonlan-broadcast-address'} // 
'255.255.255.255';
 
$mac_addr =~ s/://g;
my $packet = chr(0xff) x 6 . pack('H*', $mac_addr) x 16;
 
-   my $addr = gethostbyname('255.255.255.255');
+   my $addr = gethostbyname($broadcast_addr);
my $port = getservbyname('discard', 'udp');
my $to = Socket::pack_sockaddr_in($port, $addr);
 
diff --git a/PVE/NodeConfig.pm b/PVE/NodeConfig.pm
index 5450ab2f..e3feaaa6 100644
--- a/PVE/NodeConfig.pm
+++ b/PVE/NodeConfig.pm
@@ -97,6 +97,12 @@ my $confdesc = {
format => 'pve-iface',
optional => 1,
 },
+'wakeonlan-broadcast-address' => {
+   type => 'string',
+   description => 'IPv4 broadcast address to use when sending wake on LAN 
packet',
+   format => 'ipv4',
+   optional => 1,
+},
 'startall-onboot-delay' => {
description => 'Initial delay in seconds, before starting all the 
Virtual Guests with on-boot enabled.',
type => 'integer',
-- 
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 manager docs 0/3] add optional WoL config options

2024-03-05 Thread Christian Ebner
For certain network setups the default values currently used to send
a wake on lan magic packet are not correct, e.g. it will get send via
the interface for which the default gateway is configured.

This patches add optional configuration options to set a bind
interface, over which to send the WoL packet and/or set a broadcast
address to use.

The functionality was tested by listening on all interfaces of the
sending host via `tcpdump -i any udp port 9`, and testing the
combinations of
`pvenode config set -wakeonlan-bind-interface ` and
`pvenode config set -wakeonlan-broadcast-address `.

See also the thread in the community forum
https://forum.proxmox.com/threads/123459/

pve-manager:

Christian Ebner (2):
  fix #5255: node: wol: add optional bind interface
  fix #5255: node: wol: configurable broadcast address

 PVE/API2/Nodes.pm | 16 ++--
 PVE/NodeConfig.pm | 12 
 2 files changed, 26 insertions(+), 2 deletions(-)

pve-docs:

Christian Ebner (1):
  pvenode/wake-on-lan: mention optional config options

 pvenode.adoc | 14 ++
 1 file changed, 14 insertions(+)

-- 
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 docs 2/2] installation: document `ARC max size` option for ZFS-on-root installations

2024-03-05 Thread Christoph Heiss
This has been part of the installer since the 8.1 release.

Signed-off-by: Christoph Heiss 
---
 pve-installation.adoc | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/pve-installation.adoc b/pve-installation.adoc
index 6b44fc0..3e12727 100644
--- a/pve-installation.adoc
+++ b/pve-installation.adoc
@@ -277,6 +277,13 @@ Defines which checksumming algorithm should be used for 
`rpool`.
 Defines the `copies` parameter for `rpool`. Check the `zfs(8)` manpage for the
 semantics, and why this does not replace redundancy on disk-level.

+`ARC max size`::
+
+Defines the maximum size the ARC can grow to and thus limits the amount of
+memory ZFS will use. See also the section on
+xref:sysadmin_zfs_limit_memory_usage[how to limit ZFS memory usage] for more
+details.
+
 `hdsize`::

 Defines the total hard disk size to be used. This is useful to save free space
--
2.43.1



___
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 1/1] fix #1905: Allow moving unused disks

2024-03-05 Thread Filip Schauer

After a bit of research, here is a more descriptive commit message:


Allow moving unused disks to another storage

In the past, moving unused disks to another storage was prohibited due
to oversights in the handling of unused disks. This commit rectifies
this limitation by allowing the movement of unused disks.

Historical context:
* 16 Sep 2010 r5164 qemu-server/pve2: The disknames sub was removed.
* 17 Sep 2010 r5170 qemu-server/pve2: Unused disks were introduced.
* 28 Jan 2011 r5461 qemu-server/pve2: The same disknames sub that was
  removed in r5164 was brought back. Since unused disks were not around
  yet in r5164 the disknames sub did not consider unused disks.
* 6-8 Aug 2012 c1175c92..f91b2e45 qemu-server.git: Disk resize was
  introduced. In commit c1175c92 in sub qemu_block_resize unused disks
  were not taken into account and in commit 2f48a4f5 (8 Aug 2012) the
  resize API call was changed to only allow disks matching the ones in
  the disknames sub. Since sub disknames did not contain any unused
  disks, those were not allowed at all in the resize API call.
* 27 May 2013 586bfa78 qemu-server.git: Disk move was introduced. The
  API call implementation borrowed heavily from disk resize, including
  the behaviour of not taking unused disks into account. Thus, unused
  disk could not be moved, which persists to this day.

In summary, this behaviour was introduced because the handling of unused
disks was overlooked and it was never changed.

There is no inherent reason why unused disks should be restricted from
being moved to another storage. These disks cannot use the
qemu_drive_mirror, but they can still be moved with qemu_img_convert,
the same way as any other disk of a stopped VM.


On 19/02/2024 13:22, Thomas Lamprecht wrote:

Am 19/02/2024 um 12:11 schrieb Filip Schauer:

Allow moving unused/detached disks to another storage.

this is a repetition of the commit subject, while that is on it's
own OK, I'd rather see a description about why this is OK to do, i.e.,
why was the original check added, what changed since then, what are
the drawbacks, e.g., as the QEMU block-mirror cannot be used I imagine
only a limited set of source storage to target storage can be used
for this?



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


[pve-devel] [PATCH v4 master ceph 2/16] patches: add patch that reorders clients used by ceph-crash

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

Signed-off-by: Max Carrara 
Reviewed-by: Fabian Grünbichler 
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on master
Changes v3 --> v4:
  * none

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

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

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



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


[pve-devel] [PATCH v4 master ceph 1/16] debian: add patch to fix ceph crash dir permissions in postinst hook

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

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

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

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

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



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



[pve-devel] [PATCH v4 quincy-stable-8 ceph 4/16] patches: add patch that reorders clients used by ceph-crash

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

Signed-off-by: Max Carrara 
Reviewed-by: Fabian Grünbichler 
---
Changes v1 --> v2:
  * new
Changes v2 --> v3:
  * rebased on quincy-stable-8
Changes v3 --> v4:
  * none

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

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

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



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


[pve-devel] [PATCH v4 pve-storage 09/13] cephconfig: emit warning for lines that fail to parse

2024-03-05 Thread Max Carrara
Signed-off-by: Max Carrara 
---
Changes v3 --> v4:
  * new

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 6b7bbd4..da28940 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -70,6 +70,8 @@ sub parse_ceph_config {
 
next;
}
+
+   warn "failed to parse line - skip: $line\n";
 }
 
 return $cfg;
-- 
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 v4 pve-storage 06/16] cephconfig: support line-continuations in parser

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

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

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

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

Signed-off-by: Max Carrara 
---
Changes v2 --> v3:
  * new
Changes v3 --> v4:
  * none

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 74a92eb..80f71b0 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -19,13 +19,33 @@ sub parse_ceph_config {
 return $cfg if !defined($raw);
 
 my @lines = split /\n/, $raw;
+my @lines_normalized;
+
+my $re_comment_not_escaped = qr/(?https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



[pve-devel] [PATCH v4 pve-storage 12/16] cephconfig: remove leading whitespace on write to Ceph config

2024-03-05 Thread Max Carrara
Because continued lines (lines following lines that end with '\')
preserve whitespace, this commit removes any leading whitespace that
is added by our config writer.

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

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

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

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

Signed-off-by: Max Carrara 
---
Changes v2 --> v3:
  * new
Changes v3 --> v4:
  * rebased due to previous changes

Note: This patch may be dropped if not desired.

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index c84b78f..888be08 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -108,7 +108,7 @@ sub write_ceph_config {
# escape comment literals
$value =~ s/(#|;)/\\$1/g;
 
-   $out .= "\t $key = $value\n";
+   $out .= "$key = $value\n";
}
$out .= "\n";
 
-- 
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 v4 ceph master, ceph quincy-stable-8, pve-storage, pve-manager 0/16] Fix #4759: Configure Permissions for ceph-crash.service

2024-03-05 Thread Max Carrara
Fix #4759: Configure Permissions for ceph-crash.service - Version 4
===

Notable changes since v3


  * Both parser and writer for 'ceph.conf' now have unit tests which run
during make targets like e.g. `make deb`, thanks to `dh_auto_test`
  * The parser for 'ceph.conf' now correctly un-escapes comment literals
(found while developing unit tests)
  * The writer for 'ceph.conf' now correctly escapes comment literals
(found while developing unit tests)
  * The helper script called in 'postinst' of pve-manager for updating
'ceph.crash' in 'ceph.conf' now correctly handles an existing key
being referenced directly and removes it (thanks Friedrich!)
  * The aforementioned helper script has more verbose output, showing
explicitly what's being done to the configuration
  * The 'postinst' hook now prints an empty line before and after it
runs to make it a little more visible
  * The 'postinst' hook now also restarts 'ceph-crash.service' if the
user hasn't disabled it (thanks Friedrich!)

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


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

Summary of Changes
--

ceph (master):

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

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


ceph (quincy-stable-8):

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

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


pve-storage:

Max Carrara (9):
  cephconfig: align our parser more with Ceph's parser
  cephconfig: support line-continuations in parser
  cephconfig: allow writing arbitrary sections
  cephconfig: support escaped comment literals
  cephconfig: emit warning for lines that fail to parse
  cephconfig: change code style inside config writer
  cephconfig: change order of written sections
  cephconfig: remove leading whitespace on write to Ceph config
  test: add tests for 'ceph.conf' parser and writer

 src/Makefile   |   1 +
 src/PVE/CephConfig.pm  |  95 +++--
 src/PVE/Makefile   |   4 +
 src/PVE/test/Makefile  |   9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 402 +
 5 files changed, 490 insertions(+), 21 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 | 129 
 debian/postinst |  26 
 6 files changed, 218 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



[pve-devel] [PATCH v4 pve-storage 13/16] test: add tests for 'ceph.conf' parser and writer

2024-03-05 Thread Max Carrara
These tests attempt to cover all syntax quirks that the format of
'ceph.conf' currently allows.

One known exception however is the handling of "quoted" and "unquoted"
strings, as Ceph's own parser appears to not actually differ between
them either. Curiously, if a "quoted string" isn't able to be
parsed by Ceph's implementation, it goes on to parse it as an
"unquoted string" anyway. [0] In both cases, the result is the same -
the string is parsed with quotes.

Each test case is first tested against the parser - if the resulting
hash matches the expected hash, it is consequently passed into the
writer. The writer's result is then parsed another time and compared
against the expected hash once more.

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

Signed-off-by: Max Carrara 
---
Changes v3 --> v4:
  * new

 src/Makefile   |   1 +
 src/PVE/Makefile   |   4 +
 src/PVE/test/Makefile  |   9 +
 src/PVE/test/ceph_conf_parse_write_test.pl | 402 +
 4 files changed, 416 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 000..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 000..f76a877
--- /dev/null
+++ b/src/PVE/test/ceph_conf_parse_write_test.pl
@@ -0,0 +1,402 @@
+#!/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=> the hash that parse_ceph_config should return
+#   raw=> the raw content of the file to test
+#
+# A case is tested as follows:
+#   1. The 'raw' key is fed into parse_ceph_config
+#   2. The result is compared with 'expected'
+#   3. If correct, the resulting hash is fed into write_ceph_config
+#   4. It's output is then fed to parse_ceph_config again for one last
+#  comparison with 'expected'
+my $tests = [
+{
+   description => 'empty file',
+   expected => {},
+   raw => <<~EOF,
+   EOF
+},
+{
+   description => 'file without section',
+   expected => {},
+   raw => <<~EOF,
+   foo = bar
+   arbitrary text can go here
+
+   Rust is better than Perl
+   EOF
+},
+{
+   description => 'single section',
+   expected => {
+   foo => {
+   bar => 'baz',
+   },
+   },
+   raw => <<~EOF,
+   [foo]
+   bar = baz
+   EOF
+},
+{
+   description => 'single section, no key-value pairs',
+   expected => {
+   foo => {},
+   },
+   raw => <<~EOF,
+   [foo]
+   EOF
+},
+{
+   description => 'single section, whitespace before key',
+   expected => {
+   foo => {
+   bar => 'baz',
+   },
+   },
+   raw => <<~EOF,
+   [foo]
+\t bar = baz
+   EOF
+},
+{
+   description => 'single section, section header with preceding 
whitespace',
+   expected => {
+   foo => {
+   bar => 'baz',
+   },
+   },
+   raw => <<~EOF,
+ \t[foo]
+   bar = baz
+   EOF
+},
+{
+   description => 'single section, section header with comment',
+   expected => {
+   foo => {
+   bar => 'baz',
+   },
+   },
+   raw => <<~EOF,
+   [foo] # some comment
+   bar = baz
+   EOF
+},
+{
+   description => 'single section, section header ' .
+   'with preceding whitespace and comment',
+   expected => {
+   foo => {
+   bar => 'baz',
+   },
+   },
+   raw => <<~EOF,
+ \t  [foo] ; some comment
+   bar = baz
+   EOF
+},
+{
+  

[pve-devel] [PATCH v4 pve-manager 15/16] fix #4759: ceph: configure ceph-crash.service and its key

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

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


Expected Configuration
--

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


New Clusters


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


Existing Clusters
-

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

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

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

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


Existing `client.crash` Key
---

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


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

Signed-off-by: Max Carrara 
---
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

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

diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
index 1e959ef3..8b7ce376 100644
--- a/PVE/API2/Ceph/MON.pm
+++ b/PVE/API2/Ceph/MON.pm
@@ -450,6 +450,14 @@ __PACKAGE__->register_method ({
)
};
warn "$@" if $@;
+
+   print "Configuring keyring for ceph-crash.service\n";
+   eval {
+   PVE::Ceph::Tools::create_or_update_crash_keyring_file();
+   $cfg->{'client.crash'}->{keyring} = 
'/etc/pve/ceph/$cluster.$name.keyring';
+   cfs_write_file('ceph.conf', $cfg);
+   };
+   warn "Unable to configure keyring for ceph-crash.service: 
$@" if $@;
}
 
eval { PVE::Ceph::Services::ceph_service_cmd('enable', 
$monsection) };
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 735bb116..9b97171e 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -20,6 +20,7 @@ my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
 my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
 my $pve_ceph_cfgdir = "/etc/pve/ceph";
 
+my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
 my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
 my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
 my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
@@ -45,6 +46,7 @@ my $config_values = {
 
 my $config_files = {
 pve_ceph_cfgpath => $pve_ceph_cfgpath,
+pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
 pve_mon_key_path => $pve_mon_key_path,
 pve_c

[pve-devel] [PATCH v4 pve-storage 08/16] cephconfig: support escaped comment literals

2024-03-05 Thread Max Carrara
This commit allows our parser and writer implementations to support
escaping and un-escaping the comment literals '#' and ';' [0].

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

Signed-off-by: Max Carrara 
---
Changes v3 --> v4:
  * new

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 32ea544..6b7bbd4 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -40,6 +40,9 @@ sub parse_ceph_config {
$line .= $next_line;
}
 
+   # un-escape escaped comment literals
+   $line =~ s/\\(#|;)/$1/g;
+
push(@lines_normalized, $line);
 }
 
@@ -99,7 +102,11 @@ sub write_ceph_config {
 
$out .= "[$section]\n";
foreach my $key (sort keys %{$cfg->{$section}}) {
-   $out .= "\t $key = $cfg->{$section}->{$key}\n";
+   my $value = $cfg->{$section}->{$key};
+   # escape comment literals
+   $value =~ s/(#|;)/\\$1/g;
+
+   $out .= "\t $key = $value\n";
}
$out .= "\n";
 
-- 
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 v4 quincy-stable-8 ceph 3/16] debian: add patch to fix ceph crash dir permissions in postinst hook

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

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

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

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

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



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



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

2024-03-05 Thread Max Carrara
in order to group related sections together.

Signed-off-by: Max Carrara 
---
Changes v2 --> v3:
  * new; originally patch 07 of series v2, now adapted to this series 
Changes v3 --> v4:
  * none

 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 a361a5b..c84b78f 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -118,17 +118,20 @@ sub write_ceph_config {
 
 my @rexprs = (
qr/global/,
+
qr/client/,
qr/client\..*/,
 
qr/mds/,
-   qr/mon/,
-   qr/osd/,
-   qr/mgr/,
-
qr/mds\..*/,
+
+   qr/mon/,
qr/mon\..*/,
+
+   qr/osd/,
qr/osd\..*/,
+
+   qr/mgr/,
qr/mgr\..*/,
 
qr/.*/,
-- 
2.39.2



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



[pve-devel] [PATCH v4 pve-storage 07/16] cephconfig: allow writing arbitrary sections

2024-03-05 Thread Max Carrara
This adds support for writing arbitrary sections to 'ceph.conf' while
ensuring that already written sections are not duplicated.

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

Signed-off-by: Max Carrara 
---
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

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index 80f71b0..32ea544 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -87,6 +87,7 @@ my $parse_ceph_file = sub {
 sub write_ceph_config {
 my ($filename, $cfg) = @_;
 
+my $written_sections = {};
 my $out = '';
 
 my $cond_write_sec = sub {
@@ -94,16 +95,21 @@ sub write_ceph_config {
 
foreach my $section (sort keys %$cfg) {
next if $section !~ m/^$re$/;
+   next if exists($written_sections->{$section});
+
$out .= "[$section]\n";
foreach my $key (sort keys %{$cfg->{$section}}) {
$out .= "\t $key = $cfg->{$section}->{$key}\n";
}
$out .= "\n";
+
+   $written_sections->{$section} = 1;
}
 };
 
 &$cond_write_sec('global');
 &$cond_write_sec('client');
+&$cond_write_sec('client\..*');
 
 &$cond_write_sec('mds');
 &$cond_write_sec('mon');
@@ -115,6 +121,8 @@ sub write_ceph_config {
 &$cond_write_sec('osd\..*');
 &$cond_write_sec('mgr\..*');
 
+&$cond_write_sec('.*');
+
 return $out;
 }
 
-- 
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 v4 pve-manager 16/16] bin/make: gather helper scripts in separate variable

2024-03-05 Thread Max Carrara
Signed-off-by: Max Carrara 
---
Changes v2 --> v3:
  * new
Changes v3 --> v4:
  * 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



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



[pve-devel] [PATCH v4 pve-storage 10/16] cephconfig: change code style inside config writer

2024-03-05 Thread Max Carrara
This commit changes the code style of subroutine `write_ceph_config`
to match our style guide [0] more.

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

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

Signed-off-by: Max Carrara 
---
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

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

diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
index da28940..a361a5b 100644
--- a/src/PVE/CephConfig.pm
+++ b/src/PVE/CephConfig.pm
@@ -98,12 +98,12 @@ sub write_ceph_config {
 my $cond_write_sec = sub {
my $re = shift;
 
-   foreach my $section (sort keys %$cfg) {
+   for my $section (sort keys $cfg->%*) {
next if $section !~ m/^$re$/;
next if exists($written_sections->{$section});
 
$out .= "[$section]\n";
-   foreach my $key (sort keys %{$cfg->{$section}}) {
+   for my $key (sort keys $cfg->{$section}->%*) {
my $value = $cfg->{$section}->{$key};
# escape comment literals
$value =~ s/(#|;)/\\$1/g;
@@ -116,21 +116,27 @@ sub write_ceph_config {
}
 };
 
-&$cond_write_sec('global');
-&$cond_write_sec('client');
-&$cond_write_sec('client\..*');
+my @rexprs = (
+   qr/global/,
+   qr/client/,
+   qr/client\..*/,
 
-&$cond_write_sec('mds');
-&$cond_write_sec('mon');
-&$cond_write_sec('osd');
-&$cond_write_sec('mgr');
+   qr/mds/,
+   qr/mon/,
+   qr/osd/,
+   qr/mgr/,
 
-&$cond_write_sec('mds\..*');
-&$cond_write_sec('mon\..*');
-&$cond_write_sec('osd\..*');
-&$cond_write_sec('mgr\..*');
+   qr/mds\..*/,
+   qr/mon\..*/,
+   qr/osd\..*/,
+   qr/mgr\..*/,
 
-&$cond_write_sec('.*');
+   qr/.*/,
+);
+
+for my $re (@rexprs) {
+   $cond_write_sec->($re);
+}
 
 return $out;
 }
-- 
2.39.2



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



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

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

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

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

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

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

 4. Although not crucial for Ceph, our parser now also supports empty
sections. When a section header is successfully parsed, it gets
added to the configuration hash and the parser continues operating
on the next line.

[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
Changes v2 --> v3:
  * support comment literals (4.)
Changes v3 --> v4:
  * support empty sections
  * fix and move support for comment literals to separate patch

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

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

2024-03-05 Thread Max Carrara
This commit adds the '/etc/pve/ceph' directory to our overall expected
Ceph configuration.

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

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

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

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

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

Signed-off-by: Max Carrara 
---
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

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

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



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