From 1908faf150333b54eda13aece225697dbd862de3 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Wed, 27 Dec 2017 14:14:51 +0200 Subject: [PATCH 1/8] MXS-1527 Add test case --- query_classifier/test/expected.sql | 1 + query_classifier/test/input.sql | 1 + 2 files changed, 2 insertions(+) diff --git a/query_classifier/test/expected.sql b/query_classifier/test/expected.sql index 0aa0a9739..35ef67a02 100644 --- a/query_classifier/test/expected.sql +++ b/query_classifier/test/expected.sql @@ -15,3 +15,4 @@ QUERY_TYPE_SESSION_WRITE QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ QUERY_TYPE_READ|QUERY_TYPE_MASTER_READ +QUERY_TYPE_READ|QUERY_TYPE_SYSVAR_READ diff --git a/query_classifier/test/input.sql b/query_classifier/test/input.sql index bfa445297..cbd63c3ce 100644 --- a/query_classifier/test/input.sql +++ b/query_classifier/test/input.sql @@ -15,3 +15,4 @@ use X; select last_insert_id(); select @@last_insert_id; select @@identity; +select if(@@hostname='box02','prod_mariadb02','n'); From c6e0d1f33ce2404e48521712185c3b2933905b70 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Wed, 27 Dec 2017 15:01:44 +0200 Subject: [PATCH 2/8] Fix canonizer test programs --- query_classifier/test/canonical_tests/canonizer.c | 1 + query_classifier/test/canonical_tests/canontest.sh | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/query_classifier/test/canonical_tests/canonizer.c b/query_classifier/test/canonical_tests/canonizer.c index ad8e5fcba..dce1fe28b 100644 --- a/query_classifier/test/canonical_tests/canonizer.c +++ b/query_classifier/test/canonical_tests/canonizer.c @@ -48,6 +48,7 @@ int main(int argc, char** argv) qc_setup("qc_sqlite", NULL); qc_process_init(QC_INIT_BOTH); + qc_thread_init(QC_INIT_BOTH); infile = fopen(argv[1], "rb"); outfile = fopen(argv[2], "wb"); diff --git a/query_classifier/test/canonical_tests/canontest.sh b/query_classifier/test/canonical_tests/canontest.sh index dd931504f..b33f2d7a9 100755 --- a/query_classifier/test/canonical_tests/canontest.sh +++ b/query_classifier/test/canonical_tests/canontest.sh @@ -1,5 +1,5 @@ #! /bin/sh -if [[ $# -lt 4 ]] +if [ $# -lt 4 ] then echo "Usage: canontest.sh " exit 0 @@ -11,7 +11,7 @@ EXPECTED=$4 DIFFLOG=diff.out if [ $# -eq 5 ] -then +then EXECUTABLE=$5 else EXECUTABLE=$PWD/canonizer @@ -20,11 +20,11 @@ fi $EXECUTABLE $INPUT $OUTPUT diff $OUTPUT $EXPECTED > $DIFFLOG if [ $? -eq 0 ] -then +then echo "PASSED" >> $TESTLOG exval=0 else - echo "FAILED" >> $TESTLOG + echo "FAILED" >> $TESTLOG echo "Diff output: " >> $TESTLOG cat $DIFFLOG >> $TESTLOG exval=1 From 33b1c552e0ff637f7f1bbe424a12786d73cc4500 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Wed, 27 Dec 2017 16:09:54 +0200 Subject: [PATCH 3/8] Load qc from build directory The query classifier library will now be loaded from the build directory and not from the installation directory. --- server/core/test/testtrxcompare.cc | 2 ++ server/core/test/testtrxtracking.cc | 2 ++ 2 files changed, 4 insertions(+) diff --git a/server/core/test/testtrxcompare.cc b/server/core/test/testtrxcompare.cc index ae85f91c5..b44a7f088 100644 --- a/server/core/test/testtrxcompare.cc +++ b/server/core/test/testtrxcompare.cc @@ -183,6 +183,8 @@ int main(int argc, char* argv[]) if (mxs_log_init(NULL, ".", MXS_LOG_TARGET_DEFAULT)) { + set_libdir(strdup("../../../query_classifier/qc_sqlite")); + // We have to setup something in order for the regexes to be compiled. if (qc_setup("qc_sqlite", NULL) && qc_process_init(QC_INIT_BOTH)) { diff --git a/server/core/test/testtrxtracking.cc b/server/core/test/testtrxtracking.cc index 9f0e67b7b..50f42d600 100644 --- a/server/core/test/testtrxtracking.cc +++ b/server/core/test/testtrxtracking.cc @@ -420,6 +420,8 @@ int main(int argc, char* argv[]) if (mxs_log_init(NULL, ".", MXS_LOG_TARGET_DEFAULT)) { + set_libdir(strdup("../../../query_classifier/qc_sqlite")); + // We have to setup something in order for the regexes to be compiled. if (qc_setup("qc_sqlite", NULL) && qc_process_init(QC_INIT_BOTH)) { From 3b7275ec416f876d98b1721b49812fbb106169c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 19 Dec 2017 10:11:58 +0200 Subject: [PATCH 4/8] Use default value for "smoke" The values should only be updated if the environment variable is defined. --- maxscale-system-test/testconnections.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/maxscale-system-test/testconnections.cpp b/maxscale-system-test/testconnections.cpp index 7c9f79cfb..1800250d2 100644 --- a/maxscale-system-test/testconnections.cpp +++ b/maxscale-system-test/testconnections.cpp @@ -528,14 +528,11 @@ int TestConnections::read_env() } env = getenv("smoke"); - if ((env != NULL) && ((strcasecmp(env, "yes") == 0) || (strcasecmp(env, "true") == 0) )) + if (env) { - smoke = true; - } - else - { - smoke = false; + smoke = strcasecmp(env, "yes") == 0 || strcasecmp(env, "true") == 0; } + env = getenv("threads"); if ((env != NULL)) { From 3ca32457db549a22e50e8718616b034b142dc50e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 19 Dec 2017 09:23:44 +0200 Subject: [PATCH 5/8] Don't force verbose output The tests shouldn't force verbose output. --- maxscale-system-test/mariadb_nodes.cpp | 27 +++++++++++++++----------- maxscale-system-test/mariadb_nodes.h | 1 + 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/maxscale-system-test/mariadb_nodes.cpp b/maxscale-system-test/mariadb_nodes.cpp index e701716cc..9ee264d14 100644 --- a/maxscale-system-test/mariadb_nodes.cpp +++ b/maxscale-system-test/mariadb_nodes.cpp @@ -708,13 +708,13 @@ bool Mariadb_nodes::check_master_node(MYSQL *conn) } /** - * @brief bad_slave_thread_status Check if filed in the slave status outpur is not 'yes' + * @brief bad_slave_thread_status Check if field in the slave status outpur is not 'yes' * @param conn MYSQL struct (connection have to be open) - * @param field Filed to check + * @param field Field to check * @param node Node index - * @return false if requested filed is 'Yes' + * @return false if requested field is 'Yes' */ -static bool bad_slave_thread_status(MYSQL *conn, const char *field, int node) +bool Mariadb_nodes::bad_slave_thread_status(MYSQL *conn, const char *field, int node) { char str[1024] = ""; bool rval = false; @@ -725,23 +725,29 @@ static bool bad_slave_thread_status(MYSQL *conn, const char *field, int node) if (find_field(conn, "SHOW SLAVE STATUS;", field, str) != 0) { printf("Node %d: %s not found in SHOW SLAVE STATUS\n", node, field); - fflush(stdout); break; } - else if (strcmp(str, "Yes") == 0 || strcmp(str, "No") == 0) + + if (verbose) + { + printf("Node %d: field %s is %s\n", node, field, str); + } + + if (strcmp(str, "Yes") == 0 || strcmp(str, "No") == 0) { - printf("Node %d: filed %s is %s\n", node, field, str); break; } - printf("Node %d: filed %s is %s\n", node, field, str); + /** Any other state is transient and we should try again */ sleep(1); } if (strcmp(str, "Yes") != 0) { - printf("Node %d: %s is '%s'\n", node, field, str); - fflush(stdout); + if (verbose) + { + printf("Node %d: %s is '%s'\n", node, field, str); + } rval = true; } @@ -835,7 +841,6 @@ int Mariadb_nodes::check_replication() bool Mariadb_nodes::fix_replication() { - verbose = true; if (check_replication()) { unblock_all_nodes(); diff --git a/maxscale-system-test/mariadb_nodes.h b/maxscale-system-test/mariadb_nodes.h index 79ff180e2..88da075ac 100644 --- a/maxscale-system-test/mariadb_nodes.h +++ b/maxscale-system-test/mariadb_nodes.h @@ -480,6 +480,7 @@ private: int check_node_ssh(int node); bool check_master_node(MYSQL *conn); + bool bad_slave_thread_status(MYSQL *conn, const char *field, int node); }; class Galera_nodes : public Mariadb_nodes From c6bc1f73279f260fcaf2bd44c7e03942f3509536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 27 Dec 2017 11:37:24 +0200 Subject: [PATCH 6/8] MXS-1585: Add preliminary test case The test case attempts to simulate the environment where the crash appears to have happened. Local testing does not point out any problems. --- maxscale-system-test/CMakeLists.txt | 4 + .../cnf/maxscale.cnf.template.mxs1585 | 88 +++++++++++++++++++ maxscale-system-test/mxs1585.cpp | 87 ++++++++++++++++++ .../readwritesplit/rwsplit_route_stmt.c | 3 + 4 files changed, 182 insertions(+) create mode 100755 maxscale-system-test/cnf/maxscale.cnf.template.mxs1585 create mode 100644 maxscale-system-test/mxs1585.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 3a77efc20..7cf482ba8 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -514,6 +514,10 @@ add_test_executable(mxs1476.cpp mxs1476 mxs1476 LABELS GALERA_BACKEND) # https://jira.mariadb.org/browse/MXS-1509 add_test_executable(mxs1509.cpp mxs1509 mxs1509 LABELS REPL_BACKEND) + # MXS-1585: Crash in MaxScale 2.1.12 + # https://jira.mariadb.org/browse/MXS-1585 +add_test_executable(mxs1585.cpp mxs1585 mxs1585 LABELS REPL_BACKEND) + # 'namedserverfilter' test add_test_executable(namedserverfilter.cpp namedserverfilter namedserverfilter LABELS namedserverfilter LIGHT REPL_BACKEND) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs1585 b/maxscale-system-test/cnf/maxscale.cnf.template.mxs1585 new file mode 100755 index 000000000..71569f5c9 --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs1585 @@ -0,0 +1,88 @@ +[maxscale] +threads=###threads### + +[Galera Monitor] +type=monitor +module=galeramon +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql +monitor_interval=1000 +root_node_as_master=false + +[RW Split Router] +type=service +router=readwritesplit +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql +master_failure_mode=fail_on_write + +[Read Connection Router Slave] +type=service +router=readconnroute +router_options=slave +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql + +[Read Connection Router Master] +type=service +router=readconnroute +router_options=master +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql + +[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 + +[CLI] +type=service +router=cli + +[CLI Listener] +type=listener +service=CLI +protocol=maxscaled +socket=default + +[server1] +type=server +address=###galera_server_IP_1### +port=###galera_server_port_1### +protocol=MySQLBackend + +[server2] +type=server +address=###galera_server_IP_2### +port=###galera_server_port_2### +protocol=MySQLBackend + +[server3] +type=server +address=###galera_server_IP_3### +port=###galera_server_port_3### +protocol=MySQLBackend + +[server4] +type=server +address=###galera_server_IP_4### +port=###galera_server_port_4### +protocol=MySQLBackend + diff --git a/maxscale-system-test/mxs1585.cpp b/maxscale-system-test/mxs1585.cpp new file mode 100644 index 000000000..67769b50f --- /dev/null +++ b/maxscale-system-test/mxs1585.cpp @@ -0,0 +1,87 @@ +/** + * MXS-1585: https://jira.mariadb.org/browse/MXS-1585 + * + * Check that MaxScale doesn't crash when the master is set into maintenance + * mode when master_failure_mode is fail_on_write. + */ + +#include "testconnections.h" +#include + +static bool running = true; + +void* query_thr(void* data) +{ + TestConnections* test = (TestConnections*)data; + + while (running) + { + MYSQL* mysql = test->open_rwsplit_connection(); + + while (running) + { + if (mysql_query(mysql, "SET sql_log_bin = 0") || + mysql_query(mysql, "INSERT INTO test.mxs1585 VALUES (1)") || + mysql_query(mysql, "DELETE FROM test.mxs1585")) + { + break; + } + } + + mysql_close(mysql); + } + + return NULL; +} + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + test.connect_maxscale(); + test.try_query(test.conn_rwsplit, "DROP TABLE IF EXISTS test.mxs1585"); + test.try_query(test.conn_rwsplit, "CREATE TABLE test.mxs1585(id INT) ENGINE=MEMORY"); + test.close_maxscale_connections(); + + std::vector threads; + threads.resize(100); + + for (auto& a: threads) + { + pthread_create(&a, NULL, query_thr, &test); + } + + for (int i = 0; i < 15; i++) + { + for (int x = 1; x <= 4; x++) + { + test.ssh_maxscale(true, "maxadmin set server server%d maintenance", x); + sleep(1); + test.ssh_maxscale(true, "maxadmin clear server server%d maintenance", x); + sleep(2); + + test.ssh_maxscale(true, "maxadmin remove server server%d \"RW Split Router\" \"Galera Monitor\"", x); + sleep(1); + test.ssh_maxscale(true, "maxadmin add server server%d \"RW Split Router\" \"Galera Monitor\"", x); + sleep(2); + + test.galera->block_node(x - 1); + sleep(5); + test.galera->unblock_node(x - 1); + sleep(5); + } + } + + running = false; + + for (auto& a: threads) + { + pthread_join(a, NULL); + } + + test.connect_maxscale(); + test.try_query(test.conn_rwsplit, "DROP TABLE test.mxs1585"); + test.check_maxscale_alive(); + + return test.global_result; +} diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.c b/server/modules/routing/readwritesplit/rwsplit_route_stmt.c index 9bb12a7ba..0f7ebae2e 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.c +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.c @@ -1090,6 +1090,8 @@ bool handle_slave_is_target(ROUTER_INSTANCE *inst, ROUTER_CLIENT_SES *rses, static void log_master_routing_failure(ROUTER_CLIENT_SES *rses, bool found, DCB *master_dcb, DCB *curr_master_dcb) { + ss_dassert(!master_dcb || master_dcb->dcb_role == DCB_ROLE_BACKEND_HANDLER); + ss_dassert(!curr_master_dcb || curr_master_dcb->dcb_role == DCB_ROLE_BACKEND_HANDLER); char errmsg[MAX_SERVER_NAME_LEN * 2 + 100]; // Extra space for error message if (!found) @@ -1100,6 +1102,7 @@ static void log_master_routing_failure(ROUTER_CLIENT_SES *rses, bool found, { /** We found a master but it's not the same connection */ ss_dassert(master_dcb != curr_master_dcb); + ss_dassert(master_dcb->server && curr_master_dcb->server); if (master_dcb->server != curr_master_dcb->server) { sprintf(errmsg, "Master server changed from '%s' to '%s'", From 5ede5a4f96fd9530a77fd60333ecf43cbee5fda8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 27 Dec 2017 16:42:06 +0200 Subject: [PATCH 7/8] Fix comment removal regex The trailing comment removal pattern unnecessarily required that a leading space is present in all trailing comments. Also, the pattern didn't match if no line ending was included in the SQL statement. The subject ending should be the third valid terminator in addition to UNIX and Windows style line endings. --- server/core/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/core/utils.c b/server/core/utils.c index 433e7dbe1..47570d48f 100644 --- a/server/core/utils.c +++ b/server/core/utils.c @@ -555,7 +555,7 @@ static pcre2_code* remove_comments_re = NULL; static const PCRE2_SPTR remove_comments_pattern = (PCRE2_SPTR) "(?:`[^`]*`\\K)|" "(\\/[*](?!(M?!)).*?[*]\\/)|" - "([[:space:]](?:#.*|--[[:space:]].*(\\n|\\r\\n)))"; + "((?:#.*|--[[:space:]].*)(\\n|\\r\\n|$))"; /** * Remove SQL comments from the end of a string From b543f37a11fb9e1e29bd972542df0a23e0f65508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 27 Dec 2017 17:02:57 +0200 Subject: [PATCH 8/8] Run unit tests for each build The unit tests must pass for the build to successfully continue. --- BUILD/build_deb_local.sh | 6 ++++++ BUILD/build_rpm_local.sh | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/BUILD/build_deb_local.sh b/BUILD/build_deb_local.sh index 2a3cc8352..13b8bab1d 100755 --- a/BUILD/build_deb_local.sh +++ b/BUILD/build_deb_local.sh @@ -14,6 +14,12 @@ cmake .. $cmake_flags export LD_LIBRARY_PATH=$PWD/log_manager:$PWD/query_classifier make +if [[ "$cmake_flags" =~ "BUILD_TESTS" ]] +then + # All tests must pass otherwise the build is considered a failure + make test || exit 1 +fi + export LD_LIBRARY_PATH=$(for i in `find $PWD/ -name '*.so*'`; do echo $(dirname $i); done|sort|uniq|xargs|sed -e 's/[[:space:]]/:/g') make package res=$? diff --git a/BUILD/build_rpm_local.sh b/BUILD/build_rpm_local.sh index ae78b6698..ab28f06e0 100755 --- a/BUILD/build_rpm_local.sh +++ b/BUILD/build_rpm_local.sh @@ -12,6 +12,12 @@ cd _build cmake .. $cmake_flags make +if [[ "$cmake_flags" =~ "BUILD_TESTS" ]] +then + # All tests must pass otherwise the build is considered a failure + make test || exit 1 +fi + if [ $remove_strip == "yes" ] ; then sudo rm -rf /usr/bin/strip sudo touch /usr/bin/strip