Skip to content

[Core] Bound concurrent log requests - #1007

Merged
pallasathena92 merged 3 commits into
mainfrom
codex/log-concurrency-20260924
Sep 25, 2026
Merged

pallasathena92 merged 3 commits into
mainfrom
codex/log-concurrency-20260924

Conversation

@pallasathena92

@pallasathena92 pallasathena92 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Makes kubectl ome logs safe for services with multiple pods:

  • adds --max-log-requests with the kubectl-compatible default of 5;
  • rejects an over-limit --follow request before opening any log stream;
  • reads non-follow logs one pod at a time, so at most one response body is
    open;
  • gives follow streams one shared cancellation scope and closes every sibling
    on the first stream, output, or process error; and
  • adds --limit-bytes as a per-pod bound for one-shot reads and rejects it
    with --follow.

The implementation continues to use the typed Kubernetes
Pods(...).GetLogs(...).Stream(ctx) path, preserving kubeconfig transport
wrappers and PodLogOptions behavior.

Why we need it

The previous implementation opened every selected pod log response before it
started reading any of them. A broad selector could therefore hold as many as
200 API-server connections and goroutines at once. If one follow stream or the
output writer failed, sibling streams could remain blocked instead of stopping
with the command.

This change bounds API-server pressure, fails broad follow requests before
side effects, and makes cancellation deterministic while retaining the
component-aware output operators already use.

Fixes #: N/A

How to test

Unit and integration-style tests cover validation, preflight rejection,
maximum open responses, startup cleanup, sibling cancellation, writer and
scanner failures, process cancellation, typed log request options, and
per-pod byte limits.

The following output is from the PR binary against a local Kubernetes API
fixture with real PodList and pod /log endpoints.

One-shot logs are prefixed as before, while the server observed only one open
response at a time and received limitBytes=2048 for all three pods:

$ kubectl-ome --kubeconfig /private/tmp/ome-log-oneshot.kubeconfig \
    --namespace demo logs chat --limit-bytes=2048
[decoder/chat-decoder-0] chat-decoder-0 ready
[engine/chat-engine-0] chat-engine-0 ready
[router/chat-router-0] chat-router-0 ready

SUMMARY mode=oneshot log_requests=3 max_open=1 active=0 canceled=0 limit_bytes=2048,2048,2048

Six follow targets fail before any pod log endpoint is contacted:

$ kubectl-ome --kubeconfig /private/tmp/ome-log-six.kubeconfig \
    --namespace demo logs chat --follow
error: you are attempting to follow 6 log streams, but maximum allowed concurrency is 5, use --max-log-requests to increase the limit
$ echo $?
1

SUMMARY mode=six log_requests=0 max_open=0 active=0 canceled=0 limit_bytes=

Interrupting a two-pod follow cancels both requests and leaves no active
server-side stream:

$ kubectl-ome --kubeconfig /private/tmp/ome-log-follow.kubeconfig \
    --namespace demo logs chat --follow
[engine/chat-engine-0] chat-engine-0 following
[decoder/chat-decoder-0] chat-decoder-0 following
^Cerror: interrupt signal received

SUMMARY mode=follow log_requests=2 max_open=2 active=0 canceled=2 limit_bytes=,

Verification run for this branch:

go test ./pkg/cli/... ./cmd/kubectl-ome/... -count=1
go test -race ./pkg/cli/... ./cmd/kubectl-ome/... -count=1
go vet ./pkg/cli/... ./cmd/kubectl-ome/...
make ci-lint
go build -o /private/tmp/kubectl-ome-log-concurrency-publish ./cmd/kubectl-ome
git diff --check

Checklist

  • Tests added/updated (if applicable)
  • Docs updated (Cobra flag help documents both new flags)
  • make test passes locally (the complete CLI test/race/vet/lint/build
    gates above pass; repository CI runs the full project suite)

Signed-off-by: yifeng liu <31553858+pallasathena92@users.noreply.github.com>
Signed-off-by: yifeng liu <31553858+pallasathena92@users.noreply.github.com>
Signed-off-by: yifeng liu <31553858+pallasathena92@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 48686a90-ee8d-41d4-83f9-9daf7fd6e99e

📥 Commits

Reviewing files that changed from the base of the PR and between 738612c and 900bcf1.

📒 Files selected for processing (6)
  • pkg/cli/cmd/logs/consume.go
  • pkg/cli/cmd/logs/consume_test.go
  • pkg/cli/cmd/logs/logs.go
  • pkg/cli/cmd/logs/logs_test.go
  • pkg/cli/cmd/logs/multiplex.go
  • pkg/cli/cmd/logs/multiplex_test.go

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.


📝 Walkthrough

Walkthrough

The logs command adds limits for concurrent follow requests and per-pod bytes. It builds per-pod log targets and delegates stream handling to new consumption helpers. Multiplexing now responds to context cancellation and stream errors by closing streams.

Changes

Log streaming

Layer / File(s) Summary
Log request options
pkg/cli/cmd/logs/logs.go, pkg/cli/cmd/logs/logs_test.go
The command adds --max-log-requests and --limit-bytes, including defaults and validation. Tests cover the option values, help text, and validation rules.
Cancellation-aware multiplexing
pkg/cli/cmd/logs/multiplex.go, pkg/cli/cmd/logs/multiplex_test.go
multiplex accepts a context and cancel-cause function. It closes all streams after cancellation or a stream error. Tests check cancellation, close counts, and cancellation causes.
Per-target stream consumption
pkg/cli/cmd/logs/consume.go, pkg/cli/cmd/logs/consume_test.go, pkg/cli/cmd/logs/logs.go, pkg/cli/cmd/logs/logs_test.go
The command builds per-pod targets and delegates to consumeLogs. The helper enforces the follow request limit and handles sequential or concurrent stream consumption. Tests cover stream limits, startup failures, byte-limit request options, and default opener transport behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant LogsCommand
  participant consumeLogs
  participant openLogStream
  participant multiplex
  participant OutputWriter
  LogsCommand->>consumeLogs: pass targets and stream options
  consumeLogs->>openLogStream: open target streams
  openLogStream-->>consumeLogs: return stream readers
  consumeLogs->>multiplex: pass readers and output writer
  multiplex->>OutputWriter: write prefixed log lines
Loading

Suggested reviewers: slin1237

Merge Risk: ⚪ Minimal · up to 900bc

The log limits and cancellation behavior present no identified issue requiring a fix before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: limiting concurrent log requests.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@pallasathena92
pallasathena92 merged commit 67f7d47 into main Sep 25, 2026
16 checks passed
@pallasathena92
pallasathena92 deleted the codex/log-concurrency-20260924 branch September 25, 2026 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant