From c66ddd0223f08b129bf5bd22cea15b0a61be805a Mon Sep 17 00:00:00 2001 From: Eric Boehs Date: Mon, 25 Mar 2024 12:54:34 -0500 Subject: [PATCH 1/3] Upgrade to Sidekiq 7 (w/ working feature toggles) (#16047) * Upgrade to Sidekiq 7 This reverts commit 24571c6868c19461f7df313f05b66161d6d34b81 to restore Sidekiq 7. * fix: Don't pass nil to redis-client Sidekiq 7 required the redis gem version 5.x (we were using 4.8) which required redis-client. redis-client would throw an exception when searching for a nil key. This resolves the issue with /v0/feature_toggles experienced Mar 21st. The exception in Datadog[1] was: ``` TypeError: Unsupported command argument type: NilClass /usr/local/bundle/cache/ruby/3.2.0/gems/redis-client-0.20.0/lib/redis_client/command_builder.rb:37:in `block in generate': Unsupported command argument type: NilClass (TypeError) ... from /app/lib/common/models/redis_store.rb:51:in `find' from /app/app/controllers/concerns/authentication_and_sso_concerns.rb:131:in `set_session_object' from /app/app/controllers/concerns/authentication_and_sso_concerns.rb:67:in `load_user' ``` Following the stack trace, I opened redis-client (`bundle open redis-client`) and saw that `nil` wasn't supported as a type in the `case` (command_builder.rb:37) but our redis_store.rb allowed nil to be passed. [1]: https://vagov.ddog-gov.com/apm/error-tracking?query=env%3Aeks-prod&fromUser=false&issueId=b8ee5258-e6f4-11ee-b6c6-da7ad0900007&refresh_mode=sliding&sp=&view=spans&from_ts=1711055100302&to_ts=1711058700302&live=true * Switch Redis config back to to_h --- Gemfile | 4 +-- Gemfile.lock | 34 +++++++++++-------- config/initializers/01_redis.rb | 10 ++++-- config/initializers/sidekiq.rb | 4 ++- config/redis.yml | 3 ++ docker-compose.test.yml | 6 ++++ lib/common/models/redis_store.rb | 2 ++ lib/sidekiq/semantic_logging.rb | 5 +++ .../app/services/ask_va_api/redis_client.rb | 21 ++++++++++++ .../ask_va_api/app/services/crm/cache_data.rb | 2 +- .../ask_va_api/app/services/crm/crm_token.rb | 2 +- .../ask_va_api/app/services/redis_client.rb | 19 ----------- .../spec/services/crm/cache_data_spec.rb | 2 +- .../spec/services/redis_client_spec.rb | 4 +-- spec/lib/chip/redis_client_spec.rb | 10 ++++-- spec/lib/common/models/redis_store_spec.rb | 20 ++++++++--- spec/requests/breakers_integration_spec.rb | 1 - spec/spec_helper.rb | 6 +++- 18 files changed, 102 insertions(+), 53 deletions(-) create mode 100644 modules/ask_va_api/app/services/ask_va_api/redis_client.rb delete mode 100644 modules/ask_va_api/app/services/redis_client.rb diff --git a/Gemfile b/Gemfile index ca26ce94c50..056e8a031aa 100644 --- a/Gemfile +++ b/Gemfile @@ -174,7 +174,7 @@ end group :test do gem 'apivore', git: 'https://github.com/department-of-veterans-affairs/apivore', tag: 'v2.0.0.vsp' - gem 'fakeredis' + gem 'mock_redis' gem 'pdf-inspector' gem 'rspec_junit_formatter' gem 'rspec-retry' @@ -215,7 +215,7 @@ group :development, :test do gem 'rubocop-rails' gem 'rubocop-rspec' gem 'rubocop-thread_safety' - gem 'sidekiq', '>= 6.4.0' + gem 'sidekiq', '~> 7.2.0' gem 'timecop' gem 'webmock' gem 'yard' diff --git a/Gemfile.lock b/Gemfile.lock index 66b5c15fe00..b991b8137fc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -133,12 +133,13 @@ PATH GEM remote: https://enterprise.contribsys.com/ specs: - sidekiq-ent (2.5.3) - einhorn (>= 0.7.4) - sidekiq (>= 6.5.0, < 7) - sidekiq-pro (>= 5.5.0, < 6) - sidekiq-pro (5.5.8) - sidekiq (~> 6.0, >= 6.5.6) + sidekiq-ent (7.2.2) + einhorn (~> 1.0) + gserver + sidekiq (>= 7.2.0, < 8) + sidekiq-pro (>= 7.2.0, < 8) + sidekiq-pro (7.2.0) + sidekiq (>= 7.2.0, < 8) GEM remote: https://rubygems.org/ @@ -417,8 +418,6 @@ GEM railties (>= 5.0.0) faker (3.2.3) i18n (>= 1.8.11, < 2) - fakeredis (0.9.2) - redis (~> 4.8) faraday (2.9.0) faraday-net_http (>= 2.0, < 3.2) faraday-follow_redirects (0.3.0) @@ -634,6 +633,7 @@ GEM mini_mime (1.1.5) mini_portile2 (2.8.5) minitest (5.22.3) + mock_redis (0.44.0) msgpack (1.7.2) msgpack (1.7.2-java) multi_json (1.15.0) @@ -812,7 +812,10 @@ GEM rchardet (1.8.0) rdoc (6.6.2) psych (>= 4.0.0) - redis (4.8.1) + redis (5.1.0) + redis-client (>= 0.17.0) + redis-client (0.20.0) + connection_pool redis-namespace (1.11.0) redis (>= 4) regexp_parser (2.9.0) @@ -954,10 +957,11 @@ GEM shrine (3.5.0) content_disposition (~> 1.0) down (~> 5.1) - sidekiq (6.5.12) - connection_pool (>= 2.2.5, < 3) - rack (~> 2.0) - redis (>= 4.5.0, < 5) + sidekiq (7.2.2) + concurrent-ruby (< 2) + connection_pool (>= 2.3.0) + rack (>= 2.2.4) + redis-client (>= 0.19.0) sidekiq_alive (2.4.0) gserver (~> 0.0.1) sidekiq (>= 5, < 8) @@ -1122,7 +1126,6 @@ DEPENDENCIES facilities_api! factory_bot_rails faker - fakeredis faraday (~> 2.9) faraday-follow_redirects faraday-httpclient @@ -1170,6 +1173,7 @@ DEPENDENCIES mimemagic mini_magick mobile! + mock_redis mocked_authentication! my_health! net-sftp @@ -1234,7 +1238,7 @@ DEPENDENCIES sentry-ruby shoulda-matchers shrine - sidekiq (>= 6.4.0) + sidekiq (~> 7.2.0) sidekiq-ent! sidekiq-pro! sidekiq_alive diff --git a/config/initializers/01_redis.rb b/config/initializers/01_redis.rb index bd43c3a719d..fcf0ffd2bd5 100644 --- a/config/initializers/01_redis.rb +++ b/config/initializers/01_redis.rb @@ -4,6 +4,10 @@ REDIS_CONFIG = Rails.application.config_for(:redis).freeze # set the current global instance of Redis based on environment specific config -$redis = Redis.new(REDIS_CONFIG[:redis].to_hash) - -Redis.exists_returns_integer = true +$redis = + if Rails.env.test? + require 'mock_redis' + MockRedis.new(url: REDIS_CONFIG[:redis][:url]) + else + Redis.new(REDIS_CONFIG[:redis].to_h) + end diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index b2852d54867..41fb273cc5c 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -59,6 +59,8 @@ end # Remove the default error handler - config.error_handlers.delete_if { |handler| handler.is_a?(Sidekiq::ExceptionHandler::Logger) } + config.error_handlers.delete(Sidekiq::Config::ERROR_HANDLER) end + + Sidekiq.strict_args!(false) end diff --git a/config/redis.yml b/config/redis.yml index c3444f1575c..3cde49a7e0b 100644 --- a/config/redis.yml +++ b/config/redis.yml @@ -197,6 +197,9 @@ test: <<: *defaults redis: inherit_socket: true + url: <%= Settings.redis.app_data.url %> + sidekiq: + url: <%= Settings.redis.sidekiq.url %> production: <<: *defaults diff --git a/docker-compose.test.yml b/docker-compose.test.yml index e9f5a783f9f..907cf5c2d27 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -1,5 +1,7 @@ version: '3.4' services: + redis: + image: redis:6.2-alpine postgres: image: mdillon/postgis:11-alpine command: postgres -c shared_preload_libraries=pg_stat_statements -c pg_stat_statements.track=all -c max_connections=200 @@ -21,6 +23,8 @@ services: "Settings.database_url": "postgis://${POSTGRES_USER:-postgres}:${POSTGRES_PASSWORD:-password}@${POSTGRES_HOST:-postgres}:${POSTGRES_PORT:-5432}/${POSTGRES_DATABASE:-vets_api_development}?pool=4" "Settings.test_database_url": "postgis://${POSTGRES_USER:-postgres}:${POSTGRES_PASSWORD:-password}@${POSTGRES_HOST:-postgres}:${POSTGRES_PORT:-5432}/${POSTGRES_DATABASE:-vets_api_test}" "Settings.binaries.clamdscan": "clamscan" # Not running a separate process within the container for clamdscan, so we use clamscan which requires no daemon + "Settings.redis.app_data.url": "redis://redis:6379" + "Settings.redis.sidekiq.url": "redis://redis:6379" POSTGRES_HOST: "${POSTGRES_HOST:-postgres}" POSTGRES_PORT: "${POSTGRES_PORT:-5432}" POSTGRES_USER: "${POSTGRES_USER:-postgres}" @@ -34,7 +38,9 @@ services: DANGER_GITHUB_API_TOKEN: "${DANGER_GITHUB_API_TOKEN}" depends_on: - postgres + - redis links: - postgres + - redis volumes: test_bundle: diff --git a/lib/common/models/redis_store.rb b/lib/common/models/redis_store.rb index 755a92a5980..fa89590beb1 100644 --- a/lib/common/models/redis_store.rb +++ b/lib/common/models/redis_store.rb @@ -48,6 +48,8 @@ def initialize(attributes = {}, persisted = false) end def self.find(redis_key = nil) + return nil if redis_key.nil? + response = redis_namespace.get(redis_key) return nil unless response diff --git a/lib/sidekiq/semantic_logging.rb b/lib/sidekiq/semantic_logging.rb index 822699bd393..b6043e958ca 100644 --- a/lib/sidekiq/semantic_logging.rb +++ b/lib/sidekiq/semantic_logging.rb @@ -4,6 +4,11 @@ require 'sidekiq/job_logger' class Sidekiq::SemanticLogging < Sidekiq::JobLogger + def initialize + logger = Rails.logger + super(logger) + end + def call(_worker, item, queue) logger_tags = { class: item['class'], diff --git a/modules/ask_va_api/app/services/ask_va_api/redis_client.rb b/modules/ask_va_api/app/services/ask_va_api/redis_client.rb new file mode 100644 index 00000000000..5c25c9f5c40 --- /dev/null +++ b/modules/ask_va_api/app/services/ask_va_api/redis_client.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module AskVAApi + class RedisClient + def fetch(key) + Rails.cache.read( + key, + namespace: 'crm-api-cache' + ) + end + + def store_data(key:, data:, ttl:) + Rails.cache.write( + key, + data, + namespace: 'crm-api-cache', + expires_in: ttl + ) + end + end +end diff --git a/modules/ask_va_api/app/services/crm/cache_data.rb b/modules/ask_va_api/app/services/crm/cache_data.rb index a3d286b9f33..9b53c16c82e 100644 --- a/modules/ask_va_api/app/services/crm/cache_data.rb +++ b/modules/ask_va_api/app/services/crm/cache_data.rb @@ -4,7 +4,7 @@ module Crm class CacheData attr_reader :cache_client, :service - def initialize(service: Service.new(icn: nil), cache_client: RedisClient.new) + def initialize(service: Service.new(icn: nil), cache_client: AskVAApi::RedisClient.new) @cache_client = cache_client @service = service end diff --git a/modules/ask_va_api/app/services/crm/crm_token.rb b/modules/ask_va_api/app/services/crm/crm_token.rb index b945e181ed7..3b023fa2ced 100644 --- a/modules/ask_va_api/app/services/crm/crm_token.rb +++ b/modules/ask_va_api/app/services/crm/crm_token.rb @@ -16,7 +16,7 @@ class CrmToken def initialize @settings = Settings.ask_va_api.crm_api - @cache_client = RedisClient.new + @cache_client = AskVAApi::RedisClient.new @logger = LogService.new end diff --git a/modules/ask_va_api/app/services/redis_client.rb b/modules/ask_va_api/app/services/redis_client.rb deleted file mode 100644 index ae809c2eed2..00000000000 --- a/modules/ask_va_api/app/services/redis_client.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -class RedisClient - def fetch(key) - Rails.cache.read( - key, - namespace: 'crm-api-cache' - ) - end - - def store_data(key:, data:, ttl:) - Rails.cache.write( - key, - data, - namespace: 'crm-api-cache', - expires_in: ttl - ) - end -end diff --git a/modules/ask_va_api/spec/services/crm/cache_data_spec.rb b/modules/ask_va_api/spec/services/crm/cache_data_spec.rb index b1b247a2b5c..1e341f6469d 100644 --- a/modules/ask_va_api/spec/services/crm/cache_data_spec.rb +++ b/modules/ask_va_api/spec/services/crm/cache_data_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Crm::CacheData do let(:service) { double('Crm::Service') } - let(:cache_client) { double('RedisClient') } + let(:cache_client) { double('AskVAApi::RedisClient') } let(:cache_data_instance) { Crm::CacheData.new(service:, cache_client:) } let(:cache_data) { { topics: [{ id: 1, name: 'Topic 1' }] } } diff --git a/modules/ask_va_api/spec/services/redis_client_spec.rb b/modules/ask_va_api/spec/services/redis_client_spec.rb index fd58fdc3795..b8d2b7e74c0 100644 --- a/modules/ask_va_api/spec/services/redis_client_spec.rb +++ b/modules/ask_va_api/spec/services/redis_client_spec.rb @@ -2,8 +2,8 @@ require 'rails_helper' -RSpec.describe RedisClient do - let(:redis_client) { RedisClient.new } +RSpec.describe AskVAApi::RedisClient do + let(:redis_client) { AskVAApi::RedisClient.new } let(:token) { 'some-access-token' } describe '#fetch' do diff --git a/spec/lib/chip/redis_client_spec.rb b/spec/lib/chip/redis_client_spec.rb index cb8f8c9e618..6d5e4c7d667 100644 --- a/spec/lib/chip/redis_client_spec.rb +++ b/spec/lib/chip/redis_client_spec.rb @@ -29,11 +29,17 @@ let(:token) { 'test_token' } it 'saves entry' do - expect_any_instance_of(Redis).to receive(:set).once.with( - "chip:#{tenant_id}", 'test_token', { ex: REDIS_CONFIG[:chip][:each_ttl] } + expect_any_instance_of(Redis::Namespace).to receive(:set).once.with( + tenant_id, 'test_token', { ex: REDIS_CONFIG[:chip][:each_ttl] } ) redis_client.save(token:) end + + it 'saves entry with chip namespace' do + redis_client.save(token:) + + expect(redis_client.redis_namespace.redis.keys).to include("chip:#{tenant_id}") + end end describe 'ttl' do diff --git a/spec/lib/common/models/redis_store_spec.rb b/spec/lib/common/models/redis_store_spec.rb index d9147240c6b..0e0b3a1cfd0 100644 --- a/spec/lib/common/models/redis_store_spec.rb +++ b/spec/lib/common/models/redis_store_spec.rb @@ -62,12 +62,18 @@ describe '#save' do it 'saves serialized class to redis with the correct namespace' do - expect_any_instance_of(Redis).to receive(:set).once.with( - 'my_namespace:e66fd7b7-94e0-4748-8063-283f55efb0ea', + expect_any_instance_of(Redis::Namespace).to receive(:set).once.with( + 'e66fd7b7-94e0-4748-8063-283f55efb0ea', '{":uuid":"e66fd7b7-94e0-4748-8063-283f55efb0ea",":email":"foo@bar.com"}' ) subject.save end + + it 'saves entry with namespace' do + subject.save + + expect(subject.redis_namespace.redis.keys).to include('my_namespace:e66fd7b7-94e0-4748-8063-283f55efb0ea') + end end describe '#update' do @@ -92,12 +98,18 @@ describe '#destroy' do it 'removes itself from redis with the correct namespace' do - expect_any_instance_of(Redis).to receive(:del).once.with( - 'my_namespace:e66fd7b7-94e0-4748-8063-283f55efb0ea' + expect_any_instance_of(Redis::Namespace).to receive(:del).once.with( + 'e66fd7b7-94e0-4748-8063-283f55efb0ea' ) subject.destroy end + it "entry doesn't exists" do + subject.destroy + + expect(subject.redis_namespace.redis.keys).not_to include('my_namespace:e66fd7b7-94e0-4748-8063-283f55efb0ea') + end + it 'freezes the instance after destroy is called' do subject.destroy expect(subject.destroyed?).to eq(true) diff --git a/spec/requests/breakers_integration_spec.rb b/spec/requests/breakers_integration_spec.rb index 48b56c4b60c..512c36ce557 100644 --- a/spec/requests/breakers_integration_spec.rb +++ b/spec/requests/breakers_integration_spec.rb @@ -30,7 +30,6 @@ # Not clearing the breakers would cause subsequent RX calls to fail after the breaker is # triggered in this group. - # fakeredis/rspec has a `before` callback, but it's for the suite, not each example. Oops. Breakers.client.redis_connection.redis.flushdb end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b858a23e81a..8b036e3f589 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require 'fakeredis/rspec' require 'i18n' require 'support/spec_builders' require 'support/matchers' @@ -15,6 +14,7 @@ require 'rspec/its' require 'rspec/retry' require 'aasm/rspec' +require 'mock_redis' # By default run SimpleCov, but allow an environment variable to disable. unless ENV['NOCOVERAGE'] @@ -192,4 +192,8 @@ config.after do Timecop.return end + + config.before do + $redis.flushdb + end end From aca77669954101b015bc2368a2beaf61ac35b19a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 25 Mar 2024 17:57:26 +0000 Subject: [PATCH 2/3] Bump rubocop-rails from 2.24.0 to 2.24.1 Bumps [rubocop-rails](https://github.com/rubocop/rubocop-rails) from 2.24.0 to 2.24.1. - [Release notes](https://github.com/rubocop/rubocop-rails/releases) - [Changelog](https://github.com/rubocop/rubocop-rails/blob/master/CHANGELOG.md) - [Commits](https://github.com/rubocop/rubocop-rails/compare/v2.24.0...v2.24.1) --- updated-dependencies: - dependency-name: rubocop-rails dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index b991b8137fc..3dd7a71f6dd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -907,7 +907,7 @@ GEM rubocop-factory_bot (2.25.1) rubocop (~> 1.41) rubocop-junit-formatter (0.1.4) - rubocop-rails (2.24.0) + rubocop-rails (2.24.1) activesupport (>= 4.2.0) rack (>= 1.1) rubocop (>= 1.33.0, < 2.0) From 59181c22d4d3be37f82a91c85ba3fcb1bcea0a74 Mon Sep 17 00:00:00 2001 From: Casey Williams Date: Mon, 25 Mar 2024 11:22:35 -0700 Subject: [PATCH 3/3] Add Shaun Burdick to Decision Reviews email list (#16069) --- modules/appeals_api/config/mailinglists/report_daily.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/appeals_api/config/mailinglists/report_daily.yml b/modules/appeals_api/config/mailinglists/report_daily.yml index f1be8464b4a..a406f7074c4 100644 --- a/modules/appeals_api/config/mailinglists/report_daily.yml +++ b/modules/appeals_api/config/mailinglists/report_daily.yml @@ -21,5 +21,6 @@ production: - robin.garrison@adhocteam.us - rocio.de-santiago@coforma.io - sade@coforma.io + - shaun.burdick@coforma.io - steve.albers@va.gov - zachary.goldfine@va.gov