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
+116 −34
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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 --old-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
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:old-server external:skip"} start-old-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 {$::old_server_path eq {} && [lsearch $tags "needs:old-server"] >= 0} {
set err "Old 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 @@ -284,8 +289,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 @@ -314,7 +319,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 @@ -325,7 +330,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 @@ -342,7 +347,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 @@ -436,13 +441,17 @@ proc start_server {options {code undefined}} {
set args {}
set keep_persistence false
set config_lines {}
set start_old_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-old-server" {
set start_old_server $value
}
zuiderkwast marked this conversation as resolved.
Show resolved Hide resolved
"config" {
set baseconfig $value
}
Expand Down Expand Up @@ -493,6 +502,15 @@ proc start_server {options {code undefined}} {
return
}

if {$start_old_server} {
set executable $::old_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 @@ -583,15 +601,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 @@ -629,7 +647,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 @@ -642,6 +660,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 @@ -796,12 +815,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
7 changes: 7 additions & 0 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 ::old_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 @@ -600,6 +601,9 @@ proc print_help_screen {} {
"--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."
"--old-server-path <path>"
zuiderkwast marked this conversation as resolved.
Show resolved Hide resolved
" 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 @@ -673,6 +677,9 @@ for {set j 0} {$j < [llength $argv]} {incr j} {
} elseif {$opt eq {--port}} {
set ::port $arg
incr j
} elseif {$opt eq {--old-server-path}} {
set ::old_server_path $arg
incr j
} elseif {$opt eq {--baseport}} {
set ::baseport $arg
incr j
Expand Down
Loading