NRI plugins penalized with death if they take 2 seconds

[mbaynton]

Description

NRI plugins running in containerd by default have 2 seconds per event to provide a response. This is fine. But, if it misses a single response in that timeframe, it is closed / cut off from future events. For plugins built on github.com/containerd/nri, that results in the process exiting.

There is a specific set of errors that induce this close-the-connection behavior:

nri/pkg/adaptation/plugin.go

Lines 520 to 533 in 7b3bcee

	// isFatalError returns true if the error is fatal and the plugin connection should be closed.
	func isFatalError(err error) bool {
	switch {
	case errors.Is(err, ttrpc.ErrClosed):
	return true
	case errors.Is(err, ttrpc.ErrServerClosed):
	return true
	case errors.Is(err, ttrpc.ErrProtocol):
	return true
	case errors.Is(err, context.DeadlineExceeded):
	return true
	}
	return false
	}

The other ones in that list look very reasonable. But, I'd like to suggest that a plugin responding to one event in more than (by default) 2 seconds doesn't indicate that the plugin has entirely failed and it can probably still be used for future events, so a better behavior would be to simply time out that one event but continue.

[lengrongfu]

I was thinking if we can handle this logic by adding an auto restart plugin, we can add WithOnClose this method to handler close.

opts = append(opts, stub.WithOnClose(func() {
		log.Info("Stopping plugins.")
		closeCtx <- struct{}{}
	}))

[samuelkarp]

For plugins built on github.com/containerd/nri, that results in the process exiting.

#173 will change that behavior so the process no longer exits

But, I'd like to suggest that a plugin responding to one event in more than (by default) 2 seconds doesn't indicate that the plugin has entirely failed and it can probably still be used for future events, so a better behavior would be to simply time out that one event but continue.

Given that the process no longer exits, do you think it would be reasonable for the plugin to re-register rather than just that single event timing out?

[aojea]

I've discussed this with @grosskur recently.

My thoughts is that this seem to conflate plugin liveness with the execution timeout, and using the reconnect as a big hammer.

I think that ideally we should be some sort of keepalive to detect if the plugin is alive, grpc already has this functionality, do not know about ttrpc, so the runtimes can differentiate between "slow handler" and "dead plugin" , and handle each situation different.

[aojea]

filed containerd/ttrpc#203 , @samuelkarp can you please indicate what will be the best forum to discuss this feature? I'm stepping up to implement it but I need first an ack on the design

[Andreagit97]

filed containerd/ttrpc#203

The feature proposed here seems really useful. Is there any discussion in progress regarding it?

We use an NRI plugin in our product to extract information about the container startup, and while the extraction flow is currently below the default 2 seconds, we will always be concerned about exceeding this limit.
One alternative is to use the NRI to inject the OCI hook and use the OCI hook to do the job so that we are safe even if we exceed the 2-second timeout.
This works, but actually, it is just a workaround to avoid the timeout since we don't really need an OCI hook for our task.

Is there anything planned in this direction? Maybe a per-plugin timeout or a way to detect if the plugin is still alive