Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cross-version test framework (and a simple test) #1371

Merged
merged 8 commits into from
Jan 23, 2025
Merged

Add cross-version test framework (and a simple test) #1371

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
77c4c0d
Add replication test between old and new version
zuiderkwast Nov 27, 2024
9e90759
Add documentation for missing tags and arguments
zuiderkwast Nov 28, 2024
dc0e715
Change "old server" to "other server"
zuiderkwast Dec 18, 2024
6d3b7d6
Merge remote-tracking branch 'valkey/unstable' into upgrade-test
zuiderkwast Dec 18, 2024
05da3c9
Fix tests/README.md formatting/alignment
zuiderkwast Dec 20, 2024
1f5b83e
Merge remote-tracking branch 'valkey/unstable' into upgrade-test
zuiderkwast Jan 23, 2025
64212b0
Change other binary to valkey-7.2.8-noble-x86_64
zuiderkwast Jan 23, 2025
a0d0a39
Use 7.2.7 instead of 7.2.8
zuiderkwast Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,15 @@ jobs:
# Fail build if there are warnings
# build with TLS just for compilation coverage
run: make -j4 all-with-unit-tests SERVER_CFLAGS='-Werror' BUILD_TLS=yes USE_FAST_FLOAT=yes
- name: install old server for compatibility testing
zuiderkwast marked this conversation as resolved.
Show resolved Hide resolved
run: |
cd tests/tmp
wget https://download.valkey.io/releases/valkey-7.2.7-focal-x86_64.tar.gz
tar -xvf valkey-7.2.7-focal-x86_64.tar.gz
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer this to be in a script which can be invoked by multiple tests and provide engine version as parameter and skip downloading if it already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking keep it minimal and that we can add some abstraction for it when we need it.

What kind of script do you have in mind? A python script under utils/, a yaml file under .github/ or a TCL script under tests/? If we put it under utils/, does that mean we officially support the script?

If we add a script with these few lines, is the version parameter a string like "7.2.7" and we assume the rest of the URLs are always identical? What if we're testing something from another URL, or with a different executable name (an old redis-server, kvdb-server, etc.) Then the parameters are bigger than the script itself.

So far, we don't have any scripts that downloads anything from hard-coded URLs, apart from in the CI jobs, so it think this might be the best place for it. Can we add some github action definition that can be used from multiple github actions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sticking to Github actions options seems problematic i.e. how do other vendors/users test backward compatibility locally?

My thought is to have a python script with well defined old server name+version (enum) and download path (doesn't need to be identical). And yes, we would need to support it officially.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do other vendors/users test backward compatibility locally?

Like this:

./runtest --old-server-path ../my-other-valkey-clone/src/valkey-server

You may want to build it from source, either a source release or a git checkout, even to test a development branch against unstable.

Using a binary release from our website was just a simple way to get the CI running. Not sure it's the best way or a method I want to officially support.

My thought is to have a python script with well defined old server name+version (enum) and download path (doesn't need to be identical). And yes, we would need to support it officially.

It seems like a commitment to support a bunch of URLs to binary releases. I don't really see the benefit. Is it to have predictable tests? Each compatiblity test uses a fixed version, so it knows what to expect?

Keep in mind that it's also a different binary for a different architecture and per OS. To run it on ARM, you can't use the same binary. There are even differences between ubuntu versions, like OpenSSL version and such things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For daily are we going to run it against all supported versions? That can also be a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sounds good, and we should add some more meaningful test cases too, like the one we wanted for the light-weight cluster message and the cluster ping-extension issues we've had in the past. I just wanted to create the scaffolding and a simple test case first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this incrementally maybe? I think it's better to have this code in and running and we can adjust it as needed, for now just a wget seems OK.

- name: test
run: |
sudo apt-get install tcl8.6 tclx
./runtest --verbose --tags -slow --dump-logs
./runtest --verbose --tags -slow --dump-logs --other-server-path tests/tmp/valkey-7.2.7-focal-x86_64/bin/valkey-server
- name: module api test
run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs
- name: validate commands.def up to date
Expand Down
29 changes: 17 additions & 12 deletions tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,17 @@ There are additional runtime options that can further adjust the test suite to
match different external server configurations. All options are listed by
`./runtest --help`. The following table is just a subset of the options:

| Option | Impact |
| -------------------- | -------------------------------------------------------- |
| `--singledb` | Only use database 0, don't assume others are supported. |
| `--ignore-encoding` | Skip all checks for specific encoding. |
| `--ignore-digest` | Skip key value digest validations. |
| `--cluster-mode` | Run in strict Valkey Cluster compatibility mode. |
| `--large-memory` | Enables tests that consume more than 100MB |
| `--tls` | Run tests with TLS. See below. |
| `--tls-module` | Run tests with TLS, when TLS support is built as a module. |
| `--help` | Displays the full set of options. |
| Option | Impact |
| -------------------------- | -------------------------------------------------------- |
| `--singledb` | Only use database 0, don't assume others are supported. |
| `--ignore-encoding` | Skip all checks for specific encoding. |
| `--ignore-digest` | Skip key value digest validations. |
| `--cluster-mode` | Run in strict Valkey Cluster compatibility mode. |
| `--large-memory` | Enables tests that consume more than 100MB |
| `--tls` | Run tests with TLS. See below. |
| `--tls-module` | Run tests with TLS, when TLS support is built as a module. |
| `--other-server-path PATH` | Run compatibility tests with an other server executable. |
| `--help` | Displays the full set of options. |

Running with TLS requires the following preparations:

Expand Down Expand Up @@ -86,18 +87,22 @@ Tags can be applied in different context levels:
The following compatibility and capability tags are currently used:

| Tag | Indicates |
| --------------------- | --------- |
| ------------------------- | --------- |
| `external:skip` | Not compatible with external servers. |
| `cluster` | Uses cluster with multiple nodes. |
| `cluster:skip` | Not compatible with `--cluster-mode`. |
| `large-memory` | Test that requires more than 100MB |
| `tls` | Uses TLS. |
| `tls:skip` | Not compatible with `--tls`. |
| `needs:repl` | Uses replication and needs to be able to `SYNC` from server. |
| `ipv6` | Uses IPv6. |
| `needs:repl`, `repl` | Uses replication and needs to be able to `SYNC` from server. |
| `needs:debug` | Uses the `DEBUG` command or other debugging focused commands (like `OBJECT REFCOUNT`). |
| `needs:pfdebug` | Uses the `PFDEBUG` command. |
| `needs:config-maxmemory` | Uses `CONFIG SET` to manipulate memory limit, eviction policies, etc. |
| `needs:config-resetstat` | Uses `CONFIG RESETSTAT` to reset statistics. |
| `needs:reset` | Uses `RESET` to reset client connections. |
| `needs:save` | Uses `SAVE` or `BGSAVE` to create an RDB file. |
| `needs:other-server` | Requires `--other-server-path`. |

When using an external server (`--host` and `--port`), filtering using the
`external:skip` tags is done automatically.
Expand Down
34 changes: 34 additions & 0 deletions tests/integration/cross-version-replication.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Test replication from an older version primary.
#
# Use minimal.conf to make sure we don't use any configs not supported on the old version.

proc server_name_and_version {} {
set server_name [s server_name]
if {$server_name eq {}} {
set server_name redis
}
set server_version [s "${server_name}_version"]
return "$server_name $server_version"
}

start_server {tags {"repl needs:other-server external:skip"} start-other-server 1 config "minimal.conf"} {
set primary_name_and_version [server_name_and_version]
r set foo bar

start_server {} {
test "Start replication from $primary_name_and_version" {
r replicaof [srv -1 host] [srv -1 port]
wait_for_sync r
# The key has been transferred.
assert_equal bar [r get foo]
assert_equal up [s master_link_status]
}

test "Replicate a SET command from $primary_name_and_version" {
r -1 set baz quux
wait_for_ofs_sync [srv 0 client] [srv -1 client]
set reply [r get baz]
assert_equal $reply quux
}
}
}
46 changes: 33 additions & 13 deletions tests/support/server.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ set ::global_overrides {}
set ::tags {}
set ::valgrind_errors {}

proc start_server_error {config_file error} {
proc start_server_error {executable config_file error} {
set err {}
append err "Can't start the Valkey server\n"
append err "Can't start $executable\n"
append err "CONFIGURATION:\n"
append err [exec cat $config_file]
append err "\nERROR:\n"
Expand Down Expand Up @@ -216,6 +216,11 @@ proc tags_acceptable {tags err_return} {
return 0
}

if {$::other_server_path eq {} && [lsearch $tags "needs:other-server"] >= 0} {
set err "Other server path not provided"
return 0
}

if {$::external && [lsearch $tags "external:skip"] >= 0} {
set err "Not supported on external server"
return 0
Expand Down Expand Up @@ -289,8 +294,8 @@ proc create_server_config_file {filename config config_lines} {
close $fp
}

proc spawn_server {config_file stdout stderr args} {
set cmd [list src/valkey-server $config_file]
proc spawn_server {executable config_file stdout stderr args} {
set cmd [list $executable $config_file]
set args {*}$args
if {[llength $args] > 0} {
lappend cmd {*}$args
Expand Down Expand Up @@ -319,7 +324,7 @@ proc spawn_server {config_file stdout stderr args} {
}

# Wait for actual startup, return 1 if port is busy, 0 otherwise
proc wait_server_started {config_file stdout stderr pid} {
proc wait_server_started {executable config_file stdout stderr pid} {
set checkperiod 100; # Milliseconds
set maxiter [expr {120*1000/$checkperiod}] ; # Wait up to 2 minutes.
set port_busy 0
Expand All @@ -330,7 +335,7 @@ proc wait_server_started {config_file stdout stderr pid} {
after $checkperiod
incr maxiter -1
if {$maxiter == 0} {
start_server_error $config_file "No PID detected in log $stdout"
start_server_error $executable $config_file "No PID detected in log $stdout"
puts "--- LOG CONTENT ---"
puts [exec cat $stdout]
puts "-------------------"
Expand All @@ -347,7 +352,7 @@ proc wait_server_started {config_file stdout stderr pid} {
# Configuration errors are unexpected, but it's helpful to fail fast
# to give the feedback to the test runner.
if {[regexp {FATAL CONFIG FILE ERROR} [exec cat $stderr]]} {
start_server_error $config_file "Configuration issue prevented Valkey startup"
start_server_error $executable $config_file "Configuration issue prevented Valkey startup"
break
}
}
Expand Down Expand Up @@ -441,13 +446,17 @@ proc start_server {options {code undefined}} {
set args {}
set keep_persistence false
set config_lines {}
set start_other_server 0

# Wait for the server to be ready and check for server liveness/client connectivity before starting the test.
set wait_ready true

# parse options
foreach {option value} $options {
switch $option {
"start-other-server" {
set start_other_server $value ; # boolean, 0 or 1
}
"config" {
set baseconfig $value
}
Expand Down Expand Up @@ -498,6 +507,15 @@ proc start_server {options {code undefined}} {
return
}

if {$start_other_server} {
set executable $::other_server_path
if {![file executable $executable]} {
error "File not found or not executable: $executable"
}
} else {
set executable "src/valkey-server"
}

set data [split [exec cat "tests/assets/$baseconfig"] "\n"]
set config {}
if {$::tls} {
Expand Down Expand Up @@ -588,15 +606,15 @@ proc start_server {options {code undefined}} {
set server_started 0
while {$server_started == 0} {
if {$::verbose} {
puts -nonewline "=== ($tags) Starting server ${::host}:${port} "
puts -nonewline "=== ($tags) Starting server on ${::host}:${port} "
}

send_data_packet $::test_server_fd "server-spawning" "port $port"

set pid [spawn_server $config_file $stdout $stderr $args]
set pid [spawn_server $executable $config_file $stdout $stderr $args]

# check that the server actually started
set port_busy [wait_server_started $config_file $stdout $stderr $pid]
set port_busy [wait_server_started $executable $config_file $stdout $stderr $pid]

# Sometimes we have to try a different port, even if we checked
# for availability. Other test clients may grab the port before we
Expand Down Expand Up @@ -634,7 +652,7 @@ proc start_server {options {code undefined}} {
if {!$serverisup} {
set err {}
append err [exec cat $stdout] "\n" [exec cat $stderr]
start_server_error $config_file $err
start_server_error $executable $config_file $err
return
}
set server_started 1
Expand All @@ -647,6 +665,7 @@ proc start_server {options {code undefined}} {
if {[dict exists $config $port_param]} { set port [dict get $config $port_param] }

# setup config dict
dict set srv "executable" $executable
dict set srv "config_file" $config_file
dict set srv "config" $config
dict set srv "pid" $pid
Expand Down Expand Up @@ -801,12 +820,13 @@ proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigter
close $fd
}

set executable [dict get $srv "executable"]
set config_file [dict get $srv "config_file"]

set pid [spawn_server $config_file $stdout $stderr {}]
set pid [spawn_server $executable $config_file $stdout $stderr {}]

# check that the server actually started
wait_server_started $config_file $stdout $stderr $pid
wait_server_started $executable $config_file $stdout $stderr $pid

# update the pid in the servers list
dict set srv "pid" $pid
Expand Down
34 changes: 26 additions & 8 deletions tests/test_helper.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ set ::single_tests {}
set ::run_solo_tests {}
set ::skip_till ""
set ::external 0; # If "1" this means, we are running against external instance
set ::other_server_path {}; # Used in upgrade and inter-version tests
set ::file ""; # If set, runs only the tests in this comma separated list
set ::curfile ""; # Hold the filename of the current suite
set ::accurate 0; # If true runs fuzz tests with more iterations
Expand Down Expand Up @@ -568,40 +569,54 @@ proc send_data_packet {fd status data {elapsed 0}} {

proc print_help_screen {} {
puts [join {
"--cluster Run the cluster tests, by default cluster tests run along with all tests."
"--moduleapi Run the module API tests, this option should only be used in runtest-moduleapi which will build the test module."
# This is for terminal output, so assume default term width of 80 columns. -----|
"--cluster Run the cluster tests, by default cluster tests run along"
" with all tests."
"--moduleapi Run the module API tests, this option should only be used in"
" runtest-moduleapi which will build the test module."
"--valgrind Run the test over valgrind."
"--durable suppress test crashes and keep running"
"--stack-logging Enable OSX leaks/malloc stack logging."
"--accurate Run slow randomized tests for more iterations."
"--quiet Don't show individual tests."
"--single <unit> Just execute the specified unit (see next option). This option can be repeated."
"--single <unit> Just execute the specified unit (see next option). This"
" option can be repeated."
"--verbose Increases verbosity."
"--list-tests List all the available test units."
"--only <test> Just execute the specified test by test name or tests that match <test> regexp (if <test> starts with '/'). This option can be repeated."
"--only <test> Just execute the specified test by test name or tests that"
" match <test> regexp (if <test> starts with '/'). This option"
" can be repeated."
"--skip-till <unit> Skip all units until (and including) the specified one."
"--skipunit <unit> Skip one unit."
"--clients <num> Number of test clients (default 16)."
"--timeout <sec> Test timeout in seconds (default 20 min)."
"--force-failure Force the execution of a test that always fails."
"--config <k> <v> Extra config file argument."
"--skipfile <file> Name of a file containing test names or regexp patterns (if <test> starts with '/') that should be skipped (one per line). This option can be repeated."
"--skiptest <test> Test name or regexp pattern (if <test> starts with '/') to skip. This option can be repeated."
"--tags <tags> Run only tests having specified tags or not having '-' prefixed tags."
"--skipfile <file> Name of a file containing test names or regexp patterns (if"
" <test> starts with '/') that should be skipped (one per"
" line). This option can be repeated."
"--skiptest <test> Test name or regexp pattern (if <test> starts with '/') to"
" skip. This option can be repeated."
"--tags <tags> Run only tests having specified tags or not having '-'"
" prefixed tags."
"--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 <count> Execute the specified set of tests several times."
"--wait-server Wait after server is started (so that you can attach a debugger)."
"--wait-server Wait after server is started (so that you can attach a"
" debugger)."
"--dump-logs Dump server log on test failure."
"--io-threads Run tests with IO threads."
"--tls Run tests in TLS mode."
"--tls-module Run tests in TLS mode with Valkey module."
"--host <addr> Run tests against an external host."
"--port <port> TCP port to use against external host."
"--other-server-path <path>"
" Path to another version of valkey-server, used for inter-"
" version compatibility testing."
"--baseport <port> Initial port number for spawned valkey servers."
"--portcount <num> Port range for spawned valkey servers."
"--singledb Use a single database, avoid SELECT."
Expand Down Expand Up @@ -676,6 +691,9 @@ for {set j 0} {$j < [llength $argv]} {incr j} {
} elseif {$opt eq {--port}} {
set ::port $arg
incr j
} elseif {$opt eq {--other-server-path}} {
set ::other_server_path $arg
incr j
} elseif {$opt eq {--baseport}} {
set ::baseport $arg
incr j
Expand Down
Loading