Skip to content

fix(kubernetes): fail recovery when executor status is unknown - #1599

Open
mengdehong wants to merge 1 commit into
opensandbox-group:mainfrom
mengdehong:fix/kubernetes-scheduler-recovery
Open

fix(kubernetes): fail recovery when executor status is unknown#1599
mengdehong wants to merge 1 commit into
opensandbox-group:mainfrom
mengdehong:fix/kubernetes-scheduler-recovery

Conversation

@mengdehong

@mengdehong mengdehong commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Scheduler recovery silently treated unknown pod status as "no task running", which could leave the scheduler in an inconsistent state after restart when task-executors were temporarily unreachable.

What is changing:

  1. taskStatusCollector.Collect() now returns (map[string]*api.Task, error) so recovery can distinguish "executor confirmed empty" (ret[ip] = nil) from "query failed" (missing entry + error).
  2. recover() drops sync.Once and propagates errors, allowing recovery to be retried after a failed attempt.
  3. recoverTaskNodesStatus() validates that every pod IP has a corresponding result before mutating any taskNode, ensuring recovery either completes fully or leaves the existing state unchanged.

Runtime polling behavior remains unchanged.

Why:

Previously, when an executor was unreachable during recovery:

  • The error was logged but swallowed by Collect().
  • A missing map entry was indistinguishable from "no task".
  • The corresponding taskNode could be incorrectly treated as free.
  • sync.Once prevented recovery from being retried.

Recovery should be atomic: either fully succeed or leave scheduler state unchanged for a later retry.

Testing

  • Unit tests
    • Added status collector tests for confirmed-empty, successful, and failed queries.
    • Added recovery tests for collection errors, incomplete results, atomicity, and retryable error propagation.
    • Updated existing tests for the new internal Collect() signature.
    • Ran go test ./internal/scheduler/....
  • Integration tests
  • E2E / manual verification

Breaking Changes

  • None externally
  • Internal-only interface change: taskStatusCollector.Collect() now returns (map[string]*api.Task, error). The interface is contained within kubernetes/internal/scheduler and does not affect public APIs, SDKs, CRDs, or configuration.

Checklist

  • Linked issue or clearly described motivation
  • Added/updated tests
  • Documentation not required
  • Security impact considered
  • Backward compatibility considered

Copilot AI lite review requested due to automatic review settings August 23, 2026 05:36
@github-actions github-actions Bot added component/k8s For kubernetes runtime size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Kubernetes scheduler’s recovery and status polling behavior so “unknown” executor states (due to unreachable executors or partial collection) no longer get silently treated as “no task running,” preventing incorrect freeing of task nodes after restart.

Changes:

  • Update taskStatusCollector.Collect() to return (map[string]*api.Task, error) and explicitly differentiate “confirmed empty” (ip -> nil) from “query failed” (missing key + non-nil error).
  • Make recovery retryable and atomic by removing sync.Once, propagating collection errors, and validating completeness before mutating any taskNode.
  • Preserve last-known task Status / state during runtime polling when collection fails (avoid overwriting with nil on errors).

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
kubernetes/internal/scheduler/status_collector.go Collector now returns (map, error) and records explicit nil entries vs missing-on-error.
kubernetes/internal/scheduler/status_collector_test.go New unit tests covering empty-vs-error distinction and success path.
kubernetes/internal/scheduler/status_collector_mock.go Regenerated gomock to match new Collect() signature.
kubernetes/internal/scheduler/recovery.go Recovery now propagates errors and validates complete collection before updating nodes.
kubernetes/internal/scheduler/recovery_test.go Adds tests for atomicity, error propagation, and incomplete-collection rejection.
kubernetes/internal/scheduler/default_scheduler.go Polling now preserves last-known status/state on collection error and skips missing entries.
kubernetes/internal/scheduler/default_scheduler_test.go Updates mocks for new signature and adds polling error-preservation test.
Files not reviewed (1)
  • kubernetes/internal/scheduler/status_collector_mock.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread kubernetes/internal/scheduler/status_collector.go Outdated
@mengdehong
mengdehong force-pushed the fix/kubernetes-scheduler-recovery branch from 6759e32 to 25fcd1b Compare August 23, 2026 08:03
Propagate task-executor status collection errors so scheduler recovery can be retried instead of treating unknown pods as free. Preserve last-known status during polling failures and reject incomplete recovery results before mutating task bindings.
@mengdehong
mengdehong force-pushed the fix/kubernetes-scheduler-recovery branch from 25fcd1b to ac4da52 Compare August 23, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/k8s For kubernetes runtime size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants