Enable DNS communication between redis#40
Conversation
|
@pratheep-kumar @Sasidharan-Gopal Can you review this? |
| for _, rp := range rps.Items { | ||
| if rp.Status.PodIP == master { | ||
| podAddress := GetPodAddress(&rp, rf) | ||
| if podAddress == master { |
There was a problem hiding this comment.
can we have this is a enum or static variable rather than a dynamic string?
There was a problem hiding this comment.
podAddress will be different for different clusters, don't think we'll be able to have it as ENUM or static variable
<statefulset-name>-<ordinal-number>.<service-name>.<namespace>.svc.cluster.local
| } | ||
| // Compare master with what Redis returns | ||
| // Redis may return either DNS name or IP depending on how it was configured | ||
| if slave != "" && slave != master { |
There was a problem hiding this comment.
nit: same as above comment
| start := redisNode.Status.StartTime.Round(time.Second) | ||
| alive := time.Since(start) | ||
| r.logger.Debugf("Pod %s has been alive for %.f seconds", redisNode.Status.PodIP, alive.Seconds()) | ||
| podAddress := GetPodAddress(&redisNode, rf) |
There was a problem hiding this comment.
nit: we are doing getPodAddress() call at many places and doing computation each and every time. Is it possible to do it in a base funtion and pass it around?
| } | ||
| for _, rp := range rps.Items { | ||
| if rp.Status.PodIP == ip { | ||
| podAddress := GetPodAddress(&rp, rf) |
There was a problem hiding this comment.
we dont need getPodAddress() here right? rp.Status.PodIP this itself is enough
There was a problem hiding this comment.
when the disableIPMode is enabled, we'll have the hostname instead of IP address. in that case we'll need to GetPodAddress()
| r.logger.WithField("redisfailover", rf.ObjectMeta.Name).WithField("namespace", rf.ObjectMeta.Namespace).Infof("Making pod %s slave of %s", pod.Name, masterIP) | ||
| if err := r.redisClient.MakeSlaveOfWithPort(pod.Status.PodIP, masterIP, port, password); err != nil { | ||
| r.logger.WithField("redisfailover", rf.ObjectMeta.Name).WithField("namespace", rf.ObjectMeta.Namespace).Errorf("Make slave failed, slave ip: %s, master ip: %s, error: %v", pod.Status.PodIP, masterIP, err) | ||
| if err := r.redisClient.MakeSlaveOfWithPort(podAddress, masterIP, port, password); err != nil { |
There was a problem hiding this comment.
we can use the getPodAddress function only where the dns is required otherwise it will be too much of unnecessary changes. @samof76 any thoughts?
Overview
This PR contains changes made to add headless service support to the Redis operator, allowing Redis replicas to be configured using DNS names instead of PodIPs.
Reason for Change
Pod IP Instability: When Redis replicas are configured using PodIPs, pod restarts result in new IP addresses. This requires reconfiguration of all replicas pointing to the restarted pod, leading to:
StatefulSet DNS Naming: Kubernetes StatefulSets provide stable DNS names for each pod in the format
<pod-name>.<service-name>.<namespace>.svc.cluster.local. These DNS names remain constant across pod restarts, providing a stable network identity that aligns with Kubernetes best practices for stateful workloads.Network Identity Persistence: Using DNS names instead of IPs ensures that:
Feature Description
When
headless: trueis enabled in the RedisFailover spec, the operator:<statefulset-name>-<ordinal-number>.<service-name>.<namespace>.svc.cluster.localto configure Redis replicas