public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service
Date: Mon, 12 Feb 2024 14:34:05 +0100	[thread overview]
Message-ID: <1707743502.yuxc2lm198.astroid@yuna.none> (raw)
In-Reply-To: <20240205175419.1271680-10-m.carrara@proxmox.com>

On February 5, 2024 6:54 pm, Max Carrara wrote:
> when creating the cluster's first monitor.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * do not enable/restart `ceph-crash` anymore when creating first mon
>   * drop changes to function `ceph_service_cmd` as they are no longer
>     needed
>   * create keyring for `ceph-crash` before modifying 'ceph.conf'
>   * always set keyring for 'client.crash' section instead of only
>     if section doesn't exist already
>   * only modify the keyring file in `get_or_create_crash_keyring()`
>     if the content differs from the output of `ceph auth get-or-create`

you kinda ignored my comment about this adding yet another create keyring helper ;)

>  PVE/API2/Ceph/MON.pm | 17 ++++++++++++++++-
>  PVE/Ceph/Tools.pm    | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Ceph/MON.pm b/PVE/API2/Ceph/MON.pm
> index 1e959ef3..ae12a2d3 100644
> --- a/PVE/API2/Ceph/MON.pm
> +++ b/PVE/API2/Ceph/MON.pm
> @@ -459,11 +459,26 @@ __PACKAGE__->register_method ({
>  	    });
>  	    die $@ if $@;
>  	    # automatically create manager after the first monitor is created
> +	    # and set up keyring and config for ceph-crash.service
>  	    if ($is_first_monitor) {
>  		PVE::API2::Ceph::MGR->createmgr({
>  		    node => $param->{node},
>  		    id => $param->{node}
> -		})
> +		});
> +
> +		eval {
> +		    PVE::Ceph::Tools::get_or_create_crash_keyring();

this is called get_or_create, but it actually returns the path to, not
the key(ring).. nothing uses the return value anyway, so it could also
be called differently I guess and not return anything, but just from the
name, I'd expect the key to be returned.

> +		};
> +		warn "Unable to configure keyring for ceph-crash.service: $@" if $@;
> +
> +		PVE::Cluster::cfs_lock_file('ceph.conf', undef, sub {
> +		    my $cfg = cfs_read_file('ceph.conf');
> +
> +		    $cfg->{'client.crash'}->{keyring} = '/etc/pve/ceph/$cluster.$name.keyring';

I guess this doesn't make anything worse (since we starting from
"ceph-crash is totally broken"), but if querying or creating the keyring
failed, does it make sense to reference it in the config?

> +
> +		    cfs_write_file('ceph.conf', $cfg);
> +		});
> +		die $@ if $@;

we could move this whole part up to where we do the monitor changes, and
only lock and read and write the config once..

>  	    }
>  	};
>  
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index 273a3eb6..02a932e3 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -18,7 +18,9 @@ my $ccname = 'ceph'; # ceph cluster name
>  my $ceph_cfgdir = "/etc/ceph";
>  my $pve_ceph_cfgpath = "/etc/pve/$ccname.conf";
>  my $ceph_cfgpath = "$ceph_cfgdir/$ccname.conf";
> +my $pve_ceph_cfgdir = "/etc/pve/ceph";
>  
> +my $pve_ceph_crash_key_path = "$pve_ceph_cfgdir/$ccname.client.crash.keyring";
>  my $pve_mon_key_path = "/etc/pve/priv/$ccname.mon.keyring";
>  my $pve_ckeyring_path = "/etc/pve/priv/$ccname.client.admin.keyring";
>  my $ckeyring_path = "/etc/ceph/ceph.client.admin.keyring";
> @@ -37,12 +39,14 @@ my $ceph_service = {
>  
>  my $config_values = {
>      ccname => $ccname,
> +    pve_ceph_cfgdir => $pve_ceph_cfgdir,
>      ceph_mds_data_dir => $ceph_mds_data_dir,
>      long_rados_timeout => 60,
>  };
>  
>  my $config_files = {
>      pve_ceph_cfgpath => $pve_ceph_cfgpath,
> +    pve_ceph_crash_key_path => $pve_ceph_crash_key_path,
>      pve_mon_key_path => $pve_mon_key_path,
>      pve_ckeyring_path => $pve_ckeyring_path,
>      ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> @@ -415,6 +419,41 @@ sub get_or_create_admin_keyring {
>      return $pve_ckeyring_path;
>  }
>  
> +# requires connection to existing monitor
> +sub get_or_create_crash_keyring {
> +    my ($rados) = @_;
> +
> +    if (!defined($rados)) {
> +	$rados = PVE::RADOS->new();
> +    }
> +
> +    my $output = $rados->mon_command({
> +	prefix => 'auth get-or-create',
> +	entity => 'client.crash',
> +	caps => [
> +	    mon => 'profile crash',
> +	    mgr => 'profile crash',
> +	],
> +	format => 'plain',
> +    });
> +
> +    if (! -d $pve_ceph_cfgdir) {
> +	File::Path::make_path($pve_ceph_cfgdir);
> +    }
> +
> +    if (-f $pve_ceph_crash_key_path) {
> +	my $contents = PVE::Tools::file_get_contents($pve_ceph_crash_key_path);
> +
> +	if ($contents ne $output) {
> +	    PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +	}
> +    } else {
> +	PVE::Tools::file_set_contents($pve_ceph_crash_key_path, $output);
> +    }
> +
> +    return $pve_ceph_crash_key_path;
> +}
> +
>  # get ceph-volume managed osds
>  sub ceph_volume_list {
>      my $result = {};
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 




  reply	other threads:[~2024-02-12 13:34 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-05 17:54 [pve-devel] [PATCH v2 master ceph, quincy-stable 8 ceph, pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions " Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
2024-02-12 13:32   ` Fabian Grünbichler
2024-02-13  8:25     ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 master ceph 02/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
2024-02-12 13:33   ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 03/11] debian: add patch to fix ceph crash dir permissions in postinst hook Max Carrara
2024-02-12 13:32   ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 quincy-stable-8 ceph 04/11] patches: add patch that reorders clients used by ceph-crash Max Carrara
2024-02-12 13:33   ` Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 05/11] cephconfig: align our parser more with Ceph's parser Max Carrara
2024-02-12 13:33   ` Fabian Grünbichler
2024-02-13  8:34     ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 06/11] cephconfig: allow writing arbitrary sections Max Carrara
2024-02-12 13:33   ` Fabian Grünbichler
2024-02-13  8:46     ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-storage 07/11] amend! " Max Carrara
2024-02-12 13:33   ` Fabian Grünbichler
2024-02-13  8:50     ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 08/11] ceph: fix edge case of wrong files being deleted on purge Max Carrara
2024-02-12 13:33   ` [pve-devel] applied: " Fabian Grünbichler
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 09/11] fix #4759: ceph: configure keyring for ceph-crash.service Max Carrara
2024-02-12 13:34   ` Fabian Grünbichler [this message]
2024-02-13  9:09     ` Max Carrara
2024-02-14 12:43       ` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 10/11] ceph: create '/etc/pve/ceph' during `pveceph init` Max Carrara
2024-02-05 17:54 ` [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key Max Carrara
2024-02-12 13:34   ` Fabian Grünbichler
2024-02-13  9:25     ` Max Carrara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1707743502.yuxc2lm198.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal