Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

/ok-to-test

There are some really significant changes between v0.1.0 and v0.2.0 that were effectively a rewrite and an execution mode change (short-lived invoked binaries vs. long-lived daemon-style plugins with a Unix domain socket). NRI is certainly experimental at v0.1.0, but it's a huge change to take in an LTS.

There are some really significant changes between v0.1.0 and v0.2.0 that were effectively a rewrite and an execution mode change (short-lived invoked binaries vs. long-lived daemon-style plugins with a Unix domain socket). NRI is certainly experimental at v0.1.0, but it's a huge change to take in an LTS.

Yes, I agree that taking the new v0.2.0 style new NRI execution model in use in the LTS release would be a no go.

But if I understand what is going on here then @thaJeztah only bumps the NRI (and a few other) dependency without making any NRI-related code changes to containerd itself. The aim with this PR is to try and break the containerd->NRI->containerd circular dependency, which was present from NRI v0.1.0 and caused by containerd.Task interface being verbatim referenced in the original v0.1.0 NRI client invocation interface (which is still present in NRI and used by the v0.1.0 adapter plugin). That circular dependency should be now fixed in v0.5.0.

I guess the other alternative would be to create an NRI v0.1.x release branch with commit da8a7e53614c60c106f824caf3d120ea582a0cbe cherry-picked, then update the 1.6 release with a SHA1-exact reference to that. I don't know which one is better/worse. I guess both have pros and cons. If the other dependency updates don't cause any problems, then this PR would perhaps opt for less divergence between the branches...

I guess the other alternative would be to create an NRI v0.1.x release branch with commit da8a7e53614c60c106f824caf3d120ea582a0cbe cherry-picked, then update the 1.6 release with a SHA1-exact reference to that.

We could also then tag that as v0.1.1. I think that'd be my preference for LTS, unless we're sure that the other changes are benign.

Yes, so this is the rabbit-hole I went down: I started this PR wondering "can this module be updated in the LTS branch? So that we can cut the circular dependency?".

I checked all the "bump" PRs against main to see learn about related changes (and possibly cherry-pick), and I saw the large amount of changes for the v0.2 update, but looking twice at those changes, they seemed to all be additions, and had to be wired-up to actually do something.

So.. would this work if I wouldn't wire it up? And, it looks like from containerd's perspective, the update is indeed minimal (:party:); here's the diff of Go changes in this PR;

git diff HEAD^ -- vendor/github.com/containerd/nri/**/*.go

