Merge branch 'feature/set-redis-hard-limits' into 'master'

Set 1MB limit for reactive cache data

Closes gitlab-com/gl-infra/infrastructure#8744

See merge request gitlab-org/gitlab!21871

app/models/concerns/reactive_caching.rb

@@ -6,23 +6,22 @@ module ReactiveCaching
   extend ActiveSupport::Concern
 
   InvalidateReactiveCache = Class.new(StandardError)
+  ExceededReactiveCacheLimit = Class.new(StandardError)
 
   included do
-    class_attribute :reactive_cache_lease_timeout
-
     class_attribute :reactive_cache_key
-    class_attribute :reactive_cache_lifetime
+    class_attribute :reactive_cache_lease_timeout
     class_attribute :reactive_cache_refresh_interval
+    class_attribute :reactive_cache_lifetime
+    class_attribute :reactive_cache_hard_limit
     class_attribute :reactive_cache_worker_finder
 
     # defaults
     self.reactive_cache_key = -> (record) { [model_name.singular, record.id] }
-
     self.reactive_cache_lease_timeout = 2.minutes
-
     self.reactive_cache_refresh_interval = 1.minute
     self.reactive_cache_lifetime = 10.minutes
-
+    self.reactive_cache_hard_limit = 1.megabyte
     self.reactive_cache_worker_finder = ->(id, *_args) do
       find_by(primary_key => id)
     end
@@ -71,6 +70,8 @@ module ReactiveCaching
         if within_reactive_cache_lifetime?(*args)
           enqueuing_update(*args) do
             new_value = calculate_reactive_cache(*args)
+            check_exceeded_reactive_cache_limit!(new_value)
+
             old_value = Rails.cache.read(key)
             Rails.cache.write(key, new_value)
             reactive_cache_updated(*args) if new_value != old_value
@@ -121,5 +122,13 @@ module ReactiveCaching
 
       ReactiveCachingWorker.perform_in(self.class.reactive_cache_refresh_interval, self.class, id, *args)
     end
+
+    def check_exceeded_reactive_cache_limit!(data)
+      return unless Feature.enabled?(:reactive_cache_limit)
+
+      data_deep_size = Gitlab::Utils::DeepSize.new(data, max_size: self.class.reactive_cache_hard_limit)
+
+      raise ExceededReactiveCacheLimit.new unless data_deep_size.valid?
+    end
   end
 end

app/models/environment.rb

@@ -6,6 +6,7 @@ class Environment < ApplicationRecord
 
   self.reactive_cache_refresh_interval = 1.minute
   self.reactive_cache_lifetime = 55.seconds
+  self.reactive_cache_hard_limit = 10.megabytes
 
   belongs_to :project, required: true
 

app/models/merge_request.rb

@@ -24,6 +24,7 @@ class MergeRequest < ApplicationRecord
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
   self.reactive_cache_refresh_interval = 10.minutes
   self.reactive_cache_lifetime = 10.minutes
+  self.reactive_cache_hard_limit = 20.megabytes
 
   SORTING_PREFERENCE_FIELD = :merge_requests_sort
 

app/workers/reactive_caching_worker.rb

@@ -25,5 +25,7 @@ class ReactiveCachingWorker
       .reactive_cache_worker_finder
       .call(id, *args)
       .try(:exclusively_update_reactive_cache!, *args)
+  rescue ReactiveCaching::ExceededReactiveCacheLimit => e
+    Gitlab::ErrorTracking.track_exception(e)
   end
 end

changelogs/unreleased/feature-set-redis-hard-limits.yml

+---
+title: Sets size limits on data loaded async, like deploy boards and merge request reports
+merge_request: 21871
+author:
+type: changed

doc/administration/instance_limits.md

@@ -87,6 +87,20 @@ Plan.default.limits.update!(ci_active_jobs: 500)
 
 NOTE: **Note:** Set the limit to `0` to disable it.
 
+## Environment data on Deploy Boards
+
+[Deploy Boards](../user/project/deploy_boards.md) load information from Kubernetes about
+Pods and Deployments. However, data over 10 MB for a certain environment read from
+Kubernetes won't be shown.
+
+## Merge Request reports
+
+Reports that go over the 20 MB limit won't be loaded. Affected reports:
+
+- [Merge Request security reports](../user/project/merge_requests/index.md#security-reports-ultimate)
+- [CI/CD parameter `artifacts:expose_as`](../ci/yaml/README.md#artifactsexpose_as)
+- [JUnit test reports](../ci/junit_test_reports.md)
+
 ## Advanced Global Search limits
 
 ### Maximum field length

doc/development/reactive_caching.md

@@ -48,6 +48,12 @@ of the cache by the `reactive_cache_lifetime` value.
 Once the lifetime has expired, no more background jobs will be enqueued and calling
 `#with_reactive_cache` will again return `nil` - starting the process all over again.
 
+### 1 MB hard limit
+
+`ReactiveCaching` has a 1 megabyte default limit. [This value is configurable](#selfreactive_cache_worker_finder).
+
+If the data we're trying to cache has over 1 megabyte, it will not be cached and a handled `ReactiveCaching::ExceededReactiveCacheLimit` will be notified on Sentry.
+
 ## When to use
 
 - If we need to make a request to an external API (for example, requests to the k8s API).
@@ -228,6 +234,16 @@ be reset to `reactive_cache_lifetime`.
 self.reactive_cache_lifetime = 10.minutes
 ```
 
+#### `self.reactive_cache_hard_limit`
+
+- This is the maximum data size that `ReactiveCaching` allows to be cached.
+- The default is 1 megabyte. Data that goes over this value will not be cached
+and will silently raise `ReactiveCaching::ExceededReactiveCacheLimit` on Sentry.
+
+```ruby
+self.reactive_cache_hard_limit = 5.megabytes
+```
+
 #### `self.reactive_cache_worker_finder`
 
 - This is the method used by the background worker to find or generate the object on

spec/models/concerns/reactive_caching_spec.rb

@@ -165,11 +165,25 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
   describe '#exclusively_update_reactive_cache!' do
     subject(:go!) { instance.exclusively_update_reactive_cache! }
 
+    shared_examples 'successful cache' do
+      it 'caches the result of #calculate_reactive_cache' do
+        go!
+
+        expect(read_reactive_cache(instance)).to eq(calculation.call)
+      end
+
+      it 'does not raise the exception' do
+        expect { go! }.not_to raise_exception(ReactiveCaching::ExceededReactiveCacheLimit)
+      end
+    end
+
     context 'when the lease is free and lifetime is not exceeded' do
       before do
-        stub_reactive_cache(instance, "preexisting")
+        stub_reactive_cache(instance, 'preexisting')
       end
 
+      it_behaves_like 'successful cache'
+
       it 'takes and releases the lease' do
         expect_to_obtain_exclusive_lease(cache_key, 'uuid')
         expect_to_cancel_exclusive_lease(cache_key, 'uuid')
@@ -177,19 +191,13 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
         go!
       end
 
-      it 'caches the result of #calculate_reactive_cache' do
-        go!
-
-        expect(read_reactive_cache(instance)).to eq(calculation.call)
-      end
-
-      it "enqueues a repeat worker" do
+      it 'enqueues a repeat worker' do
         expect_reactive_cache_update_queued(instance)
 
         go!
       end
 
-      it "calls a reactive_cache_updated only once if content did not change on subsequent update" do
+      it 'calls a reactive_cache_updated only once if content did not change on subsequent update' do
         expect(instance).to receive(:calculate_reactive_cache).twice
         expect(instance).to receive(:reactive_cache_updated).once
 
@@ -202,6 +210,43 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
         go!
       end
 
+      context 'when calculated object size exceeds default reactive_cache_hard_limit' do
+        let(:calculation) { -> { 'a' * 2 * 1.megabyte } }
+
+        shared_examples 'ExceededReactiveCacheLimit' do
+          it 'raises ExceededReactiveCacheLimit exception and does not cache new data' do
+            expect { go! }.to raise_exception(ReactiveCaching::ExceededReactiveCacheLimit)
+
+            expect(read_reactive_cache(instance)).not_to eq(calculation.call)
+          end
+        end
+
+        context 'when reactive_cache_hard_limit feature flag is enabled' do
+          it_behaves_like 'ExceededReactiveCacheLimit'
+
+          context 'when reactive_cache_hard_limit is overridden' do
+            let(:test_class) { Class.new(CacheTest) { self.reactive_cache_hard_limit = 3.megabytes } }
+            let(:instance) { test_class.new(666, &calculation) }
+
+            it_behaves_like 'successful cache'
+
+            context 'when cache size is over the overridden limit' do
+              let(:calculation) { -> { 'a' * 4 * 1.megabyte } }
+
+              it_behaves_like 'ExceededReactiveCacheLimit'
+            end
+          end
+        end
+
+        context 'when reactive_cache_limit feature flag is disabled' do
+          before do
+            stub_feature_flags(reactive_cache_limit: false)
+          end
+
+          it_behaves_like 'successful cache'
+        end
+      end
+
       context 'and #calculate_reactive_cache raises an exception' do
         before do
           stub_reactive_cache(instance, "preexisting")
@@ -256,8 +301,8 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
     it { expect(subject.reactive_cache_lease_timeout).to be_a(ActiveSupport::Duration) }
     it { expect(subject.reactive_cache_refresh_interval).to be_a(ActiveSupport::Duration) }
     it { expect(subject.reactive_cache_lifetime).to be_a(ActiveSupport::Duration) }
-
     it { expect(subject.reactive_cache_key).to respond_to(:call) }
+    it { expect(subject.reactive_cache_hard_limit).to be_a(Integer) }
     it { expect(subject.reactive_cache_worker_finder).to respond_to(:call) }
   end
 end

spec/workers/reactive_caching_worker_spec.rb

@@ -14,6 +14,18 @@ describe ReactiveCachingWorker do
 
         described_class.new.perform("Environment", environment.id)
       end
+
+      context 'when ReactiveCaching::ExceededReactiveCacheLimit is raised' do
+        it 'avoids failing the job and tracks via Gitlab::ErrorTracking' do
+          allow_any_instance_of(Environment).to receive(:exclusively_update_reactive_cache!)
+            .and_raise(ReactiveCaching::ExceededReactiveCacheLimit)
+
+          expect(Gitlab::ErrorTracking).to receive(:track_exception)
+            .with(kind_of(ReactiveCaching::ExceededReactiveCacheLimit))
+
+          described_class.new.perform("Environment", environment.id)
+        end
+      end
     end
   end
 end