cnf network: add acc200 tests#932
Conversation
📝 WalkthroughWalkthroughAdds ACC200 device constants and extends the accelerator test suite with VRB (sriov-vrb) support: new VRB helpers, VRB-specific cluster/node config flows, ACC200 test context, and a rename to separate FEC and VRB node-config readiness waiters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 27021bcb260b5a608b339ea6f7efba00806978fc and 9bf4891. ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/cnf/core/network/accelerator/tests/accelerator.go (6)
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 |
![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)
tests/cnf/core/network/accelerator/tests/accelerator.go (1)
174-274: Consider refactoring duplicate test logic.The ACC200 test context (lines 174-274) closely mirrors the ACC100 context (lines 73-172), with only type and constant differences. Consider extracting shared test logic into a parameterized helper function to reduce duplication and improve maintainability.
Example approach:
func testAccelerator( deviceID string, resourceName string, envVar string, expectedPassed int, getNodeConfig func(string) (interface{}, interface{}, error), defineClusterConfig func(string, string, bool) interface{}, waitForConfig func() error, ) { // Shared test logic }This refactoring can be deferred if immediate delivery is prioritized.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d855d80 and 27021bcb260b5a608b339ea6f7efba00806978fc.
⛔ Files ignored due to path filters (11)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/groupversion_info.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/helper.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/sriovvrbclusterconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/sriovvrbnodeconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/clusterconfig.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/clusterconfiglist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/const.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/nodeconfig.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/nodeconfiglist.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
tests/cnf/core/network/accelerator/internal/tsparams/types-vars-consts.go(1 hunks)tests/cnf/core/network/accelerator/tests/accelerator.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/accelerator/tests/accelerator.go (7)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/nodeconfig.go (1)
NodeConfigBuilder(19-29)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/sriovvrbnodeconfig_types.go (1)
SriovAccelerator(16-23)tests/cnf/core/network/accelerator/internal/tsparams/types-vars-consts.go (7)
Acc200DeviceID(24-24)OperatorNamespace(48-48)Acc200ResourceName(26-26)TestNamespaceName(16-16)Acc200EnvVar(28-28)TotalNumberBbdevTests(30-30)ExpectedNumberBbdevTestsPassedForAcc200(34-34)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/clusterconfiglist.go (1)
ListClusterConfig(14-78)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/nodeconfiglist.go (1)
ListNodeConfig(14-78)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/sriov-vrb/clusterconfig.go (2)
ClusterConfigBuilder(19-29)NewClusterConfigBuilder(35-82)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/fec/vrbtypes/sriovvrbclusterconfig_types.go (7)
SriovVrbClusterConfigSpec(167-193)AcceleratorSelector(195-204)PhysicalFunctionConfig(127-138)BBDevConfig(105-108)VRB1BBDevConfig(75-79)ACC100BBDevConfig(45-62)QueueGroupConfig(32-42)
🔇 Additional comments (6)
tests/cnf/core/network/accelerator/internal/tsparams/types-vars-consts.go (1)
23-34: LGTM! ACC200 constants follow established patterns.The new ACC200 constants mirror the existing ACC100 structure and naming conventions. The lower expected test pass count (14 vs 27 for ACC100) suggests different hardware capabilities, which aligns with the VRB1/ACC200 configuration in the test file.
tests/cnf/core/network/accelerator/tests/accelerator.go (5)
13-13: LGTM! VRB imports added correctly.The new VRB-related imports support the ACC200 test implementation and follow the same pattern as existing FEC imports.
Also applies to: 27-27
101-101: LGTM! Function rename improves clarity.Renaming to
waitForFecNodeConfigToSucceedmakes it clear this waits for FEC-specific configs, distinguishing it from the new VRB equivalent.
350-363: LGTM! VRB helper function implemented correctly.The
getVrbNodeConfigWithAccCardfunction correctly mirrors the FEC equivalent and uses appropriate VRB-specific types and API calls.
409-427: LGTM! Wait functions implemented consistently.Both
waitForFecNodeConfigToSucceedandwaitForVrbNodeConfigToSucceedfollow the same polling pattern and correctly use their respective API calls. The explicit naming clarifies which config type each function waits for.Also applies to: 429-447
589-608: Verify 4G queue group configuration.The VRB cluster config sets
NumQueueGroups: 0for both Uplink4G and Downlink4G (lines 592, 597), while 5G queues useNumQueueGroups: 4(lines 602, 607). This suggests 4G functionality is intentionally disabled.Please confirm:
- Is this the correct configuration for ACC200/VRB1 hardware?
- Does the ACC200 device support 4G workloads, or is it 5G-only?
- Should the test expectations account for this configuration difference?
If this configuration is correct and documented, consider adding a brief comment explaining why 4G queues are set to 0 to help future maintainers understand the intent.
| It("node should show acc200 resource", func() { | ||
| Eventually(getNodeResource, 10*time.Minute, time.Second). | ||
| WithArguments(svnc.Object.Name, tsparams.Acc200ResourceName).To(BeNumerically(">", 0)) | ||
| }) |
There was a problem hiding this comment.
Add reportxml.ID annotations for consistency.
The ACC200 test cases are missing reportxml.ID() annotations, while the corresponding ACC100 tests have them (see lines 117 and 122). This inconsistency affects test reporting and traceability.
Apply this diff to add the IDs:
- It("node should show acc200 resource", func() {
+ It("node should show acc200 resource", reportxml.ID("XXXXX"), func() {
Eventually(getNodeResource, 10*time.Minute, time.Second).
WithArguments(svnc.Object.Name, tsparams.Acc200ResourceName).To(BeNumerically(">", 0))
})
- It("validation of acc200 resource", func() {
+ It("validation of acc200 resource", reportxml.ID("XXXXX"), func() {
Eventually(getNodeResource, 10*time.Minute, time.Second).Note: Replace "XXXXX" with appropriate test IDs from your test management system.
Also applies to: 223-273
🤖 Prompt for AI Agents
In tests/cnf/core/network/accelerator/tests/accelerator.go around lines 218-221
and also lines 223-273, several It() test cases for ACC200 are missing
reportxml.ID() annotations; add a reportxml.ID("XXXXX") call as the first
argument inside each It() (matching the pattern used in ACC100 tests), replacing
"XXXXX" with the correct test IDs from your test management system so each test
reads It(reportxml.ID("XXXXX"), "description", func() { ... }) to restore
consistent reporting and traceability.
ajaggapa
force-pushed
the
acc200testsv4
branch
from
27021bc to
9bf4891
Compare
|
Hello, reminder to please rebase this PR now that the release-4.20 branch has been cut. You may need to make some changes to migrate to klog v2, see this slack thread |
2 participants
Summary by CodeRabbit
New Features
Tests