Skip to content

Conversation

@pliurh
Copy link
Collaborator

@pliurh pliurh commented Jun 19, 2025

This reverts commit 5d64ec3.

Multus shall return an error to the caller if a DEL fails.

@coveralls
Copy link

Coverage Status

coverage: 49.824% (-3.1%) from 52.959%
when pulling 8462978 on pliurh:return_error
into 18630fd on k8snetworkplumbingwg:master.

@s1061123
Copy link
Member

Multus shall return an error to the caller if a DEL fails.

In CNI Specification, here is the sentence.

Plugins should generally complete a DEL action without error even if some resources are missing. 

So CNI specification point of view, CNI plugin should execute DEL command without error. Multus CNI is CNI Plugin and it should be alined with specification. So I suppose we should keep it.

If you have something stale resources (e.g. something cannot be cleaned up by DEL command), CNI Spec provides GC command.
https://github.com/containernetworking/cni.dev/blob/main/content/docs/spec.md#gc-clean-up-any-stale-resources

Multus CNI already supports GC command, so I strongly recommend to use GC command to cleanup stale resources, something cannot be cleaned up by failed DEL command.

@s1061123
Copy link
Member

@dougbtv PTAL?

@danwinship
Copy link

Multus shall return an error to the caller if a DEL fails.

In CNI Specification, here is the sentence.

Plugins should generally complete a DEL action without error even if some resources are missing. 

So CNI specification point of view, CNI plugin should execute DEL command without error.

That's not what it says. (Or at least, not what it meant.) It just means that a CNI plugin should not return a "not found" error if it is asked to delete something that already doesn't exist. If it is aware that there is something that it needs to delete, and it is unable to delete it, then it's supposed to return an error.

If CNI plugins were never supposed to return errors from DEL, then why would DEL even allow returning an error?

We discussed this in the CNI meeting on 2025-06-16 and @squeed agreed that Multus's interpretation of the spec is wrong.

Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link

This pull request is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 19, 2025
@JensErat
Copy link

still relevant

@github-actions github-actions bot removed the Stale label Oct 20, 2025
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

Successfully merging this pull request may close these issues.

6 participants