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

Add Redis Slave Endpoint to HAProxy Service configuration #35

Closed
wants to merge 1 commit into from

Conversation

rurkss
Copy link

@rurkss rurkss commented Jan 11, 2024

This PR introduces a new endpoint in the HAProxy Service, directing traffic to a Redis node with a 'slave' role in the cluster. The port for this is derived from the BootrapNode.Port setting. If this configuration is unset, the endpoint won't be established.

The fundamental concept of bootstrapping involves establishing a connection from a replica Redis cluster to the primary Redis cluster, specifically to its slave nodes. It seems logical to utilize Bootstrap.Port for the HAProxy service to target the slave node of the source Redis cluster.

@rurkss rurkss self-assigned this Jan 11, 2024
@rurkss rurkss requested a review from a team as a code owner January 11, 2024 20:07
@@ -186,6 +186,26 @@ func generateHAProxyConfigmap(rf *redisfailoverv1.RedisFailover, labels map[stri
server-template redis %d _redis._tcp.%s.%s.svc.cluster.local:%d check inter 1s resolvers k8s init-addr none
`, port, rf.Spec.Redis.Replicas, redisName, namespace, port)

if rf.Spec.BootstrapNode != nil && rf.Spec.BootstrapNode.Port != rf.Spec.Redis.Port.ToString() {

Choose a reason for hiding this comment

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

Might be worth noting that the redis-operator already exposes Services to get at the "master" node, or "slave" nodes of a RedisFailover. I'm not sure I fully understand the cost/benefit equation of doing the same thing with more moving parts via HAProxy.

That said, BootstrapNode.Port is

If you are wanting to migrate off of a pre-existing Redis instance, you can provide a bootstrapNode to your RedisFailover resource spec...The Port that the target Redis address is listening to. Defaults to 6379.

I'm confused, so maybe I'm reading this code wrong. Wouldn't this expose the slave nodes of the Replicating RedisFailover on the Port that the "target"/source Redis instance is exposed on?

IF I've understood this correctly, had you considered the following alternatives that avoid overloading the meaning of the Bootstrap.Port:

  1. Expose the slaves on a static port. To be clear, I'm fan of this idea yet the redis-operator already does this for the metrics exporter(s)12; so it's not that far off script and we could make it more dynamic later if the need arises.
  2. Make the HAPRoxy ports configurable like the redis-operator already does for the Redis and Sentinel nodes.

Footnotes

  1. (https://github.com/powerhome/redis-operator/blob/bd12c9ae881b5b50f34fbb511237b08c35348234/operator/redisfailover/service/generator.go#L451)

  2. (https://github.com/powerhome/redis-operator/blob/632aa3da88ee2a28ccb9fc4f571ddcb556dbd713/operator/redisfailover/service/constants.go#L5)

@rurkss rurkss closed this Jan 17, 2024
rurkss added a commit that referenced this pull request Jan 18, 2024
This Pull Request presents a different method compared to [Add Redis
Slave Endpoint to HAProxy Service
configuration](#35).
Here, the redis-operator will establish a completely new HAProxy
service, designed to route traffic specifically to Redis instances
functioning as slaves.
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