api,adaptation,stub: revise policy comments

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,20 +578,20 @@ 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)
 			seenEnv []string
 			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)

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) {

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

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;
 }
 

pkg/stub/stub.go

@@ -284,18 +284,17 @@ 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.
 //   - KeepConnected: keep the plugin registered when it exceeds the request
 //     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 {