Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove suppresses errors unnecessarily #102

Open
helsaawy opened this issue Jun 7, 2022 · 3 comments
Open

Remove suppresses errors unnecessarily #102

helsaawy opened this issue Jun 7, 2022 · 3 comments

Comments

@helsaawy
Copy link

helsaawy commented Jun 7, 2022

(*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
Copy link
Member

mikebrow commented Jun 9, 2022

@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
Copy link
Contributor

(*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
Copy link
Author

helsaawy commented Jun 9, 2022

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.

helsaawy added a commit to helsaawy/go-cni that referenced this issue Jun 10, 2022
Return errors that contain `"not found"` from CNI plugin DEL
instead of ignoring them, since they can be valid errors and signal that
the interface was not properly deleted.

See discussion here: containerd#102

Signed-off-by: Hamza El-Saawy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants