1. 06 Sep, 2020 3 commits
    • Oran Agra's avatar
      if diskless repl child is killed, make sure to reap the pid (#7742) · 573246f7
      Oran Agra authored
      Starting redis 6.0 and the changes we made to the diskless master to be
      suitable for TLS, I made the master avoid reaping (wait3) the pid of the
      child until we know all replicas are done reading their rdb.
      
      I did that in order to avoid a state where the rdb_child_pid is -1 but
      we don't yet want to start another fork (still busy serving that data to
      replicas).
      
      It turns out that the solution used so far was problematic in case the
      fork child was being killed (e.g. by the kernel OOM killer), in that
      case there's a chance that we currently disabled the read event on the
      rdb pipe, since we're waiting for a replica to become writable again.
      and in that scenario the master would have never realized the child
      exited, and the replica will remain hung too.
      Note that there's no mechanism to detect a hung replica while it's in
      rdb transfer state.
      
      The solution here is to add another pipe which is used by the parent to
      tell the child it is safe to exit. this mean that when the child exits,
      for whatever reason, it is safe to reap it.
      
      Besides that, i'm re-introducing an adjustment to REPLCONF ACK which was
      part of #6271 (Accelerate diskless master connections) but was dropped
      when that PR was rebased after the TLS fork/pipe changes (5a477946).
      Now that RdbPipeCleanup no longer calls checkChildrenDone, and the ACK
      has chance to detect that the child exited, it should be the one to call
      it so that we don't have to wait for cron (server.hz) to do that.
      573246f7
    • Oran Agra's avatar
      test infra - wait_done_loading · 1b7ba44e
      Oran Agra authored
      reduce code duplication in aof.tcl.
      move creation of clients into the test so that it can be skipped
      1b7ba44e
    • Oran Agra's avatar
      test infra - reduce disk space usage · e3e69c25
      Oran Agra authored
      this is important when running a test with --loop
      e3e69c25
  2. 23 Aug, 2020 1 commit
  3. 06 Aug, 2020 1 commit
    • Oran Agra's avatar
      Accelerate diskless master connections, and general re-connections (#6271) · c17e597d
      Oran Agra authored
      Diskless master has some inherent latencies.
      1) fork starts with delay from cron rather than immediately
      2) replica is put online only after an ACK. but the ACK
         was sent only once a second.
      3) but even if it would arrive immediately, it will not
         register in case cron didn't yet detect that the fork is done.
      
      Besides that, when a replica disconnects, it doesn't immediately
      attempts to re-connect, it waits for replication cron (one per second).
      in case it was already online, it may be important to try to re-connect
      as soon as possible, so that the backlog at the master doesn't vanish.
      
      In case it disconnected during rdb transfer, one can argue that it's
      not very important to re-connect immediately, but this is needed for the
      "diskless loading short read" test to be able to run 100 iterations in 5
      seconds, rather than 3 (waiting for replication cron re-connection)
      
      changes in this commit:
      1) sync command starts a fork immediately if no sync_delay is configured
      2) replica sends REPLCONF ACK when done reading the rdb (rather than on 1s cron)
      3) when a replica unexpectedly disconnets, it immediately tries to
         re-connect rather than waiting 1s
      4) when when a child exits, if there is another replica waiting, we spawn a new
         one right away, instead of waiting for 1s replicationCron.
      5) added a call to connectWithMaster from replicationSetMaster. which is called
         from the REPLICAOF command but also in 3 places in cluster.c, in all of
         these the connection attempt will now be immediate instead of delayed by 1
         second.
      
      side note:
      we can add a call to rdbPipeReadHandler in replconfCommand when getting
      a REPLCONF ACK from the replica to solve a race where the replica got
      the entire rdb and EOF marker before we detected that the pipe was
      closed.
      in the test i did see this race happens in one about of some 300 runs,
      but i concluded that this race is unlikely in real life (where the
      replica is on another host and we're more likely to first detect the
      pipe was closed.
      the test runs 100 iterations in 3 seconds, so in some cases it'll take 4
      seconds instead (waiting for another REPLCONF ACK).
      
      Removing unneeded startBgsaveForReplication from updateSlavesWaitingForBgsave
      Now that CheckChildrenDone is calling the new replicationStartPendingFork
      (extracted from serverCron) there's actually no need to call
      startBgsaveForReplication from updateSlavesWaitingForBgsave anymore,
      since as soon as updateSlavesWaitingForBgsave returns, CheckChildrenDone is
      calling replicationStartPendingFork that handles that anyway.
      The code in updateSlavesWaitingForBgsave had a bug in which it ignored
      repl-diskless-sync-delay, but removing that code shows that this bug was
      hiding another bug, which is that the max_idle should have used >= and
      not >, this one second delay has a big impact on my new test.
      c17e597d
  4. 04 Aug, 2020 1 commit
  5. 28 Jul, 2020 1 commit
    • Oran Agra's avatar
      Fix failing tests due to issues with wait_for_log_message (#7572) · 109b5ccd
      Oran Agra authored
      - the test now waits for specific set of log messages rather than wait for
        timeout looking for just one message.
      - we don't wanna sample the current length of the log after an action, due
        to a race, we need to start the search from the line number of the last
        message we where waiting for.
      - when attempting to trigger a full sync, use multi-exec to avoid a race
        where the replica manages to re-connect before we completed the set of
        actions that should force a full sync.
      - fix verify_log_message which was broken and unused
      109b5ccd
  6. 23 Jul, 2020 1 commit
    • Oran Agra's avatar
      Stabilize bgsave test that sometimes fails with valgrind (#7559) · 8a57969f
      Oran Agra authored
      on ci.redis.io the test fails a lot, reporting that bgsave didn't end.
      increaseing the timeout we wait for that bgsave to get aborted.
      in addition to that, i also verify that it indeed got aborted by
      checking that the save counter wasn't reset.
      
      add another test to verify that a successful bgsave indeed resets the
      change counter.
      8a57969f
  7. 21 Jul, 2020 1 commit
    • Yossi Gottlieb's avatar
      Tests: drop TCL 8.6 dependency. (#7548) · f57e844b
      Yossi Gottlieb authored
      This re-implements the redis-cli --pipe test so it no longer depends on a close feature available only in TCL 8.6.
      
      Basically what this test does is run redis-cli --pipe, generates a bunch of commands and pipes them through redis-cli, and inspects the result in both Redis and the redis-cli output.
      
      To do that, we need to close stdin for redis-cli to indicate we're done so it can flush its buffers and exit. TCL has bi-directional channels can only offers a way to "one-way close" a channel with TCL 8.6. To work around that, we now generate the commands into a file and feed that file to redis-cli directly.
      
      As we're writing to an actual file, the number of commands is now reduced.
      f57e844b
  8. 14 Jul, 2020 1 commit
  9. 13 Jul, 2020 1 commit
    • Oran Agra's avatar
      fix recently added time sensitive tests failing with valgrind (#7512) · e5227aab
      Oran Agra authored
      interestingly the latency monitor test fails because valgrind is slow
      enough so that the time inside PEXPIREAT command from the moment of
      the first mstime() call to get the basetime until checkAlreadyExpired
      calls mstime() again is more than 1ms, and that test was too sensitive.
      
      using this opportunity to speed up the test (unrelated to the failure)
      the fix is just the longer time passed to PEXPIRE.
      e5227aab
  10. 10 Jul, 2020 3 commits
    • Yossi Gottlieb's avatar
      TLS: Add missing redis-cli options. (#7456) · d9f970d8
      Yossi Gottlieb authored
      * Tests: fix and reintroduce redis-cli tests.
      
      These tests have been broken and disabled for 10 years now!
      
      * TLS: add remaining redis-cli support.
      
      This adds support for the redis-cli --pipe, --rdb and --replica options
      previously unsupported in --tls mode.
      
      * Fix writeConn().
      d9f970d8
    • Oran Agra's avatar
      stabilize tests that look for log lines (#7367) · 8e76e134
      Oran Agra authored
      tests were sensitive to additional log lines appearing in the log
      causing the search to come empty handed.
      
      instead of just looking for the n last log lines, capture the log lines
      before performing the action, and then search from that offset.
      8e76e134
    • Oran Agra's avatar
      tests/valgrind: don't use debug restart (#7404) · 69ade873
      Oran Agra authored
      * tests/valgrind: don't use debug restart
      
      DEBUG REATART causes two issues:
      1. it uses execve which replaces the original process and valgrind doesn't
         have a chance to check for errors, so leaks go unreported.
      2. valgrind report invalid calls to close() which we're unable to resolve.
      
      So now the tests use restart_server mechanism in the tests, that terminates
      the old server and starts a new one, new PID, but same stdout, stderr.
      
      since the stderr can contain two or more valgrind report, it is not enough
      to just check for the absence of leaks, we also need to check for some known
      errors, we do both, and fail if we either find an error, or can't find a
      report saying there are no leaks.
      
      other changes:
      - when killing a server that was already terminated we check for leaks too.
      - adding DEBUG LEAK which was used to test it.
      - adding --trace-children to valgrind, although no longer needed.
      - since the stdout contains two or more runs, we need slightly different way
        of checking if the new process is up (explicitly looking for the new PID)
      - move the code that handles --wait-server to happen earlier (before
        watching the startup message in the log), and serve the restarted server too.
      
      * squashme - CR fixes
      69ade873
  11. 31 May, 2020 1 commit
  12. 28 May, 2020 3 commits
  13. 27 May, 2020 2 commits
  14. 25 May, 2020 1 commit
  15. 22 May, 2020 1 commit
  16. 18 May, 2020 1 commit
  17. 17 May, 2020 2 commits
  18. 12 May, 2020 1 commit
    • Oran Agra's avatar
      fix unstable replication test · b4416280
      Oran Agra authored
      this test which has coverage for varoius flows of diskless master was
      failing randomly from time to time.
      
      the failure was:
      [err]: diskless all replicas drop during rdb pipe in tests/integration/replication.tcl
      log message of '*Diskless rdb transfer, last replica dropped, killing fork child*' not found
      
      what seemed to have happened is that the master didn't detect that all
      replicas dropped by the time the replication ended, it thought that one
      replica is still connected.
      
      now the test takes a few seconds longer but it seems stable.
      b4416280
  19. 11 May, 2020 1 commit
    • Oran Agra's avatar
      fix redis 6.0 not freeing closed connections during loading. · 905e28ee
      Oran Agra authored
      This bug was introduced by a recent change in which readQueryFromClient
      is using freeClientAsync, and despite the fact that now
      freeClientsInAsyncFreeQueue is in beforeSleep, that's not enough since
      it's not called during loading in processEventsWhileBlocked.
      furthermore, afterSleep was called in that case but beforeSleep wasn't.
      
      This bug also caused slowness sine the level-triggered mode of epoll
      kept signaling these connections as readable causing us to keep doing
      connRead again and again for ll of these, which keep accumulating.
      
      now both before and after sleep are called, but not all of their actions
      are performed during loading, some are only reserved for the main loop.
      
      fixes issue #7215
      905e28ee
  20. 04 May, 2020 1 commit
    • Oran Agra's avatar
      add daily github actions with libc malloc and valgrind · deee2c1e
      Oran Agra authored
      * fix memlry leaks with diskless replica short read.
      * fix a few timing issues with valgrind runs
      * fix issue with valgrind and watchdog schedule signal
      
      about the valgrind WD issue:
      the stack trace test in logging.tcl, has issues with valgrind:
      ==28808== Can't extend stack to 0x1ffeffdb38 during signal delivery for thread 1:
      ==28808==   too small or bad protection modes
      
      it seems to be some valgrind bug with SA_ONSTACK.
      SA_ONSTACK seems unneeded since WD is not recursive (SA_NODEFER was removed),
      also, not sure if it's even valid without a call to sigaltstack()
      deee2c1e
  21. 28 Apr, 2020 1 commit
  22. 27 Apr, 2020 1 commit
    • Oran Agra's avatar
      Keep track of meaningful replication offset in replicas too · 4447ddc8
      Oran Agra authored
      Now both master and replicas keep track of the last replication offset
      that contains meaningful data (ignoring the tailing pings), and both
      trim that tail from the replication backlog, and the offset with which
      they try to use for psync.
      
      the implication is that if someone missed some pings, or even have
      excessive pings that the promoted replica has, it'll still be able to
      psync (avoid full sync).
      
      the downside (which was already committed) is that replicas running old
      code may fail to psync, since the promoted replica trims pings form it's
      backlog.
      
      This commit adds a test that reproduces several cases of promotions and
      demotions with stale and non-stale pings
      
      Background:
      The mearningful offset on the master was added recently to solve a problem were
      the master is left all alone, injecting PINGs into it's backlog when no one is
      listening and then gets demoted and tries to replicate from a replica that didn't
      have any of the PINGs (or at least not the last ones).
      
      however, consider this case:
      master A has two replicas (B and C) replicating directly from it.
      there's no traffic at all, and also no network issues, just many pings in the
      tail of the backlog. now B gets promoted, A becomes a replica of B, and C
      remains a replica of A. when A gets demoted, it trims the pings from its
      backlog, and successfully replicate from B. however, C is still aware of
      these PINGs, when it'll disconnect and re-connect to A, it'll ask for something
      that's not in the backlog anymore (since A trimmed the tail of it's backlog),
      and be forced to do a full sync (something it didn't have to do before the
      meaningful offset fix).
      
      Besides that, the psync2 test was always failing randomly here and there, it
      turns out the reason were PINGs. Investigating it shows the following scenario:
      
      cycle 1: redis #1 is master, and all the rest are direct replicas of #1
      cycle 2: redis #2 is promoted to master, #1 is a replica of #2 and #3 is replica of #1
      now we see that when #1 is demoted it prints:
      17339:S 21 Apr 2020 11:16:38.523 * Using the meaningful offset 3929963 instead of 3929977 to exclude the final PINGs (14 bytes difference)
      17339:S 21 Apr 2020 11:16:39.391 * Trying a partial resynchronization (request e2b3f8817735fdfe5fa4626766daa938b61419e5:3929964).
      17339:S 21 Apr 2020 11:16:39.392 * Successful partial resynchronization with master.
      and when #3 connects to the demoted #2, #2 says:
      17339:S 21 Apr 2020 11:16:40.084 * Partial resynchronization not accepted: Requested offset for secondary ID was 3929978, but I can reply up to 3929964
      
      so the issue here is that the meaningful offset feature saved the day for the
      demoted master (since it needs to sync from a replica that didn't get the last
      ping), but it didn't help one of the other replicas which did get the last ping.
      4447ddc8
  23. 25 Mar, 2020 1 commit
  24. 05 Mar, 2020 1 commit
    • Oran Agra's avatar
      fix for flaky psync2 test · 27641ee4
      Oran Agra authored
      *** [err]: PSYNC2: total sum of full synchronizations is exactly 4 in tests/integration/psync2.tcl
      Expected 5 == 4 (context: type eval line 6 cmd {assert {$sum == 4}} proc ::test)
      
      issue was that sometime the test got an unexpected full sync since it
      tried to switch to the replica before it was in sync with it's master.
      27641ee4
  25. 18 Dec, 2019 1 commit
  26. 09 Oct, 2019 1 commit
  27. 07 Oct, 2019 4 commits
    • Yossi Gottlieb's avatar
      TLS: Configuration options. · 61733ded
      Yossi Gottlieb authored
      Add configuration options for TLS protocol versions, ciphers/cipher
      suites selection, etc.
      61733ded
    • Oran Agra's avatar
      TLS: Implement support for write barrier. · 6b629480
      Oran Agra authored
      6b629480
    • Oran Agra's avatar
      diskless replication rdb transfer uses pipe, and writes to sockets form the parent process. · 5a477946
      Oran Agra authored
      misc:
      - handle SSL_has_pending by iterating though these in beforeSleep, and setting timeout of 0 to aeProcessEvents
      - fix issue with epoll signaling EPOLLHUP and EPOLLERR only to the write handlers. (needed to detect the rdb pipe was closed)
      - add key-load-delay config for testing
      - trim connShutdown which is no longer needed
      - rioFdsetWrite -> rioFdWrite - simplified since there's no longer need to write to multiple FDs
      - don't detect rdb child exited (don't call wait3) until we detect the pipe is closed
      - Cleanup bad optimization from rio.c, add another one
      5a477946
    • Yossi Gottlieb's avatar
      TLS: Connections refactoring and TLS support. · b087dd1d
      Yossi Gottlieb authored
      * Introduce a connection abstraction layer for all socket operations and
      integrate it across the code base.
      * Provide an optional TLS connections implementation based on OpenSSL.
      * Pull a newer version of hiredis with TLS support.
      * Tests, redis-cli updates for TLS support.
      b087dd1d
  28. 26 Sep, 2019 1 commit
  29. 17 Jul, 2019 1 commit
    • Oran Agra's avatar
      prevent diskless replica from terminating on short read · c56b4ddc
      Oran Agra authored
      now that replica can read rdb directly from the socket, it should avoid exiting
      on short read and instead try to re-sync.
      
      this commit tries to have minimal effects on non-diskless rdb reading.
      and includes a test that tries to trigger this scenario on various read cases.
      c56b4ddc