[DRAFT] Implement CNI Status #114

MikeZappa87 · 2024-02-26T20:56:38Z

The recent CNI specification provided a couple new verbs. This is to implement Status. GC and other verbs will be added in the future in different PR.

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

aojea · 2024-02-26T20:58:24Z

cni.go

-	// Status checks the status of the cni initialization
-	Status() error
+	// Status executes the status verb of the cni plugin
+	Status(ctx context.Context) error


great, it already existed the concept

Yes. Status detected if the minimum number of networks was met. In containerd cri-plugin, its hard coded to 2 neworks. Loopback and Default CNI network configuration. I changed the existing to 'Ready' and have Status calling the status function out of libcni.

aojea · 2024-02-26T20:59:44Z

cni.go

+	}
+
+	for _, network := range c.Networks() {
+		err = network.Status(ctx)


this is something you may need to document, as this means if any of the networks is not ready, the status is not ready

You may want to do, if just one network is ready then status is ready 🤷

For the cri-plugin case, we load two networks. One is the loopback plugin as separate (

go-cni/opts.go

Line 88 in 6603d5b

func WithLoNetwork(c *libcni) error {

)
The other 'network' is the first CNI network configuration in /etc/cni/net.d.

https://github.com/containerd/containerd/blob/7f0f49b43825b76e0271ed0102bf7df560826046/internal/cri/server/service_linux.go#L113

Since this is hard coded in containerd, we probably don't need to worry about this. However we could have a configurable failure behavior. If 1 fails, they all fail, if one is successful the node has the network ready condition set. We could also return a structure that has the CNI network name and error? This would return a slice of this structure.

@mikebrow thoughts?

If one is the loopback plugin, taht is always going to be ready and not be able to provide connectivity, then it makes sense for ready to be an AND of all plugins instead of an OR, as you will need the loopback AND the other CNI to be able to provide networking to the pods

I was wondering if this should be left up to the caller of the Status function? In this case for the containerd cri plugin I think it makes sense for AND however go-cni is used by others as well, we probably shouldn't dictate behavior?

It should be AND until we have a config or parameter from the client indicating "optional"

MikeZappa87 · 2024-02-29T20:10:39Z

@aojea Since the kubelet calls the CRI-API Status RPC: https://github.com/containerd/containerd/blob/6333db77015764f7d45b407094a3bc2cc05c239b/internal/cri/server/status.go#L49

Kubelet: https://github.com/kubernetes/kubernetes/blob/3718932d9a3ea459217ca11efe1ae9ead6aa92c1/pkg/kubelet/kubelet.go#L2886

The Status returning an error will set the node to not ready. I did notice after about 2 hours, the pods with netns, started going to completed. It might have been my test however I think we should discuss.

mikebrow

let's do it with a v2 api.. since v2 is not compatible with v1...

mikebrow · 2024-03-27T15:02:25Z

cni.go

-	// Status checks the status of the cni initialization
-	Status() error
+	// Status executes the status verb of the cni plugin
+	Status(ctx context.Context) ([]*NetworkStatus, error)


StatusDetail(ctx)...?

mikebrow · 2024-03-27T15:03:26Z

cni.go

+	}
+
+	return networks, nil
+}


check.. eol at elf

mikebrow · 2024-03-27T15:03:56Z

types.go

+type NetworkStatus struct {
+	Network *Network
+	Status   error
+}


architkulkarni · 2024-07-11T17:05:44Z

Hi @MikeZappa87, do you plan to work on this? If not, I can try to work on it as a new contributor.

MikeZappa87 · 2024-07-11T17:24:56Z

Hi @MikeZappa87, do you plan to work on this? If not, I can try to work on it as a new contributor.

Hello, I need to finish this up. I started it and we needed to get a few PR's merged prior to finishing this. If you want to take it on feel free! :-)

architkulkarni · 2024-07-11T23:17:33Z

Thanks, I'll give it a shot!

architkulkarni · 2024-07-15T23:06:29Z

Hi @MikeZappa87, a couple basic questions for you if you have time:

What's the v2 API, is there a reference somewhere?
(Maybe related) The current draft PR changes the signature of Status(), and you mentioned above that kubelet calls this function already. How do we handle backwards compatibility for things like this?

More generally, it would be helpful to know what work is remaining and if we need to wait for any other PRs to be merged first. Thanks!

aojea · 2024-08-27T07:50:31Z

@aojea Since the kubelet calls the CRI-API Status RPC: https://github.com/containerd/containerd/blob/6333db77015764f7d45b407094a3bc2cc05c239b/internal/cri/server/status.go#L49

Kubelet: https://github.com/kubernetes/kubernetes/blob/3718932d9a3ea459217ca11efe1ae9ead6aa92c1/pkg/kubelet/kubelet.go#L2886

The Status returning an error will set the node to not ready. I did notice after about 2 hours, the pods with netns, started going to completed. It might have been my test however I think we should discuss.

that is the bug we want to fix, though CNI implementations need to implement CNI STATUS wisely as with great power comes great responsibility

architkulkarni · 2024-08-29T20:10:58Z

that is the bug we want to fix, though CNI implementations need to implement CNI STATUS wisely as with great power comes great responsibility

@aojea could you say a bit more about what the "bug" here is? Is it that "the pods with netns started going to completed" would be a kubelet bug?

squeed · 2024-09-09T15:20:13Z

@MikeZappa87 can we wake this back up?

MikeZappa87 · 2024-09-09T15:21:42Z

@squeed yes we can!

wip: implement status

dbc146e

aojea reviewed Feb 26, 2024

View reviewed changes

MikeZappa87 changed the title ~~WIP: Implement Status~~ WIP: Implement CNI Status Feb 27, 2024

Update tests and return type

0466ecf

mikebrow requested changes Mar 27, 2024

View reviewed changes

MikeZappa87 changed the title ~~WIP: Implement CNI Status~~ [DRAFT] Implement CNI Status Apr 15, 2024

architkulkarni mentioned this pull request Sep 3, 2024

Implement CNI STATUS #120

Draft

bleggett mentioned this pull request Sep 16, 2024

istio-cni plugin should implement CNI STATUS verb when most container runtimes impelment it. istio/istio#53160

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Implement CNI Status #114

[DRAFT] Implement CNI Status #114

MikeZappa87 commented Feb 26, 2024 •

edited

Loading

aojea Feb 26, 2024

MikeZappa87 Feb 26, 2024

aojea Feb 26, 2024

MikeZappa87 Feb 26, 2024 •

edited

Loading

MikeZappa87 Feb 27, 2024

aojea Feb 27, 2024

MikeZappa87 Feb 27, 2024

mikebrow Mar 27, 2024

MikeZappa87 commented Feb 29, 2024

mikebrow left a comment

mikebrow Mar 27, 2024 •

edited

Loading

mikebrow Mar 27, 2024

mikebrow Mar 27, 2024

architkulkarni commented Jul 11, 2024

MikeZappa87 commented Jul 11, 2024

architkulkarni commented Jul 11, 2024

architkulkarni commented Jul 15, 2024 •

edited

Loading

aojea commented Aug 27, 2024

architkulkarni commented Aug 29, 2024

squeed commented Sep 9, 2024

MikeZappa87 commented Sep 9, 2024

[DRAFT] Implement CNI Status #114

Are you sure you want to change the base?

[DRAFT] Implement CNI Status #114

Conversation

MikeZappa87 commented Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeZappa87 Feb 26, 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

Choose a reason for hiding this comment

MikeZappa87 commented Feb 29, 2024

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Jul 11, 2024

MikeZappa87 commented Jul 11, 2024

architkulkarni commented Jul 11, 2024

architkulkarni commented Jul 15, 2024 • edited Loading

aojea commented Aug 27, 2024

architkulkarni commented Aug 29, 2024

squeed commented Sep 9, 2024

MikeZappa87 commented Sep 9, 2024

MikeZappa87 commented Feb 26, 2024 •

edited

Loading

MikeZappa87 Feb 26, 2024 •

edited

Loading

mikebrow Mar 27, 2024 •

edited

Loading

architkulkarni commented Jul 15, 2024 •

edited

Loading