feat: enhance RedisReplication controller and CRD with additional sta…

…tus columns and refactor reconciliation logic - Added new printer columns "Master" and "Age" to the RedisReplication CRD for better visibility of the master node and resource age. - Refactored the reconciliation logic in the RedisReplication controller to improve clarity and maintainability by introducing a reconciler struct for handling different reconciliation tasks. - Updated the e2e tests to validate the HA setup of Redis Replication and Sentinel, ensuring consistency in master IP across different sources. - Removed obsolete test files and replaced them with a new HA setup configuration. This update improves the usability and reliability of the Redis replication feature. Signed-off-by: drivebyer <[email protected]>
OT-CONTAINER-KIT · Dec 10, 2024 · 25aab8a · 25aab8a
1 parent e9ec33f
commit 25aab8a
Show file tree

Hide file tree

Showing 8 changed files with 174 additions and 101 deletions.
diff --git a/api/v1beta2/redisreplication_types.go b/api/v1beta2/redisreplication_types.go
@@ -41,6 +41,8 @@ type RedisReplicationStatus struct {
 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
+// +kubebuilder:printcolumn:name="Master",type="string",JSONPath=".status.masterNode"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
 
 // Redis is the Schema for the redis API
 type RedisReplication struct {

diff --git a/config/crd/bases/redis.redis.opstreelabs.in_redisreplications.yaml b/config/crd/bases/redis.redis.opstreelabs.in_redisreplications.yaml
@@ -4397,7 +4397,14 @@ spec:
     storage: false
     subresources:
       status: {}
-  - name: v1beta2
+  - additionalPrinterColumns:
+    - jsonPath: .status.masterNode
+      name: Master
+      type: string
+    - jsonPath: .metadata.creationTimestamp
+      name: Age
+      type: date
+    name: v1beta2
     schema:
       openAPIV3Schema:
         description: Redis is the Schema for the redis API

diff --git a/pkg/controllers/redisreplication/redisreplication_controller.go b/pkg/controllers/redisreplication/redisreplication_controller.go
@@ -28,64 +28,51 @@ type Reconciler struct {
 }
 
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx, "Request.Namespace", req.Namespace, "Request.Name", req.Name)
 	instance := &redisv1beta2.RedisReplication{}
 
-	err := r.Client.Get(context.TODO(), req.NamespacedName, instance)
+	err := r.Client.Get(ctx, req.NamespacedName, instance)
 	if err != nil {
-		return intctrlutil.RequeueWithErrorChecking(ctx, err, "")
-	}
-	if instance.ObjectMeta.GetDeletionTimestamp() != nil {
-		if err = k8sutils.HandleRedisReplicationFinalizer(ctx, r.Client, r.K8sClient, instance); err != nil {
-			return intctrlutil.RequeueWithError(ctx, err, "")
-		}
-		return intctrlutil.Reconciled()
-	}
-	if _, found := instance.ObjectMeta.GetAnnotations()["redisreplication.opstreelabs.in/skip-reconcile"]; found {
-		return intctrlutil.RequeueAfter(ctx, time.Second*10, "found skip reconcile annotation")
-	}
-	if err = k8sutils.AddFinalizer(ctx, instance, k8sutils.RedisReplicationFinalizer, r.Client); err != nil {
-		return intctrlutil.RequeueWithError(ctx, err, "")
-	}
-	err = k8sutils.CreateReplicationRedis(ctx, instance, r.K8sClient)
-	if err != nil {
-		return intctrlutil.RequeueWithError(ctx, err, "")
-	}
-	err = k8sutils.CreateReplicationService(ctx, instance, r.K8sClient)
-	if err != nil {
-		return intctrlutil.RequeueWithError(ctx, err, "")
-	}
-	if !r.IsStatefulSetReady(ctx, instance.Namespace, instance.Name) {
-		return intctrlutil.Reconciled()
+		return intctrlutil.RequeueWithErrorChecking(ctx, err, "failed to get RedisReplication instance")
 	}
 
-	var realMaster string
-	masterNodes := k8sutils.GetRedisNodesByRole(ctx, r.K8sClient, instance, "master")
-	if len(masterNodes) > 1 {
-		logger.Info("Creating redis replication by executing replication creation commands")
-		slaveNodes := k8sutils.GetRedisNodesByRole(ctx, r.K8sClient, instance, "slave")
-		realMaster = k8sutils.GetRedisReplicationRealMaster(ctx, r.K8sClient, instance, masterNodes)
-		if len(slaveNodes) == 0 {
-			realMaster = masterNodes[0]
+	var reconcilers []reconciler
+	if k8sutils.IsDeleted(instance) {
+		reconcilers = []reconciler{
+			{typ: "finalizer", rec: r.reconcileFinalizer},
 		}
-		if err = k8sutils.CreateMasterSlaveReplication(ctx, r.K8sClient, instance, masterNodes, realMaster); err != nil {
-			return intctrlutil.RequeueAfter(ctx, time.Second*60, "")
+	} else {
+		reconcilers = []reconciler{
+			{typ: "annotation", rec: r.reconcileAnnotation},
+			{typ: "statefulset", rec: r.reconcileStatefulSet},
+			{typ: "service", rec: r.reconcileService},
+			{typ: "redis", rec: r.reconcileRedis},
+			{typ: "status", rec: r.reconcileStatus},
 		}
 	}
-	realMaster = k8sutils.GetRedisReplicationRealMaster(ctx, r.K8sClient, instance, masterNodes)
-	if err = r.UpdateRedisReplicationMaster(ctx, instance, realMaster); err != nil {
-		return intctrlutil.RequeueWithError(ctx, err, "")
-	}
-	if err = r.UpdateRedisPodRoleLabel(ctx, instance, realMaster); err != nil {
-		return intctrlutil.RequeueWithError(ctx, err, "")
+	for _, reconciler := range reconcilers {
+		result, err := reconciler.rec(ctx, instance)
+		if err != nil {
+			return intctrlutil.RequeueWithError(ctx, err, "")
+		}
+		if result.Requeue {
+			return result, nil
+		}
 	}
+
 	return intctrlutil.RequeueAfter(ctx, time.Second*10, "")
 }
 
 func (r *Reconciler) UpdateRedisReplicationMaster(ctx context.Context, instance *redisv1beta2.RedisReplication, masterNode string) error {
 	if instance.Status.MasterNode == masterNode {
 		return nil
 	}
+
+	if instance.Status.MasterNode != masterNode {
+		logger := log.FromContext(ctx)
+		logger.Info("Updating master node",
+			"previous", instance.Status.MasterNode,
+			"new", masterNode)
+	}
 	instance.Status.MasterNode = masterNode
 	if err := r.Client.Status().Update(ctx, instance); err != nil {
 		return err
@@ -118,6 +105,89 @@ func (r *Reconciler) UpdateRedisPodRoleLabel(ctx context.Context, cr *redisv1bet
 	return nil
 }
 
+type reconciler struct {
+	typ string
+	rec func(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error)
+}
+
+func (r *Reconciler) reconcileFinalizer(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	if k8sutils.IsDeleted(instance) {
+		if err := k8sutils.HandleRedisReplicationFinalizer(ctx, r.Client, r.K8sClient, instance); err != nil {
+			return intctrlutil.RequeueWithError(ctx, err, "")
+		}
+		return intctrlutil.Reconciled()
+	}
+	if err := k8sutils.AddFinalizer(ctx, instance, k8sutils.RedisReplicationFinalizer, r.Client); err != nil {
+		return intctrlutil.RequeueWithError(ctx, err, "")
+	}
+	return intctrlutil.Reconciled()
+}
+
+func (r *Reconciler) reconcileAnnotation(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	if _, found := instance.ObjectMeta.GetAnnotations()["redisreplication.opstreelabs.in/skip-reconcile"]; found {
+		return intctrlutil.RequeueAfter(ctx, time.Second*10, "found skip reconcile annotation")
+	}
+	return intctrlutil.Reconciled()
+}
+
+func (r *Reconciler) reconcileStatefulSet(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	if err := k8sutils.CreateReplicationRedis(ctx, instance, r.K8sClient); err != nil {
+		return intctrlutil.RequeueAfter(ctx, time.Second*60, "")
+	}
+	return intctrlutil.Reconciled()
+}
+
+func (r *Reconciler) reconcileService(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	if err := k8sutils.CreateReplicationService(ctx, instance, r.K8sClient); err != nil {
+		return intctrlutil.RequeueAfter(ctx, time.Second*60, "")
+	}
+	return intctrlutil.Reconciled()
+}
+
+func (r *Reconciler) reconcileRedis(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+	defer cancel()
+	logger := log.FromContext(ctx)
+
+	if !r.IsStatefulSetReady(ctx, instance.Namespace, instance.Name) {
+		logger.Info("StatefulSet not ready yet, requeuing",
+			"namespace", instance.Namespace,
+			"name", instance.Name)
+		return intctrlutil.RequeueAfter(ctx, time.Second*60, "")
+	}
+
+	var realMaster string
+	masterNodes := k8sutils.GetRedisNodesByRole(ctx, r.K8sClient, instance, "master")
+	if len(masterNodes) > 1 {
+		log.FromContext(ctx).Info("Creating redis replication by executing replication creation commands")
+		slaveNodes := k8sutils.GetRedisNodesByRole(ctx, r.K8sClient, instance, "slave")
+		realMaster = k8sutils.GetRedisReplicationRealMaster(ctx, r.K8sClient, instance, masterNodes)
+		if len(slaveNodes) == 0 {
+			realMaster = masterNodes[0]
+		}
+		if err := k8sutils.CreateMasterSlaveReplication(ctx, r.K8sClient, instance, masterNodes, realMaster); err != nil {
+			return intctrlutil.RequeueAfter(ctx, time.Second*60, "")
+		}
+	}
+	return intctrlutil.Reconciled()
+}
+
+// reconcileStatus update status and label.
+func (r *Reconciler) reconcileStatus(ctx context.Context, instance *redisv1beta2.RedisReplication) (ctrl.Result, error) {
+	var err error
+	var realMaster string
+
+	masterNodes := k8sutils.GetRedisNodesByRole(ctx, r.K8sClient, instance, "master")
+	realMaster = k8sutils.GetRedisReplicationRealMaster(ctx, r.K8sClient, instance, masterNodes)
+	if err = r.UpdateRedisReplicationMaster(ctx, instance, realMaster); err != nil {
+		return intctrlutil.RequeueWithError(ctx, err, "")
+	}
+	if err = r.UpdateRedisPodRoleLabel(ctx, instance, realMaster); err != nil {
+		return intctrlutil.RequeueWithError(ctx, err, "")
+	}
+	return intctrlutil.Reconciled()
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).

diff --git a/pkg/k8sutils/kube.go b/pkg/k8sutils/kube.go
@@ -0,0 +1,9 @@
+package k8sutils
+
+import (
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func IsDeleted(obj client.Object) bool {
+	return obj.GetDeletionTimestamp() != nil
+}
diff --git a/tests/e2e-chainsaw/v1beta2/setup/ha/chainsaw-test.yaml b/tests/e2e-chainsaw/v1beta2/setup/ha/chainsaw-test.yaml
@@ -1,3 +1,11 @@
+# This case is to test the HA setup of the Redis Replication and Sentinel
+# It will create a Redis Replication and Sentinel, then terminate the Redis Replication master pod
+# and check if the Sentinel can promote a new master pod.
+# 
+# It check three place the same pod IP:
+#   1. Status from RedisReplication
+#   2. Label from RedisReplication
+#   3. get-master-addr-by-name from Sentinel
 ---
 apiVersion: chainsaw.kyverno.io/v1alpha1
 kind: Test
@@ -7,25 +15,19 @@ spec:
   steps:
     - try:
         - apply:
-            file: replication.yaml
-        - apply:
-            file: sentinel.yaml
-        - create:
-            file: cli-pod.yaml
+            file: ha.yaml
 
-    - name: Sleep for 3 minutes
+    - name: Test Master IP consistency
       try:
         - sleep:
-            duration: 3m
-
-    - name: Test sentinel monitoring
-      try:
+            duration: 180s
         - script:
             timeout: 10s
             content: |
-              export MASTER_IP_FROM_SENTINEL=$(kubectl exec --namespace ${NAMESPACE} redis-sentinel-sentinel-0 -- redis-cli -p 26379 sentinel get-master-addr-by-name myMaster | head -n 1);
+              export MASTER_IP_FROM_STATUS=$(kubectl -n ${NAMESPACE} get pod $(kubectl -n ${NAMESPACE} get redisreplication redis-replication -o jsonpath='{.status.masterNode}') -o jsonpath='{.status.podIP}');
+              export MASTER_IP_FROM_SENTINEL=$(kubectl -n ${NAMESPACE} exec redis-sentinel-sentinel-0 -- redis-cli -p 26379 sentinel get-master-addr-by-name myMaster | head -n 1);
               export MASTER_IP_FROM_LABEL=$(kubectl -n ${NAMESPACE} get pod -l app=redis-replication,redis-role=master,redis_setup_type=replication -o jsonpath='{.items[0].status.podIP}');
-              if [ "$MASTER_IP_FROM_SENTINEL" = "$MASTER_IP_FROM_LABEL" ]; then echo "OK"; else echo "FAIL"; fi
+              if [ "$MASTER_IP_FROM_SENTINEL" = "$MASTER_IP_FROM_LABEL" ] && [ "$MASTER_IP_FROM_SENTINEL" = "$MASTER_IP_FROM_STATUS" ]; then echo "OK"; else echo "FAIL"; fi
             check:
               (contains($stdout, 'OK')): true
 
@@ -35,20 +37,18 @@ spec:
         - script:
             timeout: 10s
             content: |
-              kubectl --namespace ${NAMESPACE} delete pod redis-replication-0
-
-    - name: Sleep for 5 minutes
-      try:
+              kubectl -n ${NAMESPACE} delete pod redis-replication-0
         - sleep:
-            duration: 5m
+            duration: 30s
 
-    - name: Test sentinel monitoring
+    - name: Test Master IP consistency
       try:
         - script:
             timeout: 10s
             content: |
-              export MASTER_IP_FROM_SENTINEL=$(kubectl exec --namespace ${NAMESPACE} redis-sentinel-sentinel-0 -- redis-cli -p 26379 sentinel get-master-addr-by-name myMaster | head -n 1);
+              export MASTER_IP_FROM_STATUS=$(kubectl -n ${NAMESPACE} get pod $(kubectl -n ${NAMESPACE} get redisreplication redis-replication -o jsonpath='{.status.masterNode}') -o jsonpath='{.status.podIP}');
+              export MASTER_IP_FROM_SENTINEL=$(kubectl -n ${NAMESPACE} exec redis-sentinel-sentinel-0 -- redis-cli -p 26379 sentinel get-master-addr-by-name myMaster | head -n 1);
               export MASTER_IP_FROM_LABEL=$(kubectl -n ${NAMESPACE} get pod -l app=redis-replication,redis-role=master,redis_setup_type=replication -o jsonpath='{.items[0].status.podIP}');
-              if [ $MASTER_IP_FROM_SENTINEL = $MASTER_IP_FROM_LABEL ]; then echo "OK"; else echo "FAIL"; fi
+              if [ "$MASTER_IP_FROM_SENTINEL" = "$MASTER_IP_FROM_LABEL" ] && [ "$MASTER_IP_FROM_SENTINEL" = "$MASTER_IP_FROM_STATUS" ]; then echo "OK"; else echo "FAIL"; fi
             check:
               (contains($stdout, 'OK')): true
diff --git a/tests/e2e-chainsaw/v1beta2/setup/ha/cli-pod.yaml b/tests/e2e-chainsaw/v1beta2/setup/ha/cli-pod.yaml
diff --git a/...hainsaw/v1beta2/setup/ha/replication.yaml → tests/e2e-chainsaw/v1beta2/setup/ha/ha.yaml b/...hainsaw/v1beta2/setup/ha/replication.yaml → tests/e2e-chainsaw/v1beta2/setup/ha/ha.yaml
@@ -25,3 +25,26 @@ spec:
         resources:
           requests:
             storage: 1Gi
+---
+apiVersion: redis.redis.opstreelabs.in/v1beta2
+kind: RedisSentinel
+metadata:
+  name: redis-sentinel
+spec:
+  clusterSize: 1
+  podSecurityContext:
+    runAsUser: 1000
+    fsGroup: 1000
+  redisSentinelConfig:
+    redisReplicationName: redis-replication
+    quorum: '1'
+  kubernetesConfig:
+    image: quay.io/opstree/redis-sentinel:latest
+    imagePullPolicy: Always
+    resources:
+      requests:
+        cpu: 101m
+        memory: 128Mi
+      limits:
+        cpu: 101m
+        memory: 128Mi
diff --git a/tests/e2e-chainsaw/v1beta2/setup/ha/sentinel.yaml b/tests/e2e-chainsaw/v1beta2/setup/ha/sentinel.yaml