testing: test invocation of newer plugins with an older libcni #307
Conversation
squeed
force-pushed
the
backwards-compatability-tests
branch
from
099bbd8
to
3bb2044
Compare
|
lgtm |
| result, err := cniConfig.AddNetwork(netConf, runtimeConf) | ||
| fmt.Printf("Result: %+v", result) | ||
| if err != nil { | ||
| fmt.Printf("noop add failed: %+v", err) |
There was a problem hiding this comment.
this isn't the no-op plugin, right? maybe it could read cni add failed instead?
| IfName: ifName, | ||
| } | ||
|
|
||
| cniConfig := &libcni.CNIConfig{Path: strings.Split(os.Getenv("PATH"), ":")} |
There was a problem hiding this comment.
This assumes that the ptp plugin is in the PATH when the tests are run. That appears to be true of the top-level ./test script, but it isn't generally true. For example, if I just run
go test github.com/containernetworking/cni/libcni
from my shell, then I get a failure with error
failed to find plugin "ptp" in path [/usr/local/sbin /usr/local/bin /usr/sbin /usr/bin /sbin /bin]
Perhaps you can build the ptp plugin outside of this program, e.g. as part of the new spec you're adding to backwards_compatibility_test.go. Then pass in the path to that binary as an env var (e.g. CNI_PATH) or argument to this program. For an example of how to build a plugin as part of a test-run, look here.
That has the added benefit of making it clearer, when reading the test, that you're running a legacy runtime against a current-version plugin. Because you can see the plugin gets built right in the test.
There was a problem hiding this comment.
Good point, I'll clean this up.
A fun thing about some of the plugin tests is that they need to be run as root. libcni's tests were actually an exception - but not anymore. The test script does a sudo. The tests should probably have a bit of a cleaner warning (or maybe skip) when executed as non-root.
| } | ||
|
|
||
| func exec() int { | ||
| confStr := "{ \"name\": \"default\", \"type\":\"ptp\", \"ipam\":{ \"type\": \"host-local\", \"subnet\": \"10.1.2.0/24\"}}" |
There was a problem hiding this comment.
This config is not part of the legacy runtime. A user can change it independently of the runtime.
So I'd suggest that this string be declared in the spec It correctly handles the request from a legacy libcni that you're adding to backwards_compatibility_test.go, and then somehow injected into this program (e.g. via env var or command line args).
That would make it clearer, when reading the test, what exactly is being tested.
There was a problem hiding this comment.
Ahh, good point.
Is it safe to say that a runtime will not encounter a config file with a newer CNI version then it understands? I think so, since libcni takes a parsed NetConfig. So while the test matrix has gained another dimension, it's not too bad.
| @@ -50,4 +52,17 @@ var _ = Describe("Backwards compatibility", func() { | |||
|
|
|||
| Expect(os.RemoveAll(pluginPath)).To(Succeed()) | |||
| }) | |||
|
|
|||
| It("correctly handles the request from a legacy libcni", func() { | |||
| example := legacy_examples.V010_Client | |||
There was a problem hiding this comment.
Small nit-pick on naming: We tend to use nomenclature like "container runtime" or just "runtime" for the thing that calls CNI plugins. I think the test would read slightly better if the spec and variable names spoke of "legacy runtime" calling a "current-version plugin" or something like that.
|
Thanks @squeed, this is a great start, the right approach IMHO. I left a few small suggestions to make this test more portable and readable. |
squeed
force-pushed
the
backwards-compatability-tests
branch
2 times, most recently
from
663303d
to
4b5d9ad
Compare
|
@rosenhouse thanks for the feedback! Updated. |
|
@squeed needs a fix for gopath/src/github.com/containernetworking/cni/pkg/version/legacy_examples/example_runtime.go:27: syntax error: non-declaration statement outside function body |
squeed
force-pushed
the
backwards-compatability-tests
branch
from
4b5d9ad
to
52d618f
Compare
|
@dcbw sorry 'bout that. Off to buy a copy of vim for dummies. |
| _, err = procIn.Write([]byte(configStr)) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| err = procIn.Close() | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
You're just trying to pass a string in as standard input to the cmd, right?
If so, then just set
cmd.Stdin = strings.NewReader(configStr)before running gexec.Start. No need for StdinPipe with separateWrite and Close calls.
squeed
force-pushed
the
backwards-compatability-tests
branch
from
52d618f
to
8c6f6e0
Compare
This test compiles a trivial client with a fixed version libcni. It then creates and destroys a ptp network using the most recent plugin.
It's basically a simplified version of
ptp_test.goin a single file.