Should plugin DEL ever error?

[rosenhouse]

Issue #306 raises an interesting question that applies generally to all plugins: how should runtimes react if an ADD fails?

One possibility is for DEL to be best-effort idempotent. It tries to clean-up any & all left-over resources and swallows any errors that might arise if a resource was never created in the first-place.

Then we could recommend that a runtime call DEL after a failed ADD.

[dcbw]

@rosenhouse I agree. Two points in random order:

1. openshift-sdn and kubernetes 'kubenet' plugins both have ugly "if err != nil { run teardown code; return }" stuff in ADD error paths to handle cleanup themselves on ADD error. Would be really nice to get rid of that and be able to rely on the runtime calling DEL when an add fail happens. But we should add this as a spec requirement, not just a recommendation. The plugins that conform to say v0.5.0 could drop their icky add error path code and rely on the runtime, and it would be a runtime bug (doesn't conform to 0.5.0 of spec) or a runtime error (fail because plugin doesn't handle older spec version) if teardown didn't happen.

2. IPAM release/garbage collection (see add ipam garbageCollection kubernetes/kubernetes#34373 and kubenet/kubelet leaks ips on docker restart kubernetes/kubernetes#34278) needs to happen if the runtime gets restarted. It can often be the case that the container is already gone and thus no network namespace can be passed to the teardown. But we still need to release the IPAM allocation because the container is gone, and both skel.go and the plugins currently require the netns and will fail if it's not given, or if it's invalid. Essentially:

1. kubernetes kubelet runtime does
2. docker dies (and kills container and network namespace)
3. docker restarts
4. kubelet restarts
5. kubelet figures out the container is no longer alive
6. calls teardown to clean things up, but no netns exists because the container is gone
7. IPAM teardown fails because no CNI_NETNS
8. containers IPAM allocation leaks

[bboreham]

We could have CNI_NETNS=="" mean "clean up what you can, but there's no namespace so don't worry about that".

[dcbw]

Though we could just do this now by making the internal plugins do best-effort cleanup, without changing the spec... not sure if we'd consider that a material spec change or not.

[bboreham]

The phrasing "tries to clean-up any & all left-over resources" - is that meant to mean "... relating to the specific container ID supplied", or more broadly, clean up anything that seems to be left-over? The latter sounds dangerous.

I think I'm ok with best-efforts meaning "I looked for resources assigned to that container ID; didn't find anything; returning success"; I would still like to error on a completely invalid parameter.

[dcbw]

@bboreham yes, I would expect that the operation would still be scoped to the specific container ID.

[chenchun]

add this as a spec requirement, not just a recommendation

+1 of adding this as a spec requirement, and we could still keep the error return of DEL.
Make DEL return nothing means it can't complain any mistakes during cleanup.

[jieyu]

+1 on making this a spec requirement.

It's very hard for container runtime to react if cleanup fails. It has no idea if it should proceed with the rest of the cleanup or not.

[bboreham]

The spec was changed in #346 to say "Plugins should generally complete a DEL action without error even if some resources are missing [...] even if the container network namespace no longer exists"

So I think this should be closed.

[rosenhouse]

Agreed. Though there is still some outstanding work to make the plugins actually behave this way.

[bboreham]

FYI Kubernetes 1.6 beta seems to rely on plugins not erroring on DEL - see weaveworks/weave#2801 (comment)