fix: Prevent data race from global metrics round-tripper

[meln5674]

Fixes #13637

Motivation

This line introduces a data race by globally storing a round-tripper used by the kubernetes client. If a new request starts before the first one completes, and the first request attempts to use the same round-tripper it originally created before the second finishes upgrading its connection, it will result in a panic due to a nil net.Conn in the underlying SPDY implementation. As a result, if the controller makes too many new connections to the API server in a short-enough period of time, it will crash and restart.

While it is sensible to store a global handle to the metrics that this round-tripper records, storing the round-tripper itself is not.

Modifications

This patch retains the "context" of the metrics (i.e. the actual ctx value, plus the handle to the metrics themselves) as global, but scopes the round-tripper to each connection by refactoring the context to its own type and global variable, and uses an embedded pointer to it in the round-tripper implementation to avoid needing downstream changes.

Verification

E2E functional tests were run locally. Additionally, the PR tests identified in the issue as failing as a result of this race now pass, and the controller logs from running without the race detector were visually inspected to confirm that no panics occurred after multiple re-runs of the originally failing test.

[meln5674]

Can anyone comment on this test failure? Why is it expecting 404? That metric seems to be reporting healthily here. Was this maybe a hack added to work around something related to this same issue?

[Joibel]

I believe this is a flakey test that does just sometimes fail. I don't remember introducing it when the roundtripper was added. I've kicked it to run again.

Anyway, that's for finding this and testing the RC. I will take a proper look when I have access to a proper computer.

[meln5674]

Regarding the test-executor failure, it looks like the executor image is going missing between sometime after its loaded at the start of the test, and it looks like the same thing is happening on a lot of recent runs of it. I've seen similar issues before in my own repo's actions when the runner is low on storage and kubelet tries to garbage collect images I need later in the test. You can mitigate this with the imageGC{High,Low}ThresholdPercent kubelet config fields, which are easy enough to set in KinD, but I've never done it in K3s.

[agilgur5]

Thanks for investigating and your detailed root cause analysis!
If you want to improve on anything else in the repo, like other data races, flakey tests (which are themselves sometimes due to races), race detecting tests, or anything else, that'd be great 🙂

This LGTM but I'll defer to Alan for final approval and merge since he wrote this code recently

[Joibel]

Nice find, thank you for fixing it.