From 1881d9abbad11a76c693cafc7b1b51598c71ced8 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@redhat.com>
Date: Sun, 27 Jan 2019 07:32:21 -0600
Subject: [PATCH] msg/async: fix should_use_msgr2 behavior (including monc)

Be consistent about whether we should use v2 to connect or not, and fix
teh monclient check to use the same logic.

Signed-off-by: Sage Weil <sage@redhat.com>
---
 src/mon/MonClient.cc            | 12 ++++++------
 src/msg/Messenger.h             |  5 +++++
 src/msg/async/AsyncMessenger.cc | 21 +++++++++++++--------
 src/msg/async/AsyncMessenger.h  |  2 ++
 4 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc
index 6917ab2a1bdec..d628e819d9f65 100644
--- a/src/mon/MonClient.cc
+++ b/src/mon/MonClient.cc
@@ -382,12 +382,12 @@ void MonClient::handle_monmap(MMonMap *m)
 		     << " went away" << dendl;
       // can't find the mon we were talking to (above)
       _reopen_session();
-    } else if (monmap.get_addrs(new_rank) != con_addrs) {
-      // FIXME: we might make this a more sophisticated check later if we do
-      // multiprotocol IPV4/IPV6 and have a strict preference
-      ldout(cct,10) << " mon." << new_rank << " has addrs "
-		    << monmap.get_addrs(new_rank) << " but i'm connected to "
-		    << con_addrs << dendl;
+    } else if (messenger->should_use_msgr2() &&
+	       monmap.get_addrs(new_rank).has_msgr2() &&
+	       !con_addrs.has_msgr2()) {
+      ldout(cct,1) << " mon." << new_rank << " has (v2) addrs "
+		   << monmap.get_addrs(new_rank) << " but i'm connected to "
+		   << con_addrs << ", reconnecting" << dendl;
       _reopen_session();
     }
   }
diff --git a/src/msg/Messenger.h b/src/msg/Messenger.h
index 4c41dfce95ed8..fd23813418ad0 100644
--- a/src/msg/Messenger.h
+++ b/src/msg/Messenger.h
@@ -390,6 +390,11 @@ public:
 
   virtual int bindv(const entity_addrvec_t& addrs);
 
+
+  virtual bool should_use_msgr2() {
+    return false;
+  }
+
   /**
    * @} // Configuration
    */
diff --git a/src/msg/async/AsyncMessenger.cc b/src/msg/async/AsyncMessenger.cc
index 908052f11c41c..dba88e886f1b4 100644
--- a/src/msg/async/AsyncMessenger.cc
+++ b/src/msg/async/AsyncMessenger.cc
@@ -607,17 +607,22 @@ ConnectionRef AsyncMessenger::get_loopback_connection()
   return local_connection;
 }
 
+bool AsyncMessenger::should_use_msgr2()
+{
+  // if we are bound to v1 only, and we are connecting to a v2 peer,
+  // we cannot use the peer's v2 address. otherwise the connection
+  // is assymetrical, because they would have to use v1 to connect
+  // to us, and we would use v2, and connection race detection etc
+  // would totally break down (among other things).  or, the other
+  // end will be confused that we advertise ourselve with a v1
+  // address only (that we bound to) but connected with protocol v2.
+  return !did_bind || get_myaddrs().has_msgr2();
+}
+
 entity_addrvec_t AsyncMessenger::_filter_addrs(int type,
 					       const entity_addrvec_t& addrs)
 {
-  if (did_bind &&
-      !get_myaddrs().has_msgr2() &&
-      get_mytype() == type) {
-    // if we are bound to v1 only, and we are connecting to a peer, we cannot
-    // use the peer's v2 address (yet). otherwise the connection is assymetrical,
-    // because they would have to use v1 to connect to us, and we would use v2,
-    // and connection race detection etc would totally break down (among other
-    // things).
+  if (!should_use_msgr2()) {
     ldout(cct, 10) << __func__ << " " << addrs << " type " << type
 		   << " limiting to v1 ()" << dendl;
     entity_addrvec_t r;
diff --git a/src/msg/async/AsyncMessenger.h b/src/msg/async/AsyncMessenger.h
index 79242be035fdd..f13e30c0b962d 100644
--- a/src/msg/async/AsyncMessenger.h
+++ b/src/msg/async/AsyncMessenger.h
@@ -121,6 +121,8 @@ public:
 
   int bindv(const entity_addrvec_t& bind_addrs) override;
 
+  bool should_use_msgr2() override;
+
   /** @} Configuration functions */
 
   /**
-- 
2.39.5