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

Redis Model Cache (PROJQUAY-788) #444

Merged
merged 1 commit into from Apr 15, 2021
Merged

Conversation

alecmerdler
Copy link
Contributor

@alecmerdler alecmerdler commented Jun 16, 2020

Issue: https://issues.redhat.com/browse/PROJQUAY-788

Changelog: Add model cache implementation backed by Redis.

Docs: N/a

Testing: Add the following section to config.yaml:

DATA_MODEL_CACHE_CONFIG:
  engine: redis
  host: <redis-hostname>
  port: 6379

Details: Adds an implementation of the model cache which uses Redis. The goal is when deploying a cluster of Quay app containers, they will all use a shared Redis instance for caching requests (as opposed to a per-container memcache), which will reduce load on the database for expensive queries. Enhancements proposal.

NOTE: When using this model cache implementation, the Redis dependency changes from being low priority (effectively ephemeral) to being critical (and should be HA).
NOTE: The Quay container will still have memcached running even if not being used, unless we modify the container entrypoint to read the config.yaml.

Blocked by #745

@alecmerdler alecmerdler changed the title Redis Model Cache [WIP] Redis Model Cache Jun 16, 2020
@alecmerdler alecmerdler marked this pull request as draft June 16, 2020 20:27
@alecmerdler alecmerdler changed the title [WIP] Redis Model Cache Redis Model Cache Jun 16, 2020
@alecmerdler
Copy link
Contributor Author

Rebased with master.

@alecmerdler alecmerdler marked this pull request as ready for review April 10, 2021 23:28
@alecmerdler alecmerdler changed the title Redis Model Cache Redis Model Cache (PROJQUAY-778) Apr 10, 2021
Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

When using this model cache implementation, the Redis dependency changes from being low priority (effectively ephemeral) to being critical (and should be HA).

It depends. Most deployment aren't large enough or don't generate enough traffic to cause a thundering herd issue, and clearing the cache wouldn't be an issue.

data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Show resolved Hide resolved
@kleesc
Copy link
Member

kleesc commented Apr 12, 2021

@alecmerdler The current ticket linked is the wrong one. It should be PROJQUAY-788

data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Outdated Show resolved Hide resolved
@alecmerdler alecmerdler force-pushed the PROJQUAY-788 branch 3 times, most recently from 37f3cc7 to ea28362 Compare April 13, 2021 23:00
@kleesc
Copy link
Member

kleesc commented Apr 13, 2021

@alecmerdler Like you previously mentioned, I think we should make it so that two requests can't write to the same key at once (grab a Lock).

@alecmerdler alecmerdler changed the title Redis Model Cache (PROJQUAY-778) Redis Model Cache (PROJQUAY-788) Apr 14, 2021
@alecmerdler alecmerdler force-pushed the PROJQUAY-788 branch 2 times, most recently from 1aba288 to ee26716 Compare April 14, 2021 18:51
@alecmerdler
Copy link
Contributor Author

@kleesc Ready for another review after being rebased with the GlobalLock fix.

data/cache/impl.py Outdated Show resolved Hide resolved
data/cache/impl.py Outdated Show resolved Hide resolved
@alecmerdler alecmerdler force-pushed the PROJQUAY-788 branch 4 times, most recently from 1933971 to d73b74b Compare April 14, 2021 21:25
kleesc
kleesc previously approved these changes Apr 15, 2021
Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

LGTM

@alecmerdler
Copy link
Contributor Author

I'm seeing slow pushes (stuck in preparing state) when running this on my k8s cluster. Going to do a bit more testing to ensure it's unrelated to this code change.

Adds implementation of DataModelCache interface backed by Redis.
All containers in a Quay cluster deployment will share a single
model cache, rather than each container using its own cache.

Signed-off-by: Alec Merdler <alecmerdler@gmail.com>
Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

LGTM

@alecmerdler alecmerdler merged commit 780685c into quay:master Apr 15, 2021
@alecmerdler alecmerdler deleted the PROJQUAY-788 branch April 15, 2021 17:31
alecmerdler added a commit to alecmerdler/quay that referenced this pull request Apr 15, 2021
Adds implementation of DataModelCache interface backed by Redis.
All containers in a Quay cluster deployment will share a single
model cache, rather than each container using its own cache.

Signed-off-by: Alec Merdler <alecmerdler@gmail.com>
alecmerdler added a commit that referenced this pull request Apr 15, 2021
Adds implementation of DataModelCache interface backed by Redis.
All containers in a Quay cluster deployment will share a single
model cache, rather than each container using its own cache.

Signed-off-by: Alec Merdler <alecmerdler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants