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

THREESCALE-10180: Bump sidekiq from 6.4.2 to 7.1.5 #3576

Merged
merged 3 commits into from
Oct 24, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# https://github.com/poseidon/dnsmasq
# TODO: create our own image like https://www.redhat.com/en/blog/build-ubi-containers-github-actions-buildah-and-podman
14 changes: 7 additions & 7 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -19,8 +19,8 @@ gem 'rails', '~> 6.0'
# Also, upgrading makes this test fail: SendUserInvitationWorkerTest#test_handles_errors
gem 'mail', '~> 2.7.1'

gem "activejob-uniqueness"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

# Needed for XML serialization of ActiveRecord::Base
gem "activejob-uniqueness", github: "3scale/activejob-uniqueness", branch: "main"
gem 'activemodel-serializers-xml'

gem 'protected_attributes_continued', '~> 1.8.2'
@@ -51,11 +51,11 @@ gem '3scale_time_range', '0.0.6'
gem 'statsd-ruby', require: false

# Sidekiq
gem 'sidekiq', '~> 6.4.0', require: %w[sidekiq sidekiq/web]
gem 'sidekiq-batch'
gem 'sidekiq', '~> 7', require: %w[sidekiq sidekiq/web]
gem 'sidekiq-batch', github: '3scale/sidekiq-batch', branch: 'redis-client'
akostadinov marked this conversation as resolved.
Show resolved Hide resolved
gem 'sidekiq-cron', require: %w[sidekiq/cron sidekiq/cron/web]
gem 'sidekiq-lock'
gem 'sidekiq-throttled'
gem 'sidekiq-throttled', '~> 1.0.0.alpha.1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not especially happy using an alpha but it's the only available version supporting Sidekiq 7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rubygems most releases are alpha anyway...


gem 'sidekiq-prometheus-exporter'

@@ -75,7 +75,7 @@ gem 'formtastic', '~> 4.0'
gem 'htmlentities', '~>4.3', '>= 4.3.4'
# TODO: Not actively maintained https://github.com/activeadmin/inherited_resources#notice replace with respond_with and fix things the rails way
gem 'inherited_resources', '~> 1.12.0'
gem 'json', '~> 2.3.0'
gem 'json', '~> 2.3'

gem 'mysql2', '~> 0.5.3'

@@ -94,7 +94,7 @@ gem 'acts-as-taggable-on', '~> 8.0'
gem 'baby_squeel', '~> 1.4.3'
gem 'browser'
gem 'diff-lcs', '~> 1.2'
gem 'hiredis', '~> 0.6.3'
gem 'hiredis-client'
gem 'httpclient', github: '3scale/httpclient', branch: 'ssl-env-cert'
gem 'json-schema', git: 'https://github.com/3scale/json-schema.git'
gem 'local-fastimage_resize', '~> 3.4.0', require: 'fastimage/resize'
@@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is require: ['redis'] still needed?... just curious

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the Redis gems ecosystem is highly confusing, since all gems have basically the same name 😵‍💫. I'll try to summarize it:

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, this is extremely confusing indeed 😵‍💫
Thanks a lot for this comment, it clarifies a lot!

gem 'rest-client', '~> 2.0.2'
gem 'rubyzip', '~>1.3.0', require: false
gem 'svg-graph', require: false
75 changes: 40 additions & 35 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
GIT
remote: https://github.com/3scale/activejob-uniqueness.git
revision: fc560d1ab5552dc3a1602d4aaa7caaa8eba5791f
branch: main
specs:
activejob-uniqueness (0.2.0)
activejob (>= 4.2, < 7)
redlock (>= 1.2, < 2)

GIT
remote: https://github.com/3scale/ci_reporter_shell.git
revision: 30b30d655512891f56463e5f1fa125ea1f2df886
@@ -37,6 +28,14 @@ GIT
nokogiri (~> 1)
rspec (>= 3.0.0.a, < 4)

GIT
remote: https://github.com/3scale/sidekiq-batch.git
revision: f1c56d0b8445fd3eb337614aedba1752aa19938e
branch: redis-client
specs:
sidekiq-batch (0.1.9)
sidekiq (~> 7)

GIT
remote: https://github.com/3scale/swagger-ui_rails.git
revision: f88bb8bed4fdb57fcf5be6425f3fc10c638788d1
@@ -120,6 +119,9 @@ GEM
activejob (6.0.6.1)
activesupport (= 6.0.6.1)
globalid (>= 0.3.6)
activejob-uniqueness (0.2.5)
activejob (>= 4.2, < 7.1)
redlock (>= 1.2, < 2)
activemerchant (1.107.4)
activesupport (>= 4.2)
builder (>= 2.1.2, < 4.0.0)
@@ -383,7 +385,8 @@ GEM
hashdiff (1.0.1)
hashery (2.1.2)
hashie (3.6.0)
hiredis (0.6.3)
hiredis-client (0.17.0)
redis-client (= 0.17.0)
html-pipeline (2.12.3)
activesupport (>= 2)
nokogiri (>= 1.4)
@@ -414,7 +417,7 @@ GEM
rails-dom-testing (>= 1, < 3)
railties (>= 4.2.0)
thor (>= 0.14, < 2.0)
json (2.3.1)
json (2.6.3)
jsonpath (1.1.2)
multi_json
jwt (1.5.6)
@@ -646,10 +649,13 @@ GEM
actionview (>= 5)
recursive-open-struct (1.1.3)
redcarpet (3.5.1)
redis (4.2.5)
redis-namespace (1.7.0)
redis (>= 3.0.4)
redis-prescription (1.0.0)
redis (5.0.7)
redis-client (>= 0.9.0)
redis-client (0.17.0)
connection_pool
redis-namespace (1.11.0)
redis (>= 4)
redis-prescription (2.6.0)
redlock (1.3.2)
redis (>= 3.0.0, < 6.0)
reek (6.1.0)
@@ -756,25 +762,24 @@ GEM
shoulda-context (2.0.0)
shoulda-matchers (4.5.1)
activesupport (>= 4.2.0)
sidekiq (6.4.2)
connection_pool (>= 2.2.2)
rack (~> 2.0)
redis (>= 4.2.0)
sidekiq-batch (0.1.9)
sidekiq (>= 3)
sidekiq-cron (1.9.1)
sidekiq (7.1.6)
concurrent-ruby (< 2)
connection_pool (>= 2.3.0)
rack (>= 2.2.4)
redis-client (>= 0.14.0)
sidekiq-cron (1.10.1)
fugit (~> 1.8)
sidekiq (>= 4.2.1)
globalid (>= 1.0.1)
sidekiq (>= 6)
sidekiq-lock (0.6.0)
redis (>= 3.0.5)
sidekiq (>= 2.14.0)
sidekiq-prometheus-exporter (0.2.0)
rack (>= 1.6.0)
sidekiq (>= 4.1.0)
sidekiq-throttled (0.15.0)
concurrent-ruby
redis-prescription
sidekiq
sidekiq-throttled (1.0.0.alpha.1)
redis-prescription (~> 2.2)
sidekiq (>= 6.5)
simplecov (0.21.2)
docile (~> 1.1)
simplecov-html (~> 0.11)
@@ -906,7 +911,7 @@ GEM
yabeda (~> 0.6)
yard (0.9.27)
webrick (~> 1.7.0)
zeitwerk (2.6.8)
zeitwerk (2.6.12)
zip-zip (0.3)
rubyzip (>= 1.0.0)

