[Bug][GCS FT] Worker pods crash unexpectedly when gcs_server on head pod is killed

[kevin85421]

Why are these changes needed?

Major changes

When we kill the GCS server process (pkill gcs_server) on the head Pod, a Raylet (?) will exit the process if it cannot connect to the GCS server after RAY_gcs_rpc_server_reconnect_timeout_s (code). By default, the value is 60s.

In #634, the RAY_gcs_rpc_server_reconnect_timeout_s values for both head and workers are set to 60 seconds. This means that the head and workers will crash almost simultaneously, 60 seconds after the GCS server process is terminated. Therefore, we need to ensure that the RAY_gcs_rpc_server_reconnect_timeout_s for the workers is greater than the time it takes for a new GCS server process to become available after the old one is terminated. That is,

WORKER_RECONNECTION_TIMEOUT > HEAD_RECONNECTION_TIMEOUT + restart the head Pod + new GCS server process become ready

This PR injects RAY_gcs_rpc_server_reconnect_timeout_s env variable to workers with default value 600s if GCS FT is enabled. Hence, head will crash 60 seconds after the process is terminated, and the new GCS server process will be ready before 600 seconds. Hence, the workers will not crash.

Minor changes

• Remove rayCluster label: The label has been removed by Revise sample configs, increase memory requests, update Ray versions #761 from sample YAML files. This PR removes the label from unit tests.

• compatibility-test.py: This is just for refactoring.

• ray-cluster.ray-ft.yaml.template: Set RAY_gcs_rpc_server_reconnect_timeout_s to make the compatibility test become faster.

Related issue number

Closes #634

Checks

• I've made sure the tests are passing.
• Testing Strategy
  • Unit tests
  • Manual tests
  • This PR is not tested :(

# Build the KubeRay image (controller:latest)
helm install kuberay-operator kuberay/kuberay-operator --version 0.5.0 --set image.repository=controller,image.tag=latest

# Create a RayCluster with GCS FT
kubectl apply -f ray-cluster.external-redis.yaml

# This env var should not be set on head Pod by default
kubectl describe pod $YOUR_HEAD_POD | grep RAY_gcs_rpc_server_reconnect_timeout_s

# The env should be injected to worker Pods by default.
kubectl describe pod $YOUR_WORKER_POD | grep RAY_gcs_rpc_server_reconnect_timeout_s
# RAY_gcs_rpc_server_reconnect_timeout_s:  600

# Kill the GCS server process on the head Pod
kubectl exec -it $YOUR_HEAD_POD -- pkill gcs_server

# Expected behavior: head Pod will crash after 60 seconds, and workers will not crash.

[architkulkarni]

@iycheng do you mind just checking the PR description and confirming that the solution makes sense from the Ray side?

[architkulkarni]

Looks good to me

[architkulkarni]

Here, or somewhere else in the code, can we add a brief comment explaining why we picked this value? It's explained well in the PR description but it's helpful to have it in a code comment

[kevin85421]

Added comment in d0991ff

[architkulkarni]

Do we intend to check this value somewhere? I didn't see it in the test

[kevin85421]

This is used to accelerate the test. Some tests in compatibility-test.py will kill the GCS server process on the head Pod and wait until the new one is available. By default, the head will crash after 60 seconds. With this configuration, it will crash after 20 seconds.

[architkulkarni]

Oh right. This is another place where a code comment would be helpful

[kevin85421]

Added comment in d0991ff

[gvspraveen]

doesnt this break if envVars slice is nil? Previously len(envVars) == 0 protected from that if i am not wrong.

[kevin85421]

If the slice is empty, it will not enter the for loop.

// You can edit this code!
// Click here and start typing.
package main

import "fmt"

func envVarExists(fruitName string, fruits []string) bool {
	for _, fruit := range fruits {
                fmt.Println("Enter the loop")
		if fruitName == fruit {
			return true
		}
	}
	return false
}

func main() {
	fruits := []string{"apple", "banana", "cherry", "date"}
	fmt.Println(envVarExists("banana", fruits))     // true
	fmt.Println(envVarExists("aaaaaa", fruits))     // false
	fmt.Println(envVarExists("aaaaaa", []string{})) // false
}

# STDOUT
Enter the loop
Enter the loop
true
Enter the loop
Enter the loop
Enter the loop
Enter the loop
false
false

[kevin85421]

You can try this on https://go.dev/play/

[kevin85421]

@architkulkarni would you mind taking a look at this Ci failure (https://github.com/ray-project/kuberay/actions/runs/4758810193/jobs/8457448052?pr=1036)? Thanks! This seems to be my second time to see the error message. Thanks!

[kevin85421]

@wilsonwang371 @iycheng would you mind taking a look at this PR? You only need to take a look at pod.go and pod_test.go. It may take 10 mins to review. Thanks!

[wilsonwang371]

why do we need to set it 600? Will 120 or similar values work?

[kevin85421]

120 seconds works in my local Kind cluster, but it may not work in a production Kubernetes cluster. Users can easily change the value by setting an environment variable. In addition, it is possible for some Pods on public cloud to experience network disconnections for several minutes. That's why I decided to select a higher timeout.

[kevin85421]

Gentle ping @wilsonwang371

Does this make sense to you?

[kevin85421]

Merge this PR because this blocks #1055. Feel free to add more comments on this PR, and I will open another PR to address the comments.