Skip to content

Set 1MB limit for reactive cache data

João Alexandre Cunha requested to merge feature/set-redis-hard-limits into master

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

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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
Edited by 🤖 GitLab Bot 🤖

Merge request reports