Add vhostnet to sf #682
Add vhostnet to sf #682
Conversation
WalkthroughAdds VhostNet detection and conditional info-provider assembly when the selector requests vhost-net, moves the NeedVhostNet flag into GenericNetDeviceSelectors, adjusts a log level, updates tests to cover both vhost-net present/absent scenarios, and reformats a README table (no behavioral change there). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
depends on #681 |
Pull Request Test Coverage Report for Build 20128984235Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/auxnetdevice/auxNetDevice_test.go (1)
305-431: needVhostNet tests cover both paths; consider restoring infoprovider globals and clarifying a commentThe new
Context("with needVhostNet")does a good job exercising both cases (vhost-net present/absent) and validating envs, device specs, and basic topology for the aux device.Two small, optional cleanups:
- Avoid leaking
infoprovider.HostNet/HostTunacross testsBoth Its mutate
infoprovider.HostNetandinfoprovider.HostTunand never restore them. That’s fine today (no other tests depend on the defaults), but it’s cheap insurance to capture and restore the originals so future tests in this package don’t accidentally inherit modified paths.Example patch for the first It:
- Context("with needVhostNet", func() { - It("should provide expected environment variables and mounts", func() { - fs := &utils.FakeFilesystem{ + Context("with needVhostNet", func() { + It("should provide expected environment variables and mounts", func() { + origHostNet := infoprovider.HostNet + origHostTun := infoprovider.HostTun + defer func() { + infoprovider.HostNet = origHostNet + infoprovider.HostTun = origHostTun + }() + + fs := &utils.FakeFilesystem{ @@ - } - defer fs.Use()() - - infoprovider.HostNet = filepath.Join(fs.RootDir, "dev/vhost-net") - infoprovider.HostTun = filepath.Join(fs.RootDir, "dev/net/tun") + } + defer fs.Use()() + + infoprovider.HostNet = filepath.Join(fs.RootDir, "dev/vhost-net") + infoprovider.HostTun = filepath.Join(fs.RootDir, "dev/net/tun")And similarly for the second It:
- It("should not contain vhost related environment variables and mounts if vhost-net device does not exist", func() { - fs := &utils.FakeFilesystem{ + It("should not contain vhost related environment variables and mounts if vhost-net device does not exist", func() { + origHostNet := infoprovider.HostNet + origHostTun := infoprovider.HostTun + defer func() { + infoprovider.HostNet = origHostNet + infoprovider.HostTun = origHostTun + }() + + fs := &utils.FakeFilesystem{ @@ - } - defer fs.Use()() - - infoprovider.HostNet = filepath.Join(fs.RootDir, "dev/vhost-net") - infoprovider.HostTun = filepath.Join(fs.RootDir, "dev/net/tun") + } + defer fs.Use()() + + infoprovider.HostNet = filepath.Join(fs.RootDir, "dev/vhost-net") + infoprovider.HostTun = filepath.Join(fs.RootDir, "dev/net/tun")
- Align the comment with what’s actually asserted
Right now the comment says “validate mounts” but the assertions are on
GetDeviceSpecs():- // validate mounts + // validate device specsNeither change is required for correctness, but they make the tests a bit more robust and self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
README.md(1 hunks)cmd/sriovdp/manager_test.go(1 hunks)go.mod(3 hunks)pkg/accelerator/accelDeviceProvider_test.go(1 hunks)pkg/accelerator/accelDevice_test.go(1 hunks)pkg/accelerator/accelResourcePool_test.go(1 hunks)pkg/accelerator/accelerator_suite_test.go(1 hunks)pkg/auxnetdevice/auxNetDevice.go(1 hunks)pkg/auxnetdevice/auxNetDeviceProvider_test.go(1 hunks)pkg/auxnetdevice/auxNetDevice_test.go(2 hunks)pkg/auxnetdevice/auxNetResourcePool_test.go(1 hunks)pkg/cdi/cdi_test.go(1 hunks)pkg/devices/api_test.go(1 hunks)pkg/devices/devices_suite_test.go(1 hunks)pkg/devices/devices_test.go(1 hunks)pkg/devices/host_test.go(1 hunks)pkg/devices/rdma_test.go(1 hunks)pkg/devices/vdpa_test.go(1 hunks)pkg/factory/factory_test.go(1 hunks)pkg/infoprovider/extraInfoProvider_test.go(1 hunks)pkg/infoprovider/genericInfoProvider_test.go(1 hunks)pkg/infoprovider/infoprovider_suite_test.go(1 hunks)pkg/infoprovider/rdmaInfoProvider_test.go(1 hunks)pkg/infoprovider/uioInfoProvider_test.go(1 hunks)pkg/infoprovider/vdpaInfoProvider_test.go(1 hunks)pkg/infoprovider/vfioInfoProvider_test.go(1 hunks)pkg/infoprovider/vhostNetInfoProvider_test.go(1 hunks)pkg/netdevice/netDeviceProvider_test.go(1 hunks)pkg/netdevice/netResourcePool_test.go(1 hunks)pkg/netdevice/pciNetDevice.go(1 hunks)pkg/netdevice/pciNetDevice_test.go(1 hunks)pkg/resources/ddpSelector_test.go(1 hunks)pkg/resources/deviceSelectors_test.go(1 hunks)pkg/resources/pKeySelector_test.go(1 hunks)pkg/resources/pool_stub_test.go(1 hunks)pkg/resources/resources_suite_test.go(1 hunks)pkg/resources/server_test.go(9 hunks)pkg/types/types.go(2 hunks)pkg/utils/ddp_test.go(1 hunks)pkg/utils/netlink_provider_test.go(1 hunks)pkg/utils/utils_suite_test.go(1 hunks)pkg/utils/utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/auxnetdevice/auxNetDevice.go (1)
pkg/infoprovider/vhostNetInfoProvider.go (2)
VhostNetDeviceExist(50-53)NewVhostNetInfoProvider(45-47)
🪛 LanguageTool
README.md
[uncategorized] ~351-~351: Possible missing article found.
Context: ...N | Target device's vendor Hex code as string ...
(AI_HYDRA_LEO_MISSING_THE)
[grammar] ~351-~351: “Driver” is a singular noun. It appears that the verb form is incorrect.
Context: ... | string list Default: null | "vendors": ["8086", "15b3...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[uncategorized] ~355-~355: Possible missing article found.
Context: ... | "rootDevices": ["0000:86:00.0"] (See follow-up sections for some advance u...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~356-~356: Possible missing article found.
Context: ...t Default: null | "linkTypes": ["ether"] ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~357-~357: Possible missing article found.
Context: ... | bool values true or false Default: false | "isRdma": true ...
(AI_HYDRA_LEO_MISSING_AN)
[uncategorized] ~359-~359: Possible missing article found.
Context: ..." | N | List of vendor specific auxiliary network device types. Device type ca...
(AI_HYDRA_LEO_MISSING_THE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: test-coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (43)
pkg/accelerator/accelResourcePool_test.go (1)
18-18: ✓ Ginkgo v2 import migration looks good.The import path update from
github.com/onsi/ginkgotogithub.com/onsi/ginkgo/v2is correct and follows the standard Ginkgo v2 migration pattern. The rest of the test code is already compatible with Ginkgo v2 (no deprecated v1 APIs are in use), and the Gomega v1.38.2 dependency is compatible with this Ginkgo version.Ensure that all test imports across the codebase have been similarly updated as part of this PR-wide Ginkgo v2 migration, and verify that the full test suite executes successfully with these changes.
pkg/auxnetdevice/auxNetResourcePool_test.go (1)
21-21: Ginkgo v2 import is correct; ensure suite-level migration is consistentSwitching the dot-import to
github.com/onsi/ginkgo/v2matches Ginkgo v2 usage and is compatible with the existingDescribe/Context/Itcalls here. Please just confirm the correspondingsuite_test.gofor this package (and the module’s go.mod) are also on Ginkgo v2 so the runner and CLI flags are consistent across the suite.pkg/utils/netlink_provider_test.go (1)
6-6: Ginkgo v2 import migration looks correctThe switch to
github.com/onsi/ginkgo/v2is consistent with the existing DSL usage in this file and should be safe as-is.pkg/infoprovider/uioInfoProvider_test.go (1)
21-21: LGTM! Clean Ginkgo v2 migration.The import path update is correct. In Ginkgo v2,
DescribeTableis part of the main package rather than a separate extensions package, so this change properly supports the test functions used in this file.pkg/infoprovider/vdpaInfoProvider_test.go (1)
21-21: Ginkgo v2 import looks correct; confirm suite bootstrap and module wiringSwitching to
. "github.com/onsi/ginkgo/v2"is appropriate and should keep these tests working unchanged, assuming the suite bootstrap and go.mod are on v2 as well. Verify that no Ginkgo v1 imports remain elsewhere in the codebase and that go.mod declares the v2 dependency.pkg/infoprovider/vfioInfoProvider_test.go (1)
20-27: Ginkgo v2 import is correct; verify repo-wide consistencySwitching to
. "github.com/onsi/ginkgo/v2"is compatible with the use ofDescribe,DescribeTable, andEntryin this file. Confirm that:
go.moddepends on Ginkgo v2- all remaining imports of
github.com/onsi/ginkgoandgithub.com/onsi/ginkgo/extensions/tablewere updated consistently across the repopkg/devices/api_test.go (1)
21-21: Import path update to Ginkgo v2 is correct.The change from
github.com/onsi/ginkgotogithub.com/onsi/ginkgo/v2is appropriate for the v2 migration.GinkgoT()used in this file remains fully supported in Ginkgo v2 and is not deprecated—no changes needed there.pkg/infoprovider/vhostNetInfoProvider_test.go (1)
23-23: LGTM - Ginkgo v2 import upgrade.The import path change from
github.com/onsi/ginkgotogithub.com/onsi/ginkgo/v2follows the standard v2 migration pattern. Ensure no v1 Ginkgo imports remain mixed with this v2 import in the same package, as Ginkgo v2 does not support mixing versions.pkg/devices/vdpa_test.go (1)
24-24: LGTM! Ginkgo v2 migration.The import path update to Ginkgo v2 is part of the repository-wide test framework upgrade. No functional changes to test behavior.
pkg/resources/pKeySelector_test.go (1)
4-4: LGTM! Ginkgo v2 migration.The import path update aligns with the repository-wide upgrade to Ginkgo v2.
pkg/netdevice/pciNetDevice.go (1)
77-83: LGTM! Improved logging for missing vhost-net.The change from error to warning log level is appropriate, as the absence of
/dev/vhost-netwhenNeedVhostNetis configured may not be critical in all deployment scenarios. Adding the device address improves debuggability and aligns with the similar warning pattern used for vDPA devices (line 64).pkg/netdevice/netDeviceProvider_test.go (1)
20-20: LGTM! Ginkgo v2 migration.Framework upgrade with no functional test changes.
pkg/cdi/cdi_test.go (1)
23-23: LGTM! Ginkgo v2 migration.Import path updated as part of the repository-wide test framework upgrade.
pkg/accelerator/accelDevice_test.go (1)
20-20: LGTM! Ginkgo v2 migration.The import path update to Ginkgo v2 aligns with the broader framework upgrade across the codebase.
pkg/utils/utils_test.go (1)
8-8: LGTM! Ginkgo v2 migration.Framework upgrade with no test logic changes.
pkg/infoprovider/infoprovider_suite_test.go (1)
23-23: LGTM! Ginkgo v2 migration.Import path updated to Ginkgo v2 as part of the repository-wide test framework upgrade.
pkg/infoprovider/rdmaInfoProvider_test.go (1)
21-21: Ginkgo v2 import update is consistent and safeSwitching to
. "github.com/onsi/ginkgo/v2"preserves the existing test semantics and aligns with the repo-wide migration.Please ensure
go test ./...passes with the updated Ginkgo v2 dependency in go.mod/go.sum.pkg/resources/ddpSelector_test.go (1)
6-6: Ginkgo v2 import aligns with the test suite migrationThe updated
. "github.com/onsi/ginkgo/v2"import is correct and leaves the existing tests unaffected.Please verify the full test suite runs cleanly with Ginkgo v2.
pkg/devices/host_test.go (1)
23-23: HostDevice tests correctly migrated to Ginkgo v2Using
. "github.com/onsi/ginkgo/v2"works with the existing GinkgoT/BDD usage and keeps behavior unchanged.Consider running only this package’s tests (
go test ./pkg/devices/...) to double-check the migration locally.pkg/devices/devices_suite_test.go (1)
23-23: Devices suite now correctly targets Ginkgo v2The suite’s import of
. "github.com/onsi/ginkgo/v2"is correct and compatible withRegisterFailHandlerandRunSpecs.Please ensure there are no remaining Ginkgo v1 imports in the devices package.
pkg/devices/rdma_test.go (1)
21-21: RDMA tests use the updated Ginkgo v2 importThe move to
. "github.com/onsi/ginkgo/v2"is consistent with the rest of the suite and should not affect test behavior.A quick
go test ./pkg/devices -run RdmaSpecwould help validate this specific group after the migration.pkg/netdevice/pciNetDevice_test.go (1)
23-23: PciNetDevice tests correctly updated to Ginkgo v2The
. "github.com/onsi/ginkgo/v2"import is appropriate and keeps the existing BDD-style tests intact.Please confirm that
go test ./pkg/netdevice -run PciNetDevicepasses with the new Ginkgo version.pkg/resources/deviceSelectors_test.go (1)
4-4: DeviceSelectors tests migrated to Ginkgo v2 cleanlyUsing
. "github.com/onsi/ginkgo/v2"is consistent with the rest of the project and doesn’t alter test expectations.It’s worth running the resources tests (
go test ./pkg/resources -run DeviceSelectors) to validate the migration.pkg/accelerator/accelDeviceProvider_test.go (1)
20-20: Accelerator provider tests now depend on Ginkgo v2The updated
. "github.com/onsi/ginkgo/v2"import is correct and maintains the existing test behavior.Please re-run the accelerator tests (
go test ./pkg/accelerator/...) to ensure everything passes under Ginkgo v2.pkg/resources/pool_stub_test.go (1)
3-15: Ginkgo v2 import here looks correct and non-breakingSwitching to
. "github.com/onsi/ginkgo/v2"is compatible with the Describe/Context/It usage in this file; no other changes needed here.Please run
go test ./...after the overall migration to confirm there are no hidden v1→v2 incompatibilities in this package.pkg/infoprovider/genericInfoProvider_test.go (1)
20-25: Generic info provider tests are correctly migrated to Ginkgo v2The new
. "github.com/onsi/ginkgo/v2"import matches the Describe/It usage; nothing else in this file needs adjustment for v2.Please verify with
go test ./pkg/infoprovider/...once go.mod is updated to Ginkgo v2.pkg/accelerator/accelerator_suite_test.go (1)
17-22: Suite setup updated to Ginkgo v2 correctlyImporting
. "github.com/onsi/ginkgo/v2"works withRegisterFailHandlerandRunSpecsas used here; suite wiring remains valid.Recommend running
go test ./pkg/accelerator/...to confirm the v2 upgrade end-to-end.pkg/factory/factory_test.go (1)
17-25: Factory tests’ switch to Ginkgo v2 (including tables) is appropriateUsing
. "github.com/onsi/ginkgo/v2"is compatible with the Describe/Context/It and DescribeTable/Entry usage in this file; no additional changes appear necessary.Please run
go test ./pkg/factory/...to ensure the table-driven specs compile and run as expected under v2.pkg/resources/resources_suite_test.go (1)
3-8: Resources suite test correctly migrated to Ginkgo v2The
. "github.com/onsi/ginkgo/v2"import matches the existingRegisterFailHandler/RunSpecsusage; no further adjustments needed here.A quick
go test ./pkg/resources/...after upgrading go.mod will confirm everything is wired correctly.pkg/utils/utils_suite_test.go (1)
3-8: Utils suite import updated to Ginkgo v2 with no behavioral changeSwitching to
. "github.com/onsi/ginkgo/v2"is consistent with the existing suite setup and should be transparent at runtime.Please verify via
go test ./pkg/utils/...once the global Ginkgo v2 migration is complete.pkg/netdevice/netResourcePool_test.go (1)
17-23: NetResourcePool tests’ Ginkgo v2 migration (including tables and GinkgoT) looks soundImporting
. "github.com/onsi/ginkgo/v2"works with Describe/Context/It, DescribeTable/Entry, andGinkgoT()as used here; no additional v2-specific changes are evident.Run
go test ./pkg/netdevice/...to confirm there are no lingering v1-only usages elsewhere in this package.pkg/utils/ddp_test.go (1)
3-12: DDP tests correctly updated to use Ginkgo v2 (tables included)The new
. "github.com/onsi/ginkgo/v2"import is enough for DescribeTable/Entry and other DSL calls; no further code changes are required here.Please confirm with
go test ./pkg/utils/...that the DDP tests compile and pass under Ginkgo v2.go.mod (1)
14-14: LGTM! Ginkgo v2 upgrade is correct.The major version upgrade from Ginkgo v1.16.5 to v2.27.2 with related indirect dependencies is properly reflected. All test files across the repository have been updated to use v2 imports and APIs, confirming this is a coordinated framework migration.
Also applies to: 35-35, 41-41, 60-60, 63-63, 68-68
cmd/sriovdp/manager_test.go (1)
22-22: LGTM! Ginkgo v2 import updated correctly.Import path correctly updated to
github.com/onsi/ginkgo/v2as part of the framework migration.pkg/resources/server_test.go (3)
10-10: LGTM! Ginkgo v2 import updated.
142-143: LGTM! Test signatures correctly updated for Ginkgo v2.Ginkgo v2 removed the
done Donecallback pattern for async tests. These test functions now use the plainfunc()signature, which is the correct v2 pattern.Also applies to: 181-181, 214-214, 386-386, 420-420, 456-456
169-169: LGTM! Timeout assertions correctly migrated to Ginkgo v2 syntax.The
.WithTimeout(time.Second * N)method is the correct Ginkgo v2 way to specify timeouts onEventuallyassertions, replacing the older numeric timeout arguments.Also applies to: 177-178, 210-211, 247-248, 412-413, 415-416, 445-445, 449-449, 485-485, 489-489
pkg/devices/devices_test.go (1)
24-24: LGTM! Ginkgo v2 import updated correctly.pkg/infoprovider/extraInfoProvider_test.go (1)
18-18: LGTM! Ginkgo v2 import updated correctly.pkg/auxnetdevice/auxNetDeviceProvider_test.go (1)
23-23: LGTM! Ginkgo v2 import updated correctly.pkg/auxnetdevice/auxNetDevice.go (1)
69-75: LGTM! VhostNet support follows the established RDMA pattern.The implementation correctly:
- Checks the
NeedVhostNetselector flag (new field onAuxNetDeviceSelectors)- Verifies VhostNet device availability via
infoprovider.VhostNetDeviceExist()- Conditionally appends the VhostNet info provider when the device exists
- Logs a clear warning when the device is required but missing
This follows the same defensive pattern as the RDMA handling above (lines 59-67), maintaining consistency in how optional device features are initialized.
pkg/types/types.go (1)
133-141: NeedVhostNet selector flags look consistent and non-breakingThe new
NeedVhostNetflags onNetDeviceSelectorsandAuxNetDeviceSelectorsare wired consistently (same JSON tag, clear comment) and are additive, so they won’t break existing configs. The symmetry between net and aux-net selectors also makes the behavior easy to reason about.Also applies to: 149-155
pkg/auxnetdevice/auxNetDevice_test.go (1)
20-37: Ginkgo v2 and new imports are wired correctlyThe switch to
github.com/onsi/ginkgo/v2plus the addedfilepathandinfoproviderimports aligns with the new tests and keeps everything compiling cleanly; I don’t see any leftover v1 usage in this file.If you haven’t already, it’s worth running the full test suite to catch any lingering Ginkgo v1-specific patterns elsewhere (
go test ./...should surface them).
|
Hi @adrianchiris you can now rebase :) |
adrianchiris
force-pushed
the
add-vhostnet-to-sf
branch
from
caa886b to
6e76c64
Compare
|
@SchSeba done. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)pkg/auxnetdevice/auxNetDevice.go(1 hunks)pkg/auxnetdevice/auxNetDevice_test.go(3 hunks)pkg/netdevice/pciNetDevice.go(1 hunks)pkg/types/types.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/auxnetdevice/auxNetDevice.go
- pkg/netdevice/pciNetDevice.go
- pkg/auxnetdevice/auxNetDevice_test.go
- pkg/types/types.go
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~359-~359: Use a hyphen to join words.
Context: ...uxTypes" | N | List of vendor specific auxiliary network device types....
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-coverage
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
README.md (1)
351-360: Well-documented addition of needVhostNet selector for auxiliary devices.The new
needVhostNetselector (line 358) is properly documented and mirrors the corresponding entry in the Network devices selectors table (line 342), maintaining consistency across the documentation.
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
just a small nit everything else looks good
pkg/types/types.go
Outdated
| GenericNetDeviceSelectors | ||
| AuxTypes []string `json:"auxTypes,omitempty"` | ||
| AuxTypes []string `json:"auxTypes,omitempty"` | ||
| NeedVhostNet bool `json:"needVhostNet,omitempty"` // share vhost-net along the selected resource |
There was a problem hiding this comment.
can we just add this in GenericNetDeviceSelectors so both struct will use it?
There was a problem hiding this comment.
we can ! i though the struct is shared with the accelerator device. missread it
add support for mounting /dev/vhost-net and /dev/net/tun for AuxNetDevice similarly to PciNetDevice. Signed-off-by: adrianc <[email protected]>
adrianchiris
force-pushed
the
add-vhostnet-to-sf
branch
from
6e76c64 to
3a2cae6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/netdevice/pciNetDevice.go (1)
77-83: Make vhost-net warning robust to overridden HostNet pathThe behavior is fine, but the message hardcodes
/dev/vhost-netwhileVhostNetDeviceExist()checksinfoprovider.HostNet. If tests or deployments overrideHostNet, the log will be misleading.You could tie the message to the actual path:
- if infoprovider.VhostNetDeviceExist() { - infoProviders = append(infoProviders, infoprovider.NewVhostNetInfoProvider()) - } else { - glog.Warningf("vhost-net is required in the configuration for %s but /dev/vhost-net doesn't exist", dev.Address) - } + if infoprovider.VhostNetDeviceExist() { + infoProviders = append(infoProviders, infoprovider.NewVhostNetInfoProvider()) + } else { + glog.Warningf("vhost-net is required in the configuration for %s but %s doesn't exist", dev.Address, infoprovider.HostNet) + }pkg/auxnetdevice/auxNetDevice.go (1)
59-76: Aux vhost-net wiring matches PCI behavior; consider reusing HostNet in logThe new
nf.NeedVhostNethandling correctly mirrors the PCI path and cleanly composes the extra info provider only when/dev/vhost-netis present. Behavior looks good.As in the PCI code, you might want to reference
infoprovider.HostNetin the warning instead of hardcoding/dev/vhost-net, so the log stays accurate if the path is overridden:- if nf.NeedVhostNet { - if infoprovider.VhostNetDeviceExist() { - infoProviders = append(infoProviders, infoprovider.NewVhostNetInfoProvider()) - } else { - glog.Warningf("vhost-net is required in the configuration for %s but /dev/vhost-net doesn't exist", deviceID) - } - } + if nf.NeedVhostNet { + if infoprovider.VhostNetDeviceExist() { + infoProviders = append(infoProviders, infoprovider.NewVhostNetInfoProvider()) + } else { + glog.Warningf("vhost-net is required in the configuration for %s but %s doesn't exist", deviceID, infoprovider.HostNet) + } + }pkg/netdevice/pciNetDevice_test.go (1)
242-253: Selector wiring for NeedVhostNet is correct; double‑check HostNet/HostTun path joiningUsing:
SelectorObjs: []interface{}{&types.NetDeviceSelectors{}, &types.NetDeviceSelectors{ GenericNetDeviceSelectors: types.GenericNetDeviceSelectors{ NeedVhostNet: true, }}, },is the right way to exercise the new
GenericNetDeviceSelectors.NeedVhostNetfield in this test.One thing to verify: in this context you do
infoprovider.HostNet = filepath.Join(fs.RootDir, "/dev/vhost-net") infoprovider.HostTun = filepath.Join(fs.RootDir, "/dev/net/tun")In Go,
filepath.Joinwith a second argument starting with/discards the first component, so these likely evaluate to/dev/...instead of paths underfs.RootDir. In the new aux tests you use relative components ("dev/vhost-net"), which keepsfs.RootDirin the final path.It may be worth aligning this test with the aux behavior (using relative
"dev/..."components) so both suites clearly use the FakeFilesystem-backed paths rather than depending on the real host/dev.Also applies to: 278-280
pkg/auxnetdevice/auxNetDevice_test.go (1)
20-37: Aux vhost-net tests are thorough; consider restoring globals and tightening assertionsThe new “with needVhostNet” context does a good job validating both:
- vhost‑net present: envs include
vhostkeys and two DeviceSpecs for/dev/vhost-netand/dev/net/tun.- vhost‑net absent: only
genericenvs are present and no extra DeviceSpecs are added.A couple of small test‑hygiene tweaks you might consider:
- Around
infoprovider.HostNet/HostTunassignments, save the previous values anddeferrestore them to avoid leaking modified globals into other tests in the same package.- If you care about it, you could also assert the
Permissionsfield on the returnedpluginapi.DeviceSpecs (similar to how it’s done in the vdpa tests), though the current checks on paths are already sufficient for this PR.Also applies to: 305-435
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)pkg/auxnetdevice/auxNetDevice.go(1 hunks)pkg/auxnetdevice/auxNetDevice_test.go(3 hunks)pkg/netdevice/pciNetDevice.go(1 hunks)pkg/netdevice/pciNetDevice_test.go(1 hunks)pkg/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/auxnetdevice/auxNetDevice.go (1)
pkg/infoprovider/vhostNetInfoProvider.go (2)
VhostNetDeviceExist(50-53)NewVhostNetInfoProvider(45-47)
pkg/netdevice/pciNetDevice_test.go (1)
pkg/types/types.go (1)
GenericNetDeviceSelectors(124-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: test-coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/types/types.go (1)
123-141: Sharing NeedVhostNet via GenericNetDeviceSelectors looks correctMoving
NeedVhostNetintoGenericNetDeviceSelectorswithjson:"needVhostNet,omitempty"cleanly enables the same flag for bothNetDeviceSelectorsandAuxNetDeviceSelectorswithout API churn. The embedding and JSON shape remain consistent with existing selector patterns.
adrianchiris
merged commit 218cf28
into
k8snetworkplumbingwg:master
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.