Merge pull request #507 from ceph/wip-4635.master

Bunch of tidying up on monitor services & fix #4635

Reviewed-by: Sage Weil <sage@inktank.com>

6 files changed, 144 insertions, 101 deletions

diff --git a/PendingReleaseNotes b/PendingReleaseNotes
index e69de29bb2d..7a41aceb96b 100644
--- a/PendingReleaseNotes
+++ b/PendingReleaseNotes
@@ -0,0 +1,8 @@
+
+v0.68
+~~~~~
+
+* 'ceph osd crush set <id> <weight> <loc..>' no longer adds the osd to the
+  specified location, as that's a job for 'ceph osd crush add'.  It will
+  however continue to work just the same as long as the osd already exists
+  in the crush map.
diff --git a/src/mon/AuthMonitor.cc b/src/mon/AuthMonitor.cc
index 4fb943e7939..9f885589442 100644
--- a/src/mon/AuthMonitor.cc
+++ b/src/mon/AuthMonitor.cc
@@ -65,13 +65,13 @@ bool AuthMonitor::check_rotate()
  Tick function to update the map based on performance every N seconds
 */
 
-void AuthMonitor::tick() 
+void AuthMonitor::tick()
 {
   if (!is_active()) return;
 
   dout(10) << *this << dendl;
 
-  if (!mon->is_leader()) return; 
+  if (!mon->is_leader()) return;
 
   if (check_rotate())
     propose_pending();
@@ -84,9 +84,6 @@ void AuthMonitor::on_active()
   if (!mon->is_leader())
     return;
   mon->key_server.start_server();
-/*
-  check_rotate();
-*/
 }
 
 void AuthMonitor::create_initial()
@@ -159,7 +156,7 @@ void AuthMonitor::update_from_paxos(bool *need_bootstrap)
     // reset if we are moving to initial state.  we will normally have
     // keys in here temporarily for bootstrapping that we need to
     // clear out.
-    if (keys_ver == 0) 
+    if (keys_ver == 0)
       mon->key_server.clear_secrets();
 
     dout(20) << __func__ << " walking through version " << (keys_ver+1)
@@ -204,16 +201,6 @@ void AuthMonitor::update_from_paxos(bool *need_bootstrap)
 	   << " max_global_id=" << max_global_id
 	   << " format_version " << format_version
 	   << dendl;
- 
-  /*
-  bufferlist bl;
-  __u8 v = 1;
-  ::encode(v, bl);
-  ::encode(max_global_id, bl);
-  Mutex::Locker l(mon->key_server.get_lock());
-  ::encode(mon->key_server, bl);
-  paxos->stash_latest(version, bl);
-  */
 }
 
 void AuthMonitor::increase_max_global_id()
@@ -385,7 +372,7 @@ bool AuthMonitor::prep_auth(MAuth *m, bool paxos_writable)
   // set up handler?
   if (m->protocol == 0 && !s->auth_handler) {
     set<__u32> supported;
-    
+
     try {
       __u8 struct_v = 1;
       ::decode(struct_v, indata);
@@ -406,13 +393,19 @@ bool AuthMonitor::prep_auth(MAuth *m, bool paxos_writable)
 	  entity_name.get_type() == CEPH_ENTITY_TYPE_MDS) {
 	if (g_conf->cephx_cluster_require_signatures ||
 	    g_conf->cephx_require_signatures) {
-	  dout(1) << m->get_source_inst() << " supports cephx but not signatures and 'cephx [cluster] require signatures = true'; disallowing cephx" << dendl;
+	  dout(1) << m->get_source_inst()
+                  << " supports cephx but not signatures and"
+                  << " 'cephx [cluster] require signatures = true';"
+                  << " disallowing cephx" << dendl;
 	  supported.erase(CEPH_AUTH_CEPHX);
 	}
       } else {
 	if (g_conf->cephx_service_require_signatures ||
 	    g_conf->cephx_require_signatures) {
-	  dout(1) << m->get_source_inst() << " supports cephx but not signatures and 'cephx [service] require signatures = true'; disallowing cephx" << dendl;
+	  dout(1) << m->get_source_inst()
+                  << " supports cephx but not signatures and"
+                  << " 'cephx [service] require signatures = true';"
+                  << " disallowing cephx" << dendl;
 	  supported.erase(CEPH_AUTH_CEPHX);
 	}
       }
@@ -491,7 +484,6 @@ bool AuthMonitor::prep_auth(MAuth *m, bool paxos_writable)
     }
     if (ret == -EIO) {
       wait_for_active(new C_RetryMessage(this,m));
-      //paxos->wait_for_active(new C_RetryMessage(this, m));
       goto done;
     }
     if (caps_info.caps.length()) {
@@ -659,7 +651,7 @@ void AuthMonitor::import_keyring(KeyRing& keyring)
        ++p) {
     KeyServerData::Incremental auth_inc;
     auth_inc.name = p->first;
-    auth_inc.auth = p->second; 
+    auth_inc.auth = p->second;
     auth_inc.op = KeyServerData::AUTH_INC_ADD;
     dout(10) << " importing " << auth_inc.name << dendl;
     dout(30) << "    " << auth_inc.auth << dendl;
@@ -735,7 +727,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     getline(ss, rs);
     err = 0;
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-    //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
   } else if (prefix == "auth add" && !entity_name.empty()) {
     /* expected behavior:
@@ -858,7 +849,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     ss << "added key for " << auth_inc.name;
     getline(ss, rs);
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-    //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
   } else if ((prefix == "auth get-or-create-key" ||
 	     prefix == "auth get-or-create") &&
@@ -911,7 +901,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
 	if (auth_inc.op == KeyServerData::AUTH_INC_ADD &&
 	    auth_inc.name == entity) {
 	  wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-	  //paxos->wait_for_commit(new C_RetryMessage(this, m));
 	  return true;
 	}
       }
@@ -947,7 +936,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     rdata.append(ds);
     getline(ss, rs);
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, rdata, get_last_committed()));
-    //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
   } else if (prefix == "auth caps" && !entity_name.empty()) {
     KeyServerData::Incremental auth_inc;
@@ -970,7 +958,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     ss << "updated caps for " << auth_inc.name;
     getline(ss, rs);
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-    //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
   } else if (prefix == "auth del" && !entity_name.empty()) {
     KeyServerData::Incremental auth_inc;
@@ -986,7 +973,6 @@ bool AuthMonitor::prepare_command(MMonCommand *m)
     ss << "updated";
     getline(ss, rs);
     wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-    //paxos->wait_for_commit(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
     return true;
   }
 
diff --git a/src/mon/MonCap.cc b/src/mon/MonCap.cc
index b03873ad7dd..d8bccce9bc2 100644
--- a/src/mon/MonCap.cc
+++ b/src/mon/MonCap.cc
@@ -148,7 +148,6 @@ void MonCapGrant::expand_profile(entity_name_t name) const
     profile_grants.push_back(MonCapGrant("osd", MON_CAP_R));  // read osdmap
     profile_grants.push_back(MonCapGrant("mon getmap"));
     profile_grants.push_back(MonCapGrant("osd create"));
-    profile_grants.push_back(MonCapGrant("osd crush set"));  // FIXME: constraint this further?
     profile_grants.push_back(MonCapGrant("auth add"));
     profile_grants.back().command_args["entity"] = StringConstraint("", "osd.");
     profile_grants.back().command_args["caps_mon"] = StringConstraint("allow profile osd", "");
diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h
index e4cb9ce45ed..8d85e03ed99 100644
--- a/src/mon/MonCommands.h
+++ b/src/mon/MonCommands.h
@@ -369,13 +369,13 @@ COMMAND("osd crush set " \
 	"name=id,type=CephOsdName " \
 	"name=weight,type=CephFloat,range=0.0 " \
 	"name=args,type=CephString,n=N,goodchars=[A-Za-z0-9-_.=]", \
-	"set crushmap entry for <name> to <weight> with location <args>", \
+	"update crushmap position and weight for <name> to <weight> with location <args>", \
 	"osd", "rw", "cli,rest")
 COMMAND("osd crush add " \
 	"name=id,type=CephOsdName " \
 	"name=weight,type=CephFloat,range=0.0 " \
 	"name=args,type=CephString,n=N,goodchars=[A-Za-z0-9-_.=]", \
-	"add crushmap entry for <name> with <weight> and location <args>", \
+	"add or update crushmap position and weight for <name> with <weight> and location <args>", \
 	"osd", "rw", "cli,rest")
 COMMAND("osd crush create-or-move " \
 	"name=id,type=CephOsdName " \
diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 7b9258b1730..32413c111d3 100644
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -2621,6 +2621,36 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
     name = oss.str();
   }
 
+  // Even if there's a pending state with changes that could affect
+  // a command, considering that said state isn't yet committed, we
+  // just don't care about those changes if the command currently being
+  // handled acts as a no-op against the current committed state.
+  // In a nutshell, we assume this command  happens *before*.
+  //
+  // Let me make this clearer:
+  //
+  //   - If we have only one client, and that client issues some
+  //     operation that would conflict with this operation  but is
+  //     still on the pending state, then we would be sure that said
+  //     operation wouldn't have returned yet, so the client wouldn't
+  //     issue this operation (unless the client didn't wait for the
+  //     operation to finish, and that would be the client's own fault).
+  //
+  //   - If we have more than one client, each client will observe
+  //     whatever is the state at the moment of the commit.  So, if we
+  //     have two clients, one issuing an unlink and another issuing a
+  //     link, and if the link happens while the unlink is still on the
+  //     pending state, from the link's point-of-view this is a no-op.
+  //     If different clients are issuing conflicting operations and
+  //     they care about that, then the clients should make sure they
+  //     enforce some kind of concurrency mechanism -- from our
+  //     perspective that's what Douglas Adams would call an SEP.
+  //
+  // This should be used as a general guideline for most commands handled
+  // in this function.  Adapt as you see fit, but please bear in mind that
+  // this is the expected behavior.
+
+
   if (prefix == "osd setcrushmap" ||
       (prefix == "osd crush set" && !osdid_present)) {
     dout(10) << "prepare_command setting new crush map" << dendl;
@@ -2687,58 +2717,68 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
     goto update;
   } else if (osdid_present &&
 	     (prefix == "osd crush set" || prefix == "osd crush add")) {
-    do {
-      // <OsdName> is 'osd.<id>' or '<id>', passed as int64_t id
-      // osd crush set <OsdName> <weight> <loc1> [<loc2> ...]
-      // osd crush add <OsdName> <weight> <loc1> [<loc2> ...]
+    // <OsdName> is 'osd.<id>' or '<id>', passed as int64_t id
+    // osd crush set <OsdName> <weight> <loc1> [<loc2> ...]
+    // osd crush add <OsdName> <weight> <loc1> [<loc2> ...]
 
-      if (!osdmap.exists(id)) {
-	err = -ENOENT;
-	ss << name << " does not exist.  create it before updating the crush map";
-	goto reply;
-      }
+    if (!osdmap.exists(id)) {
+      err = -ENOENT;
+      ss << name << " does not exist.  create it before updating the crush map";
+      goto reply;
+    }
 
-      double weight;
-      cmd_getval(g_ceph_context, cmdmap, "weight", weight);
+    double weight;
+    cmd_getval(g_ceph_context, cmdmap, "weight", weight);
 
-      string args;
-      vector<string> argvec;
-      cmd_getval(g_ceph_context, cmdmap, "args", argvec);
-      map<string,string> loc;
-      parse_loc_map(argvec, &loc);
+    string args;
+    vector<string> argvec;
+    cmd_getval(g_ceph_context, cmdmap, "args", argvec);
+    map<string,string> loc;
+    parse_loc_map(argvec, &loc);
 
-      dout(0) << "adding/updating crush item id " << id << " name '"
-	      << name << "' weight " << weight << " at location "
-	      << loc << dendl;
-      CrushWrapper newcrush;
-      _get_pending_crush(newcrush);
+    if (prefix == "osd crush set"
+        && !_get_stable_crush().item_exists(id)) {
+      err = -ENOENT;
+      ss << "unable to set item id " << id << " name '" << name
+         << "' weight " << weight << " at location " << loc
+         << ": does not exist";
+      goto reply;
+    }
 
-      string action;
-      if (prefix == "osd crush set" ||
-	  newcrush.check_item_loc(g_ceph_context, id, loc, (int *)NULL)) {
-	action = "set";
-	err = newcrush.update_item(g_ceph_context, id, weight, name, loc);
-      } else {
-	action = "add";
-	err = newcrush.insert_item(g_ceph_context, id, weight, name, loc);
-	if (err == 0)
-	  err = 1;
-      }
-      if (err == 0) {
-	ss << action << " item id " << id << " name '" << name << "' weight "
-	   << weight << " at location " << loc << ": no change";
-	break;
-      }
-      if (err > 0) {
-	pending_inc.crush.clear();
-	newcrush.encode(pending_inc.crush);
-	ss << action << " item id " << id << " name '" << name << "' weight "
-	   << weight << " at location " << loc << " to crush map";
-	getline(ss, rs);
-	wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-	return true;
-      }
-    } while (false);
+    dout(5) << "adding/updating crush item id " << id << " name '"
+      << name << "' weight " << weight << " at location "
+      << loc << dendl;
+    CrushWrapper newcrush;
+    _get_pending_crush(newcrush);
+
+    string action;
+    if (prefix == "osd crush set" ||
+        newcrush.check_item_loc(g_ceph_context, id, loc, (int *)NULL)) {
+      action = "set";
+      err = newcrush.update_item(g_ceph_context, id, weight, name, loc);
+    } else {
+      action = "add";
+      err = newcrush.insert_item(g_ceph_context, id, weight, name, loc);
+      if (err == 0)
+        err = 1;
+    }
+
+    if (err < 0)
+      goto reply;
+
+    if (err == 0 && !_have_pending_crush()) {
+      ss << action << " item id " << id << " name '" << name << "' weight "
+        << weight << " at location " << loc << ": no change";
+      goto reply;
+    }
+
+    pending_inc.crush.clear();
+    newcrush.encode(pending_inc.crush);
+    ss << action << " item id " << id << " name '" << name << "' weight "
+      << weight << " at location " << loc << " to crush map";
+    getline(ss, rs);
+    wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
+    return true;
 
   } else if (prefix == "osd crush create-or-move") {
     do {
@@ -2819,41 +2859,51 @@ bool OSDMonitor::prepare_command(MMonCommand *m)
     } while (false);
 
   } else if (prefix == "osd crush link") {
-    do {
-      // osd crush link <name> <loc1> [<loc2> ...]
-      string name;
-      cmd_getval(g_ceph_context, cmdmap, "name", name);
-      vector<string> argvec;
-      cmd_getval(g_ceph_context, cmdmap, "args", argvec);
-      map<string,string> loc;
-      parse_loc_map(argvec, &loc);
+    // osd crush link <name> <loc1> [<loc2> ...]
+    string name;
+    cmd_getval(g_ceph_context, cmdmap, "name", name);
+    vector<string> argvec;
+    cmd_getval(g_ceph_context, cmdmap, "args", argvec);
+    map<string,string> loc;
+    parse_loc_map(argvec, &loc);
 
-      dout(0) << "linking crush item name '" << name << "' at location " << loc << dendl;
-      CrushWrapper newcrush;
-      _get_pending_crush(newcrush);
+    if (!osdmap.crush->name_exists(name)) {
+      err = -ENOENT;
+      ss << "item " << name << " does not exist";
+      goto reply;
+    }
+    if (osdmap.crush->check_item_loc(g_ceph_context, id, loc, (int*) NULL)) {
+      ss << "no need to move item id " << id << " name '" << name
+	 << "' to location " << loc << " in crush map";
+      err = 0;
+      goto reply;
+    }
 
-      if (!newcrush.name_exists(name)) {
-	err = -ENOENT;
-	ss << "item " << name << " does not exist";
-	break;
-      }
-      int id = newcrush.get_item_id(name);
+    dout(5) << "linking crush item name '" << name << "' at location " << loc << dendl;
+    CrushWrapper newcrush;
+    _get_pending_crush(newcrush);
 
+    if (!newcrush.name_exists(name)) {
+      err = -ENOENT;
+      ss << "item " << name << " does not exist";
+    } else {
+      int id = newcrush.get_item_id(name);
       if (!newcrush.check_item_loc(g_ceph_context, id, loc, (int *)NULL)) {
 	err = newcrush.link_bucket(g_ceph_context, id, loc);
 	if (err >= 0) {
-	  ss << "linked item id " << id << " name '" << name << "' to location " << loc << " in crush map";
+	  ss << "linked item id " << id << " name '" << name
+             << "' to location " << loc << " in crush map";
 	  pending_inc.crush.clear();
 	  newcrush.encode(pending_inc.crush);
-	  getline(ss, rs);
-	  wait_for_finished_proposal(new Monitor::C_Command(mon, m, 0, rs, get_last_committed()));
-	  return true;
 	}
       } else {
-	ss << "no need to move item id " << id << " name '" << name << "' to location " << loc << " in crush map";
+	ss << "no need to move item id " << id << " name '" << name
+           << "' to location " << loc << " in crush map";
 	err = 0;
       }
-    } while (false);
+    }
+    wait_for_finished_proposal(new Monitor::C_Command(mon, m, err, ss.str(), get_last_committed()));
+    return true;
   } else if (prefix == "osd crush rm" ||
 	     prefix == "osd crush remove" ||
 	     prefix == "osd crush unlink") {
diff --git a/src/vstart.sh b/src/vstart.sh
index 577ea4c843d..7ce4628d775 100755
--- a/src/vstart.sh
+++ b/src/vstart.sh
@@ -422,7 +422,7 @@ EOF
 	    uuid=`uuidgen`
 	    echo "add osd$osd $uuid"
 	    $SUDO $CEPH_ADM osd create $uuid
-	    $SUDO $CEPH_ADM osd crush set osd.$osd 1.0 host=localhost rack=localrack root=default
+	    $SUDO $CEPH_ADM osd crush add osd.$osd 1.0 host=localhost rack=localrack root=default
 	    $SUDO $CEPH_BIN/ceph-osd -i $osd $ARGS --mkfs --mkkey --osd-uuid $uuid
 
 	    key_fn=dev/osd$osd/keyring