Add cross-version replication test #1371

zuiderkwast · 2024-11-28T10:38:11Z

This includes a way to run two versions of the server from the TCL test framework. It's a preparation to add more cross-version tests. The runtest script accepts a new parameter

./runtest --old-server-path path/to/valkey-server

and a new tag "needs:old-server" for test cases and start_server. Tests with this tag are automatically skipped if --old-server-path is not provided.

This PR adds it in a CI job with Valkey 7.2.7 by downloading a binary release.

Fixes #76

codecov · 2024-11-28T10:53:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.59%. Comparing base (66ae8b7) to head (77c4c0d).
Report is 3 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1371      +/-   ##
============================================
- Coverage     70.78%   70.59%   -0.20%     
============================================
  Files           117      117              
  Lines         63315    63324       +9     
============================================
- Hits          44819    44705     -114     
- Misses        18496    18619     +123

see 14 files with indirect coverage changes

This includes a way to run two versions of the server from the TCL test framework. The runtest script accepts a new parameter --old-server-path path/to/valkey-server and a new tag "needs:old-server" for test cases and start_server. Includes an attempt to run it in CI as well. Signed-off-by: Viktor Söderqvist <[email protected]>

xbasel · 2024-12-03T18:18:21Z

tests/test_helper.tcl

@@ -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>"


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.

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.

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

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.

hpatro · 2024-12-03T18:22:35Z

.github/workflows/ci.yml

+          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


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.

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?

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.

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.

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

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.

tests/support/server.tcl

madolson

Simple and efficient.

madolson · 2024-12-10T00:30:41Z

.github/workflows/ci.yml

@@ -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


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

madolson · 2024-12-10T00:34:07Z

tests/test_helper.tcl

@@ -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>"


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

madolson · 2024-12-10T00:36:13Z

.github/workflows/ci.yml

+          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


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

zuiderkwast force-pushed the upgrade-test branch from 8403010 to 77c4c0d Compare November 28, 2024 11:57

zuiderkwast requested review from madolson and hpatro November 28, 2024 16:02

xbasel reviewed Dec 3, 2024

View reviewed changes

hpatro reviewed Dec 3, 2024

View reviewed changes

madolson reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cross-version replication test #1371

Add cross-version replication test #1371

zuiderkwast commented Nov 28, 2024 •

edited

Loading

codecov bot commented Nov 28, 2024 •

edited

Loading

xbasel Dec 3, 2024

zuiderkwast Dec 3, 2024

madolson Dec 10, 2024

zuiderkwast Dec 10, 2024 •

edited

Loading

hpatro Dec 3, 2024

zuiderkwast Dec 3, 2024

hpatro Dec 3, 2024

zuiderkwast Dec 3, 2024

madolson Dec 10, 2024

zuiderkwast Dec 10, 2024

madolson left a comment

madolson Dec 10, 2024

madolson Dec 10, 2024

madolson Dec 10, 2024

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

Add cross-version replication test #1371

Are you sure you want to change the base?

Add cross-version replication test #1371

Conversation

zuiderkwast commented Nov 28, 2024 • edited Loading

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast commented Nov 28, 2024 •

edited

Loading

codecov bot commented Nov 28, 2024 •

edited

Loading

zuiderkwast Dec 10, 2024 •

edited

Loading