Skip to content

Implement per service loadbalancer #35735

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
Merged
merged 1 commit into from
Nov 25, 2024
Merged

Implement per service loadbalancer #35735

merged 1 commit into from
Nov 25, 2024

Conversation

kl52752
Copy link
Contributor

@kl52752 kl52752 commented Nov 4, 2024 •
edited by pchaigno
Loading

CFP-34577 Per-service load balancing algorithm selection

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #34577

Allow users to override the load balancing algorithm for Services by setting the `service.cilium.io/lb-algorithm` annotation.

@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 4, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Nov 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 4, 2024
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@kl52752 kl52752 force-pushed the per-svc-lb branch 2 times, most recently from 711f524 to edb7e8a Compare November 6, 2024 13:10
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
1 similar comment
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@kl52752 kl52752 force-pushed the per-svc-lb branch 2 times, most recently from 29b0155 to bff3db3 Compare November 7, 2024 09:35
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@kl52752
Copy link
Contributor Author

kl52752 commented Nov 7, 2024

/cc @robscott
/cc @DamianSawicki
/cc @bowei

@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 15, 2024
@DamianSawicki

This comment was marked as outdated.

Sign in to view
@kl52752 kl52752 force-pushed the per-svc-lb branch 2 times, most recently from dbb4b6a to 9445efe Compare November 25, 2024 08:32
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@ovidiutirla

This comment was marked as outdated.

Sign in to view
@kl52752
Per-service load balancing algorithm selection
343a459 
This PR implements CFP-34577

Signed-off-by: Katarzyna Lach <katarzynalach@google.com>
@ovidiutirla
Copy link
Contributor

/test

@kl52752
Copy link
Contributor Author

kl52752 commented Nov 25, 2024

@kl52752 The Cilium L4LB XDP failure looks legit; it reproduced three times.

The test is fixed now. I needed to restore changes in useMaglev applied after code reveiw. https://github.com/cilium/cilium/pull/35735/files#diff-2b655ba613ca8386fd9f4c383200480492ee25711e24a575f3fc8b33531b4672R180

I added comment in code but we cannot check It turns out that we cannot depend on loadBalancerAlgo field because when bpf-lb-algorithm-annotation= false then algorithm is not populated in BPF maps for Service. When service is restored from maps then algorithm info is lost.

@pchaigno pchaigno enabled auto-merge November 25, 2024 14:03
@pchaigno pchaigno added this pull request to the merge queue Nov 25, 2024
Merged via the queue into cilium:main with commit cf5eeb8 Nov 25, 2024
64 checks passed
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
3ad947d 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
bef66c5 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
e1d8189 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
6bdf764 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
87fcfa2 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
2fa195c 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
4a31281 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 27, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
9da771c 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 28, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
dfb5580 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 28, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
d70f4c3 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 28, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
1b2bed7 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
borkmann added a commit that referenced this pull request Nov 29, 2024
@borkmann
cilium, bpf: Do not extend lb{4,6}_service struct via defines
b751556 
We cannot extend structs based on defines to enlarge them as this compromises
the ongoing work to achieve a clang-free Cilium runtime image, or at least
makes it significantly harder. Therefore use the upper 8 bits of the affinity
timeout field in the service master entry.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: #35735 (comment)
@DamianSawicki DamianSawicki mentioned this pull request Dec 18, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed left review comments

@borkmann borkmann borkmann approved these changes

@joamaki joamaki joamaki left review comments

@pchaigno pchaigno pchaigno left review comments

@tklauser tklauser tklauser approved these changes

@nebril nebril nebril approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@asauber asauber asauber approved these changes

@aspsk aspsk aspsk approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@ti-mo ti-mo Awaiting requested review from ti-mo ti-mo is a code owner automatically assigned from cilium/loader

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/major This PR introduces major new functionality to Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Per-service Load Balancing Algorithm Selection