From 741ee702cacd402dedeef75c7b8cbd6a1f091b7a Mon Sep 17 00:00:00 2001 From: Karthick Ariyaratnam Date: Tue, 14 May 2024 18:54:33 -0400 Subject: [PATCH 1/8] [New] Migrate zmalloc.c unit tests to new test framework. (#493) This is the actual PR which is created to migrate all tests related to zmalloc into new test framework as part of the parent issue https://github.com/valkey-io/valkey/issues/428. Signed-off-by: Karthick Ariyaratnam --- src/server.c | 1 - src/unit/test_files.h | 5 ++++ src/unit/test_zmalloc.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/zmalloc.c | 54 ----------------------------------------- src/zmalloc.h | 4 --- 5 files changed, 58 insertions(+), 59 deletions(-) create mode 100644 src/unit/test_zmalloc.c diff --git a/src/server.c b/src/server.c index a5f40c8991..fe54d9eb82 100644 --- a/src/server.c +++ b/src/server.c @@ -6932,7 +6932,6 @@ struct serverTest { {"ziplist", ziplistTest}, {"quicklist", quicklistTest}, {"zipmap", zipmapTest}, - {"zmalloc", zmalloc_test}, {"dict", dictTest}, {"listpack", listpackTest}, }; diff --git a/src/unit/test_files.h b/src/unit/test_files.h index 40361454e9..53fc0d2eb8 100644 --- a/src/unit/test_files.h +++ b/src/unit/test_files.h @@ -30,6 +30,9 @@ int test_ll2string(int argc, char **argv, int flags); int test_ld2string(int argc, char **argv, int flags); int test_fixedpoint_d2string(int argc, char **argv, int flags); int test_reclaimFilePageCache(int argc, char **argv, int flags); +int test_zmallocInitialUsedMemory(int argc, char **argv, int flags); +int test_zmallocAllocReallocCallocAndFree(int argc, char **argv, int flags); +int test_zmallocAllocZeroByteAndFree(int argc, char **argv, int flags); unitTest __test_crc64_c[] = {{"test_crc64", test_crc64}, {NULL, NULL}}; unitTest __test_crc64combine_c[] = {{"test_crc64combine", test_crc64combine}, {NULL, NULL}}; @@ -39,6 +42,7 @@ unitTest __test_kvstore_c[] = {{"test_kvstoreAdd16Keys", test_kvstoreAdd16Keys}, unitTest __test_sds_c[] = {{"test_sds", test_sds}, {NULL, NULL}}; unitTest __test_sha1_c[] = {{"test_sha1", test_sha1}, {NULL, NULL}}; unitTest __test_util_c[] = {{"test_string2ll", test_string2ll}, {"test_string2l", test_string2l}, {"test_ll2string", test_ll2string}, {"test_ld2string", test_ld2string}, {"test_fixedpoint_d2string", test_fixedpoint_d2string}, {"test_reclaimFilePageCache", test_reclaimFilePageCache}, {NULL, NULL}}; +unitTest __test_zmalloc_c[] = {{"test_zmallocInitialUsedMemory", test_zmallocInitialUsedMemory}, {"test_zmallocAllocReallocCallocAndFree", test_zmallocAllocReallocCallocAndFree}, {"test_zmallocAllocZeroByteAndFree", test_zmallocAllocZeroByteAndFree}, {NULL, NULL}}; struct unitTestSuite { char *filename; @@ -52,4 +56,5 @@ struct unitTestSuite { {"test_sds.c", __test_sds_c}, {"test_sha1.c", __test_sha1_c}, {"test_util.c", __test_util_c}, + {"test_zmalloc.c", __test_zmalloc_c}, }; diff --git a/src/unit/test_zmalloc.c b/src/unit/test_zmalloc.c new file mode 100644 index 0000000000..6c1d03e8e1 --- /dev/null +++ b/src/unit/test_zmalloc.c @@ -0,0 +1,53 @@ +#include "../zmalloc.h" +#include "test_help.h" + +int test_zmallocInitialUsedMemory(int argc, char **argv, int flags) { + UNUSED(argc); + UNUSED(argv); + UNUSED(flags); + + TEST_ASSERT(zmalloc_used_memory() == 0); + + return 0; +} + +int test_zmallocAllocReallocCallocAndFree(int argc, char **argv, int flags) { + UNUSED(argc); + UNUSED(argv); + UNUSED(flags); + + void *ptr, *ptr2; + + ptr = zmalloc(123); + TEST_PRINT_INFO("Allocated 123 bytes; used: %zu\n", zmalloc_used_memory()); + + ptr = zrealloc(ptr, 456); + TEST_PRINT_INFO("Reallocated to 456 bytes; used: %zu\n", zmalloc_used_memory()); + + ptr2 = zcalloc(123); + TEST_PRINT_INFO("Callocated 123 bytes; used: %zu\n", zmalloc_used_memory()); + + zfree(ptr); + zfree(ptr2); + TEST_PRINT_INFO("Freed pointers; used: %zu\n", zmalloc_used_memory()); + + TEST_ASSERT(zmalloc_used_memory() == 0); + + return 0; +} + +int test_zmallocAllocZeroByteAndFree(int argc, char **argv, int flags) { + UNUSED(argc); + UNUSED(argv); + UNUSED(flags); + + void *ptr; + + ptr = zmalloc(0); + TEST_PRINT_INFO("Allocated 0 bytes; used: %zu\n", zmalloc_used_memory()); + zfree(ptr); + + TEST_ASSERT(zmalloc_used_memory() == 0); + + return 0; +} diff --git a/src/zmalloc.c b/src/zmalloc.c index 550752240f..f6318d36d6 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -907,57 +907,3 @@ size_t zmalloc_get_memory_size(void) { return 0L; /* Unknown OS. */ #endif } - -#ifdef SERVER_TEST -#include "testhelp.h" -#include "serverassert.h" - -#define TEST(name) printf("test — %s\n", name); - -int zmalloc_test(int argc, char **argv, int flags) { - void *ptr, *ptr2; - - UNUSED(argc); - UNUSED(argv); - UNUSED(flags); - - printf("Malloc prefix size: %d\n", (int) PREFIX_SIZE); - - TEST("Initial used memory is 0") { - assert(zmalloc_used_memory() == 0); - } - - TEST("Allocated 123 bytes") { - ptr = zmalloc(123); - printf("Allocated 123 bytes; used: %zu\n", zmalloc_used_memory()); - } - - TEST("Reallocated to 456 bytes") { - ptr = zrealloc(ptr, 456); - printf("Reallocated to 456 bytes; used: %zu\n", zmalloc_used_memory()); - } - - TEST("Callocated 123 bytes") { - ptr2 = zcalloc(123); - printf("Callocated 123 bytes; used: %zu\n", zmalloc_used_memory()); - } - - TEST("Freed pointers") { - zfree(ptr); - zfree(ptr2); - printf("Freed pointers; used: %zu\n", zmalloc_used_memory()); - } - - TEST("Allocated 0 bytes") { - ptr = zmalloc(0); - printf("Allocated 0 bytes; used: %zu\n", zmalloc_used_memory()); - zfree(ptr); - } - - TEST("At the end used memory is 0") { - assert(zmalloc_used_memory() == 0); - } - - return 0; -} -#endif diff --git a/src/zmalloc.h b/src/zmalloc.h index 1cf4af96c4..559d27ac2b 100644 --- a/src/zmalloc.h +++ b/src/zmalloc.h @@ -168,8 +168,4 @@ __attribute__((alloc_size(2),noinline)) void *extend_to_usable(void *ptr, size_t int get_proc_stat_ll(int i, long long *res); -#ifdef SERVER_TEST -int zmalloc_test(int argc, char **argv, int flags); -#endif - #endif /* __ZMALLOC_H */ From 546cef6684e02bf22aeed9a5fca2a685babfe465 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Tue, 14 May 2024 17:09:49 -0700 Subject: [PATCH 2/8] Initial cleanup for cluster refactoring (#460) Cleaned up the minor cluster refactoring notes that were intended to be follow ups that never happened. Basically: 1. Minor style nitpicks 2. Generalized clusterNodeIsMyself so that it wasn't implementation dependent. 3. Removed getMyClusterId, and just make it an explicit call to myself's name, which seems more straightforward and removes unnecessary abstraction. 4. Remove clusterNodeGetSlaveof infavor of clusterNodeGetMaster. We already do a check if it's a replica, and if it wasn't working it would have been crashing. Signed-off-by: Madelyn Olson --- src/cluster.c | 8 ++++---- src/cluster.h | 11 ++++------- src/cluster_legacy.c | 10 +--------- src/module.c | 6 +++--- 4 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 99c02cd86d..741b05c603 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -789,8 +789,8 @@ void clusterCommandMyId(client *c) { } } -char* getMyClusterId(void) { - return clusterNodeGetName(getMyClusterNode()); +int clusterNodeIsMyself(clusterNode *n) { + return n == getMyClusterNode(); } void clusterCommandMyShardId(client *c) { @@ -1193,7 +1193,7 @@ clusterNode *getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, i if (((c->flags & CLIENT_READONLY) || pubsubshard_included) && !is_write_command && clusterNodeIsSlave(myself) && - clusterNodeGetSlaveof(myself) == n) + clusterNodeGetMaster(myself) == n) { return myself; } @@ -1286,7 +1286,7 @@ int clusterRedirectBlockedClientIfNeeded(client *c) { * replica can handle, allow it. */ if ((c->flags & CLIENT_READONLY) && !(c->lastcmd->flags & CMD_WRITE) && - clusterNodeIsSlave(myself) && clusterNodeGetSlaveof(myself) == node) + clusterNodeIsSlave(myself) && clusterNodeGetMaster(myself) == node) { node = myself; } diff --git a/src/cluster.h b/src/cluster.h index 8a2b97cad0..481fcc9736 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -60,13 +60,13 @@ int handleDebugClusterCommand(client *c); const char **clusterDebugCommandExtendedHelp(void); /* handle implementation specific cluster commands. Return 1 if handled, 0 otherwise. */ int clusterCommandSpecial(client *c); -const char** clusterCommandExtendedHelp(void); +const char **clusterCommandExtendedHelp(void); int clusterAllowFailoverCmd(client *c); void clusterPromoteSelfToMaster(void); int clusterManualFailoverTimeLimit(void); -void clusterCommandSlots(client * c); +void clusterCommandSlots(client *c); void clusterCommandMyId(client *c); void clusterCommandMyShardId(client *c); void clusterCommandShards(client *c); @@ -74,19 +74,15 @@ sds clusterGenNodeDescription(client *c, clusterNode *node, int tls_primary); int clusterNodeCoversSlot(clusterNode *n, int slot); int getNodeDefaultClientPort(clusterNode *n); -int clusterNodeIsMyself(clusterNode *n); clusterNode *getMyClusterNode(void); -char *getMyClusterId(void); int getClusterSize(void); int getMyShardSlotCount(void); int handleDebugClusterCommand(client *c); -int clusterNodePending(clusterNode *node); +int clusterNodePending(clusterNode *node); int clusterNodeIsMaster(clusterNode *n); char **getClusterNodesList(size_t *numnodes); -int clusterNodeIsMaster(clusterNode *n); char *clusterNodeIp(clusterNode *node); int clusterNodeIsSlave(clusterNode *node); -clusterNode *clusterNodeGetSlaveof(clusterNode *node); clusterNode *clusterNodeGetMaster(clusterNode *node); char *clusterNodeGetName(clusterNode *node); int clusterNodeTimedOut(clusterNode *node); @@ -106,6 +102,7 @@ clusterNode *clusterLookupNode(const char *name, int length); void clusterReplicateOpenSlots(void); /* functions with shared implementations */ +int clusterNodeIsMyself(clusterNode *n); clusterNode *getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); int clusterRedirectBlockedClientIfNeeded(client *c); void clusterRedirectClient(client *c, clusterNode *n, int hashslot, int error_code); diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index a50d0f8459..b2e9690fd9 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -6090,10 +6090,6 @@ unsigned int countChannelsInSlot(unsigned int hashslot) { return kvstoreDictSize(server.pubsubshard_channels, hashslot); } -int clusterNodeIsMyself(clusterNode *n) { - return n == server.cluster->myself; -} - clusterNode *getMyClusterNode(void) { return server.cluster->myself; } @@ -6175,7 +6171,7 @@ int handleDebugClusterCommand(client *c) { return 1; } -int clusterNodePending(clusterNode *node) { +int clusterNodePending(clusterNode *node) { return node->flags & (CLUSTER_NODE_NOADDR|CLUSTER_NODE_HANDSHAKE); } @@ -6187,10 +6183,6 @@ int clusterNodeIsSlave(clusterNode *node) { return node->flags & CLUSTER_NODE_SLAVE; } -clusterNode *clusterNodeGetSlaveof(clusterNode *node) { - return node->slaveof; -} - clusterNode *clusterNodeGetMaster(clusterNode *node) { while (node->slaveof != NULL) node = node->slaveof; return node; diff --git a/src/module.c b/src/module.c index 01abd7adfe..1d510461a2 100644 --- a/src/module.c +++ b/src/module.c @@ -9017,7 +9017,7 @@ void VM_FreeClusterNodesList(char **ids) { * is disabled. */ const char *VM_GetMyClusterID(void) { if (!server.cluster_enabled) return NULL; - return getMyClusterId(); + return clusterNodeGetName(getMyClusterNode()); } /* Return the number of nodes in the cluster, regardless of their state @@ -9064,8 +9064,8 @@ int VM_GetClusterNodeInfo(ValkeyModuleCtx *ctx, const char *id, char *ip, char * /* If the information is not available, the function will set the * field to zero bytes, so that when the field can't be populated the * function kinda remains predictable. */ - if (clusterNodeIsSlave(node) && clusterNodeGetSlaveof(node)) - memcpy(master_id, clusterNodeGetName(clusterNodeGetSlaveof(node)) ,VALKEYMODULE_NODE_ID_LEN); + if (clusterNodeIsSlave(node) && clusterNodeGetMaster(node)) + memcpy(master_id, clusterNodeGetName(clusterNodeGetMaster(node)) ,VALKEYMODULE_NODE_ID_LEN); else memset(master_id,0,VALKEYMODULE_NODE_ID_LEN); } From 6e4a61093e9ad9f6e29aa46ace9b10f537437b87 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Tue, 14 May 2024 23:48:30 -0700 Subject: [PATCH 3/8] Make it to so that unit tests build on mac (#499) The test logic is not smart enough to realize that a test is fully #ifdef'd out, so it will try to attach it to the test suite anyways. This is a minor work around for the reclaim file page test so that it will still attach the test, it will just always succeed. Also remove an unnecessary print statement that was missed in the same test. Signed-off-by: Madelyn Olson --- src/unit/test_util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unit/test_util.c b/src/unit/test_util.c index c5dad630bd..30f70c8350 100644 --- a/src/unit/test_util.c +++ b/src/unit/test_util.c @@ -266,12 +266,14 @@ static int cache_exist(int fd) { * page is currently resident in memory */ return flag&1; } +#endif int test_reclaimFilePageCache(int argc, char **argv, int flags) { UNUSED(argc); UNUSED(argv); UNUSED(flags); +#if defined(__linux__) char *tmpfile = "/tmp/redis-reclaim-cache-test"; int fd = open(tmpfile, O_RDWR|O_CREAT, 0644); TEST_ASSERT(fd >= 0); @@ -291,8 +293,6 @@ int test_reclaimFilePageCache(int argc, char **argv, int flags) { TEST_ASSERT(!cache_exist(fd)); unlink(tmpfile); - printf("reclaimFilePageCache test is ok\n"); - +#endif return 0; } -#endif From 3de5c71f48a6062b710e4eec6d513860e05e5d5c Mon Sep 17 00:00:00 2001 From: Arthur Lee Date: Wed, 15 May 2024 21:55:24 +0800 Subject: [PATCH 4/8] [Feat] Support fast fail option for tcl test cases (#482) This PR added a new option for tcl test case which will fail fast once any test cases fail. This can be useful while running redis CI pipeline, and you want to accelerate the CI pipeline. usage for example > ./runtest --single unit/type/hash --fast-fail --------- Signed-off-by: arthur.lee --- tests/instances.tcl | 9 +++++++++ tests/support/test.tcl | 5 +++++ tests/test_helper.tcl | 9 +++++++++ 3 files changed, 23 insertions(+) diff --git a/tests/instances.tcl b/tests/instances.tcl index 5b90a60093..3b487423f8 100644 --- a/tests/instances.tcl +++ b/tests/instances.tcl @@ -35,6 +35,7 @@ set ::leaked_fds_file [file normalize "tmp/leaked_fds.txt"] set ::pids {} ; # We kill everything at exit set ::dirs {} ; # We remove all the temp dirs at exit set ::run_matching {} ; # If non empty, only tests matching pattern are run. +set ::exit_on_failure 0 set ::stop_on_failure 0 set ::loop 0 @@ -298,6 +299,8 @@ proc parse_options {} { set val2 [lindex $::argv [expr $j+2]] dict set ::global_config $val $val2 incr j 2 + } elseif {$opt eq {--fast-fail}} { + set ::exit_on_failure 1 } elseif {$opt eq {--stop}} { set ::stop_on_failure 1 } elseif {$opt eq {--loop}} { @@ -316,6 +319,7 @@ proc parse_options {} { puts "--tls-module Run tests in TLS mode with Valkey module." puts "--host Use hostname instead of 127.0.0.1." puts "--config Extra config argument(s)." + puts "--fast-fail Exit immediately once the first test fails." puts "--stop Blocks once the first test fails." puts "--loop Execute the specified set of tests forever." puts "--help Shows this help." @@ -483,6 +487,11 @@ while 1 { incr ::failed # letting the tests resume, so we'll eventually reach the cleanup and report crashes + if {$::exit_on_failure} { + puts -nonewline "(Fast fail: test will exit now)" + flush stdout + exit 1 + } if {$::stop_on_failure} { puts -nonewline "(Test stopped, press enter to resume the tests)" flush stdout diff --git a/tests/support/test.tcl b/tests/support/test.tcl index 9c959d9de7..bb59ee7972 100644 --- a/tests/support/test.tcl +++ b/tests/support/test.tcl @@ -233,6 +233,11 @@ proc test {name code {okpattern undefined} {tags {}}} { incr ::num_failed send_data_packet $::test_server_fd err [join $details "\n"] + if {$::exit_on_failure} { + puts "Test error (last server port:[srv port], log:[srv stdout]), test will exit now" + flush stdout + exit 1 + } if {$::stop_on_failure} { puts "Test error (last server port:[srv port], log:[srv stdout]), press enter to teardown the test." flush stdout diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 57fb2beb13..a8df6c40a6 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -65,6 +65,7 @@ set ::active_servers {} ; # Pids of active server instances. set ::dont_clean 0 set ::dont_pre_clean 0 set ::wait_server 0 +set ::exit_on_failure 0 set ::stop_on_failure 0 set ::dump_logs 0 set ::loop 0 @@ -383,6 +384,11 @@ proc read_from_test_client fd { puts $err lappend ::failed_tests $err set ::active_clients_task($fd) "(ERR) $data" + if {$::exit_on_failure} { + puts -nonewline "(Fast fail: test will exit now)" + flush stdout + exit 1 + } if {$::stop_on_failure} { puts -nonewline "(Test stopped, press enter to resume the tests)" flush stdout @@ -564,6 +570,7 @@ proc print_help_screen {} { "--dont-clean Don't delete valkey log files after the run." "--dont-pre-clean Don't delete existing valkey log files before the run." "--no-latency Skip latency measurements and validation by some tests." + "--fastfail Exit immediately once the first test fails." "--stop Blocks once the first test fails." "--loop Execute the specified set of tests forever." "--loops Execute the specified set of tests several times." @@ -688,6 +695,8 @@ for {set j 0} {$j < [llength $argv]} {incr j} { set ::wait_server 1 } elseif {$opt eq {--dump-logs}} { set ::dump_logs 1 + } elseif {$opt eq {--fastfail}} { + set ::exit_on_failure 1 } elseif {$opt eq {--stop}} { set ::stop_on_failure 1 } elseif {$opt eq {--loop}} { From 7a9951fb8093c96ceb75da2f7245a1110b614794 Mon Sep 17 00:00:00 2001 From: Lipeng Zhu Date: Thu, 16 May 2024 09:22:50 +0800 Subject: [PATCH 5/8] Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. (#476) This patch try to correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Maybe the https://github.com/valkey-io/valkey/pull/453 optimization should depend on this. Signed-off-by: Lipeng Zhu --- src/sds.c | 14 +++++++----- src/unit/test_sds.c | 48 ++++++++++++++++++++++++----------------- tests/unit/querybuf.tcl | 2 +- 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/sds.c b/src/sds.c index 3939cc5b3b..f9ec3cc062 100644 --- a/src/sds.c +++ b/src/sds.c @@ -349,6 +349,7 @@ sds sdsResize(sds s, size_t size, int would_regrow) { * type. */ int use_realloc = (oldtype==type || (type < oldtype && type > SDS_TYPE_8)); size_t newlen = use_realloc ? oldhdrlen+size+1 : hdrlen+size+1; + size_t newsize = 0; if (use_realloc) { int alloc_already_optimal = 0; @@ -357,24 +358,27 @@ sds sdsResize(sds s, size_t size, int would_regrow) { * We aim to avoid calling realloc() when using Jemalloc if there is no * change in the allocation size, as it incurs a cost even if the * allocation size stays the same. */ - alloc_already_optimal = (je_nallocx(newlen, 0) == zmalloc_size(sh)); + newsize = zmalloc_size(sh); + alloc_already_optimal = (je_nallocx(newlen, 0) == newsize); #endif if (!alloc_already_optimal) { - newsh = s_realloc(sh, newlen); + newsh = s_realloc_usable(sh, newlen, &newsize); if (newsh == NULL) return NULL; s = (char*)newsh+oldhdrlen; + newsize -= (oldhdrlen + 1); } } else { - newsh = s_malloc(newlen); + newsh = s_malloc_usable(newlen, &newsize); if (newsh == NULL) return NULL; memcpy((char*)newsh+hdrlen, s, len); s_free(sh); s = (char*)newsh+hdrlen; s[-1] = type; + newsize -= (hdrlen + 1); } - s[len] = 0; + s[len] = '\0'; sdssetlen(s, len); - sdssetalloc(s, size); + sdssetalloc(s, newsize); return s; } diff --git a/src/unit/test_sds.c b/src/unit/test_sds.c index 15cda483fb..9826750391 100644 --- a/src/unit/test_sds.c +++ b/src/unit/test_sds.c @@ -227,32 +227,40 @@ int test_sds(int argc, char **argv, int flags) { memcmp(x, "v1={value1} {} v2=value2", 24) == 0); sdsfree(x); - /* Test sdsresize - extend */ + /* Test sdsResize - extend */ x = sdsnew("1234567890123456789012345678901234567890"); x = sdsResize(x, 200, 1); - TEST_ASSERT_MESSAGE("sdsrezie() expand len", sdslen(x) == 40); - TEST_ASSERT_MESSAGE("sdsrezie() expand strlen", strlen(x) == 40); - TEST_ASSERT_MESSAGE("sdsrezie() expand alloc", sdsalloc(x) == 200); - /* Test sdsresize - trim free space */ + TEST_ASSERT_MESSAGE("sdsReszie() expand type", x[-1] == SDS_TYPE_8); + TEST_ASSERT_MESSAGE("sdsReszie() expand len", sdslen(x) == 40); + TEST_ASSERT_MESSAGE("sdsReszie() expand strlen", strlen(x) == 40); + /* Different allocator allocates at least as large as requested size, + * to confirm the allocator won't waste too much, + * we add a largest size checker here. */ + TEST_ASSERT_MESSAGE("sdsReszie() expand alloc", sdsalloc(x) >= 200 && sdsalloc(x) < 400); + /* Test sdsResize - trim free space */ x = sdsResize(x, 80, 1); - TEST_ASSERT_MESSAGE("sdsrezie() shrink len", sdslen(x) == 40); - TEST_ASSERT_MESSAGE("sdsrezie() shrink strlen", strlen(x) == 40); - TEST_ASSERT_MESSAGE("sdsrezie() shrink alloc", sdsalloc(x) == 80); - /* Test sdsresize - crop used space */ + TEST_ASSERT_MESSAGE("sdsReszie() shrink type", x[-1] == SDS_TYPE_8); + TEST_ASSERT_MESSAGE("sdsReszie() shrink len", sdslen(x) == 40); + TEST_ASSERT_MESSAGE("sdsReszie() shrink strlen", strlen(x) == 40); + TEST_ASSERT_MESSAGE("sdsReszie() shrink alloc", sdsalloc(x) >= 80); + /* Test sdsResize - crop used space */ x = sdsResize(x, 30, 1); - TEST_ASSERT_MESSAGE("sdsrezie() crop len", sdslen(x) == 30); - TEST_ASSERT_MESSAGE("sdsrezie() crop strlen", strlen(x) == 30); - TEST_ASSERT_MESSAGE("sdsrezie() crop alloc", sdsalloc(x) == 30); - /* Test sdsresize - extend to different class */ + TEST_ASSERT_MESSAGE("sdsReszie() crop type", x[-1] == SDS_TYPE_8); + TEST_ASSERT_MESSAGE("sdsReszie() crop len", sdslen(x) == 30); + TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 30); + TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 30); + /* Test sdsResize - extend to different class */ x = sdsResize(x, 400, 1); - TEST_ASSERT_MESSAGE("sdsrezie() expand len", sdslen(x) == 30); - TEST_ASSERT_MESSAGE("sdsrezie() expand strlen", strlen(x) == 30); - TEST_ASSERT_MESSAGE("sdsrezie() expand alloc", sdsalloc(x) == 400); - /* Test sdsresize - shrink to different class */ + TEST_ASSERT_MESSAGE("sdsReszie() expand type", x[-1] == SDS_TYPE_16); + TEST_ASSERT_MESSAGE("sdsReszie() expand len", sdslen(x) == 30); + TEST_ASSERT_MESSAGE("sdsReszie() expand strlen", strlen(x) == 30); + TEST_ASSERT_MESSAGE("sdsReszie() expand alloc", sdsalloc(x) >= 400); + /* Test sdsResize - shrink to different class */ x = sdsResize(x, 4, 1); - TEST_ASSERT_MESSAGE("sdsrezie() crop len", sdslen(x) == 4); - TEST_ASSERT_MESSAGE("sdsrezie() crop strlen", strlen(x) == 4); - TEST_ASSERT_MESSAGE("sdsrezie() crop alloc", sdsalloc(x) == 4); + TEST_ASSERT_MESSAGE("sdsReszie() crop type", x[-1] == SDS_TYPE_8); + TEST_ASSERT_MESSAGE("sdsReszie() crop len", sdslen(x) == 4); + TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 4); + TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 4); sdsfree(x); return 0; } diff --git a/tests/unit/querybuf.tcl b/tests/unit/querybuf.tcl index 27a94604a2..0394b72c00 100644 --- a/tests/unit/querybuf.tcl +++ b/tests/unit/querybuf.tcl @@ -33,7 +33,7 @@ start_server {tags {"querybuf slow"}} { # Check that the initial query buffer is resized after 2 sec wait_for_condition 1000 10 { - [client_idle_sec test_client] >= 3 && [client_query_buffer test_client] == 0 + [client_idle_sec test_client] >= 3 && [client_query_buffer test_client] < $orig_test_client_qbuf } else { fail "query buffer was not resized" } From fd53f17a6196baccf6c4c279f0ae286dbcc8b11c Mon Sep 17 00:00:00 2001 From: Ping Xie Date: Thu, 16 May 2024 22:59:00 -0700 Subject: [PATCH 6/8] Use pause_process to stop a node to make Valgrind happy, hopefully (#508) Signed-off-by: Ping Xie --- tests/unit/cluster/slot-migration.tcl | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/unit/cluster/slot-migration.tcl b/tests/unit/cluster/slot-migration.tcl index f59893851f..d2cfa8e2cc 100644 --- a/tests/unit/cluster/slot-migration.tcl +++ b/tests/unit/cluster/slot-migration.tcl @@ -335,12 +335,12 @@ start_cluster 3 3 {tags {external:skip cluster} overrides {cluster-allow-replica } } -start_cluster 3 3 {tags {external:skip cluster} overrides {crash-memcheck-enabled no cluster-allow-replica-migration no cluster-node-timeout 1000} } { +start_cluster 3 3 {tags {external:skip cluster} overrides {cluster-allow-replica-migration no cluster-node-timeout 1000} } { set R1_id [R 1 CLUSTER MYID] test "CLUSTER SETSLOT with an explicit timeout" { - # Simulate a replica crash - catch {R 3 DEBUG SEGFAULT} e + # Pause the replica to simulate a failure + pause_process [srv -3 pid] # Setslot with an explicit 1ms timeoout set start_time [clock milliseconds] @@ -353,5 +353,7 @@ start_cluster 3 3 {tags {external:skip cluster} overrides {crash-memcheck-enable # Setslot should fail with not enough good replicas to write after the timeout assert_equal {NOREPLICAS Not enough good replicas to write.} $e + + resume_process [srv -3 pid] } } From 9b6232b50120ae38537f22617b3ec8efaa625602 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Thu, 16 May 2024 23:51:33 -0700 Subject: [PATCH 7/8] Automatically notify the slack channel when tests fail (#509) Adds a job that will automatically run at the end of the daily, which will collect all the failed tests and send them to the developer slack. It will include a link to the job as well. Example job that ran on my private repo: https://github.com/madolson/valkey/actions/runs/9123245899/job/25085418567 Example notification: image (Note: I removed the sassy text at the bottom from the PR) Signed-off-by: Madelyn Olson --- .github/workflows/daily.yml | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/.github/workflows/daily.yml b/.github/workflows/daily.yml index 7047a3247d..3c05bc5eb3 100644 --- a/.github/workflows/daily.yml +++ b/.github/workflows/daily.yml @@ -1073,3 +1073,35 @@ jobs: - name: validator run: ./utils/req-res-log-validator.py --verbose --fail-missing-reply-schemas ${{ (!contains(github.event.inputs.skiptests, 'valkey') && !contains(github.event.inputs.skiptests, 'module') && !contains(github.event.inputs.sentinel, 'valkey') && !contains(github.event.inputs.skiptests, 'cluster')) && github.event.inputs.test_args == '' && github.event.inputs.cluster_test_args == '' && '--fail-commands-not-all-hit' || '' }} + notify-about-job-results: + runs-on: ubuntu-latest + if: always() && github.event_name != 'workflow_dispatch' && github.repository == 'valkey-io/valkey' + needs: [test-ubuntu-jemalloc, test-ubuntu-jemalloc-fortify, test-ubuntu-libc-malloc, test-ubuntu-no-malloc-usable-size, test-ubuntu-32bit, test-ubuntu-tls, test-ubuntu-tls-no-tls, test-ubuntu-io-threads, test-ubuntu-reclaim-cache, test-valgrind-test, test-valgrind-misc, test-valgrind-no-malloc-usable-size-test, test-valgrind-no-malloc-usable-size-misc, test-sanitizer-address, test-sanitizer-undefined, test-centos7-jemalloc, test-centos7-tls-module, test-centos7-tls-module-no-tls, test-macos-latest, test-macos-latest-sentinel, test-macos-latest-cluster, build-macos, test-freebsd, test-alpine-jemalloc, test-alpine-libc-malloc, reply-schemas-validator] + steps: + - name: Collect job status + run: | + FAILED_JOBS=() + NEEDS_JSON='${{ toJSON(needs) }}' + JOBS=($(echo "$NEEDS_JSON" | jq 'keys' | tr -d '[] ,')) + for JOB in ${JOBS[@]}; do + JOB_RESULT=$(echo "$NEEDS_JSON" | jq ".[$JOB][\"result\"]" | tr -d '"') + if [ $JOB_RESULT = "failure" ]; then + FAILED_JOBS+=($JOB) + fi + done + + if [[ ${#FAILED_JOBS[@]} -ne 0 ]]; then + echo "FAILED_JOBS=${FAILED_JOBS[@]}" >> $GITHUB_ENV + echo "STATUS=failure" >> $GITHUB_ENV + else + echo "STATUS=success" >> $GITHUB_ENV + fi + - name: Notify about results + uses: ravsamhq/notify-slack-action@v2 + with: + status: ${{ env.STATUS }} + notify_when: "failure" + notification_title: "Daily test run <${{github.server_url}}/${{github.repository}}/actions/runs/${{github.run_id}}|Failure>" + message_format: ":fire: Tests failed: ${{ env.FAILED_JOBS }}" + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_NOTIFICATIONS_WEBHOOK_URL }} From efa8ba519b2d54f68fe05d03cac5b0d5352400ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Fri, 17 May 2024 13:35:31 +0200 Subject: [PATCH 8/8] Finish postponed SCAN changes (#501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 07ed0eafa98a66 introduced some SCAN improvements, but some changes were postponed to a later version (8.0), which this PR finishes: 1. Prepare to move the TYPE filtering to the scan callback as well. this was put on hold since it has side effects that can be considered a breaking change, which is that we will not attempt to do lazy expire (delete) a key that was filtered by not matching the TYPE (changing it would mean TYPE filter starts behaving the same as MATCH filter already does in that respect). 2. when the specified key TYPE filter is an unknown type, server will reply a error immediately instead of doing a full scan that comes back empty handed. Fixes #235 Release notes: > SCAN: Expired keys that don't match the TYPE argument for the SCAN are no longer deleted by SCAN Signed-off-by: Viktor Söderqvist --- src/db.c | 15 ++------------- tests/unit/scan.tcl | 29 ++++++----------------------- 2 files changed, 8 insertions(+), 36 deletions(-) diff --git a/src/db.c b/src/db.c index f5aebf896a..a3fec5d1d1 100644 --- a/src/db.c +++ b/src/db.c @@ -885,11 +885,10 @@ void scanCallback(void *privdata, const dictEntry *de) { serverAssert(!((data->type != LLONG_MAX) && o)); /* Filter an element if it isn't the type we want. */ - /* TODO: uncomment in version 8.0 if (!o && data->type != LLONG_MAX) { robj *rval = dictGetVal(de); if (!objectTypeCompare(rval, data->type)) return; - }*/ + } /* Filter element if it does not match the pattern. */ sds keysds = dictGetKey(de); @@ -1034,9 +1033,8 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) { typename = c->argv[i+1]->ptr; type = getObjectTypeByName(typename); if (type == LLONG_MAX) { - /* TODO: uncomment in version 8.0 addReplyErrorFormat(c, "unknown type name '%s'", typename); - return; */ + return; } i+= 2; } else if (!strcasecmp(c->argv[i]->ptr, "novalues")) { @@ -1195,15 +1193,6 @@ void scanGenericCommand(client *c, robj *o, unsigned long long cursor) { while ((ln = listNext(&li))) { sds key = listNodeValue(ln); initStaticStringObject(kobj, key); - /* Filter an element if it isn't the type we want. */ - /* TODO: remove this in version 8.0 */ - if (typename) { - robj* typecheck = lookupKeyReadWithFlags(c->db, &kobj, LOOKUP_NOTOUCH|LOOKUP_NONOTIFY); - if (!typecheck || !objectTypeCompare(typecheck, type)) { - listDelNode(keys, ln); - } - continue; - } if (expireIfNeeded(c->db, &kobj, 0) != KEY_VALID) { listDelNode(keys, ln); } diff --git a/tests/unit/scan.tcl b/tests/unit/scan.tcl index 2317a03e6a..ecacfaee67 100644 --- a/tests/unit/scan.tcl +++ b/tests/unit/scan.tcl @@ -100,7 +100,8 @@ proc test_scan {type} { test "{$type} SCAN unknown type" { r flushdb - # make sure that passive expiration is triggered by the scan + # Check that passive expiration is not triggered by the scan on an + # unknown key type r debug set-active-expire 0 populate 1000 @@ -109,25 +110,10 @@ proc test_scan {type} { after 2 - # TODO: remove this in server version 8.0 - set cur 0 - set keys {} - while 1 { - set res [r scan $cur type "string1"] - set cur [lindex $res 0] - set k [lindex $res 1] - lappend keys {*}$k - if {$cur == 0} break - } + assert_error "*unknown type name*" {r scan 0 type "string1"} - assert_equal 0 [llength $keys] - # make sure that expired key have been removed by scan command - assert_equal 1000 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] - - # TODO: uncomment in server version 8.0 - #assert_error "*unknown type name*" {r scan 0 type "string1"} - # expired key will be no touched by scan command - #assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] + # expired key is not touched by scan command + assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] r debug set-active-expire 1 } {OK} {needs:debug} @@ -191,11 +177,8 @@ proc test_scan {type} { assert_equal 1000 [llength $keys] - # make sure that expired key have been removed by scan command - assert_equal 1000 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] - # TODO: uncomment in server version 8.0 # make sure that only the expired key in the type match will been removed by scan command - #assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] + assert_equal 1001 [scan [regexp -inline {keys\=([\d]*)} [r info keyspace]] keys=%d] r debug set-active-expire 1 } {OK} {needs:debug}