Implement STATUS #121

architkulkarni · 2024-10-14T22:39:28Z

Based on @MikeZappa87's earlier PR #114.

This commit implements the STATUS CNI verb. It adds a new function StatusDetail to return the result of STATUS for each network.

Related info:

https://github.com/containernetworking/cni/blob/main/SPEC.md

This commit implements the STATUS CNI verb. It adds a new function StatusDetail which returns a list with the status of each network. Signed-off-by: Michael Zappa <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]> Co-authored-by: Michael Zappa <[email protected]>

cni.go

samuelkarp · 2024-10-14T23:56:27Z

cni.go

+	var networks []*NetworkStatus
+
+	for _, network := range c.Networks() {
+		if network.config.Name == "cni-loopback" {


Why this specific magic name? Can we move it out to a constant with a comment explaining?

@MikeZappa87 do you know the answer?

Actually, would it make sense to just delete this special case entirely? Even if the status will always just be "ready" for cni-loopback, it might make the code cleaner and more futureproof.

We are deprecating the cni-loopback and the return will always be the same regardless.

Done, added a comment and moved it to a constant: be034ef

cni.go

Signed-off-by: Archit Kulkarni <[email protected]>

Co-authored-by: Samuel Karp <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2024-10-18T21:50:07Z

types.go

@@ -60,3 +60,8 @@ type DNS struct {
 	// List of DNS options.
 	Options []string
 }
+
+type NetworkStatus struct {
+	Network *Network


Do we need to expose anything else here? Right now all the fields of Network are private.

Is it enough to make ifName public?

go-cni/namespace.go

Lines 26 to 30 in a45b44a

type Network struct {

cni cnilibrary.CNI

config *cnilibrary.NetworkConfigList

ifName string

}

As a consumer of the Status API does making this private or public help? Exposing ifName might be useful especially if we ever begin to support multiple interfaces upstream.

@MikeZappa87 The first use case that came to mind was just the error message, like if the kubelet sees that one of the entries of StatusDetail has an error, in the log it can report ifName along with the error. But there are probably other possible use cases I'm not aware of.

What about config, should we expose the whole config or just certain fields like Name, CNIVersion?

type NetworkConfigList struct { Name string CNIVersion string DisableCheck bool DisableGC bool LoadOnlyInlinedPlugins bool Plugins []*PluginConfig Bytes []byte }

pravk03 · 2024-10-23T00:32:23Z

cni.go

+		return nil, err
+	}
+
+	var networks []*NetworkStatus


nit: rename to networkStatus ?

Done, renamed to networkStatuses because it's a list: 03fbaaf

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2024-10-23T21:49:51Z

I don't understand the linter errors that appeared after the last two commits, they seem unrelated. Any ideas?

mikebrow · 2024-10-25T16:49:21Z

I don't understand the linter errors that appeared after the last two commits, they seem unrelated. Any ideas?

Dunno.. the last commit success build here was in june.. you could try an empty commit (1line doc change) and see if our current version of stretcher 1.8.0 , golang, lint all still work .. noting that the core repo has moved up with stretcher 1.9 now the latest

architkulkarni · 2024-10-25T18:28:05Z

A bit weird because it passed two commits ago on this PR last week: https://github.com/containerd/go-cni/actions/runs/11351584389/job/31572471088 But let me try what you suggested

mikebrow · 2024-10-25T19:22:02Z

A bit weird because it passed two commits ago on this PR last week: https://github.com/containerd/go-cni/actions/runs/11351584389/job/31572471088 But let me try what you suggested

ikr.. sure enough though even trivial non code no longer passes

mikebrow · 2024-10-25T19:34:48Z

let's see on the working run you linked for the linter 1.21.8 test we have:
setup go v3 action results in go version 1.21.13 looks good odd regarding the matrix though having the wrong version 1.21.8 vs 1.21.13
lint v3 action gives v1.56.1

on the failure here for the same 1.21.8 bucket we have
setup go v1.23.2 uhmm...
and lint v1.56.1 wonder if that combo is expected to work?

MikeZappa87 · 2024-11-17T01:55:44Z

cni_test.go

@@ -374,6 +376,12 @@ func TestLibCNIType120(t *testing.T) {

 	err = l.Remove(ctx, "container-id1", "/proc/12345/ns/net")
 	assert.NoError(t, err)
+
+	statuses, err := l.StatusDetail(ctx)


The fake configuration that is being generated, is that 1.1.0? Can you verify that I don't think the Status verb is actually being executed.

@MikeZappa87 Thanks for the comment, just want to make sure I understand everything -- In the approach currently taken by this PR, StatusDetail isn't called anywhere yet (the proposal was to have containerd call it), so you're right that the Status verb isn't executed unless we explicitly call StatusDetail. Since we do call StatusDetail in the test here, I think this line https://github.com/containerd/go-cni/pull/121/files#diff-6cdf85f441267859d068399676e5f0ad8946811bd7e951b4e80db98d9fd2489cR318 asserts that the (mocked) status verb is being executed.

I don't see 1.1.0 specified anywhere in the fake config, but I think that doesn't affect the test because the version check happens inside cni.GetStatusNetworkList, and this function is mocked out. Is that right?

architkulkarni · 2024-12-09T19:01:58Z

Closed in favor of #123

mikebrow · 2024-12-09T20:33:42Z

Closed in favor of #123

we could probably still use detail for each of the plugins vs fail first..?

architkulkarni · 2024-12-17T21:36:35Z

Sure, reopening

architkulkarni marked this pull request as ready for review October 14, 2024 22:43

architkulkarni force-pushed the archit-status2 branch from 67c32dd to a45b44a Compare October 14, 2024 23:39

samuelkarp reviewed Oct 14, 2024

View reviewed changes

architkulkarni and others added 2 commits October 15, 2024 17:48

Add unit test for StatusDetail()

7afa42f

Signed-off-by: Archit Kulkarni <[email protected]>

Update cni.go

a7839ab

Co-authored-by: Samuel Karp <[email protected]> Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni force-pushed the archit-status2 branch from 90ca69d to a7839ab Compare October 15, 2024 17:49

architkulkarni mentioned this pull request Oct 15, 2024

Implement CNI STATUS #120

Closed

architkulkarni commented Oct 18, 2024

View reviewed changes

pravk03 reviewed Oct 23, 2024

View reviewed changes

architkulkarni added 2 commits October 23, 2024 21:33

Add comment about loopback network

1d06125

Signed-off-by: Archit Kulkarni <[email protected]>

Rename networks to networkStatuses

b110ac0

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni force-pushed the archit-status2 branch from 03fbaaf to b110ac0 Compare October 23, 2024 21:34

architkulkarni mentioned this pull request Oct 25, 2024

Bump github actions dependencies to match containerd CI repo and fix lint #122

Merged

MikeZappa87 reviewed Nov 17, 2024

View reviewed changes

architkulkarni closed this Dec 9, 2024

architkulkarni reopened this Dec 17, 2024

architkulkarni marked this pull request as draft December 17, 2024 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement STATUS #121

Implement STATUS #121

architkulkarni commented Oct 14, 2024

samuelkarp Oct 14, 2024

architkulkarni Oct 15, 2024

MikeZappa87 Oct 23, 2024

architkulkarni Oct 23, 2024

architkulkarni Oct 18, 2024

architkulkarni Oct 23, 2024

MikeZappa87 Oct 28, 2024 •

edited

Loading

architkulkarni Oct 29, 2024

pravk03 Oct 23, 2024

architkulkarni Oct 23, 2024

architkulkarni commented Oct 23, 2024

mikebrow commented Oct 25, 2024

architkulkarni commented Oct 25, 2024

mikebrow commented Oct 25, 2024

mikebrow commented Oct 25, 2024

MikeZappa87 Nov 17, 2024

architkulkarni Nov 18, 2024

architkulkarni commented Dec 9, 2024

mikebrow commented Dec 9, 2024

architkulkarni commented Dec 17, 2024

	type Network struct {
	cni cnilibrary.CNI
	config *cnilibrary.NetworkConfigList
	ifName string
	}

Implement STATUS #121

Are you sure you want to change the base?

Implement STATUS #121

Conversation

architkulkarni commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeZappa87 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Oct 23, 2024

mikebrow commented Oct 25, 2024

architkulkarni commented Oct 25, 2024

mikebrow commented Oct 25, 2024

mikebrow commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Dec 9, 2024

mikebrow commented Dec 9, 2024

architkulkarni commented Dec 17, 2024

MikeZappa87 Oct 28, 2024 •

edited

Loading