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

bump memory limits for redis-[cache,store] to 7 GB #373

Merged
merged 1 commit into from
May 18, 2021

Conversation

ggilmore
Copy link
Contributor

Both redis-cache and redis-store have maxmemory 6gb in their redis.conf: https://github.com/sourcegraph/infrastructure/blob/main/docker-images/redis-cache/redis.conf#L4 and https://github.com/sourcegraph/infrastructure/blob/3d83a92ce64650f46754e2ac55c3accfe84ab0d7/docker-images/redis-store/redis.conf#L4. According to https://redis.io/topics/lru-cache#eviction-policies, redis will start evicting things from its cache once it hits that limit. However, because our k8s pod memory limits are also set to 6 GB - k8s will kill the redis pods before they have a chance to perform that eviction process.

Giving redis one more gigabyte to work with should give it enough room to do its cache eviction without being OOMKilled

Checklist

Both redis-cache and redis-store  have `maxmemory 6gb` in their redis.conf: https://github.com/sourcegraph/infrastructure/blob/main/docker-images/redis-cache/redis.conf#L4 and https://github.com/sourcegraph/infrastructure/blob/3d83a92ce64650f46754e2ac55c3accfe84ab0d7/docker-images/redis-store/redis.conf#L4.  According to https://redis.io/topics/lru-cache#eviction-policies, redis will start evicting things from its cache once it hits that limit. However, because our _k8s pod_ memory limits are also set to 6 GB - k8s will kill the redis pods before they have a chance to perform that eviction process. 

Giving redis one more gigabyte to work with should give it enough room to do its cache eviction without being OOMKilled
@ggilmore ggilmore requested review from slimsag and a team April 29, 2021 22:45
@ggilmore ggilmore enabled auto-merge (squash) April 29, 2021 22:45
@ggilmore ggilmore disabled auto-merge April 29, 2021 22:45
Copy link
Contributor

@daxmc99 daxmc99 left a comment

Choose a reason for hiding this comment

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

LGTM
Do we have a CHANGELOG entry for this? And an upgrade notice on k8s and docker?

@slimsag
Copy link
Member

slimsag commented May 17, 2021

@ggilmore @daxmc99 can this be merged? or what's the status?

@daxmc99
Copy link
Contributor

daxmc99 commented May 18, 2021

I think this is ready to go, besides a comment in the CHANGELOG

@ggilmore ggilmore merged commit dbd98e8 into master May 18, 2021
@ggilmore ggilmore deleted the ggilmore-patch-2 branch May 18, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants