[Bug] Environment variable to select Ray container is lower case sensitive.

[enozcan]

Search before asking

• I searched the issues and found no similar issues.

KubeRay Component

ray-operator

What happened + What you expected to happen

RAY=true environment variable to identify Ray container does not seem to be working properly due to the check here:

kuberay/ray-operator/controllers/ray/common/pod.go

Line 495 in 2019b4b

if env.Name == strings.ToLower("ray") && env.Value == strings.ToLower("true") {

where the check does not match with the command a few lines below:

kuberay/ray-operator/controllers/ray/common/pod.go

Line 496 in 2019b4b

log.Info("Head pod container with index " + strconv.Itoa(i) + " identified as Ray container based on env RAY=true.")

It actually checks for ray=true env variable in a case sensitive manner.

Reproduction script

--

Anything else

It must align with the logged statement for RAY=true format. Also not sure if that is still valid as I have not seen that within the docs but in the source.

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

[kevin85421]

Good catch! Would you mind opening a PR to fix it?

[Yicheng-Lu-llll]

In my understanding, It is better to still set the ray container to be the first container (containers[0]). There are segments of code that do not utilize the getRayContainerIndex method. For instance:

kuberay/ray-operator/controllers/ray/common/pod.go

Line 127 in 2019b4b

for _, port := range podTemplate.Spec.Containers[0].Ports {

kuberay/ray-operator/controllers/ray/common/pod.go

Line 139 in 2019b4b

podTemplate.Spec.Containers[0].Ports = append(podTemplate.Spec.Containers[0].Ports, metricsPort)

A thorough examination may need to fix this issue.

[enozcan]

Would you mind opening a PR to fix it?

@kevin85421 Sure, just let me know about the final decision, e.g. case sensitive or not, both for key and value.

It is better to still set the ray container to be the first container (containers[0])

@Yicheng-Lu-llll That is a strict restriction, and from user point of view having this env var option (and documenting it clearly) would be a better alternative.

[kevin85421]

• I noticed that there is a public function that is similar to getRayContainerIndex. Maybe we can consider removing getRayContainerIndex and using FindRayContainerIndex instead.

  kuberay/ray-operator/controllers/ray/utils/util.go

  Line 190 in b0649c4

  func FindRayContainerIndex(spec corev1.PodSpec) (index int) {

• What's the current behavior if Ray container is not the first one? Will it always fail?

[enozcan]

What's the current behavior if Ray container is not the first one? Will it always fail?

I believe it silently causes unexpected behavior. Say I define 2 containers, linkerd and main respectively. Operator will check for linkerd container as it’s the first one, override its command and args (via merging the original one with ray start) - since linkerd command does not have “ray start”. In that case the other container which is supposed to be Ray worker will be useless. Consider the case where linkerd container resources are relatively small compared to the main one, or pre-installed libraries do not exist in the linkerd but main container, etc. So I don’t think it always causes failure, but causes other problems that are going to be visible only when pod is inspected through describe, operator annotations, etc.

[kevin85421]

#1379 enforces the Ray container must be the first app container in a Pod.