-
Notifications
You must be signed in to change notification settings - Fork 54
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
Force to invoke all CNI plugin's delete at pods' tearing down #86
base: master
Are you sure you want to change the base?
Conversation
@s1061123: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: s1061123 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In case of multiple interfaces in pod, when the pod is deleted, forEachnetwork() is called with multiple network attachments. If forEachnetwork() causes the error at the middle of processing networks, then forEachnetwork() just returns and following network is not processed. From CNI runtime point of view, all CNI plugin should be invoked to delete interfaces. This change introduce 'force' option in forEachnetwork() and try to continue to process (i.e. delete network) even though forEachnetwork() causes the error. Signed-off-by: Tomofumi Hayashi <tohayash@redhat.com>
c20b9df
to
5103466
Compare
/assign @rajatchopra |
@s1061123 what's the error we're getting from forEachNetwork()? |
@dcbw |
@dcbw https://github.com/cri-o/ocicni/blob/master/pkg/ocicni/ocicni.go#L455-L464 here, forEachNetwork() looks all network and interface name, then check Then current code just returns, as |
@s1061123 what I"m trying to think of is why there would be dupes? There should only be a single cache file per network for the pod, each with a different interface name... |
Is it the case that the multus network (that CRIO calls directly) has the same ifname as the default CNI network that is called by multus? And during teardown, ocicni finds both files even though it was only ever called for the Multus network? If so, that's the detail I wasn't grasping this morning on the call. |
@dcbw right. So the case I met is following process:
|
@s1061123 I do think the "cleanest" solution to this is to have multus give ocicni a different cache dir so that we can cleanly separate the kubelet -> Multus calls from the Mutlus -> delegate calls. However, we could just ignore the interface conflicts on DEL since DEL is supposed to be permissive anyway. Would that work for you? |
@dcbw For multus case, I've already taken care of it in k8snetworkplumbingwg/multus-cni#638 So ocicni with multus does not have such issue so far. This PR is for non multus-cni case. Currently, as far as I know, ocicni seems to support multiple CNI invocation (as type CNI runtime should invoke CNI plugin's DEL command even if some error is happen, to prevent resource leak. However, ocicni may not invoke plugin's DEL command and may causes resource leak, if
That's why I filed this PR. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
In case of multiple interfaces in pod, when the pod is deleted,
forEachnetwork() is called with multiple network attachments.
If forEachnetwork() causes the error at the middle of processing networks,
then forEachnetwork() just returns and following network is not processed.
From CNI runtime point of view, all CNI plugin should be invoked to delete
interfaces.
This change introduce 'force' option in forEachnetwork() and try to
continue to process (i.e. delete network) even though forEachnetwork()
causes the error.