[Feature][GCS FT] Redis cleanup improvement

[kevin85421]

Search before asking

• I had searched in the issues and found no similar feature requirement.

Description

• Issue 1: https://ray-distributed.slack.com/archives/C02GFQ82JPM/p1697556705340069

  • [Bug][RayCluster] Fix RAY_REDIS_ADDRESS parsing with redis scheme and… #1556

• Issue 2: Enhance observability. For instance, display the link to the KubeRay GCS FT doc on the Ray website in the K8s Job. In addition, we can also consider create a K8s event when the K8s Job fails to clean up the Redis key.

• Issue 3: Currently, if the K8s Job fails to clean up the Redis key, KubeRay will not remove the RayCluster's finalizer. As a result, users must remove the finalizer manually. This approach is somewhat aggressive, and we might consider changing the behavior to be best-effort. For instance, if the K8s Job fails to clean up the Redis key within 5 minutes, KubeRay could issue a K8s event for observability and then remove the finalizer.

  • [Feature][GCS FT] Best-effort redis cleanup job #1766

Use case

No response

Related issues

No response

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

[kevin85421]

@rueian @evalaiyc98 will take this issue.

[rueian]

In terms of issue 3, there are two job configurations we can leverage:

1. .spec.backoffLimit
2. .spec.activeDeadlineSeconds

When setting the .spec.backoffLimit to 5, the job will be terminated after 5+1 tries which takes about 310 seconds (due to the backoff) if no pod is stuck in the pending phase.

In contrast, when setting the .spec.activeDeadlineSeconds, the job will terminated after the deadline no matter what, even if the job pod is stuck in the pending phase.

I would recommend setting the .spec.activeDeadlineSeconds if we want to guarantee that the cleanup job will terminate.

Either way, after setting up one of these options, we can then detect if the job is terminated by checking its .status.Conditions here between L283 and L284:

kuberay/ray-operator/controllers/ray/raycluster_controller.go

Lines 283 to 284 in 856a33e

	}
	if redisCleanupJob.Status.Failed > 0 {

And then remove the finalizer. For example:

				for _, condition := range redisCleanupJob.Status.Conditions {
					if condition.Status == corev1.ConditionTrue && condition.Type == batchv1.JobFailed {
						controllerutil.RemoveFinalizer(instance, common.GCSFaultToleranceRedisCleanupFinalizer)
						if err := r.Update(ctx, instance); err != nil {
							return ctrl.Result{RequeueAfter: DefaultRequeueDuration}, err
						}
						return ctrl.Result{}, nil
					}
				}
				if redisCleanupJob.Status.Failed > 0 {
					r.Log.Info("If the Redis cleanup Job has failed, we will requeue the RayCluster CR after 1 minute.")
					return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil
				}

[kevin85421]

#1557 (comment)

This makes sense.

1. activeDeadlineSeconds exists in Kubernetes 1.19, so it will not cause any compatibility issues.
2. Maybe we can also consider using the utility function from Kubernetes if we do not need to import new packages.

[kevin85421]

I will take issue 3, and @evalaiyc98 will take issue 2.

[kevin85421]

I've reconsidered issue 3. I've decided not to adopt the best-effort strategy in v1.0.0. The reason is that the Redis cleanup feature isn't stable enough, and adopting this strategy might cause us to miss some opportunities to enhance the feature.

When we are confident that the Redis cleanup feature works well in most scenarios, we can adopt the best-effort strategy to enhance the user experience during certain edge cases, such as network connection issues.

In v1.0.0, I will minimize the UX friction if the Redis cleanup fails by (1) Minimizing the computing resources for the K8s Job and (2) Setting the backoffLimit to 0 to avoid some issues related to ResourceQuota.