*: remove pkgHandle, remove HandleOptions.NetNS, add NewHandleAtWithOptions - #1197
*: remove pkgHandle, remove HandleOptions.NetNS, add NewHandleAtWithOptions#1197ti-mo wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR updates the package-level handle flow so wrappers call ChangespkgHandle accessor migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-mo
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
handle_linux_test.go (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert through
pkgHandle()instead ofpkgOptions.This test should cover the actual package-level path introduced by this PR; otherwise
ConfigureHandlecould store options correctly whilepkgHandle()drops them.🧪 Proposed test tightening
assert.NoError(t, ConfigureHandle(HandleOptions{DisableVFInfoCollection: true})) - assert.True(t, pkgOptions.DisableVFInfoCollection) + assert.True(t, pkgHandle().options.DisableVFInfoCollection)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@handle_linux_test.go` around lines 30 - 31, The test is asserting against pkgOptions directly instead of the package-level accessor path, so it does not verify what ConfigureHandle actually exposes through pkgHandle(). Update the assertion in handle_linux_test.go to read the stored option from pkgHandle() after calling ConfigureHandle, and keep the check focused on DisableVFInfoCollection so the test covers the real package-level behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@handle_linux.go`:
- Around line 38-49: pkgHandle() reads pkgOptions without synchronization while
ConfigureHandle updates it under configMu, creating a potential race on
package-level calls. Update pkgHandle to read pkgOptions while holding the same
configMu used by ConfigureHandle, and copy the options into a local value before
constructing Handle so concurrent configuration and use stay safe.
In `@handle_retry_linux_test.go`:
- Around line 60-64: Avoid reassigning dumpHandle after registering t.Cleanup,
since the closure captures the variable and will end up closing the newer retry
handle twice while leaking the original one. In handle_retry_linux_test.go, keep
the first handle in a separate variable or register cleanup immediately after
each NewHandleWithOptions call, and make the same adjustment for the second
occurrence noted in the test.
---
Nitpick comments:
In `@handle_linux_test.go`:
- Around line 30-31: The test is asserting against pkgOptions directly instead
of the package-level accessor path, so it does not verify what ConfigureHandle
actually exposes through pkgHandle(). Update the assertion in
handle_linux_test.go to read the stored option from pkgHandle() after calling
ConfigureHandle, and keep the check focused on DisableVFInfoCollection so the
test covers the real package-level behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 688a5c97-3c9b-47f2-91d2-7d0a1eeb2bfa
📒 Files selected for processing (31)
addr_linux.gobridge_linux.gochain_linux.goclass_linux.goconntrack_linux.godevlink_linux.godevlink_test.gofilter_linux.gofou_linux.gogenetlink_linux.gogtp_linux.gohandle_linux.gohandle_linux_test.gohandle_retry_linux_test.goipset_linux.golink_linux.golink_test.goneigh_linux.gonetlink_test.gonetns_linux.gonexthop_linux.goprotinfo_linux.goqdisc_linux.gordma_link_linux.goroute_linux.goroute_test.gorule_linux.gosocket_linux.govdpa_linux.goxfrm_policy_linux.goxfrm_state_linux.go
💤 Files with no reviewable changes (1)
- netlink_test.go
de084cb to
fe91d1c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@aboch PTAL |
Some context about this 10 year old thing. Initially this library only had pkg level methods. Then @mrjana and I encountered the runaway thread issue while operating across nw namespaces in Docker's libnetwork which causes havoc. Golang did not have yet a fix for it. We resorted to the handle thing which would allow the caller to pass the net ns; the first handle's method call would open the nl socket in the current namespace causing the handle to stick to it. To retain the original pkg level methods, a pkg level handle was created. Now, I do not know the current state and reliability of golang threads stitching to OS threads, but I see your change would generate a new handle each time a pkg level method is called. As a side effect, I see we lose the undocumented behavior above. People like you who use this library in their everyday work/products are in the best position to tell whether or not retaining that behavior matters. |
|
Hi @aboch, thanks for the reply. I was also working on netlink-related stuff during that era. Fortunately, the runtime gave us the necessary tools in 1.10 with
I don't think this has ever been true. At least as far back as 2016, appc/cni was calling package-level API from netns-locked goroutines: cilium/cilium@ffb26f3bc24#diff-110a0dfd47350c0dcaee4f867828908c6943b766eee7b100ab3f38f0bd816545R107-R111: err = ns.WithNetNS(hostNS, false, func(_ *os.File) error {
hostVeth, err := netlink.LinkByName(hostVethName)
if err != nil {
return fmt.Errorf("failed to lookup %q in %q: %v", hostVethName, hostNS.Name(), err)
}This requires every call to dial a new socket in order to work. Looking at the code, This patch makes this behaviour explicit and removes the ambiguity around having this globally-shared variable that's essentially unused. |
This commit removes the NetNS field from HandleOptions and adds it as an argument to newHandle(). Including it in HandleOptions was a mistake, as it locks all global functions like LinkList to the calling netns of ConfigureHandle if the latter is used. Add a NewHandleAtWithOptions to enable creating a handle with options in a specific netns. Signed-off-by: Timo Beckers <timo@incline.eu>
This commit removes pkgHandle for two main reasons: 1. It does not, actually, globally cache sockets, unlike its name would suggest. If that were the case, global functions like LinkList would always operate in the netns where the netlink socket was initially opened, which is not the case. At least, this assumption held until I introduced ConfigureHandle in bab08b3, which actually started dialing sockets on the spot (in the calling netns) to stuff them into pkgHandle. This resulted in all global netlink functions breaking when called from other network namespaces after calling ConfigureHandle. 2. Global state, especially of this nature, is widely frowned upon and typically avoided wherever possible. The PR introducing bab08b3 was passing CI, but wasn't really finished and hadn't been vetted properly since I had to take a work break. External pressure caused maintainers to rush the merge anyway. Signed-off-by: Timo Beckers <timo@incline.eu>
fe91d1c to
b9e1b86
Compare
|
@aboch Ping, resolved a merge conflict. |
2 participants
#1174 as it was merged had some problems. I never really vetted it against Cilium as I had to take a break from work and switch tasks. It had a few conceptual mistakes, mostly stemming from my lack of understanding of
pkgHandle. The most problematic being ConfigureHandle() locking all global functions to the netns of the caller of ConfigureHandle, making e.g. LinkList operate in the main netns.This PR proposes removing
pkgHandleoutright, as it only serves as a zero placeholder to promote its methods to package-global functions, and its presence being rather confusing. It also never actually holds any sockets, as doing so would break global function calls from other network namespaces, a common pattern throughout the ecosystem. Also, package-global state of this kind is not a good idea.Summary by CodeRabbit
Bug Fixes
Breaking Changes / Behavior
HandleOptionsno longer includes aNetNSfield; namespace selection is now passed explicitly when creating handles.Tests