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 replication test #1371

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

Add cross-version replication test #1371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Copy link
Member

Choose a reason for hiding this comment

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

Since I guess we might theoretically do other types of compatibility, maybe something like:

Suggested change
- name: install old server for compatibility testing
- name: install secondary server for compatibility testing

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
Comment on lines +23 to +25
Copy link
Contributor

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
Contributor

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.

- 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>"
Copy link
Member

Choose a reason for hiding this comment

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

IMHO using --old-server-path might limit the scope to only older versions, while --other-server-path or --compat-server-path feels less restrictive and better reflects its purpose for inter-version compatibility testing.

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 thought about words like "other", "alternative" server, etc. This is how I thought when I picked "old":

  • If we're testing the compatibility between an older and a newer versions, let's say Valkey 8.1 and Valkey 9.0, either upgrade, downgrade or cluster bus compatibility, those are tests that we would add in the later version, Valkey 9 in this case. We wouldn't normally add tests in an old version to test against a future version.
  • A name like "other server" is less clear when reading a test case about compatibility between and old and a new version. If you see "old server", it's very clear which one is the new version and which is the old version. A name like other would be more ambiguous.

Not sure the above are important though and I understand your point that we can also test against something like KeyDB, AWS MemoryDB, Rockskv, etc. Also the same Valkey version compiled in different ways, with/without some modules, without Lua, etc, etc. Yes, there are definitely more use cases than old and new, so I agree something like "other" is more generic.

Other just means not the system under test, which is what matters here. "Compat" doesn't seem general enough though. We may (hypothetically) want to test something against some incompatible version to test the failure modes (e.g. that we log some error and don't crash).

I'll change to "other" then, unless you have more ideas or pros/cons.

Copy link
Member

Choose a reason for hiding this comment

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

I proposed secondary in my other comment, I like other too. I don't really like compat.

Copy link
Contributor Author

@zuiderkwast zuiderkwast Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've heard secondary in the context of primary-secondary, meaning primary-replica.

I prefer "other" so far. The essence is that it's a different one than the SUT one. Other = different.

We could use "different", but --different-server-path doesn't look that great.

" 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