fix(kubernetes): fail sandbox after main container restart - #1482
fix(kubernetes): fail sandbox after main container restart#1482ruirui6946 wants to merge 10 commits into
Conversation
github-actions
Bot
added
component/k8s
c592784 to
eaa4329
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Kubernetes BatchSandbox status controller so a sandbox that previously reached Succeed is transitioned to Failed if the main (first) container restarts after the sandbox became ready, preventing “Succeeded” from masking a reset runtime (e.g., OOM + restart) as described in #1450.
Changes:
- Extend pod failure detection to treat a post-ready main-container restart as a failure signal (while ignoring sidecar restarts).
- Introduce a restart-detection baseline keyed off the existing
Readytransition time plus stable generation/replica count and stable endpoint membership. - Add focused unit tests covering post-ready restarts, pre-ready restart history, sidecar restarts, and baseline resets when pod membership changes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kubernetes/internal/controller/batchsandbox_status.go | Adds restart detection logic and a baseline mechanism, and wires it into steady-state runtime phase evaluation. |
| kubernetes/internal/controller/batchsandbox_pause_resume_test.go | Adds new test cases validating the new restart-based failure behavior and baseline reset behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c592784c5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
10a2c94 to
02f162f
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
02f162f to
4141987
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41419870f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
github-actions
Bot
added
size/L
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3aa4ab7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc16e9b6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44c4452c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a087245bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _, published := baseline.previousEndpointIPs[pod.Status.PodIP]; published { | ||
| baseline.desiredPerPod[key] = now |
There was a problem hiding this comment.
If patchBatchSandboxEndpoints succeeds but the controller crashes or the subsequent Pod baseline patch fails, the new endpoint is already visible through sandbox.opensandbox.io/endpoints while the Pod still has startedAt:0. On the next reconcile this branch sees the endpoint as published and advances the baseline to the current time, so any main-container restart that happened while the endpoint was already exposed is treated as pre-baseline and is permanently missed. Persist the original publication time or fail closed until activation succeeds.
AGENTS.md reference: kubernetes/AGENTS.md:L47-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ace0ef4de3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| status := batchSbx.Status | ||
| baseline := restartDetectionBaseline{ | ||
| batchSandboxUID: batchSbx.UID, | ||
| detectRestarts: status.Phase == sandboxv1alpha1.BatchSandboxPhaseSucceed, |
There was a problem hiding this comment.
When a previously ready single-replica sandbox temporarily becomes unready without one of the recognized waiting reasons, the first reconcile changes its phase to Pending and removes the Ready condition; if the main container then restarts, or its termination update arrives afterward, this phase check disables the persisted restart baseline. Recovery creates a fresh Ready transition, permanently hiding that post-ready restart. Preserve the last successful baseline through transient Pending states and cover this sequence with a focused regression test.
AGENTS.md reference: kubernetes/AGENTS.md:L165-L165
Useful? React with 👍 / 👎.
| publishedAt := time.Now().UnixNano() | ||
| patched, err := r.activatePublishedPodRestartBaselines(ctx, batchSbx.UID, publishedAt, view) |
There was a problem hiding this comment.
When a newly published endpoint's main container terminates before the next whole-second boundary, this baseline retains controller-clock nanoseconds, whereas metav1.Time termination timestamps round-trip through the Kubernetes API at RFC3339 second precision. The decoded FinishedAt is therefore earlier than publishedAt, so the strict After comparison ignores a restart that actually occurred after publication and loses it permanently. Use restart-count state or timestamps normalized to a common precision, with a regression test for the same-second case.
AGENTS.md reference: kubernetes/AGENTS.md:L165-L165
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49da87fa7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| status := batchSbx.Status | ||
| baseline := restartDetectionBaseline{ | ||
| batchSandboxUID: batchSbx.UID, | ||
| detectRestarts: status.Phase == sandboxv1alpha1.BatchSandboxPhaseSucceed, |
There was a problem hiding this comment.
When the sandbox uses spec.poolRef, a post-ready restart does not itself cause this detection to run: BatchSandboxReconciler.SetupWithManager only uses Owns(&corev1.Pod{}), while pooled Pods are owned by the Pool and therefore enqueue only PoolReconciler; that reconciler does not update the BatchSandbox allocation annotation for a status-only Pod change. Consequently the sandbox can remain Succeed indefinitely after its allocated main container restarts, until some unrelated BatchSandbox event occurs. Add a Pod watch that maps allocated pooled Pods back to their BatchSandbox.
AGENTS.md reference: kubernetes/AGENTS.md:L150-L154
Useful? React with 👍 / 👎.
| if _, existedWhenReady := b.previousEndpointIPs[pod.Status.PodIP]; !existedWhenReady { | ||
| return nil | ||
| } | ||
| if !pod.CreationTimestamp.IsZero() && pod.CreationTimestamp.After(b.readySince.Time) { | ||
| return nil |
There was a problem hiding this comment.
When a replacement is a prewarmed pooled Pod whose IP was reused from the removed member, this fallback treats it as the old Pod because its IP is in previousEndpointIPs and its creation predates the Ready transition. Any restart it experienced while idle after that transition is then reported as a post-ready sandbox failure before its endpoint was allocated or published. Fresh evidence in the current code is that an unannotated Pod falls back solely to endpoint IP and creation time despite the per-Pod baseline being described as UID-based; persist the prior member UID or initialize a pending baseline for every newly allocated UID.
AGENTS.md reference: kubernetes/AGENTS.md:L150-L154
Useful? React with 👍 / 👎.
Summary
Testing
go test -v ./internal/controller -run '^(TestBuildRuntimeView|TestGetPodFailureReasonAndMessage)' -count=1go test ./internal/controller -run '^Test[^C]' -count=1go test ./apis/... ./cmd/... ./examples/... ./internal/... ./pkg/... -skip TestControllers -count=1go vet ./...make manifests generate fmt(no generated output changes)The Docker-backed E2E packages cannot start in this WSL environment because Docker Desktop integration is not enabled; their setup failed before any E2E spec ran. The envtest-backed
TestControllerssuite also remains unavailable because the official Kubernetes envtest release asset download timed out. GitHub Actions jobs on this external-fork PR require maintainer approval.Fixes #1450