From 3649efde0b7d167ceaaf2c028eaa3348e1fd4e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 16 Jul 2019 21:45:13 +0300 Subject: [PATCH 1/4] MXS-2605: Remove false debug assertion The assertion doesn't count executed session commands and thus is not reliable. --- server/modules/routing/readwritesplit/readwritesplit.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index d0e1dd887..7f13893fc 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -415,8 +415,6 @@ json_t* RWSplit::diagnostics_json() const for (const auto& a : all_server_stats()) { - mxb_assert(a.second.total == a.second.read + a.second.write); - ServerStats::CurrentStats stats = a.second.current_stats(); json_t* obj = json_object(); From f139991a2c19fa0ac58654257c00aba4eb69d487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 10 Jul 2019 08:55:16 +0300 Subject: [PATCH 2/4] MXS-2559: Log source of loaded users MySQLAuth now logs the server where the users were loaded from. As only the initial loading of users causes a log message, it is still possible for the source server to change without any indication of it. --- server/modules/authenticator/MySQLAuth/dbusers.cc | 9 +++++---- server/modules/authenticator/MySQLAuth/mysql_auth.cc | 7 +++++-- server/modules/authenticator/MySQLAuth/mysql_auth.h | 3 ++- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.cc b/server/modules/authenticator/MySQLAuth/dbusers.cc index 69f17ac35..a80ade0b8 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.cc +++ b/server/modules/authenticator/MySQLAuth/dbusers.cc @@ -136,7 +136,7 @@ const char* mariadb_users_query // We only care about users that have a default role assigned "WHERE t.default_role = u.user %s;"; -static int get_users(SERV_LISTENER* listener, bool skip_local); +static int get_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv); static MYSQL* gw_mysql_init(void); static int gw_mysql_set_timeouts(MYSQL* handle); static char* mysql_format_user_entry(void* data); @@ -192,9 +192,9 @@ static char* get_users_query(const char* server_version, int version, bool inclu return rval; } -int replace_mysql_users(SERV_LISTENER* listener, bool skip_local) +int replace_mysql_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv) { - int i = get_users(listener, skip_local); + int i = get_users(listener, skip_local, srv); return i; } @@ -1092,7 +1092,7 @@ int get_users_from_server(MYSQL* con, SERVER_REF* server_ref, SERVICE* service, * @param users The users table into which to load the users * @return -1 on any error or the number of users inserted */ -static int get_users(SERV_LISTENER* listener, bool skip_local) +static int get_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv) { const char* service_user = NULL; const char* service_passwd = NULL; @@ -1148,6 +1148,7 @@ static int get_users(SERV_LISTENER* listener, bool skip_local) if (users > total_users) { + *srv = server->server; total_users = users; } diff --git a/server/modules/authenticator/MySQLAuth/mysql_auth.cc b/server/modules/authenticator/MySQLAuth/mysql_auth.cc index e9a53ca45..5107a77aa 100644 --- a/server/modules/authenticator/MySQLAuth/mysql_auth.cc +++ b/server/modules/authenticator/MySQLAuth/mysql_auth.cc @@ -787,7 +787,8 @@ static int mysql_auth_load_users(SERV_LISTENER* port) first_load = true; } - int loaded = replace_mysql_users(port, first_load); + SERVER* srv = nullptr; + int loaded = replace_mysql_users(port, first_load, &srv); bool injected = false; if (loaded <= 0) @@ -834,7 +835,9 @@ static int mysql_auth_load_users(SERV_LISTENER* port) } else if (loaded > 0 && first_load) { - MXS_NOTICE("[%s] Loaded %d MySQL users for listener %s.", service->name, loaded, port->name); + mxb_assert(srv); + MXS_NOTICE("[%s] Loaded %d MySQL users for listener %s from server %s.", + service->name, loaded, port->name, srv->name); } return rc; diff --git a/server/modules/authenticator/MySQLAuth/mysql_auth.h b/server/modules/authenticator/MySQLAuth/mysql_auth.h index 2f4c6d986..f278ce1a0 100644 --- a/server/modules/authenticator/MySQLAuth/mysql_auth.h +++ b/server/modules/authenticator/MySQLAuth/mysql_auth.h @@ -198,10 +198,11 @@ bool dbusers_save(sqlite3* src, const char* filename); * * @param service The current service * @param skip_local Skip loading of users on local MaxScale services + * @param srv Server where the users were loaded from (output) * * @return -1 on any error or the number of users inserted (0 means no users at all) */ -int replace_mysql_users(SERV_LISTENER* listener, bool skip_local); +int replace_mysql_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv); /** * @brief Verify the user has access to the database From f8ee11cf552de31d1a33d5cfed0829402b6fb68c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 17 Jul 2019 14:42:32 +0300 Subject: [PATCH 3/4] MXS-2606: Sort servers before loading users By sorting the servers in descending order based on their role we make sure that the users are loaded from a master if one is available. --- .../authenticator/MySQLAuth/dbusers.cc | 78 +++++++++++-------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.cc b/server/modules/authenticator/MySQLAuth/dbusers.cc index a80ade0b8..1cc5f53f5 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.cc +++ b/server/modules/authenticator/MySQLAuth/dbusers.cc @@ -21,6 +21,9 @@ #include #include +#include +#include + #include #include #include @@ -1025,17 +1028,17 @@ bool query_and_process_users(const char* query, MYSQL* con, sqlite3* handle, SER return rval; } -int get_users_from_server(MYSQL* con, SERVER_REF* server_ref, SERVICE* service, SERV_LISTENER* listener) +int get_users_from_server(MYSQL* con, SERVER* server, SERVICE* service, SERV_LISTENER* listener) { - if (server_ref->server->version_string[0] == 0) + if (server->version_string[0] == 0) { - mxs_mysql_update_server_version(con, server_ref->server); + mxs_mysql_update_server_version(con, server); } - char* query = get_users_query(server_ref->server->version_string, - server_ref->server->version, + char* query = get_users_query(server->version_string, + server->version, service->enable_root, - roles_are_available(con, service, server_ref->server)); + roles_are_available(con, service, server)); MYSQL_AUTH* instance = (MYSQL_AUTH*)listener->auth_instance; sqlite3* handle = get_handle(instance); @@ -1043,20 +1046,20 @@ int get_users_from_server(MYSQL* con, SERVER_REF* server_ref, SERVICE* service, bool rv = query_and_process_users(query, con, handle, service, &users); - if (!rv && have_mdev13453_problem(con, server_ref->server)) + if (!rv && have_mdev13453_problem(con, server)) { /** * Try to work around MDEV-13453 by using a query without CTEs. Masquerading as * a 10.1.10 server makes sure CTEs aren't used. */ MXS_FREE(query); - query = get_users_query(server_ref->server->version_string, 100110, service->enable_root, true); + query = get_users_query(server->version_string, 100110, service->enable_root, true); rv = query_and_process_users(query, con, handle, service, &users); } if (!rv) { - MXS_ERROR("Failed to load users from server '%s': %s", server_ref->server->name, mysql_error(con)); + MXS_ERROR("Failed to load users from server '%s': %s", server->name, mysql_error(con)); } MXS_FREE(query); @@ -1084,6 +1087,28 @@ int get_users_from_server(MYSQL* con, SERVER_REF* server_ref, SERVICE* service, return users; } +// Sorts candidates servers so that masters are before slaves which are before only running servers +static std::vector get_candidates(SERVICE* service, bool skip_local) +{ + std::vector candidates; + + for (auto server = service->dbref; server; server = server->next) + { + if (SERVER_REF_IS_ACTIVE(server) && server_is_running(server->server) + && (!skip_local || !server_is_mxs_service(server->server))) + { + candidates.push_back(server->server); + } + } + + std::sort(candidates.begin(), candidates.end(), [](SERVER* a, SERVER* b) { + return (server_is_master(a) && !server_is_master(b)) + || (server_is_slave(a) && !server_is_slave(b) && !server_is_master(b)); + }); + + return candidates; +} + /** * Load the user/passwd form mysql.user table into the service users' hashtable * environment. @@ -1112,33 +1137,18 @@ static int get_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv) sqlite3* handle = get_handle(instance); delete_mysql_users(handle); - SERVER_REF* server = service->dbref; int total_users = -1; - bool no_active_servers = true; + auto candidates = get_candidates(service, skip_local); - for (server = service->dbref; !maxscale_is_shutting_down() && server; server = server->next) + for (auto server : candidates) { - if (!SERVER_REF_IS_ACTIVE(server) || !server_is_active(server->server) - || (skip_local && server_is_mxs_service(server->server)) - || !server_is_running(server->server)) + if (MYSQL* con = gw_mysql_init()) { - continue; - } - - no_active_servers = false; - - MYSQL* con = gw_mysql_init(); - if (con) - { - if (mxs_mysql_real_connect(con, server->server, service_user, dpwd) == NULL) + if (mxs_mysql_real_connect(con, server, service_user, dpwd) == NULL) { - MXS_ERROR("Failure loading users data from backend " - "[%s:%i] for service [%s]. MySQL error %i, %s", - server->server->address, - server->server->port, - service->name, - mysql_errno(con), - mysql_error(con)); + MXS_ERROR("Failure loading users data from backend [%s:%i] for service [%s]. " + "MySQL error %i, %s", server->address, server->port, service->name, + mysql_errno(con), mysql_error(con)); mysql_close(con); } else @@ -1148,7 +1158,7 @@ static int get_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv) if (users > total_users) { - *srv = server->server; + *srv = server; total_users = users; } @@ -1164,12 +1174,12 @@ static int get_users(SERV_LISTENER* listener, bool skip_local, SERVER** srv) MXS_FREE(dpwd); - if (no_active_servers) + if (candidates.empty()) { // This service has no servers or all servers are local MaxScale services total_users = 0; } - else if (server == NULL && total_users == -1) + else if (*srv == nullptr && total_users == -1) { MXS_ERROR("Unable to get user data from backend database for service [%s]." " Failed to connect to any of the backend databases.", From 84f4688ebbbe4bdf710b6373b1d35bc0f2a0f054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 17 Jul 2019 14:45:02 +0300 Subject: [PATCH 4/4] Fix readwritesplit response count assertion The assertion in routeQuery that expects there to be at least one ongoing query would be triggered if a query was received after a master had failed but before the session would close. To make sure the internal logic stays consistent, the error handler should only decrement the expected response count if the session can continue. --- server/modules/routing/readwritesplit/rwsplitsession.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index e92d89db1..fc1faaf13 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -985,7 +985,6 @@ void RWSplitSession::handleError(GWBUF* errmsgbuf, { // We were expecting a response but we aren't going to get one mxb_assert(m_expected_responses > 0); - m_expected_responses--; errmsg += " Lost connection to master server while waiting for a result."; if (can_retry_query()) @@ -1000,6 +999,14 @@ void RWSplitSession::handleError(GWBUF* errmsgbuf, can_continue = true; send_readonly_error(m_client); } + + // Decrement the expected response count only if we know we can continue the sesssion. + // This keeps the internal logic sound even if another query is routed before the session + // is closed. + if (can_continue) + { + m_expected_responses--; + } } if (session_trx_is_active(session) && m_otrx_state == OTRX_INACTIVE)