Capture the logs from stderr of custom plugins #442
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @abansal4032. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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/test-infra repository. |
k8s-ci-robot
added
the
size/S
| stdout, err := cmd.Output() | ||
| if err != nil { | ||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout |
There was a problem hiding this comment.
I have some concern about this, especially stderr. If the stderr is too big, it could potentially cause high memory usage.
Can we implement a limit reader to limit the output size, and discard other outputs?
There was a problem hiding this comment.
Added a limit reader for stdout and stderr limiting each to 4k bytes.
abansal4032
force-pushed
the
custom-plugin-logs-capture
branch
from
e27b575 to
143e5d4
Compare
k8s-ci-robot
added
size/M
abansal4032
force-pushed
the
custom-plugin-logs-capture
branch
from
143e5d4 to
ec0fe73
Compare
| glog.Errorf("Error in starting plugin %q: error - %v", rule.Path, err) | ||
| return cpmtypes.Unknown, "Error in starting plugin. Please check the error log" | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we read stdout and stderr at the same time? Or else, the stderr side may potentially block the plugin process when the buffer is full?
And we should probably continue reading the output and discard them after reaching the limit, or else the plugin process may be blocked as well.
There was a problem hiding this comment.
Makes sense. Reading from stdout and stderr at the same time.
Also added discard once we have read the maxBytes.
abansal4032
force-pushed
the
custom-plugin-logs-capture
branch
2 times, most recently
from
955808e to
c44eb5f
Compare
| return cpmtypes.Unknown, "Error in starting plugin. Please check the error log" | ||
| } | ||
|
|
||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
nit:
var (
stdout []byte
stderr []byte
...
)
| // log the stderr from the plugin | ||
| if len(stderr) != 0 { | ||
| glog.Infof("Start logs from plugin %q \n %s\n", rule.Path, string(stderr)) | ||
| glog.Infof("End logs from plugin %q\n", rule.Path) |
There was a problem hiding this comment.
Do we need new line for this log line?
There was a problem hiding this comment.
Removed the redundant line. Did not realize Infof appends that automatically.
| stderr, stderrErr = readFromReader(stderrPipe, maxCustomPluginBufferBytes) | ||
| wg.Done() | ||
| }() | ||
| wg.Wait() |
There was a problem hiding this comment.
It seems that this may block forever.
In theory, it should not happen, because the command should be stopped after timeout, and the IO should be closed.
However, I'm not completely sure about that. Maybe we can call wg.Wait after Wait the command to stop?
There was a problem hiding this comment.
The documentation for stdoutpipe specifies: https://golang.org/src/os/exec/exec.go?s=17733:17782#L609
It is incorrect to call Wait before all reads from the pipe have completed.
Having the wg.Wait() after will call exec.Wait before we are done reading from pipes?
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
abansal4032
force-pushed
the
custom-plugin-logs-capture
branch
2 times, most recently
from
899d0f7 to
d0ab163
Compare
abansal4032
force-pushed
the
custom-plugin-logs-capture
branch
from
d0ab163 to
6acf5b1
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abansal4032, Random-Liu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
The custom plugins : health-checker and log-counter currently log to stderr and the logs are not captured in NPD logs. This change allows to read the stderr from the custom plugin and log the output.