Skip to content

fix(controller): paginate Kubernetes rollout reconciliation - #704

Closed
JiahaoTanXX (JiahaoTanXX) wants to merge 1 commit into
microsoft:mainfrom
JiahaoTanXX:fix/k8s-rollout-pagination
Closed

JiahaoTanXX (JiahaoTanXX) wants to merge 1 commit into
microsoft:mainfrom
JiahaoTanXX:fix/k8s-rollout-pagination

Conversation

@JiahaoTanXX

Copy link
Copy Markdown
Contributor

With 500 running rollouts ahead of queued work, the Kubernetes controller only reads the first 500 active rows. Later jobs are never admitted until those rows leave the active set; orphaned running rollouts beyond that window are also missed.

Add optional offset pagination to GET /api/rollouts and read all pages before reconciling jobs. Reading first avoids skipping rows when reconciliation marks earlier rollouts terminal. Job creation still respects max_jobs_per_minute. If an older server ignores offsets, detect the repeated page, warn about upgrading, and retain the previous first-page behavior instead of looping indefinitely. Concurrent external state changes remain eventually reconciled on later polls.

Related: #624 addressed a similar fixed query window in the local controller; this PR covers the Kubernetes path. The terminal completion cursor API is unchanged.

Validation on macOS arm64 / Python 3.12.13 with frozen dev, verl-cpu and docs dependencies:

  • The initial 8 focused cases on unchanged main: 6 failed, 2 passed. All 9 final cases pass, including the legacy-server compatibility regression.
  • .venv/bin/pytest -q tests: 250 passed, 4 skipped (CUDA/vLLM-dependent cases).
  • .venv/bin/pyright: 0 errors, 0 warnings.
  • Ruff lint/format, Python headers, pre-commit and git diff --cached --check: passed.
  • .venv/bin/mkdocs build --strict and uv build --no-sources: passed.

Tests use the real ASGI rollout API and controller, with Kubernetes operations simulated. No live Kubernetes cluster, model inference or GPU training was used; no throughput improvement is claimed.

AI assistance: implemented, tested and independently reviewed with OpenAI Codex.

Copilot AI balanced review requested due to automatic review settings November 12, 2026 01:50

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.

🟡 Changes recommended

The pagination test can pass despite duplicate job creation attempts because _create_job() catches and logs the mock’s AssertionError instead of failing the test.

1 open issue
What changed in this PR

This PR removes the Kubernetes controller’s fixed 500-row reconciliation window, complementing the local-controller capacity fix in #624.

Changes:

  • Add optional offset pagination to the rollout listing API.
  • Read all pages before reconciliation, preserving job-creation rate limits and falling back when older servers repeat a page.
  • Add pagination regression tests and document server upgrade requirements.
File Description
tests/​server/​test_k8s_pagination.py Adds API-backed pagination, reconciliation, rate-limit, and legacy-server tests.
docs/​30-controller-configuration.md Explains pagination behavior and server compatibility.
agentlightning/​server/​routes/​rollouts.py Adds validated offsets to filtered rollout listings.
agentlightning/​controller/​k8s_reconciler.py Fetches all pages before reconciliation and detects repeated pages.

Review effort: Balanced


async def async_create(self) -> None:
name = self.raw["metadata"]["name"]
assert name not in jobs, "an existing Job must not be created twice"
@hzy46

Copy link
Copy Markdown
Contributor

It is a design choice to only look at the first 500 jobs. It can simplify the controller. The afterward jobs won't be blocks as we only query the active jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants