[Core] Bound concurrent log requests - #1007
Conversation
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>
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
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. 📝 WalkthroughWalkthroughThe 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. ChangesLog streaming
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
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The log limits and cancellation behavior present no identified issue requiring a fix before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
What this PR does
Makes
kubectl ome logssafe for services with multiple pods:--max-log-requestswith the kubectl-compatible default of 5;--followrequest before opening any log stream;open;
on the first stream, output, or process error; and
--limit-bytesas a per-pod bound for one-shot reads and rejects itwith
--follow.The implementation continues to use the typed Kubernetes
Pods(...).GetLogs(...).Stream(ctx)path, preserving kubeconfig transportwrappers and
PodLogOptionsbehavior.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
PodListand pod/logendpoints.One-shot logs are prefixed as before, while the server observed only one open
response at a time and received
limitBytes=2048for all three pods:Six follow targets fail before any pod log endpoint is contacted:
Interrupting a two-pod follow cancels both requests and leaves no active
server-side stream:
Verification run for this branch:
Checklist
make testpasses locally (the complete CLI test/race/vet/lint/buildgates above pass; repository CI runs the full project suite)