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

Set maxmemory-policy=noeviction parameter for Chat Redis #1443

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

kevindew
Copy link
Member

This configures the Redis instance that GOV.UK Chat uses for Sidekiq to use it's own parameter group and for that group to have maxmemory-policy=noeviction. This is to resolve the warning we get from Sidekiq of:

WARNING: Your Redis instance will evict Sidekiq data under heavy load.
The 'noeviction' maxmemory policy is recommended (current policy: 'volatile-lru').
See: https://github.com/mperham/sidekiq/wiki/Using-Redis#memory

I need to confirm that this terraform a) actually works and b) doesn't mess up any other parameters.

In the subsequent commits I'm going to configure the parameters of Redis
for Sidekiq specifically, hence updating this label to reflect purpose.
This replaces the ability to change Redis version in each environment
and have different parameter groups.

The motivation for this is that we want to set specific parameters to
tune Redis for sidekiq and this will be coupled to the redis version.

This approach is also consistent with the approach for Redis version on
the shared redis instance.
This is the value that Sidekiq recommends to avoid any data being
dropped if memory is exceeded. Without this configuration option Sidekiq
complains:

```
WARNING: Your Redis instance will evict Sidekiq data under heavy load.
The 'noeviction' maxmemory policy is recommended (current policy: 'volatile-lru').
See: https://github.com/mperham/sidekiq/wiki/Using-Redis#memory
```

To resolve this I've created a parameter group so that we can set this
parameter. From googling I also understand that we need to change
cluster-enabled too and then every other parameter is the same as
default AWS - however I still need to confirm that.
Copy link
Contributor

@ianhowell-gds ianhowell-gds left a comment

Choose a reason for hiding this comment

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

LGTM

@kevindew kevindew merged commit 3549582 into main Sep 12, 2024
5 checks passed
@kevindew kevindew deleted the redis-for-sidekiq branch September 12, 2024 14:40
kevindew added a commit that referenced this pull request Sep 12, 2024
Continuing my whack-a-mole process of trying to get a Terraform plan
that will apply from
#1443.

It turns out we don't want to set cluster enabled to yes and the default
option is no, so we shouldn't have been trying to change it.
kevindew added a commit that referenced this pull request Sep 12, 2024
Continuing my whack-a-mole process of trying to get a Terraform plan
that will apply from
#1443.

It turns out we don't want to set cluster enabled to yes and the default
option is no, so we shouldn't have been trying to change it.

I've been through and compared all the other parameters and they seem to
match.
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.

2 participants