fix: set GameServer to Unhealthy on container crash before Ready in sidecar mode

pkg/gameservers/health.go

@@ -292,23 +292,17 @@ func (hc *HealthController) syncGameServer(ctx context.Context, key string) erro
 // It does this by checking a combination of the current GameServer state and annotation data that stores
 // which container instance was live if the GameServer has been marked as Ready.
 // The logic is as follows:
-//   - If the GameServer is not yet Ready, allow to restart (return true)
+//   - If the GameServer is not yet Ready, allow to restart unless SidecarContainers is enabled
 //   - If the GameServer is in a state past Ready, move to Unhealthy
 func (hc *HealthController) skipUnhealthyGameContainer(gs *agonesv1.GameServer, pod *corev1.Pod) (bool, error) {
 	if !metav1.IsControlledBy(pod, gs) {
 		// This is not the Pod we are looking for 🤖
 		return false, nil
 	}
 
-	// if Sidecar is enabled, there is no skip once the GameServer is past Ready - it's just
-	// whatever K8s/Agones wants to do. Before Ready we still skip on a game container
-	// failure, matching the non-sidecar path below: a container that dies during startup is
-	// left to normal Kubernetes handling rather than immediately failing the GameServer.
+	// With SidecarContainers, failedContainer only reports failures that cannot restart.
 	// on move to stable, this function can be deleted.
 	if runtime.FeatureEnabled(runtime.FeatureSidecarContainers) {
-		if gs.IsBeforeReady() {
-			return hc.failedContainer(pod), nil
-		}
 		return false, nil
 	}
 

pkg/gameservers/health_test.go

@@ -322,10 +322,11 @@ func TestHealthControllerSyncGameServer(t *testing.T) {
 		updated bool
 	}
 	fixtures := map[string]struct {
-		state     agonesv1.GameServerState
-		podStatus *corev1.PodStatus
-		feature   string
-		expected  expected
+		state           agonesv1.GameServerState
+		podStatus       *corev1.PodStatus
+		feature         string
+		extraContainers []corev1.Container
+		expected        expected
 	}{
 		"started": {
 			state: agonesv1.GameServerStateStarting,
@@ -358,11 +359,22 @@ func TestHealthControllerSyncGameServer(t *testing.T) {
 			},
 		},
 		"container failed before ready": {
-			state: agonesv1.GameServerStateStarting,
+			state:   agonesv1.GameServerStateStarting,
+			feature: string(agruntime.FeatureSidecarContainers) + "=false",
 			podStatus: &corev1.PodStatus{ContainerStatuses: []corev1.ContainerStatus{
 				{Name: "container", State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}}}}},
 			expected: expected{updated: false},
 		},
+		"sidecar mode, game container failed before ready while Pod is running": {
+			state:           agonesv1.GameServerStateScheduled,
+			feature:         string(agruntime.FeatureSidecarContainers) + "=true",
+			extraContainers: []corev1.Container{{Name: "other-container", Image: "container/image"}},
+			podStatus: &corev1.PodStatus{Phase: corev1.PodRunning, ContainerStatuses: []corev1.ContainerStatus{
+				{Name: "container", State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 23}}},
+				{Name: "other-container", State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}},
+			}},
+			expected: expected{updated: true},
+		},
 		"container failed after ready": {
 			state:   agonesv1.GameServerStateAllocated,
 			feature: string(agruntime.FeatureSidecarContainers) + "=false",
@@ -410,6 +422,8 @@ func TestHealthControllerSyncGameServer(t *testing.T) {
 
 			gs := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "test"}, Spec: newSingleContainerSpec(),
 				Status: agonesv1.GameServerStatus{State: test.state}}
+			gs.Spec.Template.Spec.Containers = append(gs.Spec.Template.Spec.Containers, test.extraContainers...)
+			gs.Spec.Container = gs.Spec.Template.Spec.Containers[0].Name
 			gs.ApplyDefaults()
 
 			got := false

test/e2e/gameserver_test.go

@@ -441,6 +441,52 @@ func TestGameServerRestartBeforeReadyCrash(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestGameServerUnhealthyBeforeReadyCrash(t *testing.T) {
+	if !runtime.FeatureEnabled(runtime.FeatureSidecarContainers) {
+		t.SkipNow()
+	}
+
+	t.Parallel()
+	ctx := t.Context()
+	additionalContainerName := "additional"
+	gs := framework.DefaultGameServer(framework.Namespace)
+	gs.Spec.Template.Spec.Containers[0].Image = "alpine:latest"
+	gs.Spec.Template.Spec.Containers[0].Command = []string{"/bin/sh", "-c", "sleep 2; exit 23"}
+	gs.Spec.Template.Spec.Containers = append(gs.Spec.Template.Spec.Containers, corev1.Container{
+		Name:            additionalContainerName,
+		Image:           "registry.k8s.io/pause:3.10",
+		ImagePullPolicy: corev1.PullIfNotPresent,
+	})
+
+	gsClient := framework.AgonesClient.AgonesV1().GameServers(framework.Namespace)
+	gs, err := gsClient.Create(ctx, gs, metav1.CreateOptions{})
+	require.NoError(t, err)
+	defer gsClient.Delete(ctx, gs.Name, metav1.DeleteOptions{}) //nolint:errcheck
+
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		pod, err := framework.KubeClient.CoreV1().Pods(framework.Namespace).Get(ctx, gs.Name, metav1.GetOptions{})
+		require.NoError(c, err)
+		assert.Equal(c, corev1.RestartPolicyNever, pod.Spec.RestartPolicy)
+		assert.Equal(c, corev1.PodRunning, pod.Status.Phase)
+		var gameTerminated, additionalRunning bool
+		for _, status := range pod.Status.ContainerStatuses {
+			switch status.Name {
+			case gs.Spec.Container:
+				require.NotNil(c, status.State.Terminated)
+				assert.Equal(c, int32(23), status.State.Terminated.ExitCode)
+				gameTerminated = true
+			case additionalContainerName:
+				additionalRunning = status.State.Running != nil
+			}
+		}
+		assert.True(c, gameTerminated, "game container has not terminated")
+		assert.True(c, additionalRunning, "additional container is not running")
+	}, 3*time.Minute, time.Second)
+
+	_, err = framework.WaitForGameServerState(t, gs, agonesv1.GameServerStateUnhealthy, 3*time.Minute)
+	require.NoError(t, err)
+}
+
 func TestGameServerUnhealthyAfterReadyCrash(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()