Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.57%. Comparing base (3b12132) to head (54a7d3f).
⚠️ Report is 17 commits behind head on unstable.

@@             Coverage Diff              @@
##           unstable    #2422      +/-   ##
============================================
+ Coverage     71.51%   71.57%   +0.06%     
============================================
  Files           123      123              
  Lines         67454    67493      +39     
============================================
+ Hits          48239    48308      +69     
+ Misses        19215    19185      -30     

@valkey-io/core-team just to highlight the implications of this: this basically breaks the 7.2 and 8.0 compatibility.
IIRC we only added the strict version check in 8.1 which will now become the only downgradable version.
Should we include some wording about it in the documentation? I expect it is going to be somewhat confusing.
Also, I expect some of the Cross version cluster tests will now fail. I think it is expected, but would be happy to hear if you feel otherwise.

I think that the only issue is with the failover test.
This test will attempt to first sync-up an old version which is now unsupported. It will probably not be an issue for the daily compat tests as they are being run with compatible-redis tag.

I think that for the CI tests we can run only against 8.1 and enable the relaxed rdb check in this specific test.

@zuiderkwast WDYT?

IIRC we only added the strict version check in 8.1 which will now become the only downgradable version.

Ran, yes, because of this, we consider 8.1 a stepping stone. All our customers must upgrade to (a version of our products that contain) 8.1 before they can upgrade further to 9 and later.

I think that for the CI tests we can run only against 8.1 and enable the relaxed rdb check in this specific test.

Yes.

We have discussed a way to let Valkey produce older RDB versions for even better downgrade possibilities. It would involve a config to set the RDB version and it would be up to the user to only use features supported by the chosen RDB version. I still think we should do it. Technically, it's not very difficult... #1108.

IIRC we only added the strict version check in 8.1 which will now become the only downgradable version.

Ran, yes, because of this, we consider 8.1 a stepping stone. All our customers must upgrade to (a version of our products that contain) 8.1 before they can upgrade further to 9 and later.

I think that for the CI tests we can run only against 8.1 and enable the relaxed rdb check in this specific test.

Yes.

We have discussed a way to let Valkey produce older RDB versions for even better downgrade possibilities. It would involve a config to set the RDB version and it would be up to the user to only use features supported by the chosen RDB version. I still think we should do it. Technically, it's not very difficult... #1108.

Thank you @zuiderkwast . I am mainly referring to the plan to change the CI as part of THIS pr so that we will start running the cross version compat against 8.1 + configure relaxed RDB check in the failover test

hhh There is a failure in test Send cluster message via module from other server to latest, but it is because this test should have fail IMO. It is based on the fact that CONFIG resetstat will reset the cluster msg stats (which is not true). I actually think that there might be a bug in the use of light headers in this cross version case? will check

@hpatro FYI

hhh There is a failure in test Send cluster message via module from other server to latest, but it is because this test should have fail IMO. It is based on the fact that CONFIG resetstat will reset the cluster msg stats (which is not true). I actually think that there might be a bug in the use of light headers in this cross version case? will check

@hpatro FYI

O.K after some fighting, I found the issue is known (#1708) and that is the node-id is passed NULL terminated only from 8.1 to clusterMassageRecievers. because of that the 7.2 module is unable to send the DONG message back to the originator and this is why the test fails for 7.2. So there are some AIs:

1. fix the failover test as it is now not testing correctly the message transfer.
2. @zuiderkwast / @hpatro seems like you guys have context on the issue. did we decide to backport this? if not what do we expect?
   I guess after enabling this test to work with 8.1 there will be no failure though