-
Notifications
You must be signed in to change notification settings - Fork 348
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
Improves pod deletion with user defined networks #4771
base: master
Are you sure you want to change the base?
Conversation
Delete path was printing removing the pod for every network controller like: I1009 23:16:54.375327 8134 base_network_controller_secondary.go:287] [e2e-test-endpointslices-mirror-e2e-hg7st/backend-0] addLogicalPort for NAD e2e-test-endpointslices-mirror-e2e-hg7st/gryffindor took 3.202984ms, libovsdb time 2.456846ms I1009 23:16:54.377764 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-b66l5.gryffindor I1009 23:16:54.378006 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-89gg6.gryffindor I1009 23:16:54.378095 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network cw92p_gryffindor I1009 23:16:54.378168 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-cq4pk.test-net I1009 23:16:54.378238 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-d8xg4.gryffindor I1009 23:16:54.378313 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network 26x4f_gryffindor I1009 23:16:54.378384 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-hg7st.gryffindor I1009 23:16:54.382338 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network zw5ld_blue I1009 23:16:54.382415 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-nkltz-red.red I1009 23:16:54.382479 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-nkltz-blue.blue This changes delete to return early if this pod is not configured by this network controller. Signed-off-by: Tim Rozet <trozet@redhat.com>
I have never seen tools lane fail: https://github.com/ovn-org/ovn-kubernetes/actions/runs/11298323245/job/31427992514#step:12:320 gonna re-run to see if it passes |
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.
1 QQ on l2 plus we need unit tests for pod deletes on UDNs (I can't seem to find any maybe I didn't search well)- sorry I know we don't have any today and its not supposed to be part of this PR but while we are here can we add the missing gap coverage so that we know L2's remote port is removed?
@@ -412,6 +412,27 @@ func (bsnc *BaseSecondaryNetworkController) removePodForSecondaryNetwork(pod *ka | |||
return nil | |||
} | |||
|
|||
activeNetwork, err := bsnc.getActiveNetworkForNamespace(pod.Namespace) | |||
if err != nil { | |||
return fmt.Errorf("failed looking for the active network at namespace '%s': %w", pod.Namespace, err) |
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.
nit, any value in adding the pod Name here as well to understand which pod's namespace couldn't get pulled?
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.
it will already get printed cause the calling function prints the pod key:
if err := r.ResourceHandler.DeleteResource(entry.oldObj, entry.config); err != nil {
entry.timeStamp = time.Now()
entry.failedAttempts++
if entry.failedAttempts >= MaxFailedAttempts {
klog.Errorf("Retry delete failed final attempt for %s %s: error: %v", r.ResourceHandler.ObjType, objKey, err)
} else {
klog.Infof("Retry delete failed for %s %s, will try again later: %v",
r.ResourceHandler.ObjType, objKey, err)
}
// the pod is not attached to this specific network | ||
klog.V(5).Infof("Pod %s/%s is not attached on this network controller %s", | ||
pod.Namespace, pod.Name, bsnc.GetNetworkName()) | ||
return 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.
so for L3/localnet I don't see a problem.. for L2 we usually create a remote port for the remote pod, so if we move it here will we not call pInfo, err := bsnc.deletePodLogicalPort(pod, portInfoMap[nadName], nadName)
in that case? or are the remote ports for pods in l2 removed elsewhere...
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.
this code will remove that port as well I think, but I dont see how that is related to the change here. The remote pod would still be on this network right?
Delete path was printing removing the pod for every network controller like:
I1009 23:16:54.375327 8134 base_network_controller_secondary.go:287] [e2e-test-endpointslices-mirror-e2e-hg7st/backend-0] addLogicalPort for NAD e2e-test-endpointslices-mirror-e2e-hg7st/gryffindor took 3.202984ms, libovsdb time 2.456846ms
I1009 23:16:54.377764 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-b66l5.gryffindor
I1009 23:16:54.378006 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-89gg6.gryffindor
I1009 23:16:54.378095 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network cw92p_gryffindor
I1009 23:16:54.378168 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-cq4pk.test-net
I1009 23:16:54.378238 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-d8xg4.gryffindor
I1009 23:16:54.378313 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network 26x4f_gryffindor
I1009 23:16:54.378384 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-endpointslices-mirror-e2e-hg7st.gryffindor
I1009 23:16:54.382338 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network zw5ld_blue
I1009 23:16:54.382415 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-nkltz-red.red
I1009 23:16:54.382479 8134 base_network_controller_secondary.go:416] Deleting pod: e2e-test-endpointslices-mirror-e2e-hg7st/backend-0 for network e2e-test-network-segmentation-e2e-nkltz-blue.blue
This changes delete to return early if this pod is not configured by this network controller.
Also stops getting the network mapping multiple times in a single delete.