@@ -918,7 +923,7 @@ DEPENDENCIES
3scale_time_range (= 0.0.6)
RedCloth (~> 4.3)
active-docs!
activejob-uniqueness!
activejob-uniqueness
activemerchant (~> 1.107.4)
activemodel-serializers-xml
activerecord-oracle_enhanced-adapter (~> 6.0)
@@ -968,14 +973,14 @@ DEPENDENCIES
formtastic (~> 4.0)
github-markdown
hashie
hiredis (~> 0.6.3)
hiredis-client
html-pipeline
htmlentities (~> 4.3, >= 4.3.4)
httpclient!
i18n
inherited_resources (~> 1.12.0)
jquery-rails (~> 4.4)
json (~> 2.3.0)
json (~> 2.3)
json-schema!
jwt (~> 1.5.2)
kt-paperclip (~> 7.2)
@@ -1025,7 +1030,7 @@ DEPENDENCIES
recaptcha (= 4.13.1)
record_tag_helper (~> 1.0)
redcarpet (~> 3.5.1)
redis (~> 4.2.0)
redis
redlock
reek (= 6.01)
reform (~> 2.0.3)
@@ -1046,12 +1051,12 @@ DEPENDENCIES
secure_headers (~> 6.3.0)
selenium-webdriver (~> 3.142)
shoulda (~> 4.0)
sidekiq (~> 6.4.0)
sidekiq-batch
sidekiq (~> 7)
sidekiq-batch!
sidekiq-cron
sidekiq-lock
sidekiq-prometheus-exporter
sidekiq-throttled
sidekiq-throttled (~> 1.0.0.alpha.1)
simplecov (~> 0.21.2)
slim-rails (~> 3.2)
sprockets-rails
3 changes: 1 addition & 2 deletions app/lib/backend/storage.rb
Original file line number Diff line number Diff line change
@@ -7,8 +7,7 @@ def self.parse_config
config = File.read("#{Rails.root}/config/backend_redis.yml")
config = ERB.new(config).result(binding)
config = YAML.load(config)
config = config.fetch(Rails.env).symbolize_keys
config.reverse_merge(logger: Rails.logger)
akostadinov marked this conversation as resolved.
Show resolved Hide resolved
config.fetch(Rails.env).deep_symbolize_keys
end

def initialize
4 changes: 4 additions & 0 deletions app/lib/system/error_reporting.rb
Original file line number Diff line number Diff line change
@@ -23,6 +23,10 @@ def report_error(exception, logger: Rails.logger, **parameters)
end
end

def report_sidekiq_error(exception, _context, config)
report_error(exception, logger: config.logger)
end
jlledom marked this conversation as resolved.
Show resolved Hide resolved

def report_deprecation_warning(payload)
exception = DeprecationWarning.new(payload[:message], payload[:gem_name], payload[:deprecation_horizon])

10 changes: 5 additions & 5 deletions app/lib/system/redis_pool.rb
Original file line number Diff line number Diff line change
@@ -6,11 +6,11 @@ module System
# RedisPool a simple wrapper around Redis with connection pooling
class RedisPool

def initialize(config={})
config = config.dup
pool_config = config.extract!(:pool_size, :pool_timeout)
@pool = ConnectionPool.new(size: pool_config[:pool_size] || 5, timeout: pool_config[:pool_timeout] || 5 ) do
Redis.new(config)
def initialize(config = {})
cfg = config.to_h
pool_config = cfg.extract!(:size, :pool_timeout)
@pool = ConnectionPool.new(size: pool_config[:size] || 5, timeout: pool_config[:pool_timeout] || 5 ) do
Redis.new(cfg)
end
end

1 change: 1 addition & 0 deletions app/lib/three_scale/redis_config.rb
Original file line number Diff line number Diff line change
@@ -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? }
raw_config[:size] ||= raw_config.delete(:pool_size) if raw_config.key?(:pool_size)

@config = ActiveSupport::OrderedOptions.new.merge(raw_config)
config.sentinels = parse_sentinels(sentinels) if sentinels
2 changes: 1 addition & 1 deletion app/workers/zync_worker.rb
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

delegate :perform_async, to: :class
3 changes: 2 additions & 1 deletion config/examples/backend_redis.yml
Original file line number Diff line number Diff line change
@@ -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) %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder, did you create a PR for SaaS with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pool_timeout: 5 # this is in seconds
sentinels: "<%= ENV['BACKEND_REDIS_SENTINEL_HOSTS'] %>"
name: <%= ENV['BACKEND_REDIS_SENTINEL_NAME'] %>
role: <%= ENV['BACKEND_REDIS_SENTINEL_ROLE'] %>

development:
3 changes: 2 additions & 1 deletion config/examples/redis.yml
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
base: &default
url: "<%= ENV.fetch('REDIS_URL', 'redis://localhost:6379/1') %>"
pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
pool_timeout: 5 # this is in seconds
sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
name: <%= ENV['REDIS_SENTINEL_NAME'] %>
role: <%= ENV['REDIS_SENTINEL_ROLE'] %>

development:
21 changes: 0 additions & 21 deletions config/initializers/redis_hacks.rb

This file was deleted.

4 changes: 2 additions & 2 deletions config/initializers/sidekiq.rb
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@
config.try(:reliable!)

config.redis = ThreeScale::RedisConfig.new(System::Application.config.sidekiq).config
config.error_handlers << System::ErrorReporting.method(:report_error)
config.error_handlers.replace([System::ErrorReporting.method(:report_sidekiq_error)])

config.logger.formatter = Sidekiq::Logger::Formatters::Pretty.new

@@ -37,7 +37,7 @@
# Use PROMETHEUS_EXPORTER_BIND and PROMETHEUS_EXPORTER_PORT
# if no PROMETHEUS_EXPORT_PORT given, it will start the server with default port 9394 + index
port = ENV.fetch('PROMETHEUS_EXPORTER_PORT', 9394).to_i
port += Sidekiq.options[:index].to_i
port += Sidekiq.default_configuration[:index].to_i
ENV['PROMETHEUS_EXPORTER_PORT'] ||= port.to_s

require 'yabeda/prometheus/mmap'
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
@@ -89,7 +89,7 @@ services:
- RACK_ENV=production

redis:
image: redis:alpine
image: redis:6.2-alpine
jlledom marked this conversation as resolved.
Show resolved Hide resolved
container_name: redis-compose
ports:
- "6379:6379"
5 changes: 4 additions & 1 deletion features/support/sidekiq.rb
Original file line number Diff line number Diff line change
@@ -4,7 +4,10 @@
require 'sidekiq/batch'

# Turn off Sidekiq logging which pollutes the CI logs
Sidekiq.logger = Sidekiq::Logger.new(nil, level: Logger::FATAL)
Sidekiq.configure_client do |config|
config.logger = Sidekiq::Logger.new(nil, level: Logger::FATAL)
end

Sidekiq::Testing.inline!

module Sidekiq
2 changes: 1 addition & 1 deletion fedora-manual-setup.md
Original file line number Diff line number Diff line change
@@ -50,7 +50,7 @@ podman run -d -p 3306:3306 -e MYSQL_ALLOW_EMPTY_PASSWORD=true --name mysql80 mys
[Redis](https://redis.io) is an in-memory data store used as DB for some of the data and it has to be running for the application to work. We recommend running it in a [Podman](https://podman.io/) container:

```
podman run -d -p 6379:6379 redis
podman run -d -p 6379:6379 redis:6.2-alpine
```

Alternatively, Redis can be run directly on your machine with `dnf`:
3 changes: 2 additions & 1 deletion openshift/system/config/backend_redis.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
production:
url: "<%= ENV.fetch('BACKEND_REDIS_URL', 'redis://backend-redis:6379') %>"
timeout: <%= ENV.fetch('REDIS_BACKEND_TIMEOUT', 1) %>
pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
jlledom marked this conversation as resolved.
Show resolved Hide resolved
pool_timeout: 5 # this is in seconds
sentinels: "<%= ENV['BACKEND_REDIS_SENTINEL_HOSTS'] %>"
name: <%= ENV['BACKEND_REDIS_SENTINEL_NAME'] %>
role: <%= ENV['BACKEND_REDIS_SENTINEL_ROLE'] %>
3 changes: 2 additions & 1 deletion openshift/system/config/redis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
production:
url: "<%= ENV.fetch('REDIS_URL', 'redis://system-redis/1') %>"
pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
pool_timeout: 5 # this is in seconds
sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
name: <%= ENV['REDIS_SENTINEL_NAME'] %>
role: <%= ENV['REDIS_SENTINEL_ROLE'] %>
Loading