-
Notifications
You must be signed in to change notification settings - Fork 338
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
cni/linux: ensure only the latest sandbox has external-ids:iface-id set #869
Conversation
fbbe186
to
dd58f9e
Compare
Shouldn't the old interface have been deleted by this point? Is this a sign that some previous cleanup failed? Do you have logs showing this happening? |
In the case where the sandbox dies, there's no guarantee that the previous one will have a CNI DEL issued before the replacement has a CNI ADD. I've personally seen this, and it's pretty easy to reproduce: just kill the pause container. |
1 similar comment
In the case where the sandbox dies, there's no guarantee that the previous one will have a CNI DEL issued before the replacement has a CNI ADD. I've personally seen this, and it's pretty easy to reproduce: just kill the pause container. |
Kubelet may keep multiple sandboxes running for a given pod if some are waiting for garbage collection. We need to make sure that only the latest sandbox has iface-id set to the container's namespace/name or ovn-controller may get confused about which OVS port to associate with the Pod's logical switch port. Signed-off-by: Dan Williams <dcbw@redhat.com>
@danwinship to echo what Casey said, Kubelet may keep a couple sandboxes for a given pod around up until the GC interval. Then it will delete them as they expire. Only one of the sandboxes will be "live" (eg SANDBOX_READY) but for OVN only that sandbox should have the iface-id set. |
Without knowing the OVN data model very well, is it possible to use the containerid, which will be unique, as the externalid? |
The |
// but only the latest one should have the iface-id set. | ||
uuids, _ := ovsFind("Interface", "_uuid", "external-ids:iface-id="+ifaceID) | ||
for _, uuid := range uuids { | ||
if out, err := ovsExec("remove", "Interface", uuid, "external-ids", "iface-id"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcbw if you want to remove the interface itself, then you will need "destroy".
# ovs-vsctl --help |grep 'destroy TBL'
destroy TBL REC delete RECord from TBL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@girishmg I wasn't actually intending to destroy the Interface object, just to clear its iface-id from its Interface record. The actual Interface would be destroyed when kubelet/CRI finally called CNI DEL for the sandbox.
@squeed the master doesn't have any access to sandbox-level stuff since that's all done on the node and not recorded in the Kube API AFAIK...
@girishmg I think what Casey is saying is that we can avoid this problem entirely if we name the OVN port after the CNI sandbox ID (which will be unique) rather than the pod namespace+name (which is shared between sandboxes). However, we can't do that because the master has no idea what the sandbox is. |
/lgtm |
[DownstreamMerge] 21-12-9
Kubelet may keep multiple sandboxes running for a given pod if some are
waiting for garbage collection. We need to make sure that only the
latest sandbox has iface-id set to the container's namespace/name or
ovn-controller may get confused about which OVS port to associate with
the Pod's logical switch port.
@squeed @girishmg @danwinship