calico icon indicating copy to clipboard operation
calico copied to clipboard

Add setting to enable wireguard NAPI threading

Open jrcichra opened this issue 1 year ago • 21 comments

Description

This adds a new setting to enable wireguard NAPI threading on the wireguard interfaces. NAPI threading can be used with wireguard to help increase maximum packets-per-second by distributing the tunnel's load from one core to many cores via a NAPI kernel thread.

This option is off by default and can be configured via the FelixConfiguration the same way wireguard can be toggled on and off.

Tested on a bare metal Kubernetes cluster. NAPI threading is enabled on the second pass of ensureLink, once the interface is up. I get operation not permitted on the first run before the up|peertopeer flags are set, which is why I have a flag guard.

This affects calico-node the most when it's configuring the wireguard interface. All other components are reconciling the new FelixConfiguration parameter.

Related issues/PRs

https://github.com/projectcalico/calico/issues/9223

Todos

  • [ ] Tests
  • [ ] Documentation
  • [ ] Release note

Release Note

Add setting to enable wireguard NAPI threading

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

jrcichra avatar Sep 20 '24 14:09 jrcichra

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 20 '24 14:09 CLAassistant

/sem-approve

coutinhop avatar Sep 20 '24 19:09 coutinhop

/sem-approve

mazdakn avatar Sep 24 '24 16:09 mazdakn

/sem-approve

mazdakn avatar Sep 26 '24 22:09 mazdakn

Addressed comments and rebased to resolve a conflict.

jrcichra avatar Oct 07 '24 14:10 jrcichra

/sem-approve

mazdakn avatar Oct 07 '24 18:10 mazdakn

/sem-approve

mazdakn avatar Oct 07 '24 22:10 mazdakn

/sem-approve

mazdakn avatar Oct 08 '24 17:10 mazdakn

/sem-approve

mazdakn avatar Oct 08 '24 21:10 mazdakn

I'm struggling to get the new test to pass locally. Wireguard NAPI threading only works after the wireguard interface is up. I gave ginkgo more time with 60s checking every second for a theoretical second reconcile. But ginkgo doesn't seem to be giving me the full 60 seconds to wait for the bit to change. This command returns failure near instantly on my development machine. Am I missing something with how ginkgo/gomega works?

https://pkg.go.dev/github.com/onsi/gomega#Eventually with 60s 1s should wait 60 seconds for me. It looks like for semaphore CI it waits 10 seconds. I'm hoping I can get it to wait longer for that second reconcile locally.

In production use I found it enables NAPI threading on the second reconcile of ensureLink once the up flag is set.

make fv GINKGO_FOCUS="the Wireguard threading config should be configured properly" GINKGO_ARGS="-ginkgo.progress -ginkgo.v -ginkgo.failFast" |& tee fv.log

jrcichra avatar Oct 11 '24 20:10 jrcichra

/sem-approve

mazdakn avatar Oct 11 '24 23:10 mazdakn

I think my local development is having an issue is this part of the E2E test, which is why I'm struggling to iterate on this test:

felix-0-127157-3-felixfv[stderr] docker: Error response from daemon: OCI runtime create failed: container_linux.go:377: starting container process caused: pr
ocess_linux.go:495: container init caused: rootfs_linux.go:76: mounting "/home/debian/calico/felix/bin/calico-felix-race-amd64" to rootfs at "/usr/local/bin/
calico-felix" caused: mount through procfd: not a directory: unknown: Are you trying to mount a directory onto a file (or vice-versa)? Check if the specified
 host path exists and is the expected type.
felix-1-127157-2-felixfv[stderr] docker: Error response from daemon: OCI runtime create failed: container_linux.go:377: starting container process caused: pr
ocess_linux.go:495: container init caused: rootfs_linux.go:76: mounting "/home/debian/calico/felix/bin/calico-felix-race-amd64" to rootfs at "/usr/local/bin/
calico-felix" caused: mount through procfd: not a directory: unknown: Are you trying to mount a directory onto a file (or vice-versa)? Check if the specified
 host path exists and is the expected type.

I have that file:

debian@devvm:~/calico/felix$ ls -l bin/calico-felix-race-amd64
-rwxr-xr-x 1 debian docker 149270288 Oct 16 21:35 bin/calico-felix-race-amd64
debian@devvm:~/calico/felix$
debian@devvm:~/calico/felix$ docker version
Client:
 Version:           20.10.5+dfsg1
 API version:       1.41
 Go version:        go1.15.15
 Git commit:        55c4c88
 Built:             Mon May 30 18:34:49 2022
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server:
 Engine:
  Version:          20.10.5+dfsg1
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.15.15
  Git commit:       363e9a8
  Built:            Mon May 30 18:34:49 2022
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.4.13~ds1
  GitCommit:        1.4.13~ds1-1~deb11u4
 runc:
  Version:          1.0.0~rc93+ds1
  GitCommit:        1.0.0~rc93+ds1-5+deb11u5
 docker-init:
  Version:          0.19.0
  GitCommit:

Should I use a different VM than Debian? What distro / version does CI run on?

It looks like Ubuntu is recommend so I'll try iterating on there.

jrcichra avatar Oct 16 '24 21:10 jrcichra

@jrcichra yes, please try with Ubuntu.

mazdakn avatar Oct 17 '24 00:10 mazdakn

Thanks @mazdakn I am able to run the wireguard e2e tests now with an Ubuntu 24.04 VM and docker + buildx + ipv6 networking enabled. I moved my test into a better location. I can confirm that in the E2E test the Apply() / ensureLink step is properly setting up the interface. Once the reconcile loop sets NAPI threading on, my Eventually() test passes. The only downside right now is increased E2E test duration. Since ensureLink sets wireguard NAPI threading once the wireguard link is up because that's the only time you can set NAPI threading, it slows down the E2E test. You'll see:

Creating Command [docker exec felix-0-149774-29-felixfv cat /sys/class/net/wg0/threaded]

for a bit until the reconcile loop sets NAPI threading. I'm not sure how to reduce that unless we modify how often ensureLink is executed or maybe trigger when the link status / flags are changed.

make fv GINKGO_FOCUS="the Wireguard device should be configurable" GINKGO_ARGS="-ginkgo.progress -ginkgo.v -ginkgo.failFast" |& tee fv.log
Ran 12 of 987 Specs in 790.061 seconds
SUCCESS! -- 12 Passed | 0 Failed | 0 Pending | 975 Skipped
PASS
+ status=0
+ echo 'Test batch 1 completed with status 0'
Test batch 1 completed with status 0
+ exit 0
+ st=0
+ echo 'Result: 0'
Result: 0
+ '[' 0 -ne 0 ']'
+ '[' 0 -eq 0 ']'
+ echo 'All tests passed'
All tests passed
make[1]: Leaving directory '/home/ubuntu/calico/felix'

Thanks again for walking me through this!

jrcichra avatar Oct 17 '24 14:10 jrcichra

@jrcichra Yes, I think the execution time is going to rise dramatically. I think it's better to NOT double the number of test cases. Instead you can add a single test to the current tests cases. As an example, it's possible to set the value, wait until reading from disk return the correct value. You can switch back and forth the value, and it should be enough.

mazdakn avatar Oct 18 '24 21:10 mazdakn

Agreed, I just used the same # of tests and toggle wireguard napi threading on and off now. That covers it.

Ran 6 of 891 Specs in 294.596 seconds
SUCCESS! -- 6 Passed | 0 Failed | 0 Pending | 885 Skipped

jrcichra avatar Oct 21 '24 15:10 jrcichra

@jrcichra Thanks for the changes. I think we are getting close to merging this. I left few comments about the test setup. It feel moving the test changes into an independent test make it more clean.

mazdakn avatar Oct 22 '24 17:10 mazdakn

/sem-approve

mazdakn avatar Oct 24 '24 23:10 mazdakn

@jrcichra thanks for updates. The CI failed in the pre-flight test. You need to run make generate in the root directory of Calico to generate doc update for FelixConfiguration. This is a new tool we recently added.

mazdakn avatar Oct 24 '24 23:10 mazdakn

@mazdakn thanks just rebased and generated those. I see this has the tag docs-not-required but I would like to include some docs for this change with its caveats. The performance increase from it is really great and it significantly increases the packets per second we can push through calico with wireguard enabled, but we have seen instances of complete NAPI lockups on older kernels during peer removal and I'd like to document that fact and the active investigation so users are aware if they decide to use this setting: https://lore.kernel.org/netdev/CA+wXwBTT74RErDGAnj98PqS=wvdh8eM1pi4q6tTdExtjnokKqA@mail.gmail.com/T/

We've backported this patch into our kernels which at least unwedges the NAPI lock when we drain the node: https://lore.kernel.org/netdev/[email protected]/

jrcichra avatar Oct 25 '24 14:10 jrcichra

/sem-approve

mazdakn avatar Oct 25 '24 17:10 mazdakn

@mazdakn Added documentation for this feature here: https://github.com/tigera/docs/pull/1741

jrcichra avatar Oct 28 '24 15:10 jrcichra

I don't believe that the latest CI failures are related to my change? https://tigera.semaphoreci.com/jobs/91a8c163-f4da-4366-bb1e-5869adaa0c07#L2599. I can rebase if it's a known issue.

jrcichra avatar Oct 28 '24 15:10 jrcichra

/sem-approve

mazdakn avatar Oct 28 '24 21:10 mazdakn

@jrcichra you need to rebase the PR. One test failed which is not related to this PR, and it's already fixed in the master branch.

mazdakn avatar Oct 29 '24 18:10 mazdakn

/sem-approve

mazdakn avatar Oct 29 '24 18:10 mazdakn

The CI failed in Win FV CAPZ which is a know issue and is not related to this PR.

mazdakn avatar Oct 29 '24 21:10 mazdakn

@jrcichra Thanks for your contribution.

mazdakn avatar Oct 29 '24 21:10 mazdakn

@jrcichra will you be able to cherry pick this PR to v3.29 and v3.28?

mazdakn avatar Oct 29 '24 21:10 mazdakn

@jrcichra will you be able to cherry pick this PR to v3.29 and v3.28?

Sure, I'll make PRs for those.

jrcichra avatar Oct 29 '24 21:10 jrcichra