From 7a1825cb01a1d786775250998b33a1ba90f7a077 Mon Sep 17 00:00:00 2001 From: Binh Nguyen Date: Fri, 24 Jul 2026 16:58:33 +0700 Subject: [PATCH] feat(heal): optionally protect the redis master from autoscaler eviction Add an opt-in redis.preventMasterEviction flag. When enabled, the operator annotates the current master pod with cluster-autoscaler.kubernetes.io/safe-to-evict=false and marks slaves true, so the cluster autoscaler will not drain the node running the master and force an avoidable failover. The annotation follows the master as the role moves, in both the checker and healer relabel paths. Adds a Pod.UpdatePodAnnotations service method (JSON merge patch) and the matching mock. Regenerated the CRD for the new field. Refs upstream spotahome/redis-operator#689. --- README.md | 8 +++ api/redisfailover/v1/types.go | 5 ++ ...atabases.spotahome.com_redisfailovers.yaml | 7 ++ ...atabases.spotahome.com_redisfailovers.yaml | 7 ++ ...atabases.spotahome.com_redisfailovers.yaml | 7 ++ mocks/service/k8s/Services.go | 14 ++++ operator/redisfailover/service/check.go | 38 +++++++++-- operator/redisfailover/service/constants.go | 4 ++ operator/redisfailover/service/heal.go | 22 ++++--- .../master_eviction_annotation_test.go | 65 +++++++++++++++++++ service/k8s/pod.go | 22 +++++++ 11 files changed, 185 insertions(+), 14 deletions(-) create mode 100644 operator/redisfailover/service/master_eviction_annotation_test.go diff --git a/README.md b/README.md index 9c52cc4e1..7209eebf0 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ Redis Operator creates/configures/manages redis-failovers atop Kubernetes. - [Usage](#usage) - [Reducing update churn (`--enable-hash`)](#reducing-update-churn---enable-hash) - [Reconcile interval (`--resync-period`)](#reconcile-interval---resync-period) + - [Protect the master from cluster-autoscaler eviction](#protect-the-master-from-cluster-autoscaler-eviction) - [Sentinel update strategy and PodDisruptionBudget](#sentinel-update-strategy-and-poddisruptionbudget) - [Persistence](#persistence) - [NodeAffinity and Tolerations](#nodeaffinity-and-tolerations) @@ -176,6 +177,13 @@ interval so it can recover from missed events and correct drift. This defaults t tuned with `--resync-period` (any Go duration, e.g. `--resync-period=1m`). Larger values reduce API load at the cost of slower periodic healing; smaller values react faster but poll more often. +### Protect the master from cluster-autoscaler eviction + +Setting `redis.preventMasterEviction: true` makes the operator annotate the current master pod with +`cluster-autoscaler.kubernetes.io/safe-to-evict: "false"` (and mark slaves `"true"`), so the +cluster-autoscaler will not drain the node running the master and trigger an avoidable failover. The +annotation follows the master as it moves. Defaults to `false` (no annotation is managed). + ### Sentinel update strategy and PodDisruptionBudget The sentinel `Deployment` update strategy can be overridden via `sentinel.strategy` (e.g. to set diff --git a/api/redisfailover/v1/types.go b/api/redisfailover/v1/types.go index dd36c99a8..20a74131f 100644 --- a/api/redisfailover/v1/types.go +++ b/api/redisfailover/v1/types.go @@ -77,6 +77,11 @@ type RedisSettings struct { // PodDisruptionBudgetMinAvailable overrides the PodDisruptionBudget // minAvailable for the redis pods. Defaults to 2 (or 1 when replicas <= 2). PodDisruptionBudgetMinAvailable *intstr.IntOrString `json:"podDisruptionBudgetMinAvailable,omitempty"` + // PreventMasterEviction, when true, annotates the current master pod with + // cluster-autoscaler.kubernetes.io/safe-to-evict=false so the cluster + // autoscaler will not drain the node running the master. Slaves are marked + // evictable. Defaults to false. + PreventMasterEviction bool `json:"preventMasterEviction,omitempty"` } // SentinelSettings defines the specification of the sentinel cluster diff --git a/charts/redisoperator/crds/databases.spotahome.com_redisfailovers.yaml b/charts/redisoperator/crds/databases.spotahome.com_redisfailovers.yaml index 0d07a34c4..87e30ab9a 100644 --- a/charts/redisoperator/crds/databases.spotahome.com_redisfailovers.yaml +++ b/charts/redisoperator/crds/databases.spotahome.com_redisfailovers.yaml @@ -7325,6 +7325,13 @@ spec: port: format: int32 type: integer + preventMasterEviction: + description: |- + PreventMasterEviction, when true, annotates the current master pod with + cluster-autoscaler.kubernetes.io/safe-to-evict=false so the cluster + autoscaler will not drain the node running the master. Slaves are marked + evictable. Defaults to false. + type: boolean priorityClassName: type: string replicas: diff --git a/manifests/databases.spotahome.com_redisfailovers.yaml b/manifests/databases.spotahome.com_redisfailovers.yaml index 0d07a34c4..87e30ab9a 100644 --- a/manifests/databases.spotahome.com_redisfailovers.yaml +++ b/manifests/databases.spotahome.com_redisfailovers.yaml @@ -7325,6 +7325,13 @@ spec: port: format: int32 type: integer + preventMasterEviction: + description: |- + PreventMasterEviction, when true, annotates the current master pod with + cluster-autoscaler.kubernetes.io/safe-to-evict=false so the cluster + autoscaler will not drain the node running the master. Slaves are marked + evictable. Defaults to false. + type: boolean priorityClassName: type: string replicas: diff --git a/manifests/kustomize/base/databases.spotahome.com_redisfailovers.yaml b/manifests/kustomize/base/databases.spotahome.com_redisfailovers.yaml index 0d07a34c4..87e30ab9a 100644 --- a/manifests/kustomize/base/databases.spotahome.com_redisfailovers.yaml +++ b/manifests/kustomize/base/databases.spotahome.com_redisfailovers.yaml @@ -7325,6 +7325,13 @@ spec: port: format: int32 type: integer + preventMasterEviction: + description: |- + PreventMasterEviction, when true, annotates the current master pod with + cluster-autoscaler.kubernetes.io/safe-to-evict=false so the cluster + autoscaler will not drain the node running the master. Slaves are marked + evictable. Defaults to false. + type: boolean priorityClassName: type: string replicas: diff --git a/mocks/service/k8s/Services.go b/mocks/service/k8s/Services.go index 8c68a0d45..2297f229b 100644 --- a/mocks/service/k8s/Services.go +++ b/mocks/service/k8s/Services.go @@ -873,6 +873,20 @@ func (_m *Services) UpdatePodDisruptionBudget(namespace string, podDisruptionBud return r0 } +// UpdatePodAnnotations provides a mock function with given fields: namespace, podName, annotations +func (_m *Services) UpdatePodAnnotations(namespace string, podName string, annotations map[string]string) error { + ret := _m.Called(namespace, podName, annotations) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string, map[string]string) error); ok { + r0 = rf(namespace, podName, annotations) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // UpdatePodLabels provides a mock function with given fields: namespace, podName, labels func (_m *Services) UpdatePodLabels(namespace string, podName string, labels map[string]string) error { ret := _m.Called(namespace, podName, labels) diff --git a/operator/redisfailover/service/check.go b/operator/redisfailover/service/check.go index c28aebc6f..8c40b7519 100644 --- a/operator/redisfailover/service/check.go +++ b/operator/redisfailover/service/check.go @@ -84,22 +84,48 @@ func (r *RedisFailoverChecker) CheckSentinelNumber(rf *redisfailoverv1.RedisFail return nil } -func (r *RedisFailoverChecker) setMasterLabelIfNecessary(namespace string, pod corev1.Pod) error { +// applyMasterEvictionAnnotation keeps the cluster-autoscaler safe-to-evict +// annotation in sync with a pod's role, but only when the RedisFailover opts in +// via spec.redis.preventMasterEviction. The master is pinned (false) and slaves +// are marked evictable (true). It reads the desired state off the already-fetched +// pod object and skips the patch when the annotation is already correct, so it is +// safe to call on every reconcile without extra API writes. +func applyMasterEvictionAnnotation(k8sService k8s.Services, rf *redisfailoverv1.RedisFailover, pod corev1.Pod, isMaster bool) error { + if !rf.Spec.Redis.PreventMasterEviction { + return nil + } + desired := "true" + if isMaster { + desired = "false" + } + if pod.Annotations[masterSafeToEvictAnnotation] == desired { + return nil + } + return k8sService.UpdatePodAnnotations(rf.Namespace, pod.Name, map[string]string{masterSafeToEvictAnnotation: desired}) +} + +func (r *RedisFailoverChecker) setMasterLabelIfNecessary(rf *redisfailoverv1.RedisFailover, pod corev1.Pod) error { + if err := applyMasterEvictionAnnotation(r.k8sService, rf, pod, true); err != nil { + return err + } for labelKey, labelValue := range pod.Labels { if labelKey == redisRoleLabelKey && labelValue == redisRoleLabelMaster { return nil } } - return r.k8sService.UpdatePodLabels(namespace, pod.Name, generateRedisMasterRoleLabel()) + return r.k8sService.UpdatePodLabels(rf.Namespace, pod.Name, generateRedisMasterRoleLabel()) } -func (r *RedisFailoverChecker) setSlaveLabelIfNecessary(namespace string, pod corev1.Pod) error { +func (r *RedisFailoverChecker) setSlaveLabelIfNecessary(rf *redisfailoverv1.RedisFailover, pod corev1.Pod) error { + if err := applyMasterEvictionAnnotation(r.k8sService, rf, pod, false); err != nil { + return err + } for labelKey, labelValue := range pod.Labels { if labelKey == redisRoleLabelKey && labelValue == redisRoleLabelSlave { return nil } } - return r.k8sService.UpdatePodLabels(namespace, pod.Name, generateRedisSlaveRoleLabel()) + return r.k8sService.UpdatePodLabels(rf.Namespace, pod.Name, generateRedisSlaveRoleLabel()) } // CheckAllSlavesFromMaster controlls that all slaves have the same master (the real one) @@ -123,12 +149,12 @@ func (r *RedisFailoverChecker) CheckAllSlavesFromMaster(master string, rf *redis var wrongMasterErr error for _, rp := range rps.Items { if rp.Status.PodIP == master { - err = r.setMasterLabelIfNecessary(rf.Namespace, rp) + err = r.setMasterLabelIfNecessary(rf, rp) if err != nil { return err } } else { - err = r.setSlaveLabelIfNecessary(rf.Namespace, rp) + err = r.setSlaveLabelIfNecessary(rf, rp) if err != nil { return err } diff --git a/operator/redisfailover/service/constants.go b/operator/redisfailover/service/constants.go index feb39ec6d..4f30a4a06 100644 --- a/operator/redisfailover/service/constants.go +++ b/operator/redisfailover/service/constants.go @@ -34,3 +34,7 @@ const ( redisRoleLabelMaster = "master" redisRoleLabelSlave = "slave" ) + +// masterSafeToEvictAnnotation is the cluster-autoscaler annotation used to keep +// the node running the redis master from being drained during scale-down. +const masterSafeToEvictAnnotation = "cluster-autoscaler.kubernetes.io/safe-to-evict" diff --git a/operator/redisfailover/service/heal.go b/operator/redisfailover/service/heal.go index c5baa4757..b47f52543 100644 --- a/operator/redisfailover/service/heal.go +++ b/operator/redisfailover/service/heal.go @@ -43,22 +43,28 @@ func NewRedisFailoverHealer(k8sService k8s.Services, redisClient redis.Client, l } } -func (r *RedisFailoverHealer) setMasterLabelIfNecessary(namespace string, pod v1.Pod) error { +func (r *RedisFailoverHealer) setMasterLabelIfNecessary(rf *redisfailoverv1.RedisFailover, pod v1.Pod) error { + if err := applyMasterEvictionAnnotation(r.k8sService, rf, pod, true); err != nil { + return err + } for labelKey, labelValue := range pod.Labels { if labelKey == redisRoleLabelKey && labelValue == redisRoleLabelMaster { return nil } } - return r.k8sService.UpdatePodLabels(namespace, pod.Name, generateRedisMasterRoleLabel()) + return r.k8sService.UpdatePodLabels(rf.Namespace, pod.Name, generateRedisMasterRoleLabel()) } -func (r *RedisFailoverHealer) setSlaveLabelIfNecessary(namespace string, pod v1.Pod) error { +func (r *RedisFailoverHealer) setSlaveLabelIfNecessary(rf *redisfailoverv1.RedisFailover, pod v1.Pod) error { + if err := applyMasterEvictionAnnotation(r.k8sService, rf, pod, false); err != nil { + return err + } for labelKey, labelValue := range pod.Labels { if labelKey == redisRoleLabelKey && labelValue == redisRoleLabelSlave { return nil } } - return r.k8sService.UpdatePodLabels(namespace, pod.Name, generateRedisSlaveRoleLabel()) + return r.k8sService.UpdatePodLabels(rf.Namespace, pod.Name, generateRedisSlaveRoleLabel()) } func (r *RedisFailoverHealer) MakeMaster(ip string, rf *redisfailoverv1.RedisFailover) error { @@ -79,7 +85,7 @@ func (r *RedisFailoverHealer) MakeMaster(ip string, rf *redisfailoverv1.RedisFai } for _, rp := range rps.Items { if rp.Status.PodIP == ip { - return r.setMasterLabelIfNecessary(rf.Namespace, rp) + return r.setMasterLabelIfNecessary(rf, rp) } } return nil @@ -117,7 +123,7 @@ func (r *RedisFailoverHealer) SetOldestAsMaster(rf *redisfailoverv1.RedisFailove continue } - err = r.setMasterLabelIfNecessary(rf.Namespace, pod) + err = r.setMasterLabelIfNecessary(rf, pod) if err != nil { return err } @@ -129,7 +135,7 @@ func (r *RedisFailoverHealer) SetOldestAsMaster(rf *redisfailoverv1.RedisFailove r.logger.WithField("redisfailover", rf.ObjectMeta.Name).WithField("namespace", rf.ObjectMeta.Namespace).Errorf("Make slave failed, slave pod ip: %s, master ip: %s, error: %v", pod.Status.PodIP, newMasterIP, err) } - err = r.setSlaveLabelIfNecessary(rf.Namespace, pod) + err = r.setSlaveLabelIfNecessary(rf, pod) if err != nil { return err } @@ -174,7 +180,7 @@ func (r *RedisFailoverHealer) SetMasterOnAll(masterIP string, rf *redisfailoverv continue } - err = r.setSlaveLabelIfNecessary(rf.Namespace, pod) + err = r.setSlaveLabelIfNecessary(rf, pod) if err != nil { return err } diff --git a/operator/redisfailover/service/master_eviction_annotation_test.go b/operator/redisfailover/service/master_eviction_annotation_test.go new file mode 100644 index 000000000..0c9e22cd4 --- /dev/null +++ b/operator/redisfailover/service/master_eviction_annotation_test.go @@ -0,0 +1,65 @@ +package service + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + redisfailoverv1 "github.com/dnse-tech/redis-operator/api/redisfailover/v1" + mK8SService "github.com/dnse-tech/redis-operator/mocks/service/k8s" +) + +func rfWithEvictionProtection(enabled bool) *redisfailoverv1.RedisFailover { + return &redisfailoverv1.RedisFailover{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "testns"}, + Spec: redisfailoverv1.RedisFailoverSpec{ + Redis: redisfailoverv1.RedisSettings{PreventMasterEviction: enabled}, + }, + } +} + +func podNamed(name string, annotations map[string]string) corev1.Pod { + return corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Annotations: annotations}} +} + +func TestApplyMasterEvictionAnnotation(t *testing.T) { + t.Run("flag disabled makes no annotation call", func(t *testing.T) { + ms := &mK8SService.Services{} + err := applyMasterEvictionAnnotation(ms, rfWithEvictionProtection(false), podNamed("p0", nil), true) + assert.NoError(t, err) + ms.AssertNotCalled(t, "UpdatePodAnnotations", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("master is pinned with safe-to-evict false", func(t *testing.T) { + ms := &mK8SService.Services{} + ms.On("UpdatePodAnnotations", "testns", "p0", map[string]string{ + masterSafeToEvictAnnotation: "false", + }).Once().Return(nil) + + err := applyMasterEvictionAnnotation(ms, rfWithEvictionProtection(true), podNamed("p0", nil), true) + assert.NoError(t, err) + ms.AssertExpectations(t) + }) + + t.Run("slave is marked evictable with safe-to-evict true", func(t *testing.T) { + ms := &mK8SService.Services{} + ms.On("UpdatePodAnnotations", "testns", "p1", map[string]string{ + masterSafeToEvictAnnotation: "true", + }).Once().Return(nil) + + err := applyMasterEvictionAnnotation(ms, rfWithEvictionProtection(true), podNamed("p1", nil), false) + assert.NoError(t, err) + ms.AssertExpectations(t) + }) + + t.Run("no call when annotation already at desired value", func(t *testing.T) { + ms := &mK8SService.Services{} + pod := podNamed("p0", map[string]string{masterSafeToEvictAnnotation: "false"}) + err := applyMasterEvictionAnnotation(ms, rfWithEvictionProtection(true), pod, true) + assert.NoError(t, err) + ms.AssertNotCalled(t, "UpdatePodAnnotations", mock.Anything, mock.Anything, mock.Anything) + }) +} diff --git a/service/k8s/pod.go b/service/k8s/pod.go index 50a4cee84..22fbaae0c 100644 --- a/service/k8s/pod.go +++ b/service/k8s/pod.go @@ -24,6 +24,7 @@ type Pod interface { DeletePod(namespace string, name string) error ListPods(namespace string) (*corev1.PodList, error) UpdatePodLabels(namespace, podName string, labels map[string]string) error + UpdatePodAnnotations(namespace, podName string, annotations map[string]string) error } // PodService is the pod service implementation using API calls to kubernetes. @@ -132,3 +133,24 @@ func (p *PodService) UpdatePodLabels(namespace, podName string, labels map[strin } return err } + +// UpdatePodAnnotations sets the given annotations on a pod. It uses a JSON merge +// patch so the annotations map is created when absent and existing annotations +// are left untouched, unlike the JSON-patch "replace" used for labels. +func (p *PodService) UpdatePodAnnotations(namespace, podName string, annotations map[string]string) error { + p.logger.Infof("Update pod annotations, namespace: %s, pod name: %s, annotations: %v", namespace, podName, annotations) + + patch := map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": annotations, + }, + } + payloadBytes, _ := json.Marshal(patch) + + _, err := p.kubeClient.CoreV1().Pods(namespace).Patch(context.TODO(), podName, types.MergePatchType, payloadBytes, metav1.PatchOptions{}) + recordMetrics(namespace, "Pod", podName, "PATCH", err, p.metricsRecorder) + if err != nil { + p.logger.Errorf("Update pod annotations failed, namespace: %s, pod name: %s, error: %v", namespace, podName, err) + } + return err +}