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.

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

bpf: lb: remove extra SVC lookup when backend lookup fails #31595

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Mar 25, 2024

This cleans up an additional lb*_lookup_service() in an unlikely error path. This is fragile, as we don't return the resulting service entry to the caller, who wants to apply various checks to the entry.

Historically this seems to originate from a concern that the service gets updated while the datapath is processing the entry. If the backend lookup fails, we want to go back and fetch an updated entry with the actual set of backends. But in today's code, that concern should no longer apply. See the patch description for detailed reasoning.

So let's get rid of this fragile code section.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 25, 2024
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. area/loadbalancing Impacts load-balancing and Kubernetes service implementations labels Mar 25, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 25, 2024
@julianwiedmann
Copy link
Member Author

/test

When lb*_local() finds that the selected backend no longer exists or is not
active, it performs a fresh SVC lookup for the packet. This introduces
uncertainty - we might no longer match the same SVC entry, and all earlier
checks in eg. nodeport_lb*() become invalid.

But as this additional lookup only happens in the CT_REPLY case, we have
actually obtained the backend_id from the CT entry. And there is no reason
to mistrust the initially matched SVC entry (we haven't even used it to
derive the "bad" backend). Therefore remove this additional lookup.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
All callers pass `false`, remove the redundant parameter.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title 1.16 bpf lb svc bpf: lb: remove extra SVC lookup when backend lookup fails Apr 4, 2024
@julianwiedmann julianwiedmann marked this pull request as ready for review April 4, 2024 08:54
@julianwiedmann julianwiedmann requested a review from a team as a code owner April 4, 2024 08:54
@julianwiedmann julianwiedmann added this pull request to the merge queue Apr 4, 2024
Merged via the queue into cilium:main with commit 710f0f8 Apr 4, 2024
62 checks passed
@julianwiedmann julianwiedmann deleted the 1.16-bpf-lb-svc branch April 4, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants