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

helm,test: Add standalone L4LB XDP tests in a form of Github Action #16338

Merged
merged 2 commits into from Jun 7, 2021

Conversation

brb
Copy link
Member

@brb brb commented May 27, 2021

This PR introduces a new GH action called Cilium L4LB XDP which is responsible for running the standalone LB tests.

The action starts a Fedora VM with vagrant. We do that because we need to run Kind on cgroupv2-only machine (otherwise, bpf_sock which is required by the LB health check is not guaranteed to work). Unfortunately, GH Action does not support any runner with cgroupv2-only. So instead we run Fedora 34 which has cgroupv1 disabled on the MacOS runner which supports nested virtualisation.

For now the test issues 10 requests to LB VIP from the Fedora VM. See test.sh for more details.

In addition, the PR adds the loadBalancer.standalone and friend options to enable the standalone L4LB.

Reviewable per commit.

@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 May 27, 2021
@brb brb force-pushed the pr/brb/l4lb-test branch 2 times, most recently from ebfcd81 to f88d604 Compare May 27, 2021 19:32
@brb brb added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow needs-backport/1.10 release-note/ci This PR makes changes to the CI. labels May 27, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 27, 2021
@brb brb marked this pull request as ready for review May 27, 2021 20:08
@brb brb requested review from a team as code owners May 27, 2021 20:08
@brb brb requested review from christarazi and aanm May 27, 2021 20:08
@pchaigno pchaigno self-requested a review May 27, 2021 20:26
test/l4lb-xdp/test.sh Outdated Show resolved Hide resolved
@brb brb requested a review from aanm May 28, 2021 15:47
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fairly hackish but I understand why we need all that and don't have a cleaner/simpler solution 😐

A couple comments below, but nothing really blocking. I would strongly prefer if we trigger this new workflow only on comments though. Our queue of GitHub jobs is already getting out of control when all contributors are online and sending PRs...

.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
test/l4lb-xdp/test.sh Outdated Show resolved Hide resolved
test/l4lb-xdp/test_tc_tunnel.c Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/l4lb.yaml Outdated Show resolved Hide resolved
@brb
Copy link
Member Author

brb commented Jun 1, 2021

ci-l4lb

@brb
Copy link
Member Author

brb commented Jun 4, 2021

As a follow-up, I will evaluate whether Kata Containers could be used for the same.

@errordeveloper It seems that Kata is not supported on OS X. Running it on a regular runner would be very slow (due to missing nested virtualization support). An alternative would be to run on a cloud provider VM, but I'd like to avoid this complexity.

@errordeveloper
Copy link
Contributor

...I'd like to avoid this complexity.

I can see how this solution seems less complex, but personally I actually find it rather quite convoluted. I would recommend using any hosted solution instead of using this sort of trick to make something work inside GitHub. I am not blocking the PR, just wanted to make my view clear.

@pchaigno pchaigno requested a review from kaworu June 4, 2021 11:40
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the changes LGTM. I see you've tested with pull_request uncommented and the label: https://github.com/cilium/cilium/actions/runs/905705472 👍🏻

This commit introduces the following Helm options:
- "loadBalancer.standalone" to enable the standalone Cilium L4LB.
- "loadBalancer.dsrDispatch" to choose the DSR dispatch mode.

Also, this commit replaces --node-port-{mode,acceleration} with
--bpf-lb-{mode,acceleration}, respectively. The former two were
deprecated by d73c572.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
CODEOWNERS Outdated Show resolved Hide resolved
@brb
Copy link
Member Author

brb commented Jun 7, 2021

Thanks everyone for reviewing! Addressed all feedback, got all ACKs. Marking ready to merge.

@brb brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 7, 2021
This commit introduces a new GH action called "Cilium L4LB XDP" which is
responsible for running the standalone LB tests.

The action starts a Fedora VM with vagrant. We do that because we need
to run Kind on cgroupv2-only machine (otherwise, bpf_sock which is
required by the LB health check is not guaranteed to work).
Unfortunately, GH Action does not support any runner with cgroupv2-only.
So instead we run Fedora 34 which has cgroupv1 disabled on the MacOS
runner which supports nested virtualisation.

For now the test issues 10 requests to LB VIP from the Fedora VM. See
test.sh for more details.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@aanm aanm merged commit 673ccd7 into master Jun 7, 2021
@aanm aanm deleted the pr/brb/l4lb-test branch June 7, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants