some comments inline On 07/25/2017 05:14 PM, Dominik Csapak wrote:
we now want to add a ceph-mgr daemon to every node where a ceph-mon daemon runs, as per ceph documentation recommendation, because in luminous the mgr daemons will not be automatically created/started with a monitor anymorewe also give the createmon an optional id parameter, so that one can set a custom id, and make the creation/removal of the manager optional but the default Signed-off-by: Dominik Csapak <d.csa...@proxmox.com> --- PVE/API2/Ceph.pm | 88 +++++++++++++++++++++++++++++++++++++++++++++++++------- PVE/CephTools.pm | 2 +- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm index 105ee37a..307409b4 100644 --- a/PVE/API2/Ceph.pm +++ b/PVE/API2/Ceph.pm @@ -843,11 +843,53 @@ my $find_node_ip = sub { die "unable to find local address within network '$cidr'\n"; };+my $create_mgr = sub {+ my ($rados, $id) = @_; + + my $clustername = PVE::CephTools::get_config('ccname'); + my $mgrdir = "/var/lib/ceph/mgr/$clustername-$id"; + my $mgrkeyring = "$mgrdir/keyring"; + my $mgrname = "mgr.$id"; + + die "ceph manager directory '$mgrdir' already exists\n" + if -d $mgrdir; + + mkdir $mgrdir; + my $output = $rados->mon_command({ prefix => 'auth get-or-create', + entity => $mgrname, + caps => [ + mon => 'allow profile mgr', + osd => 'allow *', + mds => 'allow *', + ], + format => 'plain'}); + PVE::Tools::file_set_contents($mgrkeyring, $output); + + run_command(["chown", 'ceph:ceph', '-R', $mgrdir]); + + PVE::CephTools::ceph_service_cmd('start', $mgrname); + PVE::CephTools::ceph_service_cmd('enable', $mgrname); +}; + +my $destroy_mgr = sub { + my ($mgrid) = @_; + + my $clustername = PVE::CephTools::get_config('ccname'); + my $mgrname = "mgr.$mgrid"; + my $mgrdir = "/var/lib/ceph/mgr/$clustername-$mgrid"; + + die "ceph manager '$mgrname' not found \n" + if ! -d $mgrdir;
I'd make a similar error messages as above which fits the check, i.e. that the manager directory does not exist. Else a failed remove could lead to a non-existing directory but still running service – where this error message may then be confusing.
+ + PVE::CephTools::ceph_service_cmd('stop', $mgrname); + File::Path::remove_tree($mgrdir); +}; + __PACKAGE__->register_method ({ name => 'createmon', path => 'mon', method => 'POST', - description => "Create Ceph Monitor", + description => "Create Ceph Monitor and Manager", proxyto => 'node', protected => 1, permissions => { @@ -857,6 +899,18 @@ __PACKAGE__->register_method ({ additionalProperties => 0, properties => { node => get_standard_option('pve-node'), + id => { + type => 'string', + optional => 1, + pattern => '[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9]?)', + description => "The ID for the monitor, when omitted the same as the nodename", + }, + monitoronly => { + type => 'boolean', + optional => 1, + default => 0, + description => "When set, only a monitor will be created.", + }, }, }, returns => { type => 'string' }, @@ -891,12 +945,13 @@ __PACKAGE__->register_method ({ }my $monid;- for (my $i = 0; $i < 7; $i++) { - if (!$cfg->{"mon.$i"}) { - $monid = $i; - last; - } + + if (defined($param->{id})) { + $monid = $param->{id}; + } else { + $monid = $param->{node}; } + die "unable to find usable monitor id\n" if !defined($monid);
This can be removed to, monid is guranteed to be defined here. Either it's the optional `id` param or else the required `node` param.
my $monsection = "mon.$monid";@@ -944,14 +999,15 @@ __PACKAGE__->register_method ({ -d $mondir && die "monitor filesystem '$mondir' already exist\n";my $monmap = "/tmp/monmap";- + + my $rados = PVE::RADOS->new(timeout => PVE::CephTools::get_config('long_rados_timeout')); + eval { mkdir $mondir;run_command("chown ceph:ceph $mondir") if $systemd_managed; if ($moncount > 0) {- my $rados = PVE::RADOS->new(timeout => PVE::CephTools::get_config('long_rados_timeout')); my $mapdata = $rados->mon_command({ prefix => 'mon getmap', format => 'plain' }); PVE::Tools::file_set_contents($monmap, $mapdata); } else { @@ -990,6 +1046,11 @@ __PACKAGE__->register_method ({ } waitpid($create_keys_pid, 0); } + + # create manager + if (!$param->{monitoronly}) { + $create_mgr->($rados, $monid);
IMO, as we have a check here if we should do or do not something with the manager, this option should rather have a 'manager'-related name, e.g 'exclude-manager', or a default to true 'include-manager'.
+ } };return $rpcenv->fork_worker('cephcreatemon', $monsection, $authuser, $worker);@@ -999,7 +1060,7 @@ __PACKAGE__->register_method ({ name => 'destroymon', path => 'mon/{monid}', method => 'DELETE', - description => "Destroy Ceph monitor.", + description => "Destroy Ceph Monitor and Manager.", proxyto => 'node', protected => 1, permissions => { @@ -1011,8 +1072,15 @@ __PACKAGE__->register_method ({ node => get_standard_option('pve-node'), monid => { description => 'Monitor ID', - type => 'integer', + type => 'string', + pattern => '[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9]?)', }, + monitoronly => {
same as above
+ type => 'boolean', + default => 0, + optional => 1, + description => "When set, removes only the monitor, not the manager" + } }, }, returns => { type => 'string' }, diff --git a/PVE/CephTools.pm b/PVE/CephTools.pm index 0d26ea3e..0c0d7c18 100644 --- a/PVE/CephTools.pm +++ b/PVE/CephTools.pm @@ -178,7 +178,7 @@ sub ceph_service_cmd {if (systemd_managed()) { - if ($service && $service =~ m/^(mon|osd|mds|radosgw)(\.([A-Za-z0-9]{1,32}))?$/) {+ if ($service && $service =~ m/^(mon|osd|mds|mgr|radosgw)(\.([A-Za-z0-9\-]{1,32}))?$/) { $service = defined($3) ? "ceph-$1\@$3" : "ceph-$1.target"; } else { $service = "ceph.target";
The remove hunk which slipped in patch 2 look good, I'd make the eval one line with the warn directly below, though. We do that most of the times if there is only one command to execute, but this is just me nitpicking. _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
