diff --git a/pkg/gameservers/health.go b/pkg/gameservers/health.go index 24c8a2f20a..32a6c0f49a 100644 --- a/pkg/gameservers/health.go +++ b/pkg/gameservers/health.go @@ -292,7 +292,7 @@ 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) { @@ -300,15 +300,9 @@ func (hc *HealthController) skipUnhealthyGameContainer(gs *agonesv1.GameServer, 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 } diff --git a/pkg/gameservers/health_test.go b/pkg/gameservers/health_test.go index fad0250c09..ee140d90fd 100644 --- a/pkg/gameservers/health_test.go +++ b/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 diff --git a/test/e2e/gameserver_test.go b/test/e2e/gameserver_test.go index 3454e5e215..3a61f2abd4 100644 --- a/test/e2e/gameserver_test.go +++ b/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()