MXS-2546 Save master server to slave connection object

Simplifies later checks and removes duplicate code. The "assume_unique_hostnames"-
setting is no longer required within the MariaDBServer-class.

server/modules/monitor/mariadbmon/cluster_discovery.cc

@@ -166,7 +166,7 @@ void MariaDBMonitor::tarjan_scc_visit_node(MariaDBServer* node,
  */
 void MariaDBMonitor::build_replication_graph()
 {
-    const bool use_hostnames = m_settings.shared.assume_unique_hostnames;
+    const bool use_hostnames = m_settings.assume_unique_hostnames;
     // First, reset all node data.
     for (MariaDBServer* server : m_servers)
     {
@@ -174,12 +174,13 @@ void MariaDBMonitor::build_replication_graph()
         server->m_node.reset_results();
     }
 
-    for (MariaDBServer* slave : m_servers)
+    for (auto slave : m_servers)
     {
         /* Check all slave connections of all servers. Connections are added even if one or both endpoints
          * are down or in maintenance. */
-        for (SlaveStatus& slave_conn : slave->m_slave_status)
+        for (auto& slave_conn : slave->m_slave_status)
         {
+            slave_conn.master_server = nullptr;
             /* IF THIS PART IS CHANGED, CHANGE THE COMPARISON IN 'sstatus_arrays_topology_equal'
              * (in MariaDBServer) accordingly so that any possible topology changes are detected. */
             if (slave_conn.slave_io_running != SlaveStatus::SLAVE_IO_NO && slave_conn.slave_sql_running)
@@ -189,8 +190,7 @@ void MariaDBMonitor::build_replication_graph()
                 bool is_external = false;
                 if (use_hostnames)
                 {
-                    found_master = get_server(slave_conn.settings.master_endpoint.host(),
+                    found_master = get_server(slave_conn.settings.master_endpoint);
-                                              slave_conn.settings.master_endpoint.port());
                     if (!found_master)
                     {
                         // Must be an external server.
@@ -203,9 +203,11 @@ void MariaDBMonitor::build_replication_graph()
                      * trust the "Master_Server_Id"-field of the SHOW SLAVE STATUS output if
                      * the slave connection has been seen connected before. This means that
                      * the graph will miss slave-master relations that have not connected
-                     * while the monitor has been running. TODO: This data should be saved so
+                     * while the monitor has been running.
-                     * that monitor restarts do not lose this information. */
+                     *
-                    if (slave_conn.seen_connected)
+                     * TODO: This data should be saved so that monitor restarts do not lose
+                     * this information. */
+                    if (slave_conn.master_server_id >= 0 && slave_conn.seen_connected)
                     {
                         // Valid slave connection, find the MariaDBServer with the matching server id.
                         found_master = get_server(slave_conn.master_server_id);
@@ -227,6 +229,7 @@ void MariaDBMonitor::build_replication_graph()
                      * access later on. */
                     slave->m_node.parents.push_back(found_master);
                     found_master->m_node.children.push_back(slave);
+                    slave_conn.master_server = found_master;
                 }
                 else if (is_external)
                 {

server/modules/monitor/mariadbmon/mariadbmon.cc

@@ -104,14 +104,15 @@ void MariaDBMonitor::reset_node_index_info()
     }
 }
 
-MariaDBServer* MariaDBMonitor::get_server(const std::string& host, int port)
+MariaDBServer* MariaDBMonitor::get_server(const EndPoint& search_ep)
 {
     // TODO: Do this with a map lookup
+    // TODO: Add DNS check here.
     MariaDBServer* found = NULL;
     for (MariaDBServer* server : m_servers)
     {
-        SERVER* srv = server->m_server_base->server;
+        EndPoint srv(server->m_server_base->server);
-        if (host == srv->address && srv->port == port)
+        if (srv == search_ep)
         {
             found = server;
             break;
@@ -212,7 +213,7 @@ bool MariaDBMonitor::configure(const MXS_CONFIG_PARAMETER* params)
     m_settings.detect_stale_slave = params->get_bool("detect_stale_slave");
     m_settings.detect_standalone_master = params->get_bool(CN_DETECT_STANDALONE_MASTER);
     m_settings.ignore_external_masters = params->get_bool("ignore_external_masters");
-    m_settings.shared.assume_unique_hostnames = params->get_bool(CN_ASSUME_UNIQUE_HOSTNAMES);
+    m_settings.assume_unique_hostnames = params->get_bool(CN_ASSUME_UNIQUE_HOSTNAMES);
     m_settings.failcount = params->get_integer(CN_FAILCOUNT);
     m_settings.failover_timeout = params->get_duration<std::chrono::seconds>(CN_FAILOVER_TIMEOUT).count();
     m_settings.switchover_timeout = params->get_duration<std::chrono::seconds>(CN_SWITCHOVER_TIMEOUT).count();
@@ -272,12 +273,12 @@ bool MariaDBMonitor::configure(const MXS_CONFIG_PARAMETER* params)
             }
         };
 
-        warn_and_enable(&m_settings.shared.assume_unique_hostnames, CN_ASSUME_UNIQUE_HOSTNAMES);
+        warn_and_enable(&m_settings.assume_unique_hostnames, CN_ASSUME_UNIQUE_HOSTNAMES);
         warn_and_enable(&m_settings.auto_failover, CN_AUTO_FAILOVER);
         warn_and_enable(&m_settings.auto_rejoin, CN_AUTO_REJOIN);
     }
 
-    if (!m_settings.shared.assume_unique_hostnames)
+    if (!m_settings.assume_unique_hostnames)
     {
         const char requires[] = "%s requires that %s is on.";
         if (m_settings.auto_failover)

server/modules/monitor/mariadbmon/mariadbmon.hh

@@ -221,6 +221,8 @@ private:
                                                * TODO: think about removing */
         bool ignore_external_masters {false}; /* Ignore masters outside of the monitor configuration.
                                                * TODO: requires work */
+        bool assume_unique_hostnames {true};  /* Are server hostnames consistent between MaxScale and
+                                               * servers */
 
         int failcount {1};  /* Number of ticks master must be down before it's considered
                              * totally down, allowing failover or master change. */
@@ -264,7 +266,7 @@ private:
     std::string diagnostics_to_string() const;
     json_t*     to_json() const;
 
-    MariaDBServer* get_server(const std::string& host, int port);
+    MariaDBServer* get_server(const EndPoint& search_ep);
     MariaDBServer* get_server(int64_t id);
     MariaDBServer* get_server(mxs::MonitorServer* mon_server);
     MariaDBServer* get_server(SERVER* server);

server/modules/monitor/mariadbmon/mariadbserver.cc

@@ -369,6 +369,10 @@ bool MariaDBServer::do_show_slave_status(string* errmsg_out)
             {
                 new_row.last_data_time = old_row->last_data_time;
             }
+
+            // Copy master server pointer from old row. If this line is not reached because old row does
+            // not exist, then the topology rebuild will set the master pointer.
+            new_row.master_server = old_row->master_server;
         }
 
         // Finally, set the connection status.
@@ -1004,6 +1008,7 @@ bool MariaDBServer::sstatus_array_topology_equal(const SlaveStatusArray& new_sla
             if (new_row.slave_io_running != old_row.slave_io_running
                 || new_row.slave_sql_running != old_row.slave_sql_running
                 || new_row.settings.master_endpoint != old_row.settings.master_endpoint
+                || new_row.settings.name != old_row.settings.name
                 || new_row.master_server_id != old_row.master_server_id)
             {
                 rval = false;
@@ -1176,39 +1181,16 @@ bool MariaDBServer::can_be_promoted(OperationType op, const MariaDBServer* demot
 
 const SlaveStatus* MariaDBServer::slave_connection_status(const MariaDBServer* target) const
 {
+    mxb_assert(target);
     // The slave node may have several slave connections, need to find the one that is
-    // connected to the parent. TODO: Use the information gathered in 'build_replication_graph'
+    // connected to the parent. Most of this has already been done in 'build_replication_graph'.
-    // to skip this function, as the contents are very similar.
     const SlaveStatus* rval = NULL;
-    if (m_settings.assume_unique_hostnames)
+    for (const SlaveStatus& ss : m_slave_status)
     {
-        // Can simply compare host:port.
+        if (ss.master_server == target)
-        EndPoint target_endpoint(target->m_server_base->server);
-        for (const SlaveStatus& ss : m_slave_status)
         {
-            if (ss.settings.master_endpoint == target_endpoint && ss.slave_sql_running
+            rval = &ss;
-                && ss.slave_io_running != SlaveStatus::SLAVE_IO_NO)
+            break;
-            {
-                rval = &ss;
-                break;
-            }
-        }
-    }
-    else
-    {
-        // Compare server id:s instead. If the master's id is wrong (e.g. never updated) this gives a
-        // wrong result. Also gives wrong result if monitor has never seen the slave connection in the
-        // connected state.
-        auto target_id = target->m_server_id;
-        for (const SlaveStatus& ss : m_slave_status)
-        {
-            auto master_id = ss.master_server_id;
-            if (master_id > 0 && master_id == target_id && ss.slave_sql_running && ss.seen_connected
-                && ss.slave_io_running != SlaveStatus::SLAVE_IO_NO)
-            {
-                rval = &ss;
-                break;
-            }
         }
     }
     return rval;

server/modules/monitor/mariadbmon/server_utils.hh

@@ -239,8 +239,10 @@ public:
 
     Settings settings;  /* User-defined settings for the slave connection. */
 
-    bool     seen_connected = false;                 /* Has this slave connection been seen connected,
+    /* If the master is a monitored server, it's written here. */
-                                                      * meaning that the master server id is correct? */
+    const MariaDBServer* master_server {nullptr};
+    /* Has this slave connection been seen connected, meaning that the master server id is correct? */
+    bool seen_connected = false;
 
     int64_t master_server_id = SERVER_ID_UNKNOWN;       /* The master's server_id value. Valid ids are
                                                          * 32bit unsigned. -1 is unread/error. */
@@ -320,8 +322,4 @@ public:
     /* Should failover/switchover enable/disable any scheduled events on the servers during
      * promotion/demotion? */
     bool handle_event_scheduler {true};
-
+};
-    // Miscellaneous settings
-    bool assume_unique_hostnames {true}; /**< Are server hostnames consistent between MaxScale and
-                                           *  servers */
-};

server/modules/monitor/mariadbmon/test/test_cycle_find.cc

@@ -152,7 +152,7 @@ int MariaDBMonitor::Test::run_tests()
 void MariaDBMonitor::Test::init_servers(int count)
 {
     clear_servers();
-    m_monitor->m_settings.shared.assume_unique_hostnames = m_use_hostnames;
+    m_monitor->m_settings.assume_unique_hostnames = m_use_hostnames;
     mxb_assert(m_monitor->m_servers.empty() && m_monitor->m_servers_by_id.empty());
 
     for (int i = 1; i < count + 1; i++)
@@ -307,7 +307,7 @@ int MariaDBMonitor::Test::check_result_cycles(CycleArray expected_cycles)
 
 MariaDBServer* MariaDBMonitor::Test::get_server(int i)
 {
-    auto rval = m_use_hostnames ? m_monitor->get_server(create_hostname(i), i) :
+    auto rval = m_use_hostnames ? m_monitor->get_server(EndPoint(create_hostname(i), i)) :
         m_monitor->get_server(i);
     mxb_assert(rval);
     return rval;