mon/MonClient: respect the priority in mon_info_t

* common/dns_resolve: collect the priority in the SRV records
  also
* mon/MonClient: only connect to the mon with highest priority
  (in context of SRV record, the lowest priority value), and prefer
  the mon with higher weight.

Fixes: http://tracker.ceph.com/issues/5249
Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/common/dns_resolve.cc b/src/common/dns_resolve.cc
index 5049be9eaa086f6289cf17829a011ef6889eb7da..c94d8a448eb55f6b2dbe32e29a3ddc647f74a2cd 100644 (file)
--- a/src/common/dns_resolve.cc
+++ b/src/common/dns_resolve.cc
@@ -258,13 +258,14 @@ int DNSResolver::resolve_ip_addr(CephContext *cct, res_state *res, const string&
 }
 
 int DNSResolver::resolve_srv_hosts(CephContext *cct, const string& service_name, 
-    const SRV_Protocol trans_protocol, map<string, entity_addr_t> *srv_hosts) {
+    const SRV_Protocol trans_protocol,
+    map<string, DNSResolver::Record> *srv_hosts) {
   return this->resolve_srv_hosts(cct, service_name, trans_protocol, "", srv_hosts);
 }
 
 int DNSResolver::resolve_srv_hosts(CephContext *cct, const string& service_name, 
     const SRV_Protocol trans_protocol, const string& domain,
-    map<string, entity_addr_t> *srv_hosts) {
+    map<string, DNSResolver::Record> *srv_hosts) {
 
 #ifdef HAVE_RES_NQUERY
   res_state res;
@@ -331,11 +332,13 @@ int DNSResolver::resolve_srv_hosts(CephContext *cct, const string& service_name,
     string srv_domain = full_srv_name.substr(full_srv_name.find(protocol)
         + protocol.length());
 
-    int port = ns_get16(ns_rr_rdata(rr) + (NS_INT16SZ * 2)); /* port = rdata + priority + weight */
+    auto rdata = ns_rr_rdata(rr);
+    uint16_t priority = ns_get16(rdata); rdata += NS_INT16SZ;
+    rdata += NS_INT16SZ;       // weight
+    uint16_t port = ns_get16(rdata); rdata += NS_INT16SZ;
     memset(full_target, 0, sizeof(full_target));
     ns_name_uncompress(ns_msg_base(handle), ns_msg_end(handle),
-                       ns_rr_rdata(rr) + (NS_INT16SZ * 3), /* comp_dn = rdata + priority + weight + port */
-                       full_target, sizeof(full_target));
+                       rdata, full_target, sizeof(full_target));
 
     entity_addr_t addr;
 #ifdef HAVE_RES_NQUERY
@@ -349,12 +352,10 @@ int DNSResolver::resolve_srv_hosts(CephContext *cct, const string& service_name,
       string target = full_target;
       assert(target.find(srv_domain) != target.npos);
       target = target.substr(0, target.find(srv_domain));
-      (*srv_hosts)[target] = addr;
+      (*srv_hosts)[target] = {priority, addr};
     }
   }
-
   return 0;
 }
 
 }
-

diff --git a/src/common/dns_resolve.h b/src/common/dns_resolve.h
index 3e8f7e46302189804f5f9443815dc37ebf08c89b..e587224d84db7a9fe313c1bfc0a113ad4da25c3d 100644 (file)
--- a/src/common/dns_resolve.h
+++ b/src/common/dns_resolve.h
@@ -18,8 +18,7 @@
 #include <resolv.h>
 
 #include "common/Mutex.h"
-
-struct entity_addr_t;
+#include "msg/msg_types.h"             // for entity_addr_t
 
 namespace ceph {
 
@@ -78,6 +77,11 @@ class DNSResolver {
     };
 
 
+    struct Record {
+      uint16_t priority;
+      entity_addr_t addr;
+    };
+
     int resolve_cname(CephContext *cct, const std::string& hostname,
         std::string *cname, bool *found);
 
@@ -103,7 +107,7 @@ class DNSResolver {
      * @returns 0 on success, negative error code on failure
      */
     int resolve_srv_hosts(CephContext *cct, const std::string& service_name,
-        const SRV_Protocol trans_protocol, std::map<std::string, entity_addr_t> *srv_hosts);
+        const SRV_Protocol trans_protocol, std::map<std::string, Record> *srv_hosts);
 
     /**
      * Returns the list of hostnames and addresses that provide a given
@@ -119,7 +123,7 @@ class DNSResolver {
      */
     int resolve_srv_hosts(CephContext *cct, const std::string& service_name,
         const SRV_Protocol trans_protocol, const std::string& domain,
-        std::map<std::string, entity_addr_t> *srv_hosts);
+        std::map<std::string, Record> *srv_hosts);
 
   private:
     DNSResolver() : lock("DNSResolver") { resolv_h = new ResolvHWrapper(); }

diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc
index 6f9ab9e4ad67aa1313446aa77c4bf7ca8b2bd78c..bc5626dad58e41687cc7b8b4c97bfc8929c09448 100644 (file)
--- a/src/mon/MonClient.cc
+++ b/src/mon/MonClient.cc
@@ -639,18 +639,24 @@ MonConnection& MonClient::_add_conn(unsigned rank, uint64_t global_id)
 
 void MonClient::_add_conns(uint64_t global_id)
 {
-  const unsigned num_mons = monmap.size();
-  vector<unsigned> ranks(num_mons);
-  for (unsigned i = 0; i < num_mons; i++) {
-    ranks[i] = i;
+  uint16_t min_priority = std::numeric_limits<uint16_t>::max();
+  for (const auto& m : monmap.mon_info) {
+    if (m.second.priority < min_priority) {
+      min_priority = m.second.priority;
+    }
+  }
+  vector<unsigned> ranks;
+  for (const auto& m : monmap.mon_info) {
+    if (m.second.priority == min_priority) {
+      ranks.push_back(monmap.get_rank(m.first));
+    }
   }
   std::random_device rd;
   std::mt19937 rng(rd());
   std::shuffle(ranks.begin(), ranks.end(), rng);
-
   unsigned n = cct->_conf->mon_client_hunt_parallel;
-  if (n == 0 || n > monmap.size()) {
-     n = num_mons;
+  if (n == 0 || n > ranks.size()) {
+    n = ranks.size();
   }
   for (unsigned i = 0; i < n; i++) {
     _add_conn(ranks[i], global_id);

diff --git a/src/test/common/dns_resolve.cc b/src/test/common/dns_resolve.cc
index 275479694cec8e2819a8daa1b508ee9425cc883d..2b2ca92854c3be9e2e1164e0e7f793887ac9d6b6 100644 (file)
--- a/src/test/common/dns_resolve.cc
+++ b/src/test/common/dns_resolve.cc
@@ -135,25 +135,25 @@ TEST_F(DNSResolverTest, resolve_srv_hosts_empty_domain) {
 #endif
   }
 
-  map<string, entity_addr_t> addrs;
+  map<string, DNSResolver::Record> records;
   DNSResolver::get_instance(resolvH)->resolve_srv_hosts(g_ceph_context, "cephmon", 
-      DNSResolver::SRV_Protocol::TCP, &addrs);
+      DNSResolver::SRV_Protocol::TCP, &records);
 
-  ASSERT_EQ(addrs.size(), (unsigned int)3);
-  map<string,entity_addr_t>::iterator it = addrs.find("mon.a");
-  ASSERT_NE(it, addrs.end());
+  ASSERT_EQ(records.size(), (unsigned int)3);
+  auto it = records.find("mon.a");
+  ASSERT_NE(it, records.end());
   std::ostringstream os;
-  os << it->second;
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.11:6789/0");
   os.str("");
-  it = addrs.find("mon.b");
-  ASSERT_NE(it, addrs.end());
-  os << it->second;
+  it = records.find("mon.b");
+  ASSERT_NE(it, records.end());
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.12:6789/0");
   os.str("");
-  it = addrs.find("mon.c");
-  ASSERT_NE(it, addrs.end());
-  os << it->second;
+  it = records.find("mon.c");
+  ASSERT_NE(it, records.end());
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.13:6789/0");
 }
 
@@ -205,25 +205,25 @@ TEST_F(DNSResolverTest, resolve_srv_hosts_full_domain) {
 #endif
   }
 
-  map<string, entity_addr_t> addrs;
+  map<string, DNSResolver::Record> records;
   DNSResolver::get_instance(resolvH)->resolve_srv_hosts(g_ceph_context, "cephmon", 
-      DNSResolver::SRV_Protocol::TCP, "ceph.com", &addrs);
+      DNSResolver::SRV_Protocol::TCP, "ceph.com", &records);
 
-  ASSERT_EQ(addrs.size(), (unsigned int)3);
-  map<string,entity_addr_t>::iterator it = addrs.find("mon.a");
-  ASSERT_NE(it, addrs.end());
+  ASSERT_EQ(records.size(), (unsigned int)3);
+  auto it = records.find("mon.a");
+  ASSERT_NE(it, records.end());
   std::ostringstream os;
-  os << it->second;
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.11:6789/0");
   os.str("");
-  it = addrs.find("mon.b");
-  ASSERT_NE(it, addrs.end());
-  os << it->second;
+  it = records.find("mon.b");
+  ASSERT_NE(it, records.end());
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.12:6789/0");
   os.str("");
-  it = addrs.find("mon.c");
-  ASSERT_NE(it, addrs.end());
-  os << it->second;
+  it = records.find("mon.c");
+  ASSERT_NE(it, records.end());
+  os << it->second.addr;
   ASSERT_EQ(os.str(), "192.168.1.13:6789/0");
 }
 
@@ -244,11 +244,11 @@ TEST_F(DNSResolverTest, resolve_srv_hosts_fail) {
 #endif
   }
 
-  map<string, entity_addr_t> addrs;
+  map<string, DNSResolver::Record> records;
   int ret = DNSResolver::get_instance(resolvH)->resolve_srv_hosts(
-      g_ceph_context, "noservice", DNSResolver::SRV_Protocol::TCP, "", &addrs);
+      g_ceph_context, "noservice", DNSResolver::SRV_Protocol::TCP, "", &records);
 
-  ASSERT_EQ(ret, 0);
-  ASSERT_EQ(addrs.size(), (unsigned int)0);
+  ASSERT_EQ(0, ret);
+  ASSERT_TRUE(records.empty());
 }