diff --git a/pkg/adaptation/failure_policy_test.go b/pkg/adaptation/failure_policy_test.go index 87fae80f..d0b19ee2 100644 --- a/pkg/adaptation/failure_policy_test.go +++ b/pkg/adaptation/failure_policy_test.go @@ -32,24 +32,17 @@ import ( validator "github.com/containerd/nri/plugins/default-validator/builtin" ) -// These tests run the failure policy through the full stack: stub, ttrpc -// and adaptation. They check the facts that the unit tests in plugin_test.go -// cannot check: +// These tests run the failure policy through the stub, ttrpc, and adaptation: // // - the stub sends the policy and the adaptation applies it, -// - a real request timeout and a real lost connection produce the errors -// that the adaptation expects, -// - the adaptation does not call the remaining plugins for a failed -// request, -// - a plugin that is kept connected receives the next request, -// - a deadline set by the runtime does not count as a plugin failure, -// - a plugin whose connection is closed does not receive requests and -// does not fail them. -// -// The unit tests cover the full matrix of errors and policies. - -// requestTimeout is the plugin request timeout for these tests. It is short -// to keep the tests fast. A plugin that must exceed it blocks until released. +// - request timeouts and lost connections produce the expected errors, +// - the adaptation skips remaining plugins when a request fails, +// - a plugin kept connected receives subsequent requests, +// - a deadline set by the runtime does not count as a plugin failure, and +// - the adaptation skips closed plugins. + +// requestTimeout is the plugin request timeout for these tests. A plugin +// that must exceed it blocks until released. const requestTimeout = 100 * time.Millisecond // lifecycleRequests sends each pod and container request to the adaptation. @@ -220,9 +213,8 @@ func TestPluginFailurePolicy(t *testing.T) { } ) - // A plugin without a policy keeps the old behavior. On a timeout, the - // adaptation drops the plugin and the request proceeds. The runtime - // does not see the timeout (#316). + // For a plugin without a policy, a timeout drops the plugin and the + // request proceeds. The runtime does not see the timeout (#316). t.Run("no policy: a timed out plugin is dropped and the request proceeds", func(t *testing.T) { slow := newPlugin("00", "slow", nil) next := newPlugin("10", "next", nil) @@ -295,8 +287,8 @@ func TestPluginFailurePolicy(t *testing.T) { var conn stdnet.Conn crash := newPlugin("00", "crash", dropAndFail) next := newPlugin("10", "next", nil) - // Keep the plugin connection so that the handler can close it. This - // emulates a plugin that dies during a request. + // Retain the connection so the handler can close it, simulating + // a plugin dying during a request. crash.options = append(crash.options, stub.WithDialer(func(path string) (stdnet.Conn, error) { c, err := stdnet.Dial("unix", path) @@ -534,16 +526,15 @@ func TestPluginFailureScenarios(t *testing.T) { require.False(t, slow10.EventQ().Has(PluginDisconnected)) }) - // Scenario B: a plugin that never answers. There is no health check and - // no counter of failures. + // Scenario B: a plugin that never answers. NRI does not perform health + // checks or count failures. // - // Today, without a policy: each request drops the plugin. The plugin - // reconnects, is still hung, and the next request times out and drops + // Without a policy, each request drops the plugin. When the plugin + // reconnects while still hung, the next request times out and drops // it again. Each request waits the full timeout. // - // With keep_connected: the same, without the reconnect. Each request - // waits the full timeout and the plugin stays registered. With - // fail_request too, each request fails. + // With keep_connected, each request waits the full timeout and the + // plugin stays registered. With fail_request, each request also fails. t.Run("B: a plugin that never answers delays each request by the full timeout", func(t *testing.T) { t.Run("no policy: dropped, reconnects, dropped again", func(t *testing.T) { hung := newPlugin("00", "hung", nil) @@ -557,7 +548,7 @@ func TestPluginFailureScenarios(t *testing.T) { require.NoError(t, hung.Wait(PluginDisconnected, time.After(startupTimeout))) // The plugin restarts. A new instance of the same plugin - // registers, and it is still hung. + // registers and remains hung. restarted := newPlugin("00", "hung", nil) blockPlugin(t, restarted) s.StartPlugins(restarted) @@ -587,12 +578,12 @@ func TestPluginFailureScenarios(t *testing.T) { }) // Scenario C: plugin 10 depends on the adjustments of plugin 00. Plugin - // 00 has no policy and times out in CreateContainer. + // 00 does not have a policy and times out in CreateContainer. // - // Today: NRI drops plugin 00 and calls plugin 10 with a container that - // lacks the adjustments of plugin 00. Nothing in the request tells - // plugin 10 that plugin 00 did not run. The container is created - // without the adjustments. + // NRI drops plugin 00 and calls plugin 10 with a container that lacks + // the adjustments of plugin 00. Nothing in the request tells plugin 10 + // that plugin 00 did not run. The container is created without the + // adjustments. t.Run("C: a plugin runs after a dropped plugin without knowing it", func(t *testing.T) { var ( b = newBlocker(t) @@ -600,7 +591,7 @@ func TestPluginFailureScenarios(t *testing.T) { slow = newPlugin("00", "slow", nil) next = newPlugin("10", "next", nil) ) - // Plugin 00 would add an environment variable, but it blocks. + // Plugin 00 blocks in CreateContainer before returning an adjustment. slow.createContainer = func(*mockPlugin, *api.PodSandbox, *api.Container) (*api.ContainerAdjustment, []*api.ContainerUpdate, error) { if err := b.handle(); err != nil { return nil, nil, err @@ -629,14 +620,13 @@ func TestPluginFailureScenarios(t *testing.T) { require.Empty(t, rpl.GetAdjust().GetEnv()) }) - // Scenario D: the required plugins check of the default validator is - // the existing guard for plugins that must be present. + // Scenario D: the required plugins check of the default validator + // guards plugins that must be present. // - // Today: the check uses the list of plugins consulted for the - // container. NRI adds a plugin to the list before it relays the - // request. A plugin that times out and is dropped is still in the - // list. The check does not catch it, and the container is created - // without the required plugin being present. + // The check uses the list of plugins consulted for the container. NRI + // adds a plugin to the list before relaying the request. A plugin that + // times out and is dropped remains in the list. The check does not + // catch it. The container is created without the required plugin. t.Run("D: the required plugins check does not catch a dropped plugin", func(t *testing.T) { b := newBlocker(t) slow := newPlugin("00", "slow", nil) diff --git a/pkg/adaptation/plugin.go b/pkg/adaptation/plugin.go index c85ac2c4..2a669107 100644 --- a/pkg/adaptation/plugin.go +++ b/pkg/adaptation/plugin.go @@ -945,22 +945,19 @@ func (p *plugin) warnDeprecatedEvent(ctx context.Context, event Event) { } // handleRequestError handles an error from a relayed request. ctx is the -// context of the runtime. It is not the context with the request timeout. +// runtime context without the per-request timeout. // -// Two kinds of errors are not plugin failures. The function returns them -// unchanged and the request fails: +// The function returns errors unchanged when: // -// - an error returned by the plugin, -// - any error after the runtime cancelled ctx or its deadline expired. -// The plugin is not responsible for that, even if the request timeout -// expired too. +// - the plugin returned the error, or +// - the runtime cancelled ctx or the runtime deadline expired. // // A fatal error is a plugin failure: the plugin did not answer the request. -// The failure policy of the plugin applies. The function takes two +// The failure policy of the plugin applies. The function makes two // independent decisions. It drops the plugin unless the policy keeps it // (see dropsPlugin). It fails the request only if the policy says so. // Otherwise the request proceeds without the plugin. A returned error -// names the plugin and the request, because the runtime reports it. +// names the plugin and the request because the runtime reports it. func (p *plugin) handleRequestError(ctx context.Context, event Event, err error) error { if ctx.Err() != nil || !isFatalError(err) { return err @@ -984,8 +981,7 @@ func (p *plugin) handleRequestError(ctx context.Context, event Event, err error) // dropsPlugin returns true if the plugin must be closed and unregistered // for the error. A fatal error drops the plugin, with one exception: a -// request timeout does not drop a plugin with KeepConnected set. The timeout -// only means that the plugin did not handle this request in time. A lost +// request timeout does not drop a plugin with KeepConnected set. A lost // connection or a protocol error always drops the plugin. func (p *plugin) dropsPlugin(err error) bool { if !isFatalError(err) { diff --git a/pkg/adaptation/plugin_test.go b/pkg/adaptation/plugin_test.go index fac6aa72..799efd44 100644 --- a/pkg/adaptation/plugin_test.go +++ b/pkg/adaptation/plugin_test.go @@ -31,8 +31,8 @@ import ( "github.com/containerd/nri/pkg/adaptation/builtin" ) -// The four possible failure policies. The zero value is the old NRI -// behavior: drop the plugin and let the request proceed without it. +// The four possible failure policies. The zero value drops the plugin +// and lets the request proceed without it. var ( dropAndProceed = &FailurePolicy{} dropAndFail = &FailurePolicy{FailRequest: true} @@ -51,7 +51,7 @@ func policyName(policy *FailurePolicy) string { } // testPlugin returns a builtin plugin with the given failure policy. A -// builtin plugin has no connection, so close() is safe to call. +// builtin plugin does not have a connection. Calling close() is safe. func testPlugin(policy *FailurePolicy) *plugin { return &plugin{ idx: "00", @@ -61,8 +61,8 @@ func testPlugin(policy *FailurePolicy) *plugin { } } -// expiredContext returns a context with an expired deadline. This is what -// the runtime context looks like when the runtime timed out, not NRI. +// expiredContext returns a context with an expired deadline, simulating +// an expired runtime context. func expiredContext(t *testing.T) context.Context { t.Helper() ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) @@ -76,8 +76,8 @@ var ( errWrappedTimeout = fmt.Errorf("call failed: %w", context.DeadlineExceeded) errProtocol = fmt.Errorf("unexpected message: %w", ttrpc.ErrProtocol) errPlugin = errors.New("plugin rejected the request") - // A plugin that returns context.DeadlineExceeded. ttrpc delivers it - // as a status error, so it does not match the context error. + // A plugin returning context.DeadlineExceeded. ttrpc delivers it + // as a status error that does not match the context error. errPluginTimeout = status.Error(codes.DeadlineExceeded, "too slow") ) @@ -172,10 +172,9 @@ func TestHandleRequestError(t *testing.T) { }) } - // Errors that are not plugin failures are returned unchanged. They - // never affect the plugin, whatever the policy. This covers errors - // returned by the plugin, cancelled requests, and any error after the - // runtime context is done. + // Non-fatal errors are returned unchanged and do not affect the + // plugin, regardless of policy. This covers errors returned by the + // plugin, cancelled requests, and expired runtime contexts. for _, policy := range allPolicies { for _, tc := range []struct { name string diff --git a/pkg/api/api.pb.go b/pkg/api/api.pb.go index b42124f6..fcd3a818 100644 --- a/pkg/api/api.pb.go +++ b/pkg/api/api.pb.go @@ -1064,10 +1064,9 @@ type ConfigureResponse struct { // Events to subscribe the plugin for. Each bit set corresponds to an // enumerated Event. Events int32 `protobuf:"varint,2,opt,name=events,proto3" json:"events,omitempty"` - // How the runtime handles a failure of the plugin to receive and respond - // to a request within a timeout period. If unset, all fields have their - // default value. The defaults match the behavior of NRI before this field - // existed. + // How the runtime handles a failure of the plugin to process a request. + // When unset, the runtime disconnects the plugin and lets the request + // proceed without it. FailurePolicy *FailurePolicy `protobuf:"bytes,3,opt,name=failure_policy,json=failurePolicy,proto3" json:"failure_policy,omitempty"` } @@ -1117,36 +1116,32 @@ func (x *ConfigureResponse) GetFailurePolicy() *FailurePolicy { return nil } -// FailurePolicy tells the runtime what to do when a plugin fails to receive -// and respond to a request within a timeout period. -// Failure types covered by the FailurePolicy include request timeout, lost -// connection, and protocol error. Non-transport/timeout errors, such as a -// plugin returning a rejection to the request, are not covered by the -// FailurePolicy. +// FailurePolicy specifies how the runtime handles a plugin failure during +// request processing. Failures include request timeouts, lost connections, +// and protocol errors. Errors returned by the plugin always fail the +// request and are not governed by this policy. // -// The runtime takes two independent decisions for each failure: one about +// The runtime makes two independent decisions for each failure: one about // the request and one about the plugin. Each field controls one decision. -// The default of each field is the behavior of NRI before this field -// existed. A plugin that does not set a policy is treated as before: the -// runtime disconnects it and the request proceeds without it. +// When unset, the runtime disconnects the plugin and lets the request +// proceed without it. type FailurePolicy struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - // When fail_request is set to true, fail the request is the policy. The - // error names the plugin and the request, like an error returned by the - // plugin. The remaining plugins do not receive the request. The - // fail_request policy defaults to false when unset. False results in the - // request proceeding on to the remaining plugins, as if the plugin were - // not subscribed to it. + // Fail the request if the plugin fails. The error names the plugin and + // the request, like an error returned by the plugin. The remaining + // plugins do not receive the request. Default: false; the request + // proceeds with the remaining plugins, as if the failed plugin were not + // subscribed to it. FailRequest bool `protobuf:"varint,1,opt,name=fail_request,json=failRequest,proto3" json:"fail_request,omitempty"` // Keep the plugin registered when it exceeds the request timeout. The // plugin continues receiving requests. A plugin that stays unresponsive - // delays each request it receives by the full timeout. Default: the - // runtime disconnects and unregisters the plugin. This field only applies - // to timeouts. A plugin that loses its connection or causes a protocol - // error is always disconnected. + // delays each request it receives by the full timeout. Default: false; + // the runtime disconnects and unregisters the plugin. This field only + // applies to timeouts. A plugin that loses its connection or causes a + // protocol error is always disconnected. KeepConnected bool `protobuf:"varint,2,opt,name=keep_connected,json=keepConnected,proto3" json:"keep_connected,omitempty"` } diff --git a/pkg/api/api.proto b/pkg/api/api.proto index bec79c60..0f0d98b2 100644 --- a/pkg/api/api.proto +++ b/pkg/api/api.proto @@ -183,39 +183,34 @@ message ConfigureResponse { // Events to subscribe the plugin for. Each bit set corresponds to an // enumerated Event. int32 events = 2; - // How the runtime handles a failure of the plugin to receive and respond - // to a request within a timeout period. If unset, all fields have their - // default value. The defaults match the behavior of NRI before this field - // existed. + // How the runtime handles a failure of the plugin to process a request. + // When unset, the runtime disconnects the plugin and lets the request + // proceed without it. FailurePolicy failure_policy = 3; } -// FailurePolicy tells the runtime what to do when a plugin fails to receive -// and respond to a request within a timeout period. -// Failure types covered by the FailurePolicy include request timeout, lost -// connection, and protocol error. Non-transport/timeout errors, such as a -// plugin returning a rejection to the request, are not covered by the -// FailurePolicy. +// FailurePolicy specifies how the runtime handles a plugin failure during +// request processing. Failures include request timeouts, lost connections, +// and protocol errors. Errors returned by the plugin always fail the +// request and are not governed by this policy. // -// The runtime takes two independent decisions for each failure: one about +// The runtime makes two independent decisions for each failure: one about // the request and one about the plugin. Each field controls one decision. -// The default of each field is the behavior of NRI before this field -// existed. A plugin that does not set a policy is treated as before: the -// runtime disconnects it and the request proceeds without it. +// When unset, the runtime disconnects the plugin and lets the request +// proceed without it. message FailurePolicy { - // When fail_request is set to true, fail the request is the policy. The - // error names the plugin and the request, like an error returned by the - // plugin. The remaining plugins do not receive the request. The - // fail_request policy defaults to false when unset. False results in the - // request proceeding on to the remaining plugins, as if the plugin were - // not subscribed to it. + // Fail the request if the plugin fails. The error names the plugin and + // the request, like an error returned by the plugin. The remaining + // plugins do not receive the request. Default: false; the request + // proceeds with the remaining plugins, as if the failed plugin were not + // subscribed to it. bool fail_request = 1; // Keep the plugin registered when it exceeds the request timeout. The // plugin continues receiving requests. A plugin that stays unresponsive - // delays each request it receives by the full timeout. Default: the - // runtime disconnects and unregisters the plugin. This field only applies - // to timeouts. A plugin that loses its connection or causes a protocol - // error is always disconnected. + // delays each request it receives by the full timeout. Default: false; + // the runtime disconnects and unregisters the plugin. This field only + // applies to timeouts. A plugin that loses its connection or causes a + // protocol error is always disconnected. bool keep_connected = 2; } diff --git a/pkg/stub/stub.go b/pkg/stub/stub.go index 82e86d92..b4d46fbd 100644 --- a/pkg/stub/stub.go +++ b/pkg/stub/stub.go @@ -284,9 +284,8 @@ func WithLogger(logger nrilog.Logger) Option { } // WithFailurePolicy sets how the runtime handles a failure of the plugin to -// process a request. A failure is a request timeout, a lost connection or a -// protocol error. The policy has two independent fields. The default of each -// field is the behavior of NRI before failure policies existed: +// process a request. A failure is a request timeout, a lost connection, or a +// protocol error. The policy has two independent fields: // // - FailRequest: fail the request with an error. Default: the request // proceeds without the plugin. @@ -294,8 +293,8 @@ func WithLogger(logger nrilog.Logger) Option { // timeout. Default: the runtime disconnects the plugin. A plugin that // loses its connection is always disconnected. // -// An error returned by the plugin always fails the request. A runtime with -// an NRI version without failure policy support ignores this option. +// An error returned by the plugin always fails the request. A runtime that +// does not support failure policies ignores this option. func WithFailurePolicy(policy *api.FailurePolicy) Option { return func(s *stub) error { if policy == nil {