Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invoke: capture and return stderr if plugin exits unexpectedly #760

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

dcbw
Copy link
Member

@dcbw dcbw commented Apr 1, 2020

The way raw_exec invokes the command doesn't actually pass back
stderr, despite ExitError having that capability. c.Run()
internally calls c.Wait() but that doesn't capture stderr and
insert it into the returned ExitError. So we have to do that
ourselves.

Fixes: #732
Fixes: #759

@bboreham @squeed @mccv1r0 @containernetworking/cni-maintainers

@dcbw dcbw mentioned this pull request Apr 1, 2020
@dcbw dcbw force-pushed the really-return-stderr branch 2 times, most recently from 9e39109 to 10bca02 Compare April 2, 2020 01:31
@coveralls
Copy link

coveralls commented Apr 2, 2020

Coverage Status

Coverage increased (+0.3%) to 75.029% when pulling 44dabed on dcbw:really-return-stderr into cf734d4 on containernetworking:master.

pkg/invoke/raw_exec.go Outdated Show resolved Hide resolved
Copy link
Member

@mars1024 mars1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

pkg/invoke/raw_exec.go Outdated Show resolved Hide resolved
@dcbw dcbw force-pushed the really-return-stderr branch from 10bca02 to 8d2fcf1 Compare April 8, 2020 14:28
The way raw_exec invokes the command doesn't actually pass back
stderr, despite ExitError having that capability.  c.Run()
internally calls c.Wait() but that doesn't capture stderr and
insert it into the returned ExitError. So we have to do that
ourselves.

Fixes: containernetworking#732
Fixes: containernetworking#759

Signed-off-by: Dan Williams <[email protected]>
@dcbw dcbw force-pushed the really-return-stderr branch from 8d2fcf1 to 44dabed Compare April 8, 2020 14:29
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@mccv1r0 mccv1r0 merged commit 7d9ec90 into containernetworking:master Apr 8, 2020
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.

if plugin segfaults/panics, invoke.ExecPlugin... claims it "failed with no error message"
6 participants