Re: [pve-devel] [PATCH docs 5/9] package-repos: align wording with pmg-docs
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
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
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
--- 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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'
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