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

plugin: fix process leak when Setup() fails #9383

Closed
wants to merge 1 commit into from

Conversation

sorindumitru
Copy link
Contributor

We've noticed some leftover processes from vault plugins on our boxes. Some
of them were even left over from previous instances of the service and reparented
to init. This could cause issues if too many of them accumulate.

When running with TRACE logging the logs showed that there was an error return by
the call to Setup() the plugin. Looking through the code it looks like we do not
call Cleanup() in that case.

Before the patch our logs used to show this:

{"@level":"trace","@message":"setup","@module":"aplugin","@timestamp":"2020-06-30T11:42:42.110935-04:00","status":"started","transport":"gRPC"}
{"@level":"trace","@message":"setup","@module":"aplugin,"@timestamp":"2020-06-30T11:42:42.110986-04:00","err":"rpc error: code = Canceled desc = context canceled","status":"finished","took":50151,"transport":"gRPC"}

and after the patch we get the trace that the plugin has existed:

{"@level":"trace","@message":"setup","@module":"aplugin","@timestamp":"2020-07-02T05:27:02.065347-04:00","status":"started","transport":"gRPC"}
{"@level":"trace","@message":"setup","@module":"aplugin,"@timestamp":"2020-07-02T05:27:02.065386-04:00","err":"rpc error: code = Canceled desc = context canceled","status":"finished","took":39222,"transport":"gRPC"}
{"@level":"trace","@message":"cleanup","@module":"aplugin","@timestamp":"2020-07-02T05:27:02.065410-04:00","status":"started","transport":"gRPC"}
{"@level":"trace","@message":"cleanup","@module":"aplugin","@timestamp":"2020-07-02T05:27:02.065883-04:00","status":"finished","took":471538,"transport":"gRPC"}
{"@level":"warn","@message":"error closing client during Kill","@module":"aplugin","@timestamp":"2020-07-02T05:27:02.065929-04:00","err":"rpc error: code = Canceled desc = grpc: the client connection is closing"}
{"@level":"debug","@message":"plugin process exited","@module":"aaplugin","@timestamp":"2020-07-02T05:27:02.118913-04:00","error":"signal: killed","path":"...","pid":92276}

We've noticed some leftover processes from vault plugins on our boxes. Some
of them were even left over from previous instances of the service and reparented
to init.  This could cause issues if too many of them accumulate.

When running with TRACE logging the logs showed that there was an error return by
the call to Setup() the plugin. Looking through the code it looks like we do not
call Cleanup() in that case.
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 2, 2020

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@sorindumitru
Copy link
Contributor Author

for the CLA, from what am I aware of this was already signed at the org level for the bloomberg org. Could the cla bot be checked to see if that org is in the allowed list?

@ncabatoff
Copy link
Collaborator

for the CLA, from what am I aware of this was already signed at the org level for the bloomberg org. Could the cla bot be checked to see if that org is in the allowed list?

I've opened a ticket with our helpdesk to sort that out.

@kalafut
Copy link
Contributor

kalafut commented Jul 15, 2020

@sorindumitru Is the email address you used in your commits listed in your GitHub profile? I believe that's what the CLA bot checks.

@sorindumitru
Copy link
Contributor Author

@kalafut Yes, it is. There was an additional check for that that failed initially, but was ok after I added it.

@ncabatoff
Copy link
Collaborator

Hi @sorindumitru,

Sorry for the continued hassle, but would you mind closing and re-submitting this PR? It sounds like that's the easiest way to move this forward.

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.

None yet

4 participants