Merge branch '22.08' into 23.02

server/modules/monitor/mariadbmon/mariadbmon.cc

@@ -499,9 +499,7 @@ bool MariaDBMonitor::Settings::post_configure(const std::map<std::string,
         warn_and_enable(&auto_rejoin, CN_AUTO_REJOIN);
     }
 
-    shared.auto_op_configured = auto_failover | auto_rejoin | switchover_on_low_disk_space
-        | enforce_read_only_slaves | enforce_read_only_servers | enforce_writable_master
-        | enforce_simple_topology;
+    shared.auto_op_configured = m_monitor->cluster_ops_configured();
     return m_monitor->post_configure();
 }
 
@@ -737,9 +735,13 @@ void MariaDBMonitor::tick()
     bool first_tick = ticks_complete() == 0;
     bool should_update_disk_space = check_disk_space_this_tick();
 
-    // Concurrently query all servers for their status.
-    auto update_task = [should_update_disk_space, first_tick](MariaDBServer* server) {
-        server->update_server(should_update_disk_space, first_tick);
+    // Concurrently query all servers for their status. Force a reconnect on all servers if a command failed
+    // due to a missing privilege. Reconnecting refreshes connection privileges.
+    bool reconnect = m_grant_fail;
+    m_grant_fail = false;
+
+    auto update_task = [should_update_disk_space, first_tick, reconnect](MariaDBServer* server) {
+        server->update_server(should_update_disk_space, first_tick, reconnect);
     };
     execute_task_all_servers(update_task);
 
@@ -752,6 +754,11 @@ void MariaDBMonitor::tick()
             m_cluster_topology_changed = true;
             server->m_topology_changed = false;
         }
+        if (server->m_cmd_grant_fail)
+        {
+            m_grant_fail = true;
+            server->m_cmd_grant_fail = false;
+        }
     }
     update_topology();
 
@@ -1230,8 +1237,8 @@ bool MariaDBMonitor::ClusterLocksInfo::time_to_update() const
 bool MariaDBMonitor::cluster_ops_configured() const
 {
     return m_settings.auto_failover || m_settings.auto_rejoin
-           || m_settings.enforce_read_only_slaves || m_settings.enforce_writable_master
-           || m_settings.switchover_on_low_disk_space;
+           || m_settings.enforce_read_only_slaves || m_settings.enforce_read_only_servers
+           || m_settings.enforce_writable_master || m_settings.switchover_on_low_disk_space;
 }
 
 namespace journal_fields

server/modules/monitor/mariadbmon/mariadbmon.hh

@@ -386,6 +386,8 @@ private:
                                              * Causes a topology rebuild on the current tick. */
     bool m_cluster_modified = false;        /* Has a cluster operation been performed this loop? Prevents
                                              * other operations during this tick. */
+    bool m_grant_fail = false;              /* Did an operation fail because of missing privs? Causes a
+                                             * reconnection on all servers at start of next tick. */
 
     DNSResolver m_resolver;                 /* DNS-resolver with cache */
 

server/modules/monitor/mariadbmon/mariadbserver.cc

@@ -2403,13 +2403,12 @@ bool MariaDBServer::update_enabled_events()
  *
  * @param server The server to update
  */
-void MariaDBServer::update_server(bool time_to_update_disk_space, bool first_tick)
+void MariaDBServer::update_server(bool time_to_update_disk_space, bool first_tick, bool reconnect)
 {
     m_new_events.clear();
-    if (m_cmd_grant_fail)
+    if (reconnect)
     {
         close_conn();
-        m_cmd_grant_fail = false;
     }
     ConnectResult conn_status = ping_or_connect();
 

server/modules/monitor/mariadbmon/mariadbserver.hh

@@ -197,7 +197,7 @@ public:
      */
     json_t* to_json() const;
 
-    void update_server(bool time_to_update_disk_space, bool first_tick);
+    void update_server(bool time_to_update_disk_space, bool first_tick, bool reconnect);
 
     std::string print_changed_slave_connections();
 

server/modules/monitor/mariadbmon/monitor_commands.cc

@@ -2212,7 +2212,7 @@ bool BackupOperation::start_replication(MariaDBServer* target, MariaDBServer* re
     bool replication_confirmed = false;
     if (!gtid.empty())
     {
-        target->update_server(false, false);
+        target->update_server(false, false, false);
         if (target->is_running())
         {
             string errmsg;
@@ -2328,7 +2328,7 @@ void BackupOperation::cleanup(MariaDBServer* source, const SlaveStatusArray& sou
         // Source server replication was stopped when starting Mariabackup. Mariabackup does not restart the
         // connections if it quits in error or is killed. Check that any slave connections are running as
         // before.
-        source->update_server(false, false);
+        source->update_server(false, false, false);
         if (source->is_running())
         {
             const auto& new_slaves = source->m_slave_status;

system-test/mariadbmonitor/mysqlmon_switchover.cc

@@ -62,42 +62,37 @@ void insert_data(TestConnections& test)
 void run(TestConnections& test)
 {
     auto& mxs = *test.maxscale;
-    mxs.wait_for_monitor();
+    auto& repl = *test.repl;
 
-    const int N = 4;
+    mxs.wait_for_monitor();
 
-    auto master = mxt::ServerInfo::MASTER | mxt::ServerInfo::RUNNING;
-    auto slave = mxt::ServerInfo::SLAVE | mxt::ServerInfo::RUNNING;
-    auto normal_status = {master, slave, slave, slave};
+    auto master = mxt::ServerInfo::master_st;
+    auto slave = mxt::ServerInfo::slave_st;
+    auto normal_status = mxt::ServersInfo::default_repl_states();
     mxs.check_servers_status(normal_status);
 
-    cout << "\nConnecting to MaxScale." << endl;
-    test.maxscale->connect_maxscale();
+    mxs.connect_maxscale();
 
-    cout << "\nCreating table." << endl;
+    test.tprintf("Creating table.");
     create_table(test);
 
-    cout << "\nInserting data." << endl;
+    test.tprintf("Inserting data.");
     insert_data(test);
 
-    cout << "\nSyncing slaves." << endl;
-    test.repl->sync_slaves();
-
-    cout << "\nTrying to do manual switchover to server2" << endl;
+    test.tprintf("Trying to do manual switchover to server2");
     test.maxctrl("call command mysqlmon switchover MySQL-Monitor server2 server1");
 
     mxs.wait_for_monitor();
     mxs.check_servers_status({slave, master, slave, slave});
 
     if (test.ok())
     {
-        cout << "\nSwitchover success. Resetting situation using async-switchover\n";
+        test.tprintf("Switchover success. Resetting situation using async-switchover.");
         test.maxctrl("call command mariadbmon async-switchover MySQL-Monitor server1");
         // Wait a bit so switch completes, then fetch results.
         mxs.wait_for_monitor(2);
         auto res = test.maxctrl("call command mariadbmon fetch-cmd-result MySQL-Monitor");
-        const char cmdname[] = "fetch-cmd-results";
-        test.expect(res.rc == 0, "%s failed: %s", cmdname, res.output.c_str());
+        test.expect(res.rc == 0, "fetch-cmd-result failed: %s", res.output.c_str());
         if (test.ok())
         {
             // The output is a json string. Check that it includes "switchover completed successfully".
@@ -107,14 +102,90 @@ void run(TestConnections& test)
         }
         mxs.check_servers_status(normal_status);
     }
+
+    if (test.ok())
+    {
+        test.tprintf("MXS-4605: Monitor should reconnect if command fails due to missing privileges.");
+        mxs.stop();
+        auto* master_srv = repl.backend(0);
+        auto conn = master_srv->open_connection();
+        conn->cmd_f("grant slave monitor on *.* to mariadbmon;");
+        conn->cmd_f("revoke super, read_only admin on *.* from mariadbmon;");
+        repl.sync_slaves();
+        // Close connections so monitor does not attempt to kill them.
+        conn = nullptr;
+        repl.close_connections();
+        repl.close_admin_connections();
+
+        mxs.start();
+
+        mxs.check_servers_status(normal_status);
+        if (test.ok())
+        {
+            auto try_switchover = [&](bool expect_success) {
+                const string switch_cmd = "call command mysqlmon switchover MySQL-Monitor server2";
+                auto res = test.maxctrl(switch_cmd);
+                if (expect_success)
+                {
+                    if (res.rc == 0)
+                    {
+                        test.tprintf("Switchover succeeded.");
+                    }
+                    else
+                    {
+                        test.add_failure("Switchover failed. Error: %s", res.output.c_str());
+                    }
+                }
+                else
+                {
+                    if (res.rc == 0)
+                    {
+                        test.add_failure("Switchover succeeded when it should have failed.");
+                    }
+                    else
+                    {
+                        test.tprintf("Switchover failed as expected. Error: %s", res.output.c_str());
+                        test.expect(res.output.find("Failed to enable read_only on") != string::npos,
+                                    "Did not find expected error message.");
+                        mxs.check_print_servers_status(normal_status);
+                    }
+                }
+                mxs.wait_for_monitor();
+            };
+
+            test.tprintf("Trying switchover, it should fail due to missing privs.");
+            try_switchover(false);
+
+            if (test.ok())
+            {
+                conn = master_srv->open_connection();
+                conn->cmd_f("grant super, read_only admin on *.* to mariadbmon;");
+                conn = nullptr;
+
+                repl.sync_slaves();
+                repl.close_admin_connections();
+
+                test.tprintf("Privileges granted. Switchover should still fail, as monitor connections are "
+                             "using the grants of their creation time.");
+                try_switchover(false);
+
+                test.tprintf("Switchover should now work.");
+                try_switchover(true);
+
+                mxs.check_print_servers_status({slave, master, slave, slave});
+            }
+        }
+
+        if (!test.ok())
+        {
+            conn = master_srv->open_connection();
+            conn->cmd_f("grant super, read_only admin on *.* to mariadbmon;");
+        }
+    }
 }
 }
 
 int main(int argc, char* argv[])
 {
-    TestConnections test(argc, argv);
-
-    run(test);
-
-    return test.global_result;
+    return TestConnections().run_test(argc, argv, run);
 }

system-test/mariadbmonitor/mysqlmon_switchover.cnf

@@ -9,7 +9,7 @@ user=mariadbmon
 password=mariadbmon
 monitor_interval=1000ms
 auto_failover=false
-auto_rejoin=true
+auto_rejoin=false
 replication_user=repl
 replication_password=repl
 backend_connect_timeout=10s
@@ -20,43 +20,15 @@ backend_write_timeout=10s
 type=service
 router= readwritesplit
 servers=server1, server2, server3, server4
-user=maxskysql
-password=skysql
-
-[Read-Connection-Router-Slave]
-type=service
-router=readconnroute
-router_options= slave
-servers=server1, server2, server3, server4
-user=maxskysql
-password=skysql
-
-[Read-Connection-Router-Master]
-type=service
-router=readconnroute
-router_options=master
-servers=server1, server2, server3, server4
-user=maxskysql
-password=skysql
+user=maxservice
+password=maxservice
 
 [RW-Split-Listener]
 type=listener
 service=RW-Split-Router
 protocol=MySQLClient
 port=4006
 
-[Read-Connection-Listener-Slave]
-type=listener
-service=Read-Connection-Router-Slave
-protocol=MySQLClient
-port=4009
-
-[Read-Connection-Listener-Master]
-type=listener
-service=Read-Connection-Router-Master
-protocol=MySQLClient
-port=4008
-
 [server1]
 type=server
 address=###node_server_IP_1###

system-test/maxtest/include/maxtest/mariadb_nodes.hh

@@ -108,6 +108,7 @@ public:
     mxt::MariaDB* admin_connection();
 
     bool ping_or_open_admin_connection();
+    void close_admin_connection();
 
     bool               update_status();
     const Status&      status() const;
@@ -480,7 +481,8 @@ public:
      *
      * @return Number of successful connections
      */
-    int ping_or_open_admin_connections();
+    int  ping_or_open_admin_connections();
+    void close_admin_connections();
 
     bool ssl() const;
     bool using_ipv6() const;

system-test/maxtest/src/mariadb_nodes.cc

@@ -1233,6 +1233,14 @@ int MariaDBCluster::ping_or_open_admin_connections()
     return rval;
 }
 
+void MariaDBCluster::close_admin_connections()
+{
+    for (auto& be : m_backends)
+    {
+        be->close_admin_connection();
+    }
+}
+
 bool MariaDBCluster::run_on_every_backend(const std::function<bool(int)>& func)
 {
     mxt::BoolFuncArray funcs;
@@ -1528,6 +1536,11 @@ bool MariaDBServer::ping_or_open_admin_connection()
     return rval;
 }
 
+void MariaDBServer::close_admin_connection()
+{
+    m_admin_conn = nullptr;
+}
+
 MariaDBServer::Version MariaDBServer::version()
 {
     auto v = m_status.version_num;