flannel: remove net conf file after DEL succeed #449
Conversation
| @@ -78,10 +78,18 @@ func doCmdDel(args *skel.CmdArgs, n *NetConf) error { | |||
| return err | |||
| } | |||
|
|
|||
| defer func() { | |||
| // remove net conf after DEL succeed | |||
| if err == nil { | |||
There was a problem hiding this comment.
To make the return of invoke.DelegateDel can be judged here, @jellonek
There was a problem hiding this comment.
You could avoid this by having (err error) as the return, i.e. a named return variable, and changing err := to err =. Both are pretty subtle points of Go behaviour.
There was a problem hiding this comment.
thanks, will update this
plugins/meta/flannel/flannel.go
Outdated
|
|
||
| return ioutil.ReadFile(path) | ||
| // cleanup should be called after DEL succeed | ||
| cleanup := func() { |
There was a problem hiding this comment.
if you set this to a no-op function on error then the caller can do cleanup() every time; doesn't have to know the coupling between error and cleanup.
mars1024
force-pushed
the
bugfix/flannel_clean
branch
from
64d0fd0 to
b0c42ce
Compare
|
Although the DEL retry mechanism of kubelet is disturbing something, but I still think the idempotency of each retry is important in CNI plugins. To avoid leaking or some wrong error messages. |
| @@ -65,10 +65,13 @@ func doCmdDel(args *skel.CmdArgs, n *NetConf) error { | |||
| return err | |||
| } | |||
|
|
|||
| // cleanup will work when no error happens | |||
| defer cleanup(err) | |||
There was a problem hiding this comment.
I think this doesn't work because it gets the value of err at the time control passes through this line.
See https://play.golang.org/p/3W5_yxRJJ_6 vs https://play.golang.org/p/-9c9Z3pvVFO
There was a problem hiding this comment.
The alternative is to make your cleanup function take a pointer to the error, and then defer cleanup(&err) and then it'll work :) https://play.golang.org/p/KUTcEEMB--n
But Brian thinks this is terrible and he's not wrong.
Signed-off-by: Bruce Ma <[email protected]>
mars1024
force-pushed
the
bugfix/flannel_clean
branch
from
b0c42ce to
53854dd
Compare
|
updated, PTAL, thanks ~ |
|
/lgtm |
consumeScratchNetConfwill remove net conf file in defer, so ifinvoke.DelegateDelfails and CNI DEL is called again, it will always be successful because net conf file has been removed already.So I think the better way is to remove net conf file after
invoke.DelegateDelsucceed, this will make sure the CNI DEL retry will go throughinvoke.DelegateDelevery time.Signed-off-by: Bruce Ma [email protected]