THREESCALE-10180: Bump sidekiq from 6.4.2 to 7.1.5 #3576
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
dependabot
bot
added
dependencies
jlledom
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3576 +/- ##
==========================================
+ Coverage 94.34% 94.36% +0.02%
==========================================
Files 2799 2798 -1
Lines 92720 92935 +215
==========================================
+ Hits 87476 87700 +224
+ Misses 5244 5235 -9
☔ View full report in Codecov by Sentry. |
| @@ -123,7 +123,7 @@ memcached-container: &memcached-container | |||
| image: memcached:1.5-alpine | |||
|
|
|||
| redis-container: &redis-container | |||
| image: redis:4.0-alpine | |||
| image: redis:6.2-alpine | |||
There was a problem hiding this comment.
Redis 4 is not supported by Sidekiq 7. CircleCI needs to upgrade it's image to 6.2. That's fine, since 6.2 is the only supported version also for porta: https://access.redhat.com/articles/2798521
| gem 'sidekiq-cron', require: %w[sidekiq/cron sidekiq/cron/web] | ||
| gem 'sidekiq-lock' | ||
| gem 'sidekiq-throttled' | ||
| gem 'sidekiq-throttled', '~> 1.0.0.alpha.1' |
There was a problem hiding this comment.
Not especially happy using an alpha but it's the only available version supporting Sidekiq 7
There was a problem hiding this comment.
In rubygems most releases are alpha anyway...
| @@ -139,7 +139,7 @@ def on_complete(_bid, options) | |||
| # The number of retries is usually controlled at the origin of the failure using ThreeScale::SidekiqRetrySupport::Worker#last_attempt?, | |||
| # but in this case we are not really using Sidekiq retries, but rather re-enqueueing the job manually here | |||
| manual_retry_count = options['manual_retry_count'].to_i + 1 | |||
| perform_async(event.event_id, event.data, manual_retry_count) | |||
| perform_async(event.event_id, event.data.to_json, manual_retry_count) | |||
There was a problem hiding this comment.
Fixed a parameter not being sent as string to Redis. I wonder why this haven't reported errors to Bugsnag in production, as it's deprecated since Redis 6.4
test/unit/redis_hacks_test.rb
Outdated
There was a problem hiding this comment.
No needed since we removed the patch, however, we keep testing the same functionality in test/unit/redis_connection_error_test.rb
config/initializers/redis_hacks.rb
Outdated
There was a problem hiding this comment.
We don't need the patch because we don't use the old hiredis gem anymore.
config/examples/backend_redis.yml
Outdated
| size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %> | ||
| pool_timeout: 5 # this is in seconds | ||
| sentinels: "<%= ENV['BACKEND_REDIS_SENTINEL_HOSTS'] %>" | ||
| name: "<%= ENV['BACKEND_REDIS_SENTINEL_NAME'] %>" |
There was a problem hiding this comment.
I updated the examples to encourage using :size over :pool_size, and to make customers know about the new :name param, which is mandatory when using redis sentinels.
There was a problem hiding this comment.
Maybe we should set :name to some default value (e.g. default) if it's missing?
Do you know if having "" for name would cause any problems?
I'm looking here https://github.com/redis-rb/redis-client/blob/1ab081c1d0e47df5d55e011c9390c70b2eef6731/lib/redis_client/sentinel_config.rb#L32-L38
and it seems that the error is thrown only if the value is nil, but I'm not sure if it can "break" somewhere in another place if the name is empty...
There was a problem hiding this comment.
We can't set a default because the name is must be a user defined redis sentinel group. The :name param is only mandatory when using redis sentinels, it can be nil for standalone and for master-replicas redis installations.
There was a problem hiding this comment.
For clients using redis sentinels, the :name param is mandatory now and it will break their configurations if they don't add it. So we must add a release note to mention this.
There was a problem hiding this comment.
Ah OK, I thought it was only a client thing...
In that case - wouldn't it be better to keep the name nil if BACKEND_REDIS_SENTINEL_NAME is missing?
From the link I pasted above, it seems that in this case name will be derived from the URL host, and otherwise fail.
But if it is set to empty - will it work as expected?... 🤔
This is just theoretical thinking, I haven't tried it myself.
There was a problem hiding this comment.
I think it makes more sense to set it to nil when the env variable isn't set. Although, from my tests in local, I'd say there's no difference in the behavior. Either the name matches exactly the sentinel group name, or it fails, no matter if empty, nil or a default value set by redis-client.
app/lib/three_scale/redis_config.rb
Outdated
| @@ -6,6 +6,7 @@ def initialize(redis_config = {}) | |||
| raw_config = (redis_config || {}).symbolize_keys | |||
| sentinels = raw_config.delete(:sentinels).presence | |||
| raw_config.delete_if { |key, value| value.blank? } | |||
| set_pool_size(raw_config) | |||
There was a problem hiding this comment.
| set_pool_size(raw_config) | |
| raw_config[:size] ||= raw_config[:pool_size] if raw_config.key?(:pool_size) |
But it's fine either way.
There was a problem hiding this comment.
This wouldn't work because it doesn't remove the :pool_size param.
There was a problem hiding this comment.
| set_pool_size(raw_config) | |
| raw_config[:size] ||= raw_config.delete(:pool_size) if raw_config.key?(:pool_size) |
|
|
||
| exception = assert_raises(RedisClient::CannotConnectError) { redis._client.send(:connect) } | ||
| assert_equal 'No sentinels available', exception.message | ||
| end |
There was a problem hiding this comment.
It's the same as test/unit/redis_hacks_test.rb. Just I renamed it to still test the behavior, even when we removed the monkey patch.
| @@ -1,9 +1,10 @@ | |||
| base: &default | |||
| url: "<%= ENV.fetch('BACKEND_REDIS_URL', 'redis://localhost:6379/6') %>" | |||
| timeout: <%= ENV.fetch('BACKEND_REDIS_TIMEOUT', 1) %> | |||
| pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %> | |||
| size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %> | |||
There was a problem hiding this comment.
Just a reminder, did you create a PR for SaaS with this?
There was a problem hiding this comment.
No, but I think we don't need it just now, because it's backwards compatible: https://github.com/3scale/porta/pull/3576/files#diff-ad73aa14ad3c1b2d53ac0808de8bbf07f16747d251bf28258622caa5bc7b21c5R9
So, we can update it when it is already deployed.
| @@ -107,7 +107,7 @@ gem 'ratelimit' | |||
| gem 'recaptcha', '4.13.1', require: 'recaptcha/rails' | |||
| gem 'redcarpet', '~>3.5.1', require: false | |||
| gem 'RedCloth', '~>4.3', require: false | |||
| gem 'redis', '~> 4.2.0', require: ['redis', 'redis/connection/hiredis'] | |||
| gem 'redis', require: ['redis'] | |||
There was a problem hiding this comment.
is require: ['redis'] still needed?... just curious
There was a problem hiding this comment.
Sorry, I actually misread it 😬 I thought this was for the new redis-client gem...
So, we have two gems: redis for non-sidekiq stuff and redis-client for sidekiq stuff, right?
But for redis we dropped hiredis, why is that? It is not compatible?
There was a problem hiding this comment.
I'm wondering, what if we use redis-client for non-sidekiq also? 🤔
I guess the configs are compatible, so this will be more or less changing this line only? https://github.com/3scale/porta/blob/dependabot/bundler/sidekiq-7.1.5/app/lib/system/redis_pool.rb#L13
(and a couple of more)
There was a problem hiding this comment.
All the Redis gems ecosystem is highly confusing, since all gems have basically the same name 😵💫. I'll try to summarize it:
- First we have the
redisgem: https://github.com/redis/redis-rb- It provides a Ruby API for the Redis protocol, that is, ruby methods like
:sadd,:hset, etc. - Also provides thread safe connections with connections pools.
- It provides a Ruby API for the Redis protocol, that is, ruby methods like
redisneeds a client, since 5.0 they useredis-client: https://github.com/redis-rb/redis-client- It provides the connection to server but it's not thread safe and doesn't implement the redis protocol, only provides a
callmethod to be used like this:client.call("SADD", "key", "value").
- It provides the connection to server but it's not thread safe and doesn't implement the redis protocol, only provides a
redis-clientrequires a redis driver. It can work with a builtin Ruby driver, which is slow, or withhiredis, which is faster.hiredis-clientis the gem that provideshiredisforredis-client. https://github.com/redis-rb/redis-client/tree/master/hiredis-clienthiredis-clientis just a ruby wrap for the Chiredislibrary: https://github.com/redis/hiredis- On the other hand, there's a ruby gem called
hiredis: https://github.com/redis/hiredis-rb- It's also a wrapper for the C library. However, this one is outdated and doesn't support TLS connections.
So before updating redis-rb we had:
redis(4.X) -> builtin client ->hiredis(ruby) ->hiredis(C)
And now:
redis(5.X) ->redis-client->hiredis-client->hiredis(C)
So, we have two gems: redis for non-sidekiq stuff and redis-client for sidekiq stuff, right?
Kind of, porta uses redis API (e.g. /app/models/billing_summary.rb), but redis depends on redis-client, so they aren't two different gems to do the same. Sidekiq calls redis-client directly (See here)
But for redis we dropped hiredis, why is that? It is not compatible?
Because it doesn't support TLS (redis/hiredis-rb#58)
I'm wondering, what if we use redis-client for non-sidekiq also?
Porta uses redis API, so we would need to invent some solution like Sidekiq does. Apisonator also has something similar (https://github.com/3scale/apisonator/blob/v3.4.3/lib/3scale/backend/storage_async/client.rb)
There was a problem hiding this comment.
OMG, this is extremely confusing indeed 😵💫
Thanks a lot for this comment, it clarifies a lot!
There was a problem hiding this comment.
Actually... I think there might be one thing missing here...
While I was investigating switching redis for redis-client globally, I stumbled on the following:
We are using activejob-uniqueness, which is our own fork, based on version 0.2.0, and I think the only purpose of it was to support the old Ruby 2.4 (which we don't need to support anymore).
That version was not compatible with Sidekiq 7 (not sure about Sidekiq 6 though): https://github.com/veeqo/activejob-uniqueness/blob/main/CHANGELOG.md#added-1
I guess we need to upgrade, because some of our jobs use this.
Good catch. Done. |
| @@ -20,7 +20,7 @@ gem 'rails', '~> 6.0' | |||
| gem 'mail', '~> 2.7.1' | |||
|
|
|||
| # Needed for XML serialization of ActiveRecord::Base | |||
| gem "activejob-uniqueness", github: "3scale/activejob-uniqueness", branch: "main" | |||
| gem "activejob-uniqueness" | |||
There was a problem hiding this comment.
Can you please also swap it with the next line?
The # Needed for XML serialization of ActiveRecord::Base comment actually belongs to 'activemodel-serializers-xml', so having it here is confusing 😬
jlledom
force-pushed
the
dependabot/bundler/sidekiq-7.1.5
branch
from
c2479c8 to
de032a9
Compare
* Revert "Hotfix for hiredis-client on ppc64le (#3606)" This reverts commit 8865ce7. * Revert "Fix regression: boot:redis task (#3604)" This reverts commit eb88cc2. * Revert "Merge pull request #3576 from 3scale/dependabot/bundler/sidekiq-7.1.5" This reverts commit 90ff4b8, reversing changes made to 9068ff9. * Remove activejob-uniqueness fork * Keep the correct job parameter format
See https://github.com/sidekiq/sidekiq/blob/main/docs/7.0-API-Migration.md and https://github.com/sidekiq/sidekiq/blob/main/docs/7.0-Upgrade.md
Bumps sidekiq from 6.4.2 to 7.1.5.
Changelog
Sourced from sidekiq's changelog.
... (truncated)
Commits
f597a0aprep4862d90Optimize bundler startup, thanks Bumbler!174eab9Crash if the user tries to specify RESP2, fixes #6061916a564Change rails logger broadcasting configuration (#6060)d7795cefix flaky filtering test70323b1Update author and project social media links. (#6059)8874e68Open source Web UI filtering (#6052)1757a96Previous commit fixes #6054a1a220fModify inline testing's client override so as to not conflict with batch's cl...aadc77aminor bumpDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)You can disable automated security fix PRs for this repo from the Security Alerts page.