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.

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

Update sidekiq to version 7+ #25988

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

mjankowski
Copy link
Contributor

As of now I just have the commits necessary to resolve the dep update, and get rid of errors/warnings after the update. Have done basically zero QA here.

Opening as draft to work through what's needed. Will leave comments as I research.

@renchap
Copy link
Sponsor Member

renchap commented Jul 14, 2023

I started researching this yesterday and the big blocker is redis-namespace support being dropped.

We need a clear migration path to avoid phantom jobs and will be hard to properly implement and/or document.

Some details here: https://www.mikeperham.com/2017/04/10/migrating-from-redis-namespace/

@renchap
Copy link
Sponsor Member

renchap commented Jul 15, 2023

Another potential issue here is the removal of hiredis for Sidekiq, we will need to test this in a high-load environment to test how it performs, but if this means upgrading to Sidekiq 7 and we are seing issues, then it will hard to rollback?

@renchap renchap added dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code labels Jul 15, 2023
@mjankowski
Copy link
Contributor Author

I started researching this yesterday and the big blocker is redis-namespace support being dropped.

We need a clear migration path to avoid phantom jobs and will be hard to properly implement and/or document.

Some details here: https://www.mikeperham.com/2017/04/10/migrating-from-redis-namespace/

Agreed - I think this will actually be very straightforward code-wise (very few changes), slightly more complicated dependency-wise (more research needed to ensure we have upgraded/dropped appropriately) ... but the most complexity/planning will be in the upgrade/migration/documentation area.

@mjankowski
Copy link
Contributor Author

Another potential issue here is the removal of hiredis for Sidekiq, we will need to test this in a high-load environment to test how it performs, but if this means upgrading to Sidekiq 7 and we are seing issues, then it will hard to rollback?

Sidekiq switched from the redis gem to the redis-client gem. I think there are a few questions here:

  • I found a hiredis-client (used in the PR here) which I think does for redis-client what the hiredis gem (Removed in this PR) was doing for the redis gem in terms of using hiredis lib. Need to confirm.
  • Need to research to what extent our other non-sidekiq usage of redis relies specifically on the redis gem -vs- what could also be updated to use redis-client so we could drop redis entirely. Also might be fine to keep both.

@renchap
Copy link
Sponsor Member

renchap commented Jul 15, 2023

The redis question is a bit more complex. Currently we do not support complex Redis setups, for example with TLS (mandatory for some hosted Redis offerings) or distributed Redis setups (Redis Sentinel or Redis Cluster), as they are not supported by hiredis.

There is more context here:
#19824
#23516

If we are starting to touch Redis's handling, we most probably want to ensure we move to libraries supporting those features, as they are needed for many setups (and we will probably need clustering quite soon for mastodon.social)

@mjankowski
Copy link
Contributor Author

Ah, thanks for that background, that all makes sense re: future needs.

So I think the requirement there is that at minimum we should make sure that a sidekiq upgrade does not reduce our ability to get onto distributed/clustered redis setup in near future; and if possible, adopts libraries that makes that easier.

I'll keep that in mind while thinking about the migration + deps here.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@renchap
Copy link
Sponsor Member

renchap commented Oct 16, 2023

The main task here seems to be the migration path for servers with a namespaced Redis configuration.

This seems doable, but will require an extensive documentation about the steps.

What could work is:

  • if you use a namespace for Redis, then you need to switch to using a different Redis DB for each instance
  • we do not care about cache, it will be reset on upgrade
  • what do we do for "app" data? Should it be migrated? Could it be dropped without too much impact?
  • for sidekiq:
    • prepare a new configuration using a Redis DB instead of the namespace
    • stop Sidekiq scheduled jobs
    • run pre-upgrade migrations
    • start Sidekiq & web servers with the new code (and new config)
    • new jobs are now enqueued onto the new Redis, only older jobs are remaining on the old Sidekiq redis
    • let your old Sidekiq processes run until the queues are empty. We should probably provide a command that will output the status of those queue. And what about retries, can we simply ignore those and loose those jobs? Should we have a command to transfer them to the new redis?
    • stop your old sidekiq processes
    • run the post migrations

@renchap
Copy link
Sponsor Member

renchap commented Oct 27, 2023

An issue with using Redis databases: redis pub/sub (used for streaming) does not support databases: https://redis.io/docs/interact/pubsub/#database--scoping

The documentation advises to add a scope in the front of the keys, similar to the namespacing we support at the moment

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski mjankowski force-pushed the update-sidekiq branch 2 times, most recently from 8fd2d1e to 0f631b4 Compare July 3, 2024 14:48
@mjankowski
Copy link
Contributor Author

Rebased, updated sidekiq to 7.3.0

@renchap
Copy link
Sponsor Member

renchap commented Jul 3, 2024

As discussed above, the big issue here is migration for people using namespaces. We definitely want to upgrade to Sidekiq 7, but we need to have a migration path for those servers.

The approach I discussed above does not really work because you would loose your retries.
I think we should go with a migration script (a bit similar to "the big migration" from this article, copying the keys from the original redis server + namespace to a new server (or redis database), stripping the namespace prefix.

This would require downtime but I dont see how we can avoid this.

As this is independent from this PR (but a pre-requisite), this should probably be implemented in a new PR, which would then unlock this one.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

This pull request has resolved merge conflicts and is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants