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

Add WireGuard Configurations to Automated Network Throughput Tests #32134

Merged
merged 2 commits into from
May 7, 2024

Conversation

learnitall
Copy link
Contributor

@learnitall learnitall commented Apr 22, 2024

This PR expands the test matrix of the network throughput workflow to include WireGuard configurations. This will allow for increased visibility into the performance of WireGuard over time, allowing for us to catch regressions.

Add WireGuard configurations to automated network throughput tests

@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 Apr 22, 2024
@learnitall learnitall force-pushed the pr/learnitall/add-wg-throughput branch from 5fc8ae2 to 875ee07 Compare May 2, 2024 21:36
@learnitall
Copy link
Contributor Author

Successful run: https://github.com/cilium/cilium/actions/runs/8791561641/job/24126842792?pr=32134

Artifacts can be found in the cilium-scale-results bucket. I took a look at the values and they pass a sanity check. For example, the throughput results for WireGuard's pod-to-pod TCP_STREAM test across nodes comes in higher than IPSec but lower than without transparent encryption.

@learnitall learnitall changed the title Pr/learnitall/add wg throughput Add WireGuard Configurations to Network Throughput Tests May 2, 2024
@learnitall learnitall changed the title Add WireGuard Configurations to Network Throughput Tests Add WireGuard Configurations to Automated Network Throughput Tests May 2, 2024
@learnitall learnitall added the release-note/ci This PR makes changes to the CI. label May 2, 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 May 2, 2024
@learnitall learnitall added kind/performance There is a performance impact of this. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 2, 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 May 2, 2024
@learnitall learnitall added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. feature/wireguard Relates to Cilium's Wireguard feature labels May 2, 2024
@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 2, 2024
@learnitall learnitall marked this pull request as ready for review May 2, 2024 21:45
@learnitall learnitall requested review from a team as code owners May 2, 2024 21:45
@learnitall learnitall requested review from giorio94 and brlbil May 2, 2024 21:45
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

/lgtm, just a minor nit inline.

Should this change be marked for backport to all stable branches?

.github/workflows/net-perf-gke.yaml Outdated Show resolved Hide resolved
learnitall added 2 commits May 6, 2024 14:28
This commit adds WireGuard configurations to the network performance
test workflow, enabling observability into WireGuard throughput and
latency performance over time.

Signed-off-by: Ryan Drew <[email protected]>
The network performance workflow has a step named `vars` which
determines an appropriate value for an environment variable named OWNER.
This environment variable is added as a label into the GKE cluster the
workflow creates. This commit fixes a bug where, if the branch the
workflow is ran from contained a forward-slash, cluster creation would
fail, as GKE cluster labels cannot contain forward slashes.

Signed-off-by: Ryan Drew <[email protected]>
@learnitall learnitall added the dont-merge/preview-only Only for preview or testing, don't merge it. label May 6, 2024
@learnitall learnitall force-pushed the pr/learnitall/add-wg-throughput branch 2 times, most recently from 6a65717 to 6d96e8a Compare May 7, 2024 15:40
@learnitall learnitall removed the dont-merge/preview-only Only for preview or testing, don't merge it. label May 7, 2024
@learnitall
Copy link
Contributor Author

/test

@learnitall learnitall added dont-merge/blocked Another PR must be merged before this one. and removed dont-merge/blocked Another PR must be merged before this one. labels May 7, 2024
@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 May 7, 2024
@learnitall
Copy link
Contributor Author

Should this change be marked for backport to all stable branches?

I just realized I forgot about this @giorio94, sorry about that. We can wait to backport. Right now we're only running the scale tests on main, as we haven't yet configured Ariane to run the scale test on different branches.

@ldelossa ldelossa added this pull request to the merge queue May 7, 2024
Merged via the queue into main with commit 49ca139 May 7, 2024
64 checks passed
@ldelossa ldelossa deleted the pr/learnitall/add-wg-throughput branch May 7, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. feature/wireguard Relates to Cilium's Wireguard feature kind/performance There is a performance impact of this. 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.

4 participants