removing unnecessary exit from plugin processes#1848
removing unnecessary exit from plugin processes#1848balopat merged 2 commits intoGoogleContainerTools:masterfrom
Conversation
balopat
requested review from
dgageot,
nkubala,
priyawadhwa and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1848 +/- ##
==========================================
+ Coverage 49.18% 49.22% +0.04%
==========================================
Files 166 166
Lines 7287 7287
==========================================
+ Hits 3584 3587 +3
+ Misses 3357 3351 -6
- Partials 346 349 +3
Continue to review full report at Codecov.
|
| err := <-errCh | ||
|
|
||
| if err == cancelError { | ||
| hashiplugin.CleanupClients() |
There was a problem hiding this comment.
this is the key removal - on Ctrl+C the plugin was self-destructing.
nkubala
left a comment
nkubala
left a comment
There was a problem hiding this comment.
great find, thanks for digging into this!
as we discussed offline, if the skaffold main process dies unexpectedly, the plugin RPC server processes will be abandoned. doesn't need to come in this PR, but we should at least open an issue to implement either
- a mechanism to tie the child processes to the parent processes so when the parent dies the children go with it, or
- a heartbeat coming from the plugin server to the skaffold main, so when it realizes the main process is dead it can kill itself.
priyawadhwa
left a comment
priyawadhwa
left a comment
There was a problem hiding this comment.
thanks for fixing this! had one question
| } | ||
| if err != nil { | ||
| if errors.Cause(err) != runner.ErrorConfigurationChanged { | ||
| plugin.CleanupClients() |
There was a problem hiding this comment.
just a question -- why did you opt to have these twice and remove the defer function?
There was a problem hiding this comment.
because you're not suppose to call defer in a for loop this way - it might actually end up causing problems across dev cycles. one nice explanation is here: https://blog.learngoprogramming.com/gotchas-of-defer-in-go-1-8d070894cb01 - I could have switched over to an anonymous function as well...
5 participants
We thought that the default warning message means something is wrong when we saw it by default from hashicorps go-plugin after a Ctrl+C signal is received:
It turns out it behaves exactly what it needs to: it swallows the signal and ignores it - leaving the chance for the main process to cleanup.
The
plugins.CleanupPlugins()call should be called from the main process which is now implemented there.The logging level for plugins can be configured via the
DefaultPluginLogLevelthat is now set toInfoinstead ofDebug. A follow-up PR could be to get this from an environment variable that the main process sets for the plugin to set the same level of debugging as the main process.