Set 1MB limit for reactive cache data
What does this MR do?
Uses the Gitlab::Utils::DeepSize
to calculate the size of the data being cached on ReactiveCaching
. If the data goes over ReactiveCaching.reactive_cache_hard_limit
(default 1 MB), then the key is not cached and we communicate a ReactiveCaching::ExceededReactiveCacheLimit
error to Sentry.
Classes that know that their caches need more than 1 MB can override the ReactiveCaching.reactive_cache_hard_limit
method. So, we did the following customizations:
MergeRequest.reactive_cache_hard_limit = 20 MB
Environment.reactive_cache_hard_limit = 10 MB
Fallback plan
If things go south, we have a Feature Flag that we can disable to stop limiting:
...
def check_exceeded_reactive_cache_limit!(data)
return false unless Feature.enabled?(:reactive_cache_limit)
...
On what ground did we define 1 MB, 10 MB and 20 MB
In https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/8744#note_268471168 we assessed that the only usage of ReactiveCaching
that goes over 1 MB were related to the MergeRequest
, which are the Ci::Compare*
keys, and the Environment
.
So, 1 MB should be a good enough for a limit as long as we handle these two edge cases.
Moreover we acknowledged that, among keys related to MergeRequest
, the biggest offender was the ones associated with Ci::CompareSastReportsService
, which consumed a total of 29360584(29 MB) bytes over 3 keys of this type. That is an average of 9.8 MB per key. So I doubled this value to 20MB to give some room for Standard deviation. Still, I think an issue should be opened to investigate whether these kind of keys could be slimmed down.
Same was done for Environment
. There was 144704944(144 MB) for 13 keys, which would have given an average of (11 MB). Although for this kind of keys we did an analysis and find out one of the culprits for it growing so big. So we created another MR that is expected to reduce these keys size significantly (!23179 (merged)), as we could reduce a sample key size by /18, thus we think we're safe with setting a limit of 10 MB.
Conditions Before Merging
Notes
- This change was authorized to be dealt in a regular MR as discussed with the AppSec team: https://gitlab.com/gitlab-org/gitlab/issues/14015#note_260729436
- Relates to https://gitlab.com/gitlab-org/gitlab/issues/14015
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team