diff --git a/vendor/github.com/containerd/nri/client.go b/vendor/github.com/containerd/nri/client.go
index 78c8fd094..a45138d36 100644
--- a/vendor/github.com/containerd/nri/client.go
+++ b/vendor/github.com/containerd/nri/client.go
@@ -25,10 +25,9 @@ import (
 	"os/exec"
 	"sync"
 
-	"github.com/containerd/containerd"
-	"github.com/containerd/containerd/oci"
 	types "github.com/containerd/nri/types/v1"
-	"github.com/pkg/errors"
+
+	oci "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 const (
@@ -74,13 +73,29 @@ type Sandbox struct {
 	Labels map[string]string
 }
 
+// process is a subset of containerd's Process interface.
+type process interface {
+	// ID of the process
+	ID() string
+	// Pid is the system specific process id
+	Pid() uint32
+}
+
+// Task is ta subset of containerd's Task interface.
+type Task interface {
+	process
+
+	// Spec returns the current OCI specification for the task
+	Spec(context.Context) (*oci.Spec, error)
+}
+
 // Invoke the ConfList of nri plugins
-func (c *Client) Invoke(ctx context.Context, task containerd.Task, state types.State) ([]*types.Result, error) {
+func (c *Client) Invoke(ctx context.Context, task Task, state types.State) ([]*types.Result, error) {
 	return c.InvokeWithSandbox(ctx, task, state, nil)
 }
 
 // InvokeWithSandbox invokes the ConfList of nri plugins
-func (c *Client) InvokeWithSandbox(ctx context.Context, task containerd.Task, state types.State, sandbox *Sandbox) ([]*types.Result, error) {
+func (c *Client) InvokeWithSandbox(ctx context.Context, task Task, state types.State, sandbox *Sandbox) ([]*types.Result, error) {
 	if len(c.conf.Plugins) == 0 {
 		return nil, nil
 	}
@@ -107,7 +122,7 @@ func (c *Client) InvokeWithSandbox(ctx context.Context, task containerd.Task, st
 		r.Conf = p.Conf
 		result, err := c.invokePlugin(ctx, p.Type, r)
 		if err != nil {
-			return nil, errors.Wrapf(err, "plugin: %s", p.Type)
+			return nil, fmt.Errorf("plugin: %s: %w", p.Type, err)
 		}
 		r.Results = append(r.Results, result)
 	}
@@ -141,7 +156,7 @@ func (c *Client) invokePlugin(ctx context.Context, name string, r *types.Request
 	}
 	var result types.Result
 	if err := json.Unmarshal(out, &result); err != nil {
-		return nil, errors.Errorf("failed to unmarshal plugin output: %s: %s", err.Error(), msg)
+		return nil, fmt.Errorf("failed to unmarshal plugin output %s: %w", msg, err)
 	}
 	if result.Err() != nil {
 		return nil, result.Err()
diff --git a/vendor/github.com/containerd/nri/types/v1/types.go b/vendor/github.com/containerd/nri/types/v1/types.go
index c46827807..0c3d0d95a 100644
--- a/vendor/github.com/containerd/nri/types/v1/types.go
+++ b/vendor/github.com/containerd/nri/types/v1/types.go
@@ -18,8 +18,7 @@ package v1
 
 import (
 	"encoding/json"
-
-	"github.com/pkg/errors"
+	"errors"
 )

So from containerd's perspective it looks ok. For consumers of containerd as module, it will force them to update the dependency as well, which is both the bit we "want" (cut circular dependencies), and "not" (?) want (newer NRI features); but the NRI module code I'm pulling in through this PR did not advertise anything about "what features are supported", so "what does?"

... and that's basically how my eye landed on the CRI API version ... and my questions about that, because containerd v1.6 already using CRI API v0.25.x (thus potentially indicating "I support k8s v1.25??") felt off 😂

We could also then tag that as v0.1.1. I think that'd be my preference for LTS, unless we're sure that the other changes are benign.

If we'd do that; does that mean we could downgrade the CRI-API to v0.22.5?

Yes, so this is the rabbit-hole I went down: I started this PR wondering "can this module be updated in the LTS branch? So that we can cut the circular dependency?".

I checked all the "bump" PRs against main to see learn about related changes (and possibly cherry-pick), and I saw the large amount of changes for the v0.2 update, but looking twice at those changes, they seemed to all be additions, and had to be wired-up to actually do something.

So.. would this work if I wouldn't wire it up? And, it looks like from containerd's perspective, the update is indeed minimal (:party:); here's the diff of Go changes in this PR;

git diff HEAD^ -- vendor/github.com/containerd/nri/**/*.go

diff --git a/vendor/github.com/containerd/nri/client.go b/vendor/github.com/containerd/nri/client.go
index 78c8fd094..a45138d36 100644
--- a/vendor/github.com/containerd/nri/client.go
+++ b/vendor/github.com/containerd/nri/client.go
@@ -25,10 +25,9 @@ import (
 	"os/exec"
 	"sync"
 
-	"github.com/containerd/containerd"
-	"github.com/containerd/containerd/oci"
 	types "github.com/containerd/nri/types/v1"
-	"github.com/pkg/errors"
+
+	oci "github.com/opencontainers/runtime-spec/specs-go"
 )
...
 import (
 	"encoding/json"
-
-	"github.com/pkg/errors"
+	"errors"
 )

So from containerd's perspective it looks ok. For consumers of containerd as module, it will force them to update the dependency as well, which is both the bit we "want" (cut circular dependencies), and "not" (?) want (newer NRI features); but the NRI module code I'm pulling in through this PR did not advertise anything about "what features are supported", so "what does?"

But as you pointed out, we would not be really pulling in any of the newer NRI features. We'd simply educe the containerd.Task interface surface/requirements to the subset which is really in use by the original v0.1.0-tagged NRI version. Wouldn't that make such a change then acceptable ?

The other change in the diff is native error wrapping instead of pkg/errors. I guess that could be a problem at the moment as it was introduced in golang 1.20 and release/1.6 states 1.19 as the golang requirement. OTOH, golang 1.19 has been EOL'd since 08-Aug-2023... so I probably that'll need to be bumped anyway (should fall into the 'including library dependency, toolchain (including Go) and other version updates which are needed to ensure each release is built with fully supported dependencies and remains usable by containerd clients'-category of a 'wider range patches').

We could also then tag that as v0.1.1. I think that'd be my preference for LTS, unless we're sure that the other changes are benign.

If we'd do that; does that mean we could downgrade the CRI-API to v0.22.5?

Simply trying to fit that into the context of what the LTS release description states, I suspect that would be a 'no', even if that is mostly based on what is omitted there instead of what is being said.

• I opened [release/0.1 backport] remove containerd as dependency nri#58 with a cherry-pick

I'll update this to use the v0.1.1 tag if we agree on going that path (i.e. if we merge and tag containerd/nri#58)

v0.1.1 was tagged; moved this out of draft 👍