fix: move GameServer to Shutdown when the game container exits cleanly and the Pod stays Running - #4737
Merged
markmandel merged 9 commits intoSep 19, 2026
Conversation
…y and the Pod stays Running With SidecarContainers enabled, the SucceededController only moved a GameServer to Shutdown once the Pod reached the Succeeded phase. A Pod only reaches Succeeded once every container in `containers` has terminated, so a long-lived non-sidecar container held the Pod in Running after the game server container exited 0, and the GameServer ran forever. agones-dev#4663 covered the non-zero exit code path in the HealthController; this is the exit code 0 counterpart. The SucceededController now also treats a Pod as completed when the game server container has terminated with exit code 0 and the Pod is RestartPolicy: Never, mirroring the HealthController check. Also documents that hostPort on sidecar (init) containers does not work on most clusters: the kubelet only passes port mappings declared on regular containers to the CNI when creating the Pod sandbox, so the recommended "use a native sidecar" workaround is not available to containers that need a GameServer port. Closes agones-dev#4728 Signed-off-by: Subham K. <me@growly.gg>
Member
|
/gcbrun |
There was a problem hiding this comment.
🟡 Changes recommended
User-facing and controller documentation still contradict the newly implemented lifecycle behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates GameServer lifecycle handling so a cleanly exited game container triggers shutdown even when another container keeps the Pod running.
Changes:
- Detects clean game-container termination independently of Pod phase.
- Adds unit and end-to-end coverage.
- Updates sidecar lifecycle and port-mapping documentation.
File summaries
| File | Description |
|---|---|
pkg/gameservers/succeeded.go |
Adds container-level completion detection. |
pkg/gameservers/succeeded_test.go |
Tests completion and shutdown behavior. |
test/e2e/gameserver_test.go |
Covers clean exit with a long-lived container. |
site/content/en/docs/Guides/health-checking.md |
Documents updated lifecycle semantics. |
site/content/en/docs/Reference/gameserver.md |
Clarifies native-sidecar port limitations. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
nrwiersma
reviewed
Sep 14, 2026
Collaborator
|
Build Failed 😭 Build Id: a02934e2-0152-4a68-8f04-3fe480968ccc Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
…e gate Review feedback on agones-dev#4737: The SidecarContainers feature gate only moves the Agones SDK server into initContainers; it says nothing about whether the user's own containers are sidecars, so it is the wrong signal for "something other than the game server can hold this Pod in Running". Check the Pod shape instead: more than one entry in `containers`, and RestartPolicy: Never so a terminated container is final. The e2e test expected an ACK from `CRASH 0`, but handleCrash calls os.Exit before replying, so SendGameServerUDP always timed out and the test failed. Ignore the reply, as TestGameServerUnhealthyAfterReadyCrashWithGenericContainer already does. Also reword the controller docs and health checking rule 5, which still said this only happens once the Pod reaches Succeeded. Signed-off-by: Subham K. <me@growly.gg>
nrwiersma
reviewed
Sep 14, 2026
- Split the gameServerContainerCompleted assertions into named subtests. - The Pod does not outlive the GameServer, so say the containers keep running until the GameServer is shutdown, not deleted. - Drop "on most clusters" for the hostPort-on-sidecar warning, in both docs that carry it: Kubernetes is what does not map the port, and whether a CNI picks it up anyway is already covered below. Signed-off-by: Subham K. <me@growly.gg>
There was a problem hiding this comment.
🟡 Changes recommended
The new completion logic omits OnFailure, races with failed-Pod handling, and introduces an avoidable one-minute test delay.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
nrwiersma
approved these changes
Sep 14, 2026
- A Failed Pod already makes the GameServer Unhealthy via the HealthController's failedPod(), which watches the same updates. Bail out on that phase so the two controllers can't race to write the GameServer's state. - OnFailure never restarts a container that exited 0, and nothing validates the Pod's restartPolicy, so a long-lived container could still strand a GameServer on one. Only reject Always. - The e2e test's SendGameServerUDP waited a minute for an ACK that CRASH 0 never sends, and dumped failure diagnostics on the way out. Write to the socket inside the poll loop instead, which also re-sends if the packet is dropped. Signed-off-by: Subham K. <me@growly.gg>
Member
|
/gcbrun |
There was a problem hiding this comment.
🟡 Changes recommended
The health-checking documentation overstates behavior when users override the default Pod restart policy.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
site/content/en/docs/Guides/health-checking.md:82
- This paragraph is unconditional, but the direct non-zero-exit path only applies to
restartPolicy: Never, and the clean-exit path rejectsAlways. A user may override the default Pod restart policy, so the documented outcomes are not generally true. Scope this statement to the default policy (the behavior described by rules 4 and 5).
after the game server container has exited. Agones does not depend on the Pod phase for this: it watches the game
server container directly, and will move the `GameServer` to `Unhealthy` on a non-zero exit code (rule 4) or to
`Shutdown` on a clean exit (rule 5) as soon as the game server container terminates. The Pod's own phase, however,
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Collaborator
|
Build Failed 😭 Build Id: 4f369f4b-0593-49ef-9b3f-11aaa3dceb19 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Member
|
/gcbrun |
Collaborator
|
Build Succeeded 🥳 Build Id: 67df54e9-564b-4350-bc79-0b4e60533dae The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
markmandel
reviewed
Sep 17, 2026
markmandel
left a comment
markmandel
left a comment
Member
There was a problem hiding this comment.
Looks good - just some docs things I wanted to poke at 👍🏻
Signed-off-by: Subham K. <me@growly.gg>
Simplify the restart policy check, version-gate the clean-exit health checking rule, and clarify the sidecar port CNI warning. Signed-off-by: Subham K. <me@growly.gg>
Member
|
/gcbrun |
Collaborator
|
Build Succeeded 🥳 Build Id: d7249135-7182-4e27-a32a-c26c12cd0de4 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
markmandel
approved these changes
Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / Why we need it:
The
SucceededControlleronly moved aGameServertoShutdownonce the Pod reached theSucceededphase.A Pod only reaches
Succeededonce every container incontainershas terminated, so a long-lived non-sidecar container holds the Pod inRunningafter the game server container exits0, and theGameServerruns forever.The controller now also treats the Pod as completed when the game server container has terminated with exit code
0, on a Pod with more than one entry incontainersandRestartPolicy: Never— the conditions under which something other than the game server can hold the Pod inRunning, and a terminated container is final. #4663 covered the non-zero exit code path in theHealthController; this is the exit code0counterpart.Docs
health-checking.md: rule 5 of theSidecarContainerslist said theGameServermoves toShutdown"once the Pod completes", which no longer holds. It now matches rule 4's wording and fires as soon as the game server container exits.health-checking.md: the "additional workloads" section now says Agones watches the game server container directly, so a long-lived container no longer keeps a finishedGameServeralive (gated behindpublishVersion="1.61.0").gameserver.md/health-checking.md: the existing "some CNIs may not support ports on init containers" warning is replaced with the actual cause. The kubelet only passeshostPortmappings from regularcontainersto the CNI when it creates the Pod sandbox (kuberuntime_sandbox.go), so ahostPorton a native sidecar never reaches theportmapplugin. This matters for Health Checks not running when having "Completed" containers in Pods #4728 because "make it a native sidecar" is the suggested workaround, and it is not available to a container that needs aGameServerport (as @nrwiersma pointed out). This is also whyTestGameServerWithPortsMappedToInitSidecarContainersis skipped.Which issue(s) this PR fixes:
Closes #4728
Did you use AI tools in preparing this PR?:
Y
Special notes for your reviewer:
Since the first push:
SidecarContainersfeature gate check from the new code path, per @nrwiersma. The gate only moves the SDK server intoinitContainersand says nothing about whether the user's own containers are sidecars. It was also redundant —RestartPolicy: Neveris only defaulted inside that gate today — and it wrongly excluded anyone who setsrestartPolicy: Neverthemselves.ACKfromCRASH 0, buthandleCrashcallsos.Exitbefore replying, soSendGameServerUDPalways timed out. It now ignores the reply, asTestGameServerUnhealthyAfterReadyCrashWithGenericContaineralready does.The e2e test still skips without
SidecarContainers, sinceDefaultGameServeronly getsRestartPolicy: Nevervia that gate.