Re: [pve-devel] [PATCH v3 ceph master, ceph quincy-stable 8, pve-storage, pve-manager 00/13] Fix #4759: Configure Permissions for ceph-crash.service

[Friedrich Weber <f.weber@proxmox.com>]

Quickly tested the patch series on my existing Ceph Quincy cluster, did
not encounter major issues -- the keyring was created and the Ceph
config was rewritten accordingly. After a restart of `ceph-crash`, it
correctly posts crashes (produced with `kill -n11 $(pidof ceph-osd)`)
again and does not write any potentially misleading messages to the
journal. Nice!

Didn't have time yet to test these patches when setting up a new
cluster, but I'll try to do so this week and report back.

Two minor things I've noticed so far:

- the `ceph-crash` service does not restart after installing the patched
ceph-base package, so the reordering done by patches 02+04 does not take
effect immediately: ceph-crash posts crash logs just fine, but logs to
the journal that it can't find a keyring. After a restart of ceph-crash,
the patch takes effect, so only a tiny inconvenience, but still: Not
sure if restarting the service is something we'd want to do in a
postinst -- is this an acceptable thing to do in a postinst?

- Might there be issues in a mixed-version cluster scenario, so if some
node A already has an updated pve-storage package (patches 05-10), but
node B doesn't yet? One thing I noticed is that node A will add the
[client.crash] section, but node B may remove it again when it needs to
rewrite the Ceph config (e.g. when creating a monitor). I don't find
this particular issue too concerning, as hopefully node B will be
updated eventually as well and reinstate the [client.crash] section. But
I wonder if there could be other more serious issues?

On 16/02/2024 15:56, Max Carrara wrote:
> This marks version 03 of the patch series "Fix #4759: Configure
> Permissions for ceph-crash.service". Older versions can be found below.
> 
> Notable changes since v2
> ------------------------
> 
>   * The 'ceph.conf' parser in pve-storage is now equivalent to Ceph's
>     and even supports continued lines
>   * The addition of the '/etc/pve/ceph' directory has been moved into a
>     separate patch in order to preserve the context of its purpose in
>     the git history
>   * The debian `postinst` hook for pve-manager is now version-guarded
>     and uses a separate Perl helper script instead of doing everything
>     in BASH
> 
> 
> 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
> 
> 
> 
> 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 (6):
>   cephconfig: align our parser more with Ceph's parser
>   cephconfig: support line-continuations in parser
>   cephconfig: allow writing arbitrary sections
>   cephconfig: change code style inside config writer
>   cephconfig: change order of written sections
>   cephconfig: remove leading whitespace on write to Ceph config
> 
>  src/PVE/CephConfig.pm | 80 +++++++++++++++++++++++++++++++------------
>  1 file changed, 58 insertions(+), 22 deletions(-)
> 
> 
> 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 | 104 ++++++++++++++++++++++++++++++++++++++++
>  debian/postinst         |  16 +++++++
>  6 files changed, 183 insertions(+), 3 deletions(-)
>  create mode 100755 bin/pve-init-ceph-crash
>