Honor max_output_length for custom plugin stdout capture#1288
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kv-cache The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @kv-cache! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @kv-cache. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
size/M
Custom plugin stdout was read into a hardcoded 4 KiB buffer (maxCustomPluginBufferBytes = 1024*4) and only then truncated to the configured max_output_length, so the effective limit was min(4096, max_output_length). Any plugin configured with max_output_length > 4096 was silently capped at 4096 bytes, and a JSON-emitting plugin would be cut mid-token into invalid output. Drive the stdout read limit from max_output_length, bounded by a 1 MiB safety ceiling against a runaway plugin. stderr keeps a small fixed cap since it is only used for diagnostic logging and never becomes the plugin output. Adds a regression test and fixture asserting that a plugin emitting more than 4 KiB is captured up to max_output_length rather than the old fixed buffer size. Signed-off-by: Vineeth Rao Kanaparthi <vineeth0025@gmail.com>
kv-cache
force-pushed
the
fix/custom-plugin-honor-max-output-length
branch
from
ecb0aec to
25f099b
Compare
k8s-ci-robot
added
cncf-cla: yes
DigitalVeer
left a comment
DigitalVeer
left a comment
There was a problem hiding this comment.
Thanks for this fix! I notice two points worth addressing:
- The read window now shrinks below the old 4 KiB for small max_output_length, so default-config behavior change
- The 1 mib ceiling silently ignores larger configured values and may be too large
| // Capture enough stdout to honor the configured max_output_length, bounded | ||
| // by a hard safety ceiling. Previously this was a fixed 4 KiB buffer that | ||
| // silently truncated plugins configured with a larger max_output_length. | ||
| stdoutCapture := int64(*p.config.PluginGlobalConfig.MaxOutputLength) |
There was a problem hiding this comment.
This changes the default config behavior. run() trims whitespace after capture so the old code always had a 4 KiB window to find non-whitespace content in. Now that window is max_output_length itself which is 80 bytes by default.
I reproduced this is by having a plugin prints 80 spaces then DiskHung: /dev/sda1 unresponsive and exiting w/ a status code of 1. Right now NPD reports the message, but this PR reports "". The output is lost when the plugin fires.
Keeping the old 4 KiB as a floor fixes the >4 KiB truncation without changing anything else:
stdoutCapture := int64(*p.config.PluginGlobalConfig.MaxOutputLength)
if stdoutCapture < 4096 {
stdoutCapture = 4096
}
if stdoutCapture > maxCustomPluginStdoutCeilingBytes {
stdoutCapture = maxCustomPluginStdoutCeilingBytes
}A leading whitespace test case that tests this is worthwhile
| // size; this ceiling only caps the (misconfigured) case where max_output_length | ||
| // itself exceeds it. It must be >= the largest supported max_output_length so a | ||
| // plugin's configured limit is never silently truncated by the read buffer. | ||
| const maxCustomPluginStdoutCeilingBytes = 1024 * 1024 |
There was a problem hiding this comment.
max_output_length > 1 MiB gets capped here with no log. Would it be better to check it once at config load and log or reject out-of-range values there?
1 mib may also be too big. The captured results still end up in resultChan which holds 1000 entries. That is 1000 * 1 MiB =~ about 1 GiB of RAM held by a node daemon. And the output goes verbatim into NodeCondition.Message and event messages.
3 participants
What this PR does / why we need it:
Custom-plugin stdout is read into a hardcoded 4 KiB buffer
(
maxCustomPluginBufferBytes = 1024 * 4) and only then truncated to theconfigured
max_output_length, so the effective limit ismin(4096, max_output_length). Any plugin configured withmax_output_length > 4096is silently capped at 4096 bytes — the configuredvalue has no effect. For a plugin that emits JSON, the cut lands mid-token and
produces invalid output that a strict consumer discards entirely.
This drives the stdout read limit from
max_output_length, bounded by a 1 MiBsafety ceiling to protect against a runaway plugin. stderr keeps a small fixed
cap since it is used only for diagnostic logging (
logPluginStderr) and is neverincluded in the returned output. No behavior change for the default config
(
max_output_length = 80).Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
Adds
TestPluginRunCaptureRespectsMaxOutputLengthplus a fixture that emits16384 bytes. With
max_output_length = 8192the captured output must be exactly8192 bytes — this fails on the current code (capped at 4096) and passes with the
fix.
go test -short -race ./pkg/custompluginmonitor/...passes; gofmt/vet clean.This raises the capture ceiling so a plugin's configured
max_output_lengthtakes effect; it does not change that output beyond
max_output_lengthis stillbyte-truncated.
/kind bug
Does this PR introduce a user-facing change?:
Note:
Made with AI