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

bpf: misc cleanups #24291

Merged
merged 2 commits into from Mar 13, 2023
Merged

bpf: misc cleanups #24291

merged 2 commits into from Mar 13, 2023

Conversation

julianwiedmann
Copy link
Member

Two small drive-by cleanups, while reviewing code in this area.

@julianwiedmann
bpf: nodeport: clean up a fib_redirect() in tail_nodeport_nat_egress_…
4c6c00c 
…ipv4()

This code path is IPv4-only. So prefer fib_redirect_v4() over the
generic fib_redirect().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: clean up NAT target address
4ad4ab9 
Initialize target.addr along with the other fields of the ipv*_nat_target,
and clarify that it's not actually 0 for nodeport_snat_fwd_ipv4().

This also let's us collapse two TUNNEL_MODE sections in
tail_nodeport_nat_egress_ipv4() into one.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Mar 10, 2023
@julianwiedmann
Copy link
Member Author

/test

@borkmann
Copy link
Member

ack, both lgtm

@julianwiedmann julianwiedmann marked this pull request as ready for review March 10, 2023 15:19
@julianwiedmann julianwiedmann requested a review from a team as a code owner March 10, 2023 15:19
@borkmann
Copy link
Member

interesting, looks like ARPing test failed

	 [12:48:52] FAIL: node_linux_test.go:2248: linuxPrivilegedIPv4OnlyTestSuite.TestArpPingHandling
	 [12:48:52] 
	 [12:48:52] node_linux_test.go:2452:
	 [12:48:52]     // deleteNeighbor is invoked async too
	 [12:48:52]     wait(nodev1.Identity(), "veth0", nil, true)
	 [12:48:52] node_linux_test.go:2369:
	 [12:48:52]     c.Assert(err, check.IsNil)
	 [12:48:52] ... value *errors.errorString = &errors.errorString{s:"timeout reached while waiting for condition"} ("timeout reached while waiting for condition")
	 [12:48:52] 
	 [12:48:52] level=warning msg="Attempted to deallocate a node ID that wasn't allocated" nodeID=0 subsys=linux-datapath
	 [12:48:52] level=warning msg="Attempted to deallocate a node ID that wasn't allocated" nodeID=0 subsys=linux-datapath
	 [12:48:52] OOPS: 35 passed, 1 FAILED

@julianwiedmann
Copy link
Member Author

interesting, looks like ARPing test failed

	 [12:48:52] FAIL: node_linux_test.go:2248: linuxPrivilegedIPv4OnlyTestSuite.TestArpPingHandling
	 [12:48:52] 
	 [12:48:52] node_linux_test.go:2452:
	 [12:48:52]     // deleteNeighbor is invoked async too
	 [12:48:52]     wait(nodev1.Identity(), "veth0", nil, true)
	 [12:48:52] node_linux_test.go:2369:
	 [12:48:52]     c.Assert(err, check.IsNil)
	 [12:48:52] ... value *errors.errorString = &errors.errorString{s:"timeout reached while waiting for condition"} ("timeout reached while waiting for condition")
	 [12:48:52] 
	 [12:48:52] level=warning msg="Attempted to deallocate a node ID that wasn't allocated" nodeID=0 subsys=linux-datapath
	 [12:48:52] level=warning msg="Attempted to deallocate a node ID that wasn't allocated" nodeID=0 subsys=linux-datapath
	 [12:48:52] OOPS: 35 passed, 1 FAILED

-> opened #24326, re-running.

@julianwiedmann
Copy link
Member Author

/test-runtime

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 13, 2023
@borkmann borkmann merged commit a1b3fb3 into cilium:master Mar 13, 2023
@julianwiedmann julianwiedmann deleted the 1.14-bpf-cleanup branch March 13, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@julianwiedmann @borkmann