logmon: recover from shutting down call locally #5616
Conversation
Retry if we detect shutting down during Start() api call is started, locally.
| } | ||
|
|
||
| func (h *logmonHook) prestartOneLoop(ctx context.Context, req *interfaces.TaskPrestartRequest) error { | ||
| // attach to a running logmon if state indicates one |
There was a problem hiding this comment.
I opted not to change the logic here as the logic is somewhat brittle and I don't want to make it worse. But the idea is that if grpc call fails with shutting down, h.logmonPluginClient would be non-nill and would be marked as Exited, so we'll create a new logmon instance on retries.
| @@ -73,5 +73,8 @@ func (p *Plugin) GRPCServer(broker *plugin.GRPCBroker, s *grpc.Server) error { | |||
| } | |||
|
|
|||
| func (p *Plugin) GRPCClient(ctx context.Context, broker *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) { | |||
| return &logmonClient{client: proto.NewLogMonClient(c)}, nil | |||
| return &logmonClient{ | |||
| doneCtx: ctx, | |||
There was a problem hiding this comment.
I'm not one 100% percent following the plugin ctx and how it's set during the initialization phase - but this follows the pattern in other driver plugins , e.g. https://github.com/hashicorp/nomad/blob/v0.9.0/plugins/drivers/plugin.go#L39-L49 .
Interestingly the logmon plugin client doesn't embed BasePluginClient, so might want to follow up on that with 0.9.2.
notnoop
force-pushed
the
b-retry-logmon-start-errs-2
branch
from
c0114c7
to
94c9c57
Compare
notnoop
force-pushed
the
b-retry-logmon-start-errs-2
branch
from
94c9c57
to
978fc65
Compare
| // then we kill process while Start call is running | ||
| require.NoError(t, proc.Signal(syscall.SIGSTOP)) | ||
| // sleep for the signal to take effect | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
How can you guarantee that the sleep is sufficient? Wondering if this will be flaky in CI
There was a problem hiding this comment.
I don't know of a good way to test if a process is stopped - unlike killing. If it's flaky in CI, we can react - but considering that we will overhaul logmon plugin method to be inline with other plugin clients and this code would probably change, I'd avoid overengineering and react to CI problems as they come.
There was a problem hiding this comment.
So I take that back :). ended up using gopsutil.Process utility to verify process sleep status in 658a734!
| h.logger.Warn("logmon shutdown while making request", "error", err) | ||
|
|
||
| if tries > 3 { | ||
| return err |
There was a problem hiding this comment.
maybe log here that its out of retries before returning error?
logmon: recover from shutting down call locally
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Alternative to #5615 - where we detect logmon plugin shutting down during
logmon.Startcall and retry it after killing the process.Here, we try 4 times with 1-second backoff. I opted to use a very simple backoff strategy, and punt on a more robust strategy for logmon failures to 0.9.2.
The condition is very hard to test, as plugin needs to be exiting but not exited yet prior to Start call. Open for suggestions.