cnff-network: Added ipforwarding tests#1333
cnff-network: Added ipforwarding tests#1333evgenLevin wants to merge 1 commit intorh-ecosystem-edge:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 1 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request updates the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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 |
![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/internal/netnmstate/netnmstate.go (1)
420-435: Minor API inconsistency with existing update function.
UpdatePolicyAndWaitUntilItsDegradedreturns(*nmstate.PolicyBuilder, error)while the existingUpdatePolicyAndWaitUntilItsAvailable(line 77) returns onlyerror. Consider aligning the signatures for consistency, or document why the differing return types are intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/core/network/internal/netnmstate/netnmstate.go` around lines 420 - 435, The function UpdatePolicyAndWaitUntilItsDegraded currently returns (*nmstate.PolicyBuilder, error) while the existing UpdatePolicyAndWaitUntilItsAvailable returns only error; make the API consistent by choosing one signature and updating code accordingly. Either change UpdatePolicyAndWaitUntilItsDegraded to return only error (update its signature, remove nmstatePolicy from the return, and ensure callers of UpdatePolicyAndWaitUntilItsDegraded are adjusted) or change UpdatePolicyAndWaitUntilItsAvailable to return (*nmstate.PolicyBuilder, error) and propagate the builder where needed; locate the functions UpdatePolicyAndWaitUntilItsDegraded and UpdatePolicyAndWaitUntilItsAvailable and the nmstate.PolicyBuilder usages to perform the corresponding signature and caller updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/core/network/metallb/tests/ip-forwarding.go`:
- Around line 324-325: The test is accidentally marked as focused using FIt
which will skip other tests; change the test declaration from FIt("Basic
functionality with MetalLB", reportxml.ID("80340"), func() { ... }) to a normal
It invocation (It("Basic functionality with MetalLB", reportxml.ID("80340"),
func() { ... })) so the test runs normally with the suite; update the occurrence
of FIt to It in the test function definition to remove the focus artifact.
- Around line 583-586: The IPv6 Context's AfterAll only calls cleanupIPFwdNNCP
and omits cleanupIPFwdContextResources, leaving BGP peers/IP
pools/advertisements/services created in the IPv6 BeforeAll uncleaned; update
the IPv6 Context AfterAll to call cleanupIPFwdContextResources() in addition to
cleanupIPFwdNNCP (same order/arguments used in the IPv4 Context AfterAll) so all
context resources created in BeforeAll are properly torn down.
---
Nitpick comments:
In `@tests/cnf/core/network/internal/netnmstate/netnmstate.go`:
- Around line 420-435: The function UpdatePolicyAndWaitUntilItsDegraded
currently returns (*nmstate.PolicyBuilder, error) while the existing
UpdatePolicyAndWaitUntilItsAvailable returns only error; make the API consistent
by choosing one signature and updating code accordingly. Either change
UpdatePolicyAndWaitUntilItsDegraded to return only error (update its signature,
remove nmstatePolicy from the return, and ensure callers of
UpdatePolicyAndWaitUntilItsDegraded are adjusted) or change
UpdatePolicyAndWaitUntilItsAvailable to return (*nmstate.PolicyBuilder, error)
and propagate the builder where needed; locate the functions
UpdatePolicyAndWaitUntilItsDegraded and UpdatePolicyAndWaitUntilItsAvailable and
the nmstate.PolicyBuilder usages to perform the corresponding signature and
caller updates.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf6fd7f4-6811-47c3-84e7-9b9bd5680d9b
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/network/operator.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/nmstate/policy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/nmstate/shared.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (3)
go.modtests/cnf/core/network/internal/netnmstate/netnmstate.gotests/cnf/core/network/metallb/tests/ip-forwarding.go
evgenLevin
force-pushed
the
ipforwatrding
branch
2 times, most recently
from
858ab82 to
5b68fcd
Compare
| const ( | ||
| labelIPForwardingTestCases = "ipforwarding" | ||
| ipFwdNNCPName = "ip-fwd-policy" | ||
| ipFwdBridgeNADName = "internal" |
There was a problem hiding this comment.
A few of these const we already have in metallb/internal/consts.go
| sriovInterfaces, err = NetConfig.GetSriovInterfaces(2) | ||
| if err != nil || len(sriovInterfaces) < 2 { | ||
| Skip("Skipping: need at least 2 SR-IOV interfaces for IP forwarding test") | ||
| } |
There was a problem hiding this comment.
| } | |
| secInterfaces, err = NetConfig.GetSriovInterfaces(2) | |
| if err != nil || len(secInterfaces) < 2 { | |
| Skip("Skipping: need at least 2 secondary interfaces for IP forwarding test") |
| state2, _ := frr.BGPNeighborshipHasState(frrRouter2, neighborIP2, "Established") | ||
|
|
||
| return state1 && state2 | ||
| }, 4*time.Minute, tsparams.DefaultRetryInterval).Should(BeTrue(), |
There was a problem hiding this comment.
| }, 4*time.Minute, tsparams.DefaultRetryInterval).Should(BeTrue(), | |
| }, 3*time.Minute, tsparams.DefaultRetryInterval).Should(BeTrue(), |
slightly more than 2 mins should be enough for bgp sessions
evgenLevin
force-pushed
the
ipforwatrding
branch
from
5b68fcd to
8321972
Compare
evgenLevin
force-pushed
the
ipforwatrding
branch
from
8321972 to
d0b7f8d
Compare
3 participants
Summary by CodeRabbit
Tests
Chores