From 07ca20b7b9a91d4349c25c0d2148da794516a4f5 Mon Sep 17 00:00:00 2001 From: doyoubi Date: Mon, 4 Sep 2017 11:21:17 +0000 Subject: [PATCH 1/5] Fix slowlog crash when redirection fails --- src/command.c | 3 +++ src/slowlog.c | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/command.c b/src/command.c index 618a7eb..1a94dcf 100644 --- a/src/command.c +++ b/src/command.c @@ -1120,6 +1120,9 @@ void cmd_mark(struct command *cmd, int fail) if (cmd->parent->cmd_done_count == cmd->parent->cmd_count) { root = cmd->parent; } + if (fail) { + cmd->parent->cmd_fail = true; + } } if (root != NULL && conn_register(root->client) == CORVUS_ERR) { diff --git a/src/slowlog.c b/src/slowlog.c index 614d620..eb72672 100644 --- a/src/slowlog.c +++ b/src/slowlog.c @@ -111,7 +111,7 @@ struct slowlog_entry *slowlog_create_entry(struct command *cmd, int64_t remote_l return entry; } -// Return NULL when `cmd` is not a multiple keys command +// Return NULL when `cmd` is not a multiple keys command or fail to connect to redis struct slowlog_entry *slowlog_create_sub_entry(struct command *cmd, int64_t total_latency) { switch (cmd->cmd_type) { @@ -135,6 +135,12 @@ struct slowlog_entry *slowlog_create_sub_entry(struct command *cmd, int64_t tota slowest_sub_cmd = c; } } + // When corvus fails to redirect commands to redis, the `remote_latency` above + // may become zero or nagative and produce a NULL `slowest_sub_cmd`. + // In this case we just ignore it. + if (NULL == slowest_sub_cmd) { + return NULL; + } return slowlog_create_entry(slowest_sub_cmd, max_remote_latency / 1000, total_latency); } @@ -201,6 +207,7 @@ bool slowlog_need_log(struct command *cmd, long long latency) { int slowlog_log_slower_than = ATOMIC_GET(config.slowlog_log_slower_than); return slowlog_log_slower_than >= 0 + && !cmd->cmd_fail && slowlog_type_need_log(cmd) // for those couldn't be forwarded and their cmd->data have been deallocated && cmd->data.elements > 0 From c65758f378194628ef8797a43c5a61a55d3f5c5e Mon Sep 17 00:00:00 2001 From: doyoubi Date: Tue, 5 Sep 2017 02:45:12 +0000 Subject: [PATCH 2/5] Fix travis test --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index a1ab155..25a5607 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,8 +27,11 @@ install: - sudo redis-server /etc/redis/redis-cluster-8001.conf - sudo redis-server /etc/redis/redis-cluster-8002.conf - sudo redis-server /etc/redis/redis-cluster-8003.conf + - redis-server -v + - sudo pip install redis==2.10.5 - sudo pip install ruskit + - pip freeze - ruskit create localhost:800{0,1,2} - sed -i 's/node localhost:8000,localhost:8001,localhost:8002/node 127.0.0.1:8000/' corvus.conf @@ -49,7 +52,6 @@ before_script: - ./src/corvus ./corvus.conf > corvus.log 2>&1 & script: - - redis-server -v - awk '/ERROR SUMMARY/ {if ($4 == 0) exit 0; else exit 1}' valgrind.log - make clean || true - make test From 37767b0d38b00cfbe571fd27af6def57b5b9547e Mon Sep 17 00:00:00 2001 From: doyoubi Date: Tue, 5 Sep 2017 03:52:12 +0000 Subject: [PATCH 3/5] Still create slowlog for fail commands --- src/command.c | 10 +++++++--- src/slowlog.c | 1 - 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/command.c b/src/command.c index 1a94dcf..9e1d44d 100644 --- a/src/command.c +++ b/src/command.c @@ -1120,9 +1120,13 @@ void cmd_mark(struct command *cmd, int fail) if (cmd->parent->cmd_done_count == cmd->parent->cmd_count) { root = cmd->parent; } - if (fail) { - cmd->parent->cmd_fail = true; - } + // In some cases cmd->cmd_fail is true though, + // cmd->parent->cmd_fail is not. But since `cmd_fail` + // implys a not null `fail_reason` (and maybe some other rules), + // now we keep it unchanged. + // if (fail) { + // cmd->parent->cmd_fail = true; + // } } if (root != NULL && conn_register(root->client) == CORVUS_ERR) { diff --git a/src/slowlog.c b/src/slowlog.c index eb72672..1d6c0fd 100644 --- a/src/slowlog.c +++ b/src/slowlog.c @@ -207,7 +207,6 @@ bool slowlog_need_log(struct command *cmd, long long latency) { int slowlog_log_slower_than = ATOMIC_GET(config.slowlog_log_slower_than); return slowlog_log_slower_than >= 0 - && !cmd->cmd_fail && slowlog_type_need_log(cmd) // for those couldn't be forwarded and their cmd->data have been deallocated && cmd->data.elements > 0 From 7aebff542f9e3c373ea22133933afd19d4a1997c Mon Sep 17 00:00:00 2001 From: doyoubi Date: Wed, 6 Sep 2017 03:22:42 +0000 Subject: [PATCH 4/5] Add tests for crash case --- tests/test_slowlog.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/test_slowlog.c b/tests/test_slowlog.c index 3e89df4..f307353 100644 --- a/tests/test_slowlog.c +++ b/tests/test_slowlog.c @@ -223,10 +223,48 @@ TEST(test_slowlog_statsd) { PASS(NULL); } +TEST(test_failed_command_slowlog) { + struct mbuf buf; + struct command cmd; + memset(&cmd, 0, sizeof(struct command)); + STAILQ_INIT(&cmd.sub_cmds); + + struct reader r = {0}; + const char cmd_data[] = "*2\r\n$4\r\nMGET\r\n$4\r\nkey1\r\n"; + buf.pos = (uint8_t*)cmd_data; + buf.last = (uint8_t*)cmd_data + strlen(cmd_data); + reader_init(&r); + reader_feed(&r, &buf); + ASSERT(parse(&r, MODE_REQ) != -1); + cmd.data = r.data; + cmd.prefix = NULL; + + struct command sub_cmd; + sub_cmd.parent = &cmd; + STAILQ_INSERT_TAIL(&cmd.sub_cmds, &sub_cmd, sub_cmd_next); + + // When corvus fails to redirect command, these two fields may be zero. + cmd.rep_time[0] = 0; + cmd.rep_time[1] = 0; + sub_cmd.rep_time[0] = 0; + sub_cmd.rep_time[1] = 0; + + struct slowlog_entry *entry = slowlog_create_entry(&cmd, 0, 666233); + ASSERT(entry->remote_latency == 0); + ASSERT(entry->total_latency == 666233); + slowlog_dec_ref(entry); + entry = slowlog_create_sub_entry(&cmd, 666233); + ASSERT(NULL == entry); + + redis_data_free(&cmd.data); + PASS(NULL); +} + TEST_CASE(test_slowlog) { RUN_TEST(test_slowlog_create_entry); RUN_TEST(test_slowlog_create_entry_with_prefix); RUN_TEST(test_slowlog_create_entry_with_long_arg); RUN_TEST(test_slowlog_statsd); RUN_TEST(test_entry_get_set); + RUN_TEST(test_failed_command_slowlog); } From 115da6c5e6ec1f90c06cf435ad12aacfd44de942 Mon Sep 17 00:00:00 2001 From: doyoubi Date: Fri, 8 Sep 2017 11:15:02 +0000 Subject: [PATCH 5/5] Install redis 3.0.7 to fix travis ci test --- .travis.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.travis.yml b/.travis.yml index 25a5607..c3e03f4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,6 +7,13 @@ before_install: - sudo apt-get install -qq valgrind install: + - wget http://download.redis.io/releases/redis-3.0.7.tar.gz + - tar xzf redis-3.0.7.tar.gz + - cd redis-3.0.7 + - make + - sudo cp src/redis-server `which redis-server` + - cd ../ + - sudo mkdir -p /etc/redis /var/lib/redis/cluster/{8000,8001,8002,8003} - sudo chmod -R 777 /etc/redis