Failure policy by aojea · Pull Request #329 · containerd/nri

aojea · 2026-10-15T19:50:04Z

When a plugin exceeds the request timeout or looses the connection, NRI closes the plugin and returns nil, nil to the runtime: the plugin is unregistered and the runtime never learns that anything failed, so the pod or container is created without the plugin having processed it. However, a plugin that returns an error fails the request.

This behavior is causing production problems and reported now by multiple end users, this is now more problematic because machines with large number of RDMA NICs get close to the default 2 seconds timeout and usually exceed it, so the Pod may get only a subset of NICs it requested, that is very problematic for these kind of workloads. See for a more detailed explanation kubernetes-sigs/dranet#370. That same PR has an attempt to solve it from the plugin itself but is very very very hacky.

Reported in #316 (a required plugin times out in RunPodSandbox and pod creation continues; originally kubernetes-sigs/dranet#310) and discussed in #114 and #90 (plugins dropped for a single slow response, "the DeadlineExceeded error is also hidden"), #142 and #45 (requests for a per-plugin failure policy) and #273 (consistent failure handling, which names exactly this asymmetry between explicit errors failing closed and timeouts failing open).

To avoid breaking changes we propose to add a FailurePolicy to ConfigureResponse, declared by the plugin together with its event subscription:

	`FAIL_OPEN` (default)	`FAIL_CLOSED`
plugin returns an error	request fails, plugin kept	same
request timeout, plugin still connected	plugin unregistered, request proceeds without it	request fails, plugin stays registered
connection lost	plugin unregistered, request proceeds without it	plugin unregistered, request fails

FAIL_OPEN is the zero value and keeps the existing behavior and is backwards compatible with plugins that does not implement the new field.

A FAIL_CLOSED plugin which exceeds the timeout but is still connected is still registered: the timeout only means that it failed to handle this particular request in time, not that it is gone, and dropping it would make all subsequent requests proceed without it, which is precisely what the plugin opted out of.

Plugin developers UX

s, err := stub.New(p,
    stub.WithPluginName("dra.net"),
    stub.WithPluginIdx("00"),
    stub.WithFailurePolicy(api.FailurePolicy_FAIL_CLOSED),
)

Fixes #316
Refs #114, #90, #142, #45, #273

/assign @klihub @mikebrow @samuelkarp @chrishenzie
/cc @gauravkghildiyal

The 13 request relays share an identical block deciding whether an error returned for a plugin is fatal, in which case the plugin is closed and the request proceeds without it, or is returned to the runtime and fails the request. Move it into a single handleRequestError() helper so that this policy can be reasoned about, changed and unit tested in one place. No functional change. Signed-off-by: Antonio Ojea <aojea@google.com>

MikeZappa87 · 2026-10-15T21:12:03Z

+1

@aojea will this require any changes to containerd outside of the go mod version bump? Lmk how I can help

mikebrow

looking good to me...

@klihub @samuelkarp this reads close to what we've talked about before in having must run/complete plugins.

Thoughts?

klihub · 2026-10-16T07:03:42Z

looking good to me...

@klihub @samuelkarp this reads close to what we've talked about before in having must run/complete plugins.

Thoughts?

@mikebrow @aojea This looks to me like a reasonable way to get rid of the current hardcoded blunt policy and let plugins instead declare their self-scoped preferences.

I'm mostly wondering if we should make the policy preference more explicit/split wrt. how the plugin itself vs. the affected request are treated in connection of a protocol failure. IOW, if we should use, instead of an enum with multiple implied (and AFAICT largely orthogonal) semantic conventions, a more explicit expression with something like this

...
// PluginFailurePolicy specifies how the runtime should treat the plugin itself and
// the associated request, when a plugin fails to process a request , due to a request
// timeout, lost connection, or protocol error.
message PluginFailurePolicy {
    // Be blunt and kick the plugin out, or be forgiving and allow it to continue processing requests.
    bool keep_connected = 1; 
    // Fail the associated request with an error immediately, or continue processing it.
    bool process_request = 2;
}
...

Those names and the associated semantics are inverted to what we'd ideally have, and it mostly comes down to (implied semantics of) defaults and backward-compatibility. We could have these also as bool disconnect_plugin, fail_request as well, if we define a DefaultPluginFailurePolicy with the correct backward-compatible semantics which we fill in on the adaptation side if the response lacks one, with the only danger being that the message definition alone itself would give the wrong impression about default behavior.

aojea · 2026-10-17T09:00:15Z

@klihub agree with you, I just defined the API based on the observed behavior, however, this proto should define the runtime/plugin behaviors, and the composition of those values will provide the FAIL_CLOSED or FAIL_OPEN user facing behavior. Plugin developers can now describe what they can influence, instead of having to choose what we invented for them.
I have update the PR with two booleans as you suggest but some differences:

message FailurePolicy {
  bool fail_request   = 1;
  bool keep_connected = 2;
}

I went with fail_request + keep_connected rather than disconnect_plugin + fail_request with a DefaultPluginFailurePolicy because backwards compatibility.

the zero value represents current behavior,
an old plugin that never sets the field, a new plugin that sets nil, and a new plugin that sets &FailurePolicy{} all behave identically, without any substitution on the adaptation side;
the proto definition is an accurate description of the defaults, which was the danger you pointed out with the inverted variant;
the polarity is mixed (one "keep", one "fail"), but I think is simpler/better to have better zero values than having to default them.

Two rules keep the axes from contradicting each other:

keep_connected only applies to timeouts. A lost connection or a protocol error always disconnects the plugin..
Errors explicitly returned by the plugin and requests cancelled by the runtime are not "plugin failures" and are never subject to the policy; they always fail the request as today.

Using a message rather than two scalars on ConfigureResponse also leaves room for extensions in the same place (e.g. a plugin-requested timeout, which is the actual root cause of the RDMA case, or per-event overrides) without touching ConfigureResponse again.

Tests now cover the full matrix (3 failure kinds × nil + 4 policies) at the unit level, and all 4 combinations end-to-end through the stub, including validators.

When a plugin fails to process a request, because it exceeds the request timeout or loses its connection, the adaptation closes the plugin and lets the request proceed without it. It does not notify the runtime. A plugin that returns an error, in contrast, fails the request. Some plugins must process every request they subscribe to, for example to set up networking or to inject devices. For them, this behavior produces pods and containers that were never set up, with no error. A plugin that exceeds the timeout once is also unregistered for good, although it is still connected (containerd#316, containerd#142, containerd#114, containerd#90, containerd#273). Add a FailurePolicy to ConfigureResponse. The plugin declares it together with its event subscription. The runtime takes two independent decisions when a plugin fails to process a request: one about the request and one about the plugin. The policy has one field for each decision, instead of named presets that bundle both: - fail_request: fail the request with an error that names the plugin and the request, like an error returned by the plugin. Default: the request proceeds with the remaining plugins. - keep_connected: keep a plugin that exceeds the request timeout registered. The timeout only means that the plugin did not handle this request in time. Default: the plugin is unregistered. This field only applies to timeouts. A plugin that loses its connection or causes a protocol error is always unregistered. The default of each field is the old behavior. A plugin without a policy, or built against an NRI version without this field, is treated as before. The proto definition describes the defaults as they are. The adaptation does not substitute anything. An error returned by the plugin is never subject to the policy. Neither is any error after the runtime cancelled its own context or its deadline expired. Until now, a runtime deadline that expired during a request was treated as a plugin timeout and unregistered the plugin. The stub gets a WithFailurePolicy() option. A runtime with an NRI version without failure policy support ignores the field. Fixes: containerd#316 Signed-off-by: Antonio Ojea <aojea@google.com>

mikebrow

some nits on wording..

At the higher level I think we need a bit more for set of plugin cases.

Problem: When the runtime client included a timeout, the total timeout for all requests sent to all plugins will timeout.

Problem: Without having some sort of health checking, a plugin failing every request due to timeout is going to be problematic for the node. Probably need a plugin restart process and a health check process triggered by some number of timeouts.

Something else to consider: With the default behavior, when fail_request is false for a plugin in a chain of dependent plugins, continuing on to the subsequent plugins is problematic, possibly not if the plugin was some optional feature and/or not really a necessary step in the chain. A depends list is probably needed.. and if we do that I could see timeout being a sum of one or more types (sum of the chain tree, for all required chains/plugins)

Apply the wording suggestions from the review of containerd#329 to the FailurePolicy comments in api.proto, and regenerate api.pb.go. Suggested-by: Mike Brown <brownwm@us.ibm.com> Signed-off-by: Antonio Ojea <aojea@google.com>

The review of containerd#329 raised scenarios that the failure policy does not address. Add a test for each one. Each test states the current behavior so that follow-up work has a baseline: A. The runtime sets a deadline on an operation. NRI calls plugins one after the other, so a slow plugin consumes the budget of the plugins after it. The runtime gets its own error back, and the plugin that was in flight is not blamed. B. A plugin that never answers delays each request by the full timeout. Without a policy, NRI drops it, it reconnects, and the next request drops it again. With keep_connected, the same happens without the reconnect. There is no health check and no counter. C. A plugin that depends on an earlier plugin runs after NRI dropped that plugin. Nothing in the request tells it so. The container is created without the adjustments of the dropped plugin. D. The required plugins check of the default validator does not catch a dropped plugin. NRI adds a plugin to the list of consulted plugins before it relays the request. A plugin kept connected after a timeout can receive a request while an earlier handler still runs. Protect the pod and container maps of the mock plugin with a lock, so that the tests pass under the race detector. Signed-off-by: Antonio Ojea <aojea@google.com>

aojea · 2026-10-19T19:41:57Z

@mikebrow applied all the wording suggestions in a separate commit to simplify the review (will squash before merge).

I added one test per scenario in TestPluginFailureScenarios for each one of your points so we can assert what happens today:

Runtime deadline vs. sum of plugin timeouts (A). This does not change: plugins run sequentially under the runtime's context, so a slow plugin consumes the budget of the plugins after it. However, this fixes the problem when a runtime deadline is expiring mid-request, before it was considered a plugin timeout and removing the plugin. How to split the overall budget across plugins looks like deserve its own discussion.
A plugin that fails every request (B). I don't think we can assume a plugin times out because it is stuck, and we should not use the lifecycle hooks as a liveness probe, or at least not generalize it. Also, today a hung plugin is dropped, reconnects, is still hung, and the next request times out and drops it again. With keep_connected the only difference is that we skip the reconnect; the cost per request is the same. And the plugin developer has to opt in to this behavior.
Dependent plugin chains (C, D). CreateContainer adds a plugin to the "consulted" list before relaying, so a dropped plugin is still listed and the validator cannot know whether it processed the request. Right now fail_request is the only way for a plugin to guarantee that. A dependency system is too complex to define here and I think it deserves its own discussion.

This is far from perfect, but it moves us to a better state, and the two booleans looks like good primitives to start fixing the rest of things

Thoughts

mikebrow · 2026-10-19T21:37:35Z

@mikebrow applied all the wording suggestions in a separate commit to simplify the review (will squash before merge).

thanks

I added one test per scenario in TestPluginFailureScenarios for each one of your points so we can assert what happens today:

good idea

Runtime deadline vs. sum of plugin timeouts (A). This does not change: plugins run sequentially under the runtime's context, so a slow plugin consumes the budget of the plugins after it. However, this fixes the problem when a runtime deadline is expiring mid-request, before it was considered a plugin timeout and removing the plugin. How to split the overall budget across plugins looks like deserve its own discussion.

budget management is what I was thinking of as well (for some follow-up), I was more noting that we should consider the use case.. which you have done via a test

A plugin that fails every request (B). I don't think we can assume a plugin times out because it is stuck, and we should not use the lifecycle hooks as a liveness probe, or at least not generalize it. Also, today a hung plugin is dropped, reconnects, is still hung, and the next request times out and drops it again. With keep_connected the only difference is that we skip the reconnect; the cost per request is the same. And the plugin developer has to opt in to this behavior.

I agree, we can't assume it's necessarily stuck, it may be just to slow for the budget/individual call. Thus mentioning a health check of some type to help make that determination. But yes it could be any reason and restarts are not always going to help, even restarting the runtime and node may not help. Answer may come via the recommended deployment patterns, as a systemd service w/health checking or as a pod via k8s health checking. Anyhow definitely needs a lot more thought and out of scope for this PR.

Dependent plugin chains (C, D). CreateContainer adds a plugin to the "consulted" list before relaying, so a dropped plugin is still listed and the validator cannot know whether it processed the request. Right now fail_request is the only way for a plugin to guarantee that. A dependency system is too complex to define here and I think it deserves its own discussion.

Agreed deserves it's own discussion.

This is far from perfect, but it moves us to a better state, and the two booleans looks like good primitives to start fixing the rest of things

nod

Thoughts

LGTM

mikebrow

LGTM very nice touch on the extra scenarios.

aojea · 2026-10-22T08:09:23Z

kindly ping @klihub for feedback

klihub

@aojea This looks good to me now. Thank you for taking care of this!

aojea · 2026-10-22T17:13:11Z

+1

@aojea will this require any changes to containerd outside of the go mod version bump? Lmk how I can help

@mikebrow I take you in your offer 😄 , it will be nice to get this into containerd , maybe we need to tag?

MikeZappa87 · 2026-10-22T17:15:21Z

We need this to be tagged, if its just a version bump I can do that really quick

MikeZappa87 · 2026-10-22T17:22:19Z

@aojea once the tag is ready I can update this branch
containerd/containerd@main...MikeZappa87:containerd:mzappa/nriversionbump

containerd/containerd#14453

@mikebrow it looks like I am able to create releases? Is that new?

mikebrow · 2026-10-22T17:41:37Z

@aojea once the tag is ready I can update this branch containerd/containerd@main...MikeZappa87:containerd:mzappa/nriversionbump

containerd/containerd#14453

@mikebrow it looks like I am able to create releases? Is that new?

SGTM .. suggest also pinging the maintainer slack to see if there are objections....

aojea mentioned this pull request Oct 15, 2026

Fail RunPodSandbox before NRI timeout expires and optimize multi-NIC attachment kubernetes-sigs/dranet#370

Closed

mikebrow reviewed Oct 15, 2026

View reviewed changes

klihub requested review from klihub and mikebrow October 16, 2026 17:47

aojea force-pushed the failure-policy branch from 25504fb to 6dbf7d5 Compare October 17, 2026 08:50

aojea force-pushed the failure-policy branch 2 times, most recently from 3ec95b6 to 6ab72d5 Compare October 17, 2026 20:29

aojea force-pushed the failure-policy branch from 6ab72d5 to e4785d7 Compare October 18, 2026 08:34

mikebrow reviewed Oct 19, 2026

View reviewed changes

api: apply review wording suggestions to FailurePolicy

9ba3d2d

Apply the wording suggestions from the review of containerd#329 to the FailurePolicy comments in api.proto, and regenerate api.pb.go. Suggested-by: Mike Brown <brownwm@us.ibm.com> Signed-off-by: Antonio Ojea <aojea@google.com>

aojea force-pushed the failure-policy branch from e7ed322 to 0a7f870 Compare October 19, 2026 19:26

mikebrow approved these changes Oct 19, 2026

View reviewed changes

mikebrow mentioned this pull request Oct 20, 2026

Identity Plugin #288

Open

klihub approved these changes Oct 22, 2026

View reviewed changes

mikebrow merged commit 6606a2f into containerd:main Oct 22, 2026
16 checks passed

mikebrow added this to the 1.0 milestone Oct 22, 2026

mikebrow added the enhancement New feature or request label Oct 22, 2026

aojea mentioned this pull request Oct 27, 2026

nri: fail pod requests when the plugin times out kubernetes-sigs/dranet#401

Open

Conversation

aojea commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeZappa87 commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

klihub commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aojea commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aojea commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow commented Oct 19, 2026

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented Oct 22, 2026

Uh oh!

klihub left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aojea commented Oct 22, 2026

Uh oh!

MikeZappa87 commented Oct 22, 2026

Uh oh!

MikeZappa87 commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow commented Oct 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aojea commented Oct 15, 2026 •

edited

Loading

MikeZappa87 commented Oct 15, 2026 •

edited

Loading

klihub commented Oct 16, 2026 •

edited

Loading

aojea commented Oct 17, 2026 •

edited

Loading

mikebrow left a comment •

edited

Loading

aojea commented Oct 19, 2026 •

edited

Loading

mikebrow left a comment •

edited

Loading

klihub left a comment •

edited

Loading

MikeZappa87 commented Oct 22, 2026 •

edited

Loading