search

containerd / go-cni

A generic CNI library to provide APIs for CNI plugin interactions
Apache License 2.0
146 stars 56 forks source link

`Remove` suppresses errors unnecessarily #102

Open helsaawy opened 2 years ago

helsaawy commented 2 years ago

(*libcni).Remove ignores not found errors, which suppresses valid errors from plugins that are cannot find the resource required to properly delete an interface.

A blanket ignore prevents the runtime from identifying and issue, and can cause future errors if the runtime tries to re-use the same interface name.

mikebrow commented 2 years ago

@MikeZappa87 is this something we could handle (or unhandle as it may be where version of plugin > some_version) either with check() or from plugin version information we may scrape from setup?

MikeZappa87 commented 2 years ago

(*libcni).Remove ignores not found errors, which suppresses valid errors from plugins that are cannot find the resource required to properly delete an interface.

A blanket ignore prevents the runtime from identifying and issue, and can cause future errors if the runtime tries to re-use the same interface name.

In the case you mentioned reusing the same interface, isn’t the network namespace deleted and recreated? I am curious about the motivation for this conditional however it’s most likely safe to remove or handle very specific cases. Have you seen cases on the Windows side where this has caused an issue?

helsaawy commented 2 years ago

Have you seen cases on the Windows side where this has caused an issue?

We see a bug where the CNI cannot find the namespace, and therefore does not delete the network and interfaces specified (we are working on fixing that bug). However, containerd does not see the error, so we do not catch it until another pod is created with the same desired IP address, which then fails.

I do not have context as to why the strings.Contains(err.Error(), "not found") check was added, unfortunately.