Repository navigation
fix: set GameServer to Unhealthy on container crash before Ready in sidecar mode - #4770
Conversation
|
/gcbrun |
|
Build Succeeded 🥳 Build Id: 4a94d678-dfb7-4af6-bf8f-c49e93ea58dc 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
left a comment
markmandel
left a comment
There was a problem hiding this comment.
Using:
agones/test/e2e/gameserver_test.go
Line 331 in deab29c
I'd like a e2e test for this as well to make sure we have an integration test.
| // 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() { |
There was a problem hiding this comment.
well that makes things easier.
85a4094 to
77cb703
Compare
Hi @markmandel, I added e2e coverage to TestGameServerRestartBeforeReadyCrash with SidecarContainers enabled. |
77cb703 to
2f4652c
Compare
|
/gcbrun |
There was a problem hiding this comment.
🟡 Changes recommended
The end-to-end test can fail intermittently because its fixed two-second crash races the exact-state wait for Scheduled.
1 open finding
What changed in this PR
Fixes GameServers remaining Scheduled when their game container crashes before Ready in sidecar mode.
Changes:
- Removes the pre-Ready unhealthy skip for sidecar mode.
- Adds unit and end-to-end regression coverage.
| File | Description |
|---|---|
pkg/gameservers/health.go |
Allows unrestartable container failures to become Unhealthy. |
pkg/gameservers/health_test.go |
Covers pre-Ready failure with another running container. |
test/e2e/gameserver_test.go |
Adds sidecar-mode crash regression test. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestGameServerUnHealthyBeforeReadyCrash(t *testing.T) { |
There was a problem hiding this comment.
Very small nit - I didn't even see it - but would be a nice change. But not suuuuuper important.
There was a problem hiding this comment.
Agreed, renamed it.
markmandel
added
area/build-tools
|
Build Succeeded 🥳 Build Id: a3ec64f0-0b80-4244-ad1f-648132baf5e9 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: |
c58ee83 to
8b82682
Compare
|
/gcbrun |
|
Build Failed 😭 Build Id: e46af990-94c8-430a-8f5b-964d117bac84 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
oops, just updated the linters and you got caught :) sorry about that! |
…idecar mode Signed-off-by: bknote71 <bknote71@gmail.com>
Head branch was pushed to by a user without write access
8b82682 to
34e757d
Compare
|
/gcbrun |
|
Build Succeeded 🥳 Build Id: c44b4e27-2fb8-4709-9fa3-f3f9f191b438 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: |
What type of PR is this?
/kind bug
What this PR does / Why we need it:
In sidecar mode, a game container that exits with a non-zero code cannot restart.
Remove the skip for game container failures before Ready.
When another container keeps the Pod Running, that skip can leave the GameServer Scheduled indefinitely.
Add a regression test for this case.
Which issue(s) this PR fixes:
Closes #4769
Did you use AI tools in preparing this PR?:
Yes. This PR was written in part with the assistance of generative AI.
Special notes for your reviewer: