some comments inline
On 08/23/2017 07:02 PM, Alwin Antreich wrote:
add version check to ceph init to require luminous or higher and
fix #1481: check existence of ceph binaries before use
Signed-off-by: Alwin Antreich <a.antre...@proxmox.com>
---
PVE/API2/Ceph.pm | 17 ++++++++++++++++-
PVE/CephTools.pm | 44 +++++++++++++++++++++++++++++++++++++++-----
2 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index c4d6ffcb..2a87cb67 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -233,6 +233,8 @@ __PACKAGE__->register_method ({
PVE::CephTools::setup_pve_symlinks();
+ PVE::CephTools::check_ceph_installed(undef, "ceph_osd");
+
my $bluestore = $param->{bluestore} // 0;
my $journal_dev;
@@ -827,7 +829,13 @@ __PACKAGE__->register_method ({
code => sub {
my ($param) = @_;
- PVE::CephTools::check_ceph_installed();
+ my $version = PVE::CephTools::get_local_version(1);
+
+ if (!$version || $version < 12) {
+ die "Ceph version luminous or higher is required\n";
+ } else {
+ PVE::CephTools::check_ceph_installed();
+ }
# simply load old config if it already exists
my $cfg = PVE::CephTools::parse_ceph_config();
@@ -982,6 +990,11 @@ __PACKAGE__->register_method ({
code => sub {
my ($param) = @_;
+ PVE::CephTools::check_ceph_installed(undef, "ceph_mon");
+
+ PVE::CephTools::check_ceph_installed(undef, "ceph_mgr")
+ if (!$param->{'exclude-manager'});
+
PVE::CephTools::check_ceph_inited();
PVE::CephTools::setup_pve_symlinks();
@@ -1220,6 +1233,8 @@ __PACKAGE__->register_method ({
code => sub {
my ($param) = @_;
+ PVE::CephTools::check_ceph_installed(undef, "ceph_mgr");
+> PVE::CephTools::check_ceph_inited();
my $rpcenv = PVE::RPCEnvironment::get();
diff --git a/PVE/CephTools.pm b/PVE/CephTools.pm
index 0c0d7c18..f13d2796 100644
--- a/PVE/CephTools.pm
+++ b/PVE/CephTools.pm
@@ -20,7 +20,12 @@ my $pve_ckeyring_path =
"/etc/pve/priv/$ccname.client.admin.keyring";
my $ceph_bootstrap_osd_keyring =
"/var/lib/ceph/bootstrap-osd/$ccname.keyring";
my $ceph_bootstrap_mds_keyring =
"/var/lib/ceph/bootstrap-mds/$ccname.keyring";
-my $ceph_bin = "/usr/bin/ceph";
+my $ceph_service = {
+ ceph_bin => "/usr/bin/ceph",
+ ceph_mon => "/usr/bin/ceph-mon",
+ ceph_mgr => "/usr/bin/ceph-mgr",
+ ceph_osd => "/usr/bin/ceph-osd"
+};
my $config_hash = {
ccname => $ccname,
@@ -32,6 +37,23 @@ my $config_hash = {
long_rados_timeout => 60,
};
+sub get_local_version {
+ my ($noerr) = @_;
+
+ if (PVE::CephTools::check_ceph_installed($noerr)) {
+ my $ceph_version;
+ run_command([$ceph_service->{ceph_bin}, '--version'],
+ noerr => $noerr,
+ outfunc => sub { $ceph_version = shift; });
+ if ($ceph_version && $ceph_version =~ /^ceph.*\s((\d+)\.(\d+)\.(\d+))/)
{
+ # return (version, major, minor, patch) : major;
+ return wantarray ? ($1, $2, $3, $4) : $2;
+ }
+ }
+
+ return undef;
+}
+
sub get_config {
my $key = shift;
@@ -60,11 +82,23 @@ sub purge_all_ceph_files {
}
sub check_ceph_installed {
- my ($noerr) = @_;
+ my ($noerr, $service) = @_;
Our convention is to have "noerr" variables at the end, so that they can be
easily omitted.
I understand your approach, as service is new and can also be omitted so you
put it after
"noerr" to not affect existing code. But as this is used on only two places in
the same file
(CephTools) it could be worth to hold our convention up and swap them.
Honestly, I can live with both.
- if (! -x $ceph_bin) {
- die "ceph binaries not installed\n" if !$noerr;
- return undef;
+ if (!defined($service)) {
+ if (! -x $ceph_service->{ceph_bin}) {
One indentation level to much, there should be just a single tab here, omitting
the four spaces.
+ die "binary not installed: $ceph_service->{ceph_bin}\n" if
!$noerr;
the last tab should be four spaces instead. I know our indentation level can be
seen as a bit special
but a good editor can take care of it so that you do not need to worry :)
+ return undef;
+ }
+
+ } elsif (defined($service)) {
This is the exact opposite check of the if above, so it will always be taken if
the above
branch wasn't and vice versa. This should also be an else branch.
+ foreach my $key (keys %$ceph_service) {
+ next if $service ne $key;
+
+ if (! -x $ceph_service->{$key}){
Why not use $ceph_service->{$service} directly, making the foreach loop
unnecessary?
Then both branches look suspiciously similar, you could merge now both branches.
Somewhat like:
...
my ($service, $noerr) = @_;
$service = 'ceph_bin' if !defined($service);
if (! -x $ceph_service->{$service}) {
...
or, if you like this alternative more:
...
my ($tool, $noerr) = @_;
my $service = defined($tool) ? $ceph_service->{$tool} :
$ceph_service->{ceph_bin};
if (! -x $service) {
...
+ die "binary not installed: $ceph_service->{$key}\n" if !$noerr;
+ return undef;
+ }
+ }
}
return 1;
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
