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

Fixes #36037 - Manage Redis service for Redis cache #1109

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Feb 2, 2023

This automatically manages Redis when the cache store type is set to Redis, but no URLs have been provided. It's assumed that when a URL is provided that the service is managed in another way.

This ignores the edge case where the user wants to run it on localhost, but with a different database than 0.

Currently a draft since it doesn't have tests and I haven't verified it yet. The end goal of this is that we can set up Redis caching by default. File based caching is just not the right fit for large deployments, even if it's stored in tmpfs.

@ekohl ekohl force-pushed the 36037-manage-redis-for-cache branch from fbbd9c3 to 538f512 Compare February 2, 2023 15:36
@ekohl ekohl marked this pull request as ready for review February 15, 2023 18:53
@ekohl
Copy link
Member Author

ekohl commented Feb 15, 2023

In https://bugzilla.redhat.com/show_bug.cgi?id=2165092#c4 it was confirmed this works.

@evgeni
Copy link
Member

evgeni commented Feb 15, 2023

Won't this clash with the redis service for pulpcore (in the setups with pulp)?

@ekohl
Copy link
Member Author

ekohl commented Feb 16, 2023

There we use DB 8 (https://github.com/theforeman/puppet-pulpcore/blob/ef0762aacb44d455fd1e2f06725f92c1ed63b896/manifests/init.pp#L221) so using DB 0 shouldn't clash. Dynflow uses DB 6 (by default):

$dynflow_redis_url = "redis://localhost:${redis::port}/6"

@evgeni
Copy link
Member

evgeni commented Feb 16, 2023

Okay, this just sounds like magic numbers to me, but I guess that's OK for now? :)

@ekohl
Copy link
Member Author

ekohl commented Feb 16, 2023

They certainly are magic. Perhaps it's something we should document, but not sure how/where.

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

I guess tests would be cool? ;-)

This automatically manages Redis when the cache store type is set to
Redis, but no URLs have been provided. It's assumed that when a URL is
provided that the service is managed in another way.

This ignores the edge case where the user wants to run it on localhost,
but with a different database than 0.
@ekohl ekohl force-pushed the 36037-manage-redis-for-cache branch from 538f512 to dfa6a86 Compare February 16, 2023 13:07
@ekohl
Copy link
Member Author

ekohl commented Feb 16, 2023

Now you made me file an issue to monitor the cache. But here are at least some basic tests.

@ekohl
Copy link
Member Author

ekohl commented Feb 17, 2023

@evgeni enough tests now?

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

There can never be enough tests ;)

@ekohl
Copy link
Member Author

ekohl commented Feb 17, 2023

Patches welcome? ;)

@ekohl ekohl merged commit dfa6a86 into theforeman:master Feb 17, 2023
@ekohl ekohl deleted the 36037-manage-redis-for-cache branch February 17, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants