1. 29 Mar, 2022 4 commits
    • Oran Agra's avatar
      fix daily.yaml skip filters (#10490) · 16d206ee
      Oran Agra authored
      * missing parenthesis meant that the ubuntu and centos jobs were not
        skipped
      * the recently divided freebsd, macos, and valgrind jobs, which are now
        split into distict jobs for redis, modules, sentinel, cluster. were
        all executed, producing a build, but not running anything.
        now they're filtered at the job level
      * iothreads was missing from the skip list defaults, so was not skipped
      16d206ee
    • Viktor Söderqvist's avatar
      redis-cli: Do DNS lookup before sending CLUSTER MEET (#10436) · 35bb0212
      Viktor Söderqvist authored
      Affects `--cluster create` and `--cluster add-node`.
      35bb0212
    • zhaozhao.zz's avatar
      show cluster.links in MEMORY STATS (#10302) · 001e1925
      zhaozhao.zz authored
      001e1925
    • Oran Agra's avatar
      improve malloc efficiency for cluster slots_info_pairs (#10488) · 3b1e65a3
      Oran Agra authored
      This commit improve malloc efficiency of the slots_info_pairs mechanism in cluster.c
      by changing adlist into an array being realloced with greedy growth mechanism
      
      Recently the cluster tests are consistently failing when executed with ASAN in the CI.
      I tried to track down the commit that started it, and it appears to be #10293.
      Looking at the commit, i realize it didn't affect this test / flow, other than the
      replacement of the slots_info_pairs from sds to list.
      
      I concluded that what could be happening is that the slot range is very fragmented,
      and that results in many allocations.
      with sds, it results in one allocation and also, we have a greedy growth mechanism,
      but with adlist, we just have many many small allocations.
      this probably causes stress on ASAN, and causes it to be slow at termination.
      3b1e65a3
  2. 28 Mar, 2022 4 commits
  3. 27 Mar, 2022 3 commits
    • Oran Agra's avatar
      Fix new / failing cluster slot migration test (#10482) · 0b21ef8d
      Oran Agra authored
      #10381 fixed an issue in `redis-cli --cluster reshard` that used to fail it (redis-cli) because
      of a race condition.
      the race condition is / was that when moving the last slot from a node, sometimes the PONG
      messages delivering the configuration change arrive to that node before the SETSLOT arrives
      to it, and it becomes a replica.
      other times the the SETSLOT arrive first, and then PONG **doesn't** demote it.
      
      **however**, the PR also added a new test that suffers from exactly the same race condition,
      and the tests started failing a lot.
      
      The fact is (if i understand it correctly), that this test (the one being deleted here), isn't related
      to the fix that PR fixed (which was to fix redis-cli).
      The race condition in the cluster code still happens, and as long as we don't solve it, there's
      no reason to test it.
      
      For now, even if my understandings are wrong, i'm gonna delete that failing test, since as far as
      i understand, #10381 didn't introduce any new risks for that matter (which are gonna be
      compromised by removing this check), this race existed since forever, and still exists, and the
      fact that redis-cli is now immune to it is still being tested.
      
      Additional work should be carried to fix it, and i live it for other PRs to handle.
      0b21ef8d
    • Moti Cohen's avatar
      Fix Sentinel reconnect test following ACL change (#10480) · 37d761ba
      Moti Cohen authored
      Replace condition with wait_for_condition On "Verify sentinel that restarted 
      failed to reconnect master after ACL change"
      
      The reason we reach it, is because the test is fast enough to modify ACL and
      test sentinel connection status with the server - before its scheduled operation
      got the chance to update connection status with the server:
      ```
      /* Perform scheduled operations for the specified Redis instance. */
      void sentinelHandleRedisInstance(sentinelRedisInstance *ri) {
          /* ========== MONITORING HALF ============ */
          /* Every kind of instance */
          sentinelReconnectInstance(ri);
      ```
      37d761ba
    • Binbin's avatar
      Cleanups in redis.conf (#10452) · f25e688e
      Binbin authored
      Did some cleanups:
      1. local local typo
      2. replace the only slave word in the file
      3. add FUNCTION FLUSH to `lazyfree-lazy-user-flush` description
      4. thought it would be better to use these, there are actually "four" options
      5. the the typo
      6. remove a extra space
      7. change comment next to `activedefrag no` to match the default value
      f25e688e
  4. 26 Mar, 2022 1 commit
  5. 25 Mar, 2022 1 commit
    • zhaozhao.zz's avatar
      optimize(remove) usage of client's pending_querybuf (#10413) · 78bef6e1
      zhaozhao.zz authored
      To remove `pending_querybuf`, the key point is reusing `querybuf`, it means master client's `querybuf` is not only used to parse command, but also proxy to sub-replicas.
      
      1. add a new variable `repl_applied` for master client to record how many data applied (propagated via `replicationFeedStreamFromMasterStream()`) but not trimmed in `querybuf`.
      
      2. don't sdsrange `querybuf` in `commandProcessed()`, we trim it to `repl_applied` after the whole replication pipeline processed to avoid fragmented `sdsrange`. And here are some scenarios we cannot trim to `qb_pos`:
          * we don't receive complete command from master
          * master client blocked because of client pause
          * IO threads operate read, master client flagged with CLIENT_PENDING_COMMAND
      
          In these scenarios, `qb_pos` points to the part of the current command or the beginning of next command, and the current command is not applied yet, so the `repl_applied` is not equal to `qb_pos`.
      
      Some other notes:
      * Do not do big arg optimization on master client, since we can only sdsrange `querybuf` after data sent to replicas.
      * Set `qb_pos` and `repl_applied` to 0 when `freeClient` in `replicationCacheMaster`.
      * Rewrite `processPendingCommandsAndResetClient` to `processPendingCommandAndInputBuffer`, let `processInputBuffer` to be called successively after `processCommandAndResetClient`.
      78bef6e1
  6. 22 Mar, 2022 5 commits
    • Oran Agra's avatar
      Split daily CI into smaller chunks (#10469) · 1a57af62
      Oran Agra authored
      this should aid find the CI issues with freebsd and macos runs, and also
      get faster results from valgrind and tls
      1a57af62
    • Oran Agra's avatar
      crash log, print killer pid only when si_code is SI_USER (#10454) · 6761d10c
      Oran Agra authored
      Avoid printing "Killed by PID" when si_code != SI_USER.
      Apparently SI_USER isn't always set to 0. e.g. on Mac it's 0x10001 and the check that did <= was wrong.
      6761d10c
    • Meir Shpilraien (Spielrein)'s avatar
      Add new RM_Call flags for script mode, no writes, and error replies. (#10372) · f3855a09
      Meir Shpilraien (Spielrein) authored
      The PR extends RM_Call with 3 new capabilities using new flags that
      are given to RM_Call as part of the `fmt` argument.
      It aims to assist modules that are getting a list of commands to be
      executed from the user (not hard coded as part of the module logic),
      think of a module that implements a new scripting language...
      
      * `S` - Run the command in a script mode, this means that it will raise an
        error if a command which are not allowed inside a script (flaged with the
        `deny-script` flag) is invoked (like SHUTDOWN). In addition, on script mode,
        write commands are not allowed if there is not enough good replicas (as
        configured with `min-replicas-to-write`) and/or a disk error happened.
      
      * `W` - no writes mode, Redis will reject any command that is marked with `write`
        flag. Again can be useful to modules that implement a new scripting language
        and wants to prevent any write commands.
      
      * `E` - Return errors as RedisModuleCallReply. Today the errors that happened
        before the command was invoked (like unknown commands or acl error) return
        a NULL reply and set errno. This might be missing important information about
        the failure and it is also impossible to just pass the error to the user using
        RM_ReplyWithCallReply. This new flag allows you to get a RedisModuleCallReply
        object with the relevant error message and treat it as if it was an error that was
        raised by the command invocation.
      
      Tests were added to verify the new code paths.
      
      In addition small refactoring was done to share some code between modules,
      scripts, and `processCommand` function:
      1. `getAclErrorMessage` was added to `acl.c` to unified to log message extraction
        from the acl result
      2. `checkGoodReplicasStatus` was added to `replication.c` to check the status of
        good replicas. It is used on `scriptVerifyWriteCommandAllow`, `RM_Call`, and
        `processCommand`.
      3. `writeCommandsGetDiskErrorMessage` was added to `server.c` to get the error
        message on persistence failure. Again it is used on `scriptVerifyWriteCommandAllow`,
        `RM_Call`, and `processCommand`.
      f3855a09
    • yiyuaner's avatar
      Fix an off by one error in zzlStrtod (#10465) · 08aed7e7
      yiyuaner authored
      When vlen = sizeof(buf), the statement buf[vlen] = '\0' accessing the buffer buf is an off by one error.
      08aed7e7
    • zhaozhao.zz's avatar
      config rewrite enhancement, in case of line longer than 1024 (#8009) · 79db037a
      zhaozhao.zz authored
      When rewrite the config file, we need read the old config file first,
      but the CONFIG_MAX_LEN is 1024, so if some lines are longer than it,
      it will generate a wrong config file, and redis cannot reboot from
      the new config file.
      
      Rename CONFIG_MAX_LINE to CONFIG_READ_LEN
      79db037a
  7. 21 Mar, 2022 5 commits
  8. 20 Mar, 2022 1 commit
    • 郭伟光's avatar
      unblockClient: avoid to reset client when the client was shutdown-blocked (#10440) · fae5b1a1
      郭伟光 authored
      fix #10439. see https://github.com/redis/redis/pull/9872
      When executing SHUTDOWN we pause the client so we can un-pause it
      if the shutdown fails.
      this could happen during the timeout, if the shutdown is aborted, but could
      also happen from withing the initial `call()` to shutdown, if the rdb save fails.
      in that case when we return to `call()`, we'll crash if `c->cmd` has been set to NULL.
      
      The call stack is:
      ```
      unblockClient(c)
      replyToClientsBlockedOnShutdown()
      cancelShutdown()
      finishShutdown()
      prepareForShutdown()
      shutdownCommand()
      ```
      
      what's special about SHUTDOWN in that respect is that it can be paused,
      and then un-paused before the original `call()` returns.
      tests where added for both failed shutdown, and a followup successful one.
      fae5b1a1
  9. 18 Mar, 2022 1 commit
    • sundb's avatar
      Restore ::singledb after cluster test (#10441) · b9656adb
      sundb authored
      When ::singledb is 0, we will use db 9 for the test db.
      Since ::singledb is set to 1 in the cluster-related tests, but not restored, some subsequent
      tests associated with db 9 will fail.
      b9656adb
  10. 17 Mar, 2022 1 commit
  11. 16 Mar, 2022 5 commits
    • Madelyn Olson's avatar
    • Viktor Söderqvist's avatar
      Fix redis-cli CLUSTER SETSLOT race conditions (#10381) · 69017fa2
      Viktor Söderqvist authored
      After migrating a slot, send CLUSTER SETSLOT NODE to the destination
      node first to make sure the slot isn't left without an owner in case
      the destination node crashes before it is set as new owner.
      
      When informing the source node, it can happen that the destination
      node has already informed it and if the source node has lost its
      last slot, it has already turned itself into a replica. Redis-cli
      should ignore this error in this case.
      69017fa2
    • Binbin's avatar
      Fix module redact test for valgrind (#10432) · 61b7e591
      Binbin authored
      The new module redact test will fail with valgrind:
      ```
      [err]: modules can redact arguments in tests/unit/moduleapi/auth.tcl
      Expected 'slowlog reset' to be equal to 'auth.redact 1 (redacted) 3 (redacted)' (context: type eval line 12 cmd {assert_equal {slowlog reset} [lindex [lindex [r slowlog get] 2] 3]} proc ::test)
      ```
      
      The reason is that with `slowlog-log-slower-than 10000`,
      `slowlog get` will have a chance to exceed 10ms.
      
      Made two changes to avoid failure:
      1. change `slowlog-log-slower-than` from 10000 to -1, distable it.
      2. assert to use the previous execution result.
      
      In theory, the second one can actually be left unchanged, but i
      think it will be better if it is changed.
      61b7e591
    • Harkrishn Patro's avatar
      Add new cluster shards command (#10293) · 45ccae89
      Harkrishn Patro authored
      
      
      Implement a new cluster shards command, which provides a flexible and extensible API for topology discovery.
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      45ccae89
    • Madelyn Olson's avatar
      Add module API for redacting command arguments (#10425) · 416c9ac2
      Madelyn Olson authored
      Add module API for redacting client commands
      416c9ac2
  12. 15 Mar, 2022 3 commits
    • Wen Hui's avatar
      Sentinel: update command json files (#10374) · c30de707
      Wen Hui authored
      c30de707
    • ranshid's avatar
      make sort/ro commands validate external keys access patterns (#10106) (#10340) · 1078e30c
      ranshid authored
      
      
      Currently the sort and sort_ro can access external keys via `GET` and `BY`
      in order to make sure the user cannot violate the authorization ACL
      rules, the decision is to reject external keys access patterns unless ACL allows
      SORT full access to all keys.
      I.e. for backwards compatibility, SORT with GET/BY keeps working, but
      if ACL has restrictions to certain keys, these features get permission denied.
      
      ### Implemented solution
      We have discussed several potential solutions and decided to only allow the GET and BY
      arguments when the user has all key permissions with the SORT command. The reasons
      being that SORT with GET or BY is problematic anyway, for instance it is not supported in
      cluster mode since it doesn't declare keys, and we're not sure the combination of that feature
      with ACL key restriction is really required.
      **HOWEVER** If in the fullness of time we will identify a real need for fine grain access
      support for SORT, we would implement the complete solution which is the alternative
      described below.
      
      ### Alternative (Completion solution):
      Check sort ACL rules after executing it and before committing output (either via store or
      to COB). it would require making several changes to the sort command itself. and would
      potentially cause performance degradation since we will have to collect all the get keys
      instead of just applying them to a temp array and then scan the access keys against the
      ACL selectors. This solution can include an optimization to avoid the overheads of collecting
      the key names, in case the ACL rules grant SORT full key-access, or if the ACL key pattern
      literal matches the one used in GET/BY. It would also mean that authorization would be
      O(nlogn) since we will have to complete most of the command execution before we can
      perform verification
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      1078e30c
    • yoav-steinberg's avatar
      Optimization: remove `updateClientMemUsage` from i/o threads. (#10401) · cf6dcb7b
      yoav-steinberg authored
      In a benchmark we noticed we spend a relatively long time updating the client
      memory usage leading to performance degradation.
      Before #8687 this was performed in the client's cron and didn't affect performance.
      But since introducing client eviction we need to perform this after filling the input
      buffers and after processing commands. This also lead me to write this code to be
      thread safe and perform it in the i/o threads.
      
      It turns out that the main performance issue here is related to atomic operations
      being performed while updating the total clients memory usage stats used for client
      eviction (`server.stat_clients_type_memory[]`). This update needed to be atomic
      because `updateClientMemUsage()` was called from the IO threads.
      
      In this commit I make sure to call `updateClientMemUsage()` only from the main thread.
      In case of threaded IO I call it for each client during the "fan-in" phase of the read/write
      operation. This also means I could chuck the `updateClientMemUsageBucket()` function
      which was called during this phase and embed it into `updateClientMemUsage()`.
      
      Profiling shows this makes `updateClientMemUsage()` (on my x86_64 linux) roughly x4 faster.
      cf6dcb7b
  13. 14 Mar, 2022 2 commits
  14. 13 Mar, 2022 3 commits
  15. 10 Mar, 2022 1 commit
    • Binbin's avatar
      Initialize help when using redis-cli help or redis-cli ? (#10382) · 1797330e
      Binbin authored
      The following usage will output an empty newline:
      ```
      > redis-cli help set
      empty line
      ```
      
      The reason is that in interactive mode, we have called
      `cliInitHelp`, which initializes help.
      
      When using `redis-cli help xxx` or `redis-cli help ? xxx`,
      we can't match the command due to empty `helpEntries`,
      so we output an empty newline.
      
      In this commit, we will call `cliInitHelp` to init the help.
      Note that in this case, we need to call `cliInitHelp` (COMMAND DOCS)
      every time, which i think is acceptable.
      
      So now the output will look like:
      ```
      [redis]# src/redis-cli help get
      
        GET key
        summary: Get the value of a key
        since: 1.0.0
        group: string
      
      [redis]#
      ```
      
      Fixes #10378
      
      This PR also fix a redis-cli crash when using `--ldb --eval`:
      ```
      [root]# src/redis-cli --ldb --eval test.lua test 1
      Lua debugging session started, please use:
      quit    -- End the session.
      restart -- Restart the script in debug mode again.
      help    -- Show Lua script debugging commands.
      
      * Stopped at 1, stop reason = step over
      -> 1   local num = redis.call('GET', KEYS[1]);
      redis-cli: redis-cli.c:718: cliCountCommands: Assertion
      `commandTable->element[i]->type == 1' failed.
      Aborted
      ```
      Because in ldb mode, `COMMAND DOCS` or `COMMAND` will
      return an array, only with one element, and the type
      is `REDIS_REPLY_STATUS`, the result is `<error> Unknown
      Redis Lua debugger command or wrong number of arguments`.
      
      So if we are in the ldb mode, and init the Redis HELP, we
      will get the wrong response and crash the redis-cli.
      In ldb mode we don't initialize HELP, help is only initialized
      after the lua debugging session ends.
      
      It was broken in #10043
      1797330e