handle: add NewHandleWithOptions(), deprecate DisableVFInfoCollection()#1174
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a one-time global configurator Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
force-pushed
the
tb/handle-options
branch
from
5797a29 to
03c80dd
Compare
ti-mo
force-pushed
the
tb/handle-options
branch
2 times, most recently
from
1c0d4e6 to
1b5de88
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handle_linux_test.go`:
- Around line 21-27: TestConfigureHandle mutates package globals (pkgHandle and
configDone) and never restores them, breaking isolation; update the test to save
orig := pkgHandle and defer restoring pkgHandle and clearing configDone under
configMu (acquire configMu, set configDone = false, release) so the global state
is returned to its original condition after the test, then run ConfigureHandle
assertions as before. Ensure you reference and restore pkgHandle, and protect
writes to configDone using configMu to avoid races.
In `@handle_linux.go`:
- Around line 197-201: The comment for NewHandleWithOptions is misleading
because it states "If opts is nil" although opts is passed by value as
HandleOptions; update the docstring to reflect that the zero value or empty
HandleOptions will trigger defaults instead of nil. Edit the comment above
NewHandleWithOptions to say something like "If opts is the zero value, default
options will be used" and ensure references to opts elsewhere (if any) use the
same non-nil wording; target the NewHandleWithOptions function and the
HandleOptions type in your update.
- Around line 69-70: The lookupByDump boolean must be made concurrency-safe:
change the Handle.lookupByDump field from a plain bool to sync/atomic.Bool and
add the "sync/atomic" import; then replace direct reads/writes in LinkByName and
LinkByAlias with h.lookupByDump.Load() and h.lookupByDump.Store(true/false)
respectively (use Load where the code currently reads the flag and Store where
it writes it) so concurrent goroutines no longer race on that field.
🪄 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: 1aebd270-4458-430c-a1b4-86a5efc154a6
📒 Files selected for processing (3)
handle_linux.gohandle_linux_test.golink_linux.go
There was a problem hiding this comment.
Pull request overview
This PR introduces a declarative HandleOptions configuration flow for netlink Handle creation via NewHandleWithOptions, and adds a one-time configuration entrypoint for the package-global default handle used by the package-level APIs.
Changes:
- Added
NewHandleWithOptions(opts HandleOptions, ...)and repurposedHandleOptionsto carry exported, user-facing settings (DisableVFInfoCollection,RetryInterrupted,NetNS). - Added
ConfigureHandle(opts HandleOptions)to configure the package-widepkgHandleonce per process. - Deprecated
(*Handle).DisableVFInfoCollection()in favor ofHandleOptions.DisableVFInfoCollection, removed the unreleasedRetryInterrupted()handle mutator, and updated link lookup code paths accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
handle_linux.go |
Adds new handle configuration APIs and reshapes HandleOptions usage across handle creation/request construction. |
link_linux.go |
Switches VF info fetching to the new DisableVFInfoCollection option and moves dump-fallback state to Handle.lookupByDump. |
handle_linux_test.go |
Adds a unit test covering the new one-time ConfigureHandle behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is not really a configuration option, but rather an internal cache for remembering the lookup fallback behaviour. Signed-off-by: Timo Beckers <timo@incline.eu>
ti-mo
force-pushed
the
tb/handle-options
branch
from
1b5de88 to
40804be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
link_linux.go (1)
2032-2047:⚠️ Potential issue | 🟠 MajorHonor
DisableVFInfoCollectionin dump-based lookup paths too.This only skips
RTEXT_FILTER_VFon the directRTM_GETLINKrequest. If the kernel falls back tolinkByNameDump()/linkByAliasDump(), those paths go throughHandle.LinkList(), andlink_linux.goLine 2532 still unconditionally adds the VF mask. So the new option is ignored on older kernels, andLinkList()itself still pays the VF-info cost.Suggested fix
func (h *Handle) LinkList() ([]Link, error) { // NOTE(vish): This duplicates functionality in net/iface_linux.go, but we need // to get the message ourselves to parse link type. req := h.newNetlinkRequest(unix.RTM_GETLINK, unix.NLM_F_DUMP) msg := nl.NewIfInfomsg(unix.AF_UNSPEC) req.AddData(msg) - attr := nl.NewRtAttr(unix.IFLA_EXT_MASK, nl.Uint32Attr(nl.RTEXT_FILTER_VF)) - req.AddData(attr) + if !h.options.DisableVFInfoCollection { + attr := nl.NewRtAttr(unix.IFLA_EXT_MASK, nl.Uint32Attr(nl.RTEXT_FILTER_VF)) + req.AddData(attr) + } msgs, executeErr := req.Execute(unix.NETLINK_ROUTE, unix.RTM_NEWLINK)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@link_linux.go` around lines 2032 - 2047, The VF info filter (RTEXT_FILTER_VF) is only skipped for the RTM_GETLINK path but still unconditionally added in dump-based paths (linkByNameDump/linkByAliasDump/Handle.LinkList), so update the dump request builders to respect h.options.DisableVFInfoCollection: wherever nl.NewRtAttr(unix.IFLA_EXT_MASK, nl.Uint32Attr(nl.RTEXT_FILTER_VF)) is added in the dump code paths (e.g., inside linkByNameDump, linkByAliasDump and the LinkList request construction), guard that addition with a check like if !h.options.DisableVFInfoCollection before creating/adding the attribute so older-kernel dump lookups honor the option and avoid collecting VF info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handle_linux_test.go`:
- Around line 24-34: The cleanup must close the real handle and signal its done
channel before restoring originals to avoid leaking FDs/goroutines: inside the
t.Cleanup closure (where ConfigureHandle was used) check if pkgHandle is the
configured (non-nil and not equal to orig) and call its Close method, and if
configDone is the configured channel (not equal to origDone) either close it or
send the done signal expected by the code under test; only after
closing/signaling the configured resources restore pkgHandle=orig and
configDone=origDone while holding configMu. Ensure you reference the
ConfigureHandle installation flow and the pkgHandle/configDone variables when
implementing this.
---
Outside diff comments:
In `@link_linux.go`:
- Around line 2032-2047: The VF info filter (RTEXT_FILTER_VF) is only skipped
for the RTM_GETLINK path but still unconditionally added in dump-based paths
(linkByNameDump/linkByAliasDump/Handle.LinkList), so update the dump request
builders to respect h.options.DisableVFInfoCollection: wherever
nl.NewRtAttr(unix.IFLA_EXT_MASK, nl.Uint32Attr(nl.RTEXT_FILTER_VF)) is added in
the dump code paths (e.g., inside linkByNameDump, linkByAliasDump and the
LinkList request construction), guard that addition with a check like if
!h.options.DisableVFInfoCollection before creating/adding the attribute so
older-kernel dump lookups honor the option and avoid collecting VF info.
🪄 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: d17c85f9-e76e-446b-b0c4-55f20bc898f1
📒 Files selected for processing (3)
handle_linux.gohandle_linux_test.golink_linux.go
This commit repurposes HandleOptions as a declarative configuration mechanism for Handle. It can be passed to NewHandleWithOptions. It also removes the unreleased Handle.RetryInterrupted and deprecates DisableVFInfoCollection in favor of its HandleOptions counterpart. Signed-off-by: Timo Beckers <timo@incline.eu>
ti-mo
force-pushed
the
tb/handle-options
branch
from
40804be to
9d05034
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handle_linux.go`:
- Around line 29-38: ConfigureHandle currently assigns opts into a bare Handle
so fields like NetNS and RetryInterrupted that are applied during socket
creation (see newHandle and newNetlinkRequest) are not honored; update
ConfigureHandle to create a fully-initialized package handle (use the same
initialization logic as newHandle) instead of simply assigning options to an
empty Handle, propagate and return any error from that initialization, and then
set pkgHandle and configDone only after successful initialization so global APIs
observe NetNS, RetryInterrupted, and any socket setup correctly.
🪄 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: 2b756e14-a8dc-48bd-8333-43dfc65a9e93
📒 Files selected for processing (3)
handle_linux.gohandle_linux_test.golink_linux.go
✅ Files skipped from review due to trivial changes (1)
- handle_linux_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- link_linux.go
This commit introduces the funcion ConfigureHandle to replace the global package handle with one configured using the given HandleOptions. It can only be called once per process. Signed-off-by: Timo Beckers <timo@incline.eu>
ti-mo
force-pushed
the
tb/handle-options
branch
from
9d05034 to
6b21e75
Compare
Contributor
|
Hi @ti-mo any update on this one? Thanks! |
Contributor
|
Hi @ti-mo any update on this PR it's blocking as from bump the package |
Contributor
Author
Contributor
|
Hi @aboch can I ask your help reviewing this one as it blocks us in k8snetworkplumbingwg/sriov-cni#404 Thanks! |
Collaborator
|
LGTM 3 independent changes commits, merging as is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repurpose HandleOptions as a declarative configuration mechanism for Handle. It can be passed to NewHandleWithOptions.
It also removes the unreleased Handle.RetryInterrupted and deprecates DisableVFInfoCollection in favor of its HandleOptions counterpart.
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Deprecated