Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improves pod deletion with user defined networks #4771

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improves pod deletion with user defined networks #4771

wants to merge 1 commit into from

Conversation

trozet
Copy link
Contributor

@trozet trozet commented Oct 11, 2024 •
edited
Loading

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.

@trozet
Improves pod deletion with user defined networks
2c69b51 
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>
@trozet trozet requested a review from a team as a code owner October 11, 2024 19:38
@trozet trozet requested a review from girishmg October 11, 2024 19:38
@tssurya
Copy link
Member

tssurya commented Oct 14, 2024

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

tssurya
tssurya requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@tssurya tssurya left a 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?

go-controller/pkg/ovn/base_network_controller_secondary.go
@@ -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)
Copy link
Member

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?

Copy link
Contributor Author

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)
					}

go-controller/pkg/ovn/base_network_controller_secondary.go
// 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
Copy link
Member

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...

Copy link
Contributor Author

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?

@tssurya tssurya added feature/user-defined-network-segmentation All PRs related to User defined network segmentation kind/bug All issues that are bugs and PRs opened to fix bugs labels Oct 16, 2024
@tssurya tssurya added this to the v1.1.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tssurya tssurya tssurya requested changes

@girishmg girishmg Awaiting requested review from girishmg girishmg is a code owner automatically assigned from ovn-org/ovn-kubernetes-members

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
feature/user-defined-network-segmentation All PRs related to User defined network segmentation kind/bug All issues that are bugs and PRs opened to fix bugs
Projects
OVN Kubernetes Project
Status: In Progress
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@trozet @tssurya