Gitlab release 16.8 breaks compatibility with redis < 6.2

added devopssystems docs-only documentation groupdistribution labels

Thanks for the report @Sp4rKy.

It looks like the Sidekiq gem was upgraded to 7.1.6 in GitLab 16.8 !138882 (merged)

And it does look like Sidekiq requires Redis 6.2+ https://github.com/sidekiq/sidekiq/blob/v7.1.6/README.md

@schin1 Just a heads up. Please feel free to correct the labels if they are incorrect - I wasn't sure if this belongs to groupdistribution or groupscalability.

I've started a docs MR to address this !142873 (merged)

!142881 (merged) also bumps the system check. We need to backport this to 16.8.

I see that Redis 6.0 has reached end-of-life in https://endoflife.date/redis, but I'd like to understand why Sidekiq 7 started requiring Redis 6.2.

I disabled Sidekiq's Redis 6.2 requirement and tried to see what happened if my GDK tried to talk to a Redis 6.0 instance. I kept seeing these errors:

ERR Error running script (call to f_000a4c2f82df69db1780053377a2c9b0741570e0): @user_script:2: ERR value is not an integer or out of range"}

I think that's coming from this Redis EVALSHA command command:

1706239453.059279 [0 192.168.65.1:27091] "EVALSHA" "000a4c2f82df69db1780053377a2c9b0741570e0" "1" "retry" "1706239453.058377"
1706239453.059414 [0 lua] "zrange" "retry" "-inf" "1706239453.058377" "byscore" "limit" "0" "1"

This seems like it's coming from our Sidekiq middleware evaluating Lua scripts, though.

https://github.com/sidekiq/sidekiq/issues/6043 mentions the HELLO command, but RESP3 should be supported in both Redis 6.0 and 6.2 (https://redis.io/commands/hello).

Oh, the byscore parameter for ZRANGE is only available in Redis 6.2 (https://redis.io/commands/zrange/), and that's used in Sidekiq: https://github.com/sidekiq/sidekiq/blob/312672deab31c23810a272d1b06c33c84afaf9dd/lib/sidekiq/scheduled.rb#L15.

It looks like Redis 6.2 is indeed required.

@schin1 I think we should consider downgrading to Sidekiq 7.0.x in light of !142955 (merged) and other issues. I think Sidekiq 7.1.0 started enforcing Redis 6.2+ with https://github.com/sidekiq/sidekiq/issues/5788 (commit: https://github.com/sidekiq/sidekiq/commit/1aca434e5cd830cbd0020f1f82ef3a3cb6b2d477).

Unfortunately, Sidekiq 7.0.x still enforces Redis 6.2, but if I disable the check Sidekiq seems to work okay.

So our options are:

Keep Sidekiq 7.x and belatedly announce Redis 6.0 is no longer supported (current state).
Roll back to Sidekiq 6.x and announce Redis 6.0 deprecation.
Use Sidekiq 7.0.9, patch the Redis check, and announce Redis 6.0 deprecation.

I'm inclined to go with 1, but there might be an argument in favor of 2 since it's better to announce the deprecation to give users time to prepare.

Roll back to Sidekiq 6.x and announce Redis 6.0 deprecation.

announcing the sidekiq deprecation would provide better user experience but rolling back to Sidekiq 6 would block

redis gem upgrade which blocks Redis Cluster scaling since "redirection support in pipeline during resharding" is only supported in redis-cluster-client (https://github.com/redis-rb/redis-cluster-client/issues/37) -- on that note, I'll make a note to pause on the MR's progress until we make this decision
sidekiq horizontal scaling (gitlab-com/gl-infra/scalability#2541 (comment 1592705997))

It looks like the zrangebyscore->zrange is the only change that requires redis 6.2. Could another option be to patch the https://github.com/sidekiq/sidekiq/blob/main/lib/sidekiq/redis_client_adapter.rb#L57 with a zrange to convert commands with byscore option into a zrangebyscore? Also override LUA_ZPOPBYSCORE to use the older zrangebyscore option. But this is similar to option 3 since the goal is to maintain sidekiq 7.x while giving users time to upgrade.

Option 3 might be a better balance between 1 and 2; we keep Sidekiq 7 while letting us announce the deprecation.

Yeah, if we are going to patch Sidekiq anyway, I was thinking we could just patch the zrange call and keep it at 7.2. So we keep the bug fixes from 7.1 to 7.2.

Not sure if we are affected with any of those though.

@engwan actually, would it be possible to patch the sidekiq cli? bin/sidekiq-cluster runs bundle exec sidekiq internally. The version check happens before launch in the run method (https://github.com/sidekiq/sidekiq/blob/312672deab31c23810a272d1b06c33c84afaf9dd/lib/sidekiq/cli.rb#L41). i.e. our application code gets loaded inside Sidekiq::CLI.run.

Would we have to vendor it to remove the Redis check?

I went ahead and prepared !142986 (merged) as a draft. Though vendoring the gem is not as ideal since that would prevent us from upgrading sidekiq easily for as long as the deprecation notice lasts.

Given we have a number of customers using Azure Cache for Redis, which only supports Redis 6 right now, I think vendoring may be the best short-term solution for now.

Given we have a number of customers using Azure Cache for Redis, which only supports Redis 6 right now, I think vendoring may be the best short-term solution for now.

To summarise the state of this issue: We have updated the documentation and the system check for Redis. Tentative follow-up tasks in bold below:

Next, we can (1) add Redis 6.0 support using !142986 (merged) and also pick it into 16.8 to support users in cloud ecosystems without managed Redis >= 6.2.

Once the Redis 6 support is available, we can (2) re-update the documentation and system checks. As highlighted in !142881 (comment 1747063752), the system check should warn about Redis 6.0 usage since it has reached EOL.

We could also (3) use Redis 6.0 docker containers in our CI instead of 6.2 so that we can catch such issues earlier.

mentioned in merge request !142873 (merged)

Hey @Sp4rKy, thank you for creating this issue!

To get the right eyes on your issue more quickly, we encourage you to follow the issue triage best practices.

PLEASE NOTE: You will need to use the @gitlab-bot label command to apply labels to this issue.

To set expectations, GitLab product managers or team members can't make any promise if they will proceed with this. However, we believe everyone can contribute, and welcome you to work on this proposed change, feature or bug fix. There is a bias for action, so you don't need to wait. Try and spin up that merge request yourself.

If you need help doing so, we're always open to mentor you to drive this change.

This message was generated automatically. You're welcome to improve it.