diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 53b7d0329b0..2a343c9a181 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -499,9 +499,7 @@ bool MariaDBMonitor::Settings::post_configure(const std::mapcluster_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 diff --git a/server/modules/monitor/mariadbmon/mariadbmon.hh b/server/modules/monitor/mariadbmon/mariadbmon.hh index b5f6f5ebfee..8fb3b4c97b1 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.hh +++ b/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 */ diff --git a/server/modules/monitor/mariadbmon/mariadbserver.cc b/server/modules/monitor/mariadbmon/mariadbserver.cc index 44e4b010eb2..101639c0b56 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.cc +++ b/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(); diff --git a/server/modules/monitor/mariadbmon/mariadbserver.hh b/server/modules/monitor/mariadbmon/mariadbserver.hh index 3183ea90c63..8c7dccccba4 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.hh +++ b/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(); diff --git a/server/modules/monitor/mariadbmon/monitor_commands.cc b/server/modules/monitor/mariadbmon/monitor_commands.cc index 1509359016a..bcb2f130c5e 100644 --- a/server/modules/monitor/mariadbmon/monitor_commands.cc +++ b/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; diff --git a/system-test/mariadbmonitor/mysqlmon_switchover.cc b/system-test/mariadbmonitor/mysqlmon_switchover.cc index 64a4446e9e8..ff575f98b63 100644 --- a/system-test/mariadbmonitor/mysqlmon_switchover.cc +++ b/system-test/mariadbmonitor/mysqlmon_switchover.cc @@ -62,28 +62,24 @@ 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(); @@ -91,13 +87,12 @@ void run(TestConnections& test) 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); } diff --git a/system-test/mariadbmonitor/mysqlmon_switchover.cnf b/system-test/mariadbmonitor/mysqlmon_switchover.cnf index 4361cfbaab5..1a052ec477e 100644 --- a/system-test/mariadbmonitor/mysqlmon_switchover.cnf +++ b/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,24 +20,8 @@ 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 @@ -45,18 +29,6 @@ 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### diff --git a/system-test/maxtest/include/maxtest/mariadb_nodes.hh b/system-test/maxtest/include/maxtest/mariadb_nodes.hh index 0871976780a..d3b5c4fffd9 100644 --- a/system-test/maxtest/include/maxtest/mariadb_nodes.hh +++ b/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; diff --git a/system-test/maxtest/src/mariadb_nodes.cc b/system-test/maxtest/src/mariadb_nodes.cc index b537c9ccd75..6e65444cf23 100644 --- a/system-test/maxtest/src/mariadb_nodes.cc +++ b/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& 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;