[RayCluster] Containers not ready status reflects structured reason

Signed-off-by: Spencer Peterson <spencerjp@google.com>

Author: spencer-p

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

@@ -37,6 +37,7 @@ const (
 	ServeName           = "serve"
 	ClusterDomainEnvKey = "CLUSTER_DOMAIN"
 	DefaultDomainName   = "cluster.local"
+	ContainersNotReady  = "ContainersNotReady"
 )
 
 // TODO (kevin85421): Define CRDType here rather than constant.go to avoid circular dependency.
@@ -91,16 +92,6 @@ func FindHeadPodReadyCondition(headPod *corev1.Pod) metav1.Condition {
 		headPodReadyCondition.Status = metav1.ConditionStatus(cond.Status)
 		headPodReadyCondition.Message = cond.Message
 
-		// Add details from failed or waiting container statuses if available.
-		details := containerStatusDetails(headPod)
-		if details != "" {
-			if headPodReadyCondition.Message == "" {
-				headPodReadyCondition.Message = details
-			} else {
-				headPodReadyCondition.Message += "; " + details
-			}
-		}
-
 		// Determine the reason; default to HeadPodRunningAndReady if the headPod is ready but no specific reason is provided
 		reason := cond.Reason
 		if cond.Status == corev1.ConditionTrue && reason == "" {
@@ -112,22 +103,34 @@ func FindHeadPodReadyCondition(headPod *corev1.Pod) metav1.Condition {
 			headPodReadyCondition.Reason = reason
 		}
 
+		// If reason is ContainersNotReady, then replace it with an available
+		// container status that may illuminate why the container is not ready.
+		if reason == ContainersNotReady {
+			reason, message, ok := firstNotReadyContainerStatus(headPod)
+			if ok {
+				if headPodReadyCondition.Message != "" {
+					headPodReadyCondition.Message += "; "
+				}
+				headPodReadyCondition.Message += message
+				headPodReadyCondition.Reason = reason
+			}
+		}
+
 		// Since we're only interested in the PodReady condition, break after processing it
 		break
 	}
 	return headPodReadyCondition
 }
 
-func containerStatusDetails(pod *corev1.Pod) string {
-	var details []string
+func firstNotReadyContainerStatus(pod *corev1.Pod) (reason string, message string, ok bool) {
 	for _, status := range pod.Status.ContainerStatuses {
 		if status.State.Waiting != nil {
-			details = append(details, fmt.Sprintf("%s: %s: %s", status.Name, status.State.Waiting.Reason, status.State.Waiting.Message))
+			return status.State.Waiting.Reason, status.Name + ": " + status.State.Waiting.Message, true
 		} else if status.State.Terminated != nil {
-			details = append(details, fmt.Sprintf("%s: %s: %s", status.Name, status.State.Terminated.Reason, status.State.Terminated.Message))
+			return status.State.Terminated.Reason, status.Name + ": " + status.State.Terminated.Message, true
 		}
 	}
-	return strings.Join(details, ", ")
+	return "", "", false
 }
 
 // FindRayClusterSuspendStatus returns the current suspend status from two conditions:

ray-operator/controllers/ray/utils/util_test.go

@@ -347,6 +347,7 @@ func createRayHeadPodWithPhaseAndCondition(phase corev1.PodPhase, status corev1.
 				{
 					Type:   corev1.PodReady,
 					Status: status,
+					Reason: ContainersNotReady,
 				},
 			},
 		},
@@ -979,16 +980,19 @@ func TestFindHeadPodReadyCondition(t *testing.T) {
 
 func TestFindHeadPodReadyMessage(t *testing.T) {
 	tests := []struct {
-		name            string
-		message         string
-		expectedMessage string
-		status          []corev1.ContainerStatus
+		name        string
+		message     string
+		wantMessage string
+		wantReason  string
+		status      []corev1.ContainerStatus
 	}{{
-		name: "no message no status want nothing",
+		name:       "no message no status want original reason",
+		wantReason: ContainersNotReady,
 	}, {
-		name:            "only reason",
-		message:         "TooEarlyInTheMorning",
-		expectedMessage: "TooEarlyInTheMorning",
+		name:        "no container status want original reason",
+		message:     "TooEarlyInTheMorning",
+		wantMessage: "TooEarlyInTheMorning",
+		wantReason:  ContainersNotReady,
 	}, {
 		name:    "one reason one status",
 		message: "containers not ready",
@@ -1001,9 +1005,10 @@ func TestFindHeadPodReadyMessage(t *testing.T) {
 				},
 			},
 		}},
-		expectedMessage: `containers not ready; ray: ImagePullBackOff: Back-off pulling image royproject/roy:latest: ErrImagePull: rpc error: code = NotFound`,
+		wantReason:  "ImagePullBackOff",
+		wantMessage: `containers not ready; ray: Back-off pulling image royproject/roy:latest: ErrImagePull: rpc error: code = NotFound`,
 	}, {
-		name:    "one reason two statuses",
+		name:    "one reason two statuses only copy first",
 		message: "aesthetic problems",
 		status: []corev1.ContainerStatus{{
 			Name: "indigo",
@@ -1022,7 +1027,8 @@ func TestFindHeadPodReadyMessage(t *testing.T) {
 				},
 			},
 		}},
-		expectedMessage: "aesthetic problems; indigo: BadColor: too blue, circle: BadGeometry: too round",
+		wantReason:  "BadColor",
+		wantMessage: "aesthetic problems; indigo: too blue",
 	}, {
 		name: "no reason one status",
 		status: []corev1.ContainerStatus{{
@@ -1034,7 +1040,8 @@ func TestFindHeadPodReadyMessage(t *testing.T) {
 				},
 			},
 		}},
-		expectedMessage: "my-image: Crashed: bash not found",
+		wantReason:  "Crashed",
+		wantMessage: "my-image: bash not found",
 	}}
 
 	for _, tc := range tests {
@@ -1043,8 +1050,11 @@ func TestFindHeadPodReadyMessage(t *testing.T) {
 			pod.Status.Conditions[0].Message = tc.message
 			pod.Status.ContainerStatuses = tc.status
 			cond := FindHeadPodReadyCondition(pod)
-			if cond.Message != tc.expectedMessage {
-				t.Errorf("FindHeadPodReadyCondition(...) returned condition with message %q, but wanted %q", cond.Message, tc.expectedMessage)
+			if cond.Message != tc.wantMessage {
+				t.Errorf("FindHeadPodReadyCondition(...) returned condition with message %q, but wanted %q", cond.Message, tc.wantMessage)
+			}
+			if cond.Reason != tc.wantReason {
+				t.Errorf("FindHeadPodReadyCondition(...) returned condition with reason %q, but wanted %q", cond.Reason, tc.wantReason)
 			}
 		})
 	}