cnf: network add ovnk routeadvertisement step to FRR test case #822
Conversation
📝 WalkthroughWalkthroughAdds OVN-K RouteAdvertisement integration to MetalLB FRR-K8s tests: new test context with lifecycle hooks, seven new exported helpers for OVN-K RouteAdvertisement lifecycle and verification, dynamic verification of pod-network routes via OVN-K, and an updated BGP peer helper signature to accept an optional DisableMP flag. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (10)
📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)
🔇 Additional comments (8)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed 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: 5
♻️ Duplicate comments (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go (1)
436-469: Remove duplicate WithBGPNeighborDisableMP.Same signature already defined earlier; see prior comment and diff.
🧹 Nitpick comments (17)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go (2)
380-385: Clarify TopologyKey precedence and give concrete examples.Ambiguity: “Ignored if Placement is set” could be read as cluster‑level or device‑set‑level. Recommend scoping it explicitly and adding examples.
Apply this diff to the comment:
- // TopologyKey is the Kubernetes topology label that the - // StorageClassDeviceSets will be distributed across. Ignored if - // Placement is set + // TopologyKey is the Kubernetes topology label key used to distribute + // StorageClassDeviceSets across topology domains (e.g., "topology.kubernetes.io/zone" or "topology.kubernetes.io/hostname"). + // Ignored if StorageDeviceSet.Placement is set.
391-391: Low risk — field reorder OK; optional MinLength validation recommendedRepo search found only the autogenerated deepcopy and a unit test that constructs StorageDeviceSet (tests/system-tests/vcore/internal/vcorecommon/odf.go); no YAML/JSON string comparisons or golden files that would be broken by key-order changes. Optional: add kubebuilder validation to enforce non-empty Name.
- Name string `json:"name"` + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"`Location: vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go:391.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.go (1)
1-3: Fix package doc typo.Spelling and punctuation nit.
Apply:
-// Package types contains shared types accrorss API Schema definitions +// Package types contains shared types across API schema definitions.vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
14-14: Invalid/unused kubebuilder marker.Extra “+kubebuilder::singular=…” (double colon) is not a valid marker and is redundant given Line 13’s resource marker already sets singular.
Apply:
-// +kubebuilder::singular=routeadvertisementsvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
1-1: Optional: add go:build tag for modern toolchainsConsider adding the corresponding //go:build !ignore_autogenerated line above the legacy // +build tag to silence build-tag linters. Since this file is generated, do it via controller-gen configuration, not manual edits.
Do you want me to check your controller-gen version/flags and propose the exact change?
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
42-46: Doc fix: wrong NetworkSelectionType mentionedThe comment says SecondaryUserDefinedNetworks but should say NetworkAttachmentDefinitions.
Apply this diff:
-// selected NetworkAttachmentDefinitions when NetworkSelectionType is -// 'SecondaryUserDefinedNetworks'. +// selected NetworkAttachmentDefinitions when NetworkSelectionType is +// 'NetworkAttachmentDefinitions'.
72-76: Typo in comment (“mot” → “not”)Minor doc nit.
-// selector will mot match anything, while an empty ({}) selector will match +// selector will not match anything, while an empty ({}) selector will match
81-85: Typo in comment (“servind” → “serving”)Minor doc nit.
-// namespaceSelector select the primary UserDefinedNetworks that are servind +// namespaceSelector selects the primary UserDefinedNetworks that are servingtest_routeadv.go (1)
56-60: Redundant error print before log.Fatallog.Fatal already prints the error; the preceding Printf is noise.
- if err != nil { - fmt.Printf("RouteAdvertisement creation failed with error: %v\n", err) - log.Fatal(err) - } + if err != nil { + log.Fatal(err) + }vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (2)
51-56: Don’t silently ignore invalid selectors; return an errorQuietly dropping bad selectors can yield surprising broad lists. Bubble parse errors instead.
listOpts := []runtimeClient.ListOption{} if passedOptions.LabelSelector != "" { selector, parseErr := labels.Parse(passedOptions.LabelSelector) - if parseErr == nil { - listOpts = append(listOpts, runtimeClient.MatchingLabelsSelector{Selector: selector}) - } + if parseErr != nil { + return nil, fmt.Errorf("invalid label selector %q: %w", passedOptions.LabelSelector, parseErr) + } + listOpts = append(listOpts, runtimeClient.MatchingLabelsSelector{Selector: selector}) } if passedOptions.FieldSelector != "" { selector, parseErr := fields.ParseSelector(passedOptions.FieldSelector) - if parseErr == nil { - listOpts = append(listOpts, runtimeClient.MatchingFieldsSelector{Selector: selector}) - } + if parseErr != nil { + return nil, fmt.Errorf("invalid field selector %q: %w", passedOptions.FieldSelector, parseErr) + } + listOpts = append(listOpts, runtimeClient.MatchingFieldsSelector{Selector: selector}) }Also applies to: 57-62
16-18: Optional: accept context from callerTaking a context parameter avoids hardcoded context.TODO and lets callers control timeouts/cancellation.
-func ListRouteAdvertisements(apiClient *clients.Settings, options ...metav1.ListOptions) ([]*RouteAdvertisementBuilder, error) { +func ListRouteAdvertisements(ctx context.Context, apiClient *clients.Settings, options ...metav1.ListOptions) ([]*RouteAdvertisementBuilder, error) { ... -err = apiClient.Client.List(context.TODO(), routeAdvertisementList, listOpts...) +err = apiClient.Client.List(ctx, routeAdvertisementList, listOpts...)tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
127-127: Avoid fixed sleep; wait on a concrete readiness signal.Use Eventually on route visibility instead of a 30s sleep to reduce flakiness.
- time.Sleep(30 * time.Second) + Eventually(func() string { + routes, err := frr.VerifyBGPReceivedRoutesOnFrrNodes(frrk8sPods) + if err != nil { + return "" + } + return routes + }, time.Minute, tsparams.DefaultRetryInterval).ShouldNot(BeEmpty(), "BGP routes not yet visible")
724-729: Use shared password constant; avoid hard‑coding.Keeps config consistent with BGPPeer setup.
- frrConfig. - WithBGPPassword("bgp-test", 0, 0). + frrConfig. + WithBGPPassword(tsparams.BGPPassword, 0, 0). WithPort(179, 0, 0). WithBGPNeighborDisableMP(true, 0, 0)
81-101: Streamline RA cleanup and ignore NotFound.Consider pulling the RA and deleting with NotFound ignored to avoid double GET/Exists and races.
- routeAdv := ovn.NewRouteAdvertisementBuilder( /* ... */ ) - if routeAdv != nil && routeAdv.Exists() { - err := routeAdv.Delete() + routeAdv := ovn.NewRouteAdvertisementBuilder( /* ... */ ) + if routeAdv != nil { + err := routeAdv.Delete() // rely on Delete to ignore NotFound (see vendor fix) if err != nil { By(fmt.Sprintf("Failed to delete RouteAdvertisement: %v", err)) } else { By("RouteAdvertisement deleted successfully") } - } else { - By("RouteAdvertisement does not exist or builder creation failed") }
828-829: Minor wording nit in failure message.Fix grammar and double space.
- "Failed the blocked route was received.") + "Failed: the blocked route was received.")vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go (1)
1-1: Addgo:buildline for Go 1.17+ build tags.Controller‑gen typically emits both forms; keep the existing
+buildline and add the new one.+//go:build !ignore_autogenerated // +build !ignore_autogeneratedvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (1)
208-216: Delete should ignore NotFound.Avoid surfacing benign races where the object disappears between Exists and Delete.
- err := builder.apiClient.Delete(context.TODO(), builder.Definition) - - if err != nil { - return fmt.Errorf("can not delete RouteAdvertisement: %w", err) - } + err := builder.apiClient.Delete(context.TODO(), builder.Definition) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("can not delete RouteAdvertisement: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e61e5b3 and 748a3698a32e3687a40fd92ea914d44245a230af.
📒 Files selected for processing (16)
go.mod(1 hunks)test_routeadv.go(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(13 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/types.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/zz_generated.deepcopy.go(0 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go(2 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go(1 hunks)
💤 Files with no reviewable changes (1)
- vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code graph analysis (8)
test_routeadv.go (3)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(79-79)PodNetwork(83-83)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (1)
NewRouteAdvertisementBuilder(32-94)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (3)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (1)
RouteAdvertisementBuilder(20-29)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
AddToScheme(13-13)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisementsList(71-75)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (6)
ClusterUserDefinedNetworkSelector(71-77)NetworkSelector(21-47)NetworkAttachmentDefinitionSelector(104-116)PrimaryUserDefinedNetworkSelector(80-86)SecondaryUserDefinedNetworkSelector(89-101)NetworkSelectors(14-14)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(71-75)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go (2)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (5)
RouteAdvertisements(19-25)RouteAdvertisementsList(71-75)RouteAdvertisementsSpec(30-55)AdvertisementType(79-79)RouteAdvertisementsStatus(58-66)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (3)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (3)
RouteAdvertisements(19-25)AdvertisementType(79-79)RouteAdvertisementsSpec(30-55)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (5)
AddToScheme(13-13)Kind(17-19)GroupName(10-10)SchemeGroupVersion(11-11)Resource(22-24)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
NewRouteAdvertisementBuilder(32-94)RouteAdvertisementBuilder(20-29)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(79-79)PodNetwork(83-83)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)tests/cnf/core/network/metallb/internal/tsparams/consts.go (4)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPTestPeer(36-36)BGPPassword(21-21)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgppeer.go (1)
NewBPGPeerBuilder(33-85)tests/cnf/core/network/metallb/internal/frr/frr.go (1)
IsProtocolConfigured(286-300)tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
DefaultRetryInterval(76-76)
🔇 Additional comments (12)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go (1)
42-42: Doc tweak looks fine.No functional change; comment remains accurate.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/types.go (1)
147-166: Comment-only touch — OK.No functional changes in this hunk. LGTM.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
28-29: Harden CEL validation for nodeSelector vs PodNetworkReplace brittle nested has() checks with a parent guard; I confirmed the XValidation markers exist and the NetworkSelectionType JSON tag is networkSelectionType — run controller-gen to ensure the new CEL rule compiles with your k8s version.
File: vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (lines ~28-29)
-// +kubebuilder:validation:XValidation:rule="(!has(self.nodeSelector.matchLabels) && !has(self.nodeSelector.matchExpressions)) || !('PodNetwork' in self.advertisements)",message="If 'PodNetwork' is selected for advertisement, a 'nodeSelector' can't be specified as it needs to be advertised on all nodes" +// +kubebuilder:validation:XValidation:rule="!('PodNetwork' in self.advertisements) || !has(self.nodeSelector) || (size(self.nodeSelector.matchLabels) == 0 && size(self.nodeSelector.matchExpressions) == 0)",message="If 'PodNetwork' is selected for advertisement, 'nodeSelector' must be empty (applies to all nodes)" // +kubebuilder:validation:XValidation:rule="!self.networkSelectors.exists(i, i.networkSelectionType != 'DefaultNetwork' && i.networkSelectionType != 'ClusterUserDefinedNetworks')",message="Only DefaultNetwork or ClusterUserDefinedNetworks can be selected"go.mod (1)
287-287: Avoid long‑lived replace to a personal fork — pin commit & sync go.mod/vendor
Verified: WithBGPNeighborDisableMP only appears in vendor (vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go) and tests (tests/cnf/core/network/metallb/tests/frrk8-tests.go:728). vendor/modules.txt lists github.com/rh-ecosystem-edge/eco-goinfra v0.0.0-20250911153636-0e95468f6567 — which differs from the gkopels pseudo‑version referenced in the PR (v0.0.0-20250901090738-8d39017a34ca).
Actionable steps: pin the require to the exact pseudo‑version you mean to vendor (or re‑vendor to the forked pseudo‑version), add a TODO with the upstream PR/commit reference, align the go.mod require with the replace to avoid confusion during tidying/vendoring, and remove the replace once upstream merges (or move the fork under an org namespace if it must persist).
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
19-19: Autogenerated deepcopy code — OK to vendor as-isGenerated by controller-gen; pointer and slice deep-copies look correct. No action.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.go (1)
1-6: Package markers look correctGroup name and deepcopy-gen markers are fine.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (1)
74-83: LGTM on wrapping listed objectsCopying each item and setting both Definition and Object is consistent with other builders.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
16-21: XValidation set looks sound — union marker names confirmedThe API‑Unions KEP defines the markers // +unionDiscriminator and // +unionMember[=][,optional]. LGTM. If controller-gen version differences matter, verify your controller-tools/controller-gen supports these markers; I can look up the exact controller-tools/controller-gen release that added support.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (2)
9-14: Scheme registration looks correct.Group/version, builder, and AddToScheme wiring are standard and OK.
26-34: Confirm CRD Kind vs. resource naming (plural/singular).Kind is “RouteAdvertisements” (plural). That’s unusual; please confirm the installed CRD Kind matches this exact string to avoid GVK mismatches at runtime.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go (1)
29-137: Deepcopy implementations look valid.Struct fields and slices are deeply copied correctly.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (1)
61-69: Confirm Kind string matches CRD.TypeMeta.Kind is set to "RouteAdvertisements". Please verify the CRD Kind is identical to avoid server rejecting creates.
test_routeadv.go
Outdated
| routeAdv := ovn.NewRouteAdvertisementBuilder( | ||
| apiClient.Client, | ||
| "test-routeadv", | ||
| advertisements, | ||
| nodeSelector, | ||
| frrConfigurationSelector, | ||
| networkSelectors) |
There was a problem hiding this comment.
*Compile-time type mismatch: pass clients.Settings, not apiClient.Client
NewRouteAdvertisementBuilder expects *clients.Settings; you’re passing the underlying controller-runtime client. This won’t compile and also bypasses scheme attachment.
Apply this diff:
- routeAdv := ovn.NewRouteAdvertisementBuilder(
- apiClient.Client,
+ routeAdv := ovn.NewRouteAdvertisementBuilder(
+ apiClient,
"test-routeadv",
advertisements,
nodeSelector,
frrConfigurationSelector,
networkSelectors)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| routeAdv := ovn.NewRouteAdvertisementBuilder( | |
| apiClient.Client, | |
| "test-routeadv", | |
| advertisements, | |
| nodeSelector, | |
| frrConfigurationSelector, | |
| networkSelectors) | |
| routeAdv := ovn.NewRouteAdvertisementBuilder( | |
| apiClient, | |
| "test-routeadv", | |
| advertisements, | |
| nodeSelector, | |
| frrConfigurationSelector, | |
| networkSelectors) |
🤖 Prompt for AI Agents
In test_routeadv.go around lines 40 to 46, the NewRouteAdvertisementBuilder call
is passing apiClient.Client (a controller-runtime client) but the function
expects *clients.Settings; replace the apiClient.Client argument with the
*clients.Settings instance (e.g., apiClient or apiClient.Settings pointer that
you already have) so the builder receives the correct type and the scheme
remains attached.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go
Outdated
Show resolved
Hide resolved
| return err == nil || !errors.IsNotFound(err) | ||
| } |
There was a problem hiding this comment.
Fix Exists() logic: treat only a successful GET as existence.
Current logic returns true on any non-NotFound error, masking auth/network errors and breaking Create/Delete flows.
- return err == nil || !errors.IsNotFound(err)
+ return err == nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return err == nil || !errors.IsNotFound(err) | |
| } | |
| return err == nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go
around lines 236-237, the Exists() logic currently returns true for any error
that is not a NotFound, which incorrectly treats auth/network errors as
existence; change the return so only a successful GET counts as existence (i.e.,
return err == nil), and ensure calling code handles/propagates other errors
rather than relying on Exists() to claim existence.
gkopels
changed the title
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
2 times, most recently
from
489b3d4 to
e2ac446
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (18)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
142-146: Use the same RouteAdvertisement name as in cleanup.Creation uses "default" while cleanup may delete a different RA if we change the name later. Use the shared routeAdvName here too.
Apply:
- _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName)
725-730: Use the shared BGP password constant; avoid magic values.Replace the string literal with tsparams.BGPPassword for consistency.
Apply:
- WithBGPPassword("bgp-test", 0, 0). + WithBGPPassword(tsparams.BGPPassword, 0, 0).vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go (1)
380-385: New TopologyKey: confirm semantics; consider adding cross-field validation.Behavior note says it’s ignored when Placement is set. If that’s the intended contract, consider enforcing it to prevent confusing specs by rejecting CRs that set both fields.
Type-level Kubebuilder validation (place above
type StorageDeviceSet struct {):// +kubebuilder:validation:XValidation:rule="!(has(self.placement) && has(self.topologyKey))",message="Set either placement or topologyKey, not both"Optionally, expand the comment with an example key (e.g., topology.kubernetes.io/zone) to guide users.
tests/cnf/core/network/metallb/tests/common.go (1)
547-551: Reduce duplicate status fetch and enrich failure context.You fetch BGP status twice. Consider using an Eventually body that asserts on the fetched object once, and include key details on failure (e.g., present route keys) to speed up triage.
Would you like me to propose a Gomega Eventually-with-G form to assert Exists and emit current bgpStatus.Routes on failures?
vendor/modules.txt (1)
2265-2266: Replace mapping to a personal fork.Prefer reverting to upstream rh-ecosystem-edge/eco-goinfra when possible. Pin a tag/commit and track a follow‑up issue to remove the replace once upstream carries these types.
If helpful, I can draft the follow‑up issue template.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/clients/clients.go (1)
173-176: Register OVN RouteAdvertisement types in the client scheme.Solid. Given this central registration, the extra AttachScheme in ovn.ListRouteAdvertisements becomes redundant (harmless). Consider removing the per-call attach to reduce repetition.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.go (1)
1-3: Fix package doc typos.Minor copyedit for clarity.
Apply:
-// Package types contains shared types accrorss API Schema definitions +// Package types contains shared types across API schema definitions. // +k8s:deepcopy-gen=packagevendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (2)
24-29: Defensive scheme attach.Attaching the scheme here is safe; with central registration in clients.SetScheme it’s redundant. Keeping it is fine if you expect external callers; otherwise you can drop it.
51-62: Surface invalid selector errors instead of silently ignoring.Currently parse errors are ignored, which can hide typos and return surprising results.
Apply:
- if passedOptions.LabelSelector != "" { - selector, parseErr := labels.Parse(passedOptions.LabelSelector) - if parseErr == nil { - listOpts = append(listOpts, runtimeClient.MatchingLabelsSelector{Selector: selector}) - } - } + if passedOptions.LabelSelector != "" { + selector, parseErr := labels.Parse(passedOptions.LabelSelector) + if parseErr != nil { + return nil, fmt.Errorf("invalid LabelSelector %q: %w", passedOptions.LabelSelector, parseErr) + } + listOpts = append(listOpts, runtimeClient.MatchingLabelsSelector{Selector: selector}) + } - if passedOptions.FieldSelector != "" { - selector, parseErr := fields.ParseSelector(passedOptions.FieldSelector) - if parseErr == nil { - listOpts = append(listOpts, runtimeClient.MatchingFieldsSelector{Selector: selector}) - } - } + if passedOptions.FieldSelector != "" { + selector, parseErr := fields.ParseSelector(passedOptions.FieldSelector) + if parseErr != nil { + return nil, fmt.Errorf("invalid FieldSelector %q: %w", passedOptions.FieldSelector, parseErr) + } + listOpts = append(listOpts, runtimeClient.MatchingFieldsSelector{Selector: selector}) + }vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
9-14: Prefer const for GroupName.Small polish to reflect immutability.
Apply:
-var ( - GroupName = "k8s.ovn.org" +const GroupName = "k8s.ovn.org" + +var ( SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1"} SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes) AddToScheme = SchemeBuilder.AddToScheme )vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
13-16: Fix kubebuilder marker; remove invalid duplicate.The double-colon marker is invalid and can confuse code generators. Also, singular should typically be the singular Kind.
Apply:
-// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisements -// +kubebuilder::singular=routeadvertisements +// +kubebuilder:resource:path=routeadvertisements,scope=Cluster,shortName=ra,singular=routeadvertisementIf an existing CRD depends on a different “singular”, keep it as-is; otherwise aligning to “routeadvertisement” matches conventions.
51-57: Required field shouldn’t be omitempty; keep uniqueness rule.Since advertisements is Required with Min/MaxItems, drop omitempty to avoid ambiguity.
Apply:
- Advertisements []AdvertisementType `json:"advertisements,omitempty"` + Advertisements []AdvertisementType `json:"advertisements"`vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (3)
30-85: Validate required NetworkSelectors on builder construction.Spec enforces MinItems=1; guard early to avoid server-side validation failures.
if len(advertisements) == 0 { @@ } + if len(networkSelectors) == 0 { + glog.V(100).Infof("RouteAdvertisement 'networkSelectors' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'networkSelectors' cannot be empty" + return builder + }
286-297: WithNetworkSelectors: enforce non-empty input (MinItems=1).Mirror builder constructor validation.
func (builder *RouteAdvertisementBuilder) WithNetworkSelectors(networkSelectors types.NetworkSelectors) *RouteAdvertisementBuilder { if valid, _ := builder.validate(); !valid { return builder } glog.V(100).Infof("Setting RouteAdvertisement %s networkSelectors to %v", builder.Definition.Name, networkSelectors) - builder.Definition.Spec.NetworkSelectors = networkSelectors + if len(networkSelectors) == 0 { + glog.V(100).Infof("RouteAdvertisement 'networkSelectors' cannot be empty") + builder.errorMsg = "RouteAdvertisement 'networkSelectors' cannot be empty" + return builder + } + builder.Definition.Spec.NetworkSelectors = networkSelectors return builder }
143-160: Optional: Make Create idempotent by populating Object when the resource already exists.Helps callers that rely on Object being set post-Create.
glog.V(100).Infof("Creating the RouteAdvertisement %s", builder.Definition.Name) var err error - if !builder.Exists() { - err = builder.apiClient.Create(context.TODO(), builder.Definition) - if err == nil { - builder.Object = builder.Definition - } - } + if builder.Exists() { + builder.Object, _ = builder.Get() + return builder, nil + } + err = builder.apiClient.Create(context.TODO(), builder.Definition) + if err == nil { + builder.Object = builder.Definition + } return builder, errvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
42-46: Fix doc: wrong discriminator target.Should reference NetworkAttachmentDefinitions, not SecondaryUserDefinedNetworks.
-// 'SecondaryUserDefinedNetworks'. +// 'NetworkAttachmentDefinitions'.
72-76: Typos in comment (“mot” → “not”).-// selector will mot match anything, while an empty ({}) selector will match +// selector will not match anything, while an empty ({}) selector will match
81-86: Grammar fixes in comment.-// namespaceSelector select the primary UserDefinedNetworks that are servind +// namespaceSelector selects the primary UserDefinedNetworks that are serving
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 489b3d4efb405f8b38a02b6192c7ddff644d25d5 and e2ac446921051327896d461ee8bb588264320f71.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
go.mod(2 hunks)test_routeadv.go(1 hunks)tests/cnf/core/network/metallb/internal/frr/frr.go(1 hunks)tests/cnf/core/network/metallb/tests/common.go(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(13 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/clients/clients.go(2 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/imagestream/imagestream.go(0 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/types.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/zz_generated.deepcopy.go(0 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go(2 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go(1 hunks)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go(1 hunks)vendor/modules.txt(4 hunks)
💤 Files with no reviewable changes (2)
- vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/imagestream/imagestream.go
- vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/zz_generated.deepcopy.go
✅ Files skipped from review due to trivial changes (2)
- vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.go
- tests/cnf/core/network/metallb/internal/frr/frr.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- test_routeadv.go
🧰 Additional context used
🧬 Code graph analysis (9)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/clients/clients.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
AddToScheme(13-13)
tests/cnf/core/network/metallb/tests/common.go (1)
tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
DefaultRetryInterval(76-76)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (4)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/clients/clients.go (1)
Settings(51-68)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (1)
RouteAdvertisementBuilder(19-28)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
AddToScheme(13-13)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
RouteAdvertisementsList(93-97)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (3)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (3)
RouteAdvertisements(19-25)AdvertisementType(61-61)RouteAdvertisementsSpec(30-57)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (4)
Kind(17-19)GroupName(10-10)SchemeGroupVersion(11-11)Resource(22-24)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go (3)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (5)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)RouteAdvertisementsSpec(30-57)AdvertisementType(61-61)RouteAdvertisementsStatus(74-88)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)vendor/github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1/types.go (1)
Condition(218-240)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
NewRouteAdvertisementBuilder(31-85)RouteAdvertisementBuilder(19-28)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(61-61)PodNetwork(65-65)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)tests/cnf/core/network/metallb/internal/tsparams/consts.go (4)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPTestPeer(36-36)BGPPassword(21-21)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgppeer.go (1)
NewBPGPeerBuilder(33-85)tests/cnf/core/network/metallb/internal/frr/frr.go (1)
IsProtocolConfigured(286-300)tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
DefaultRetryInterval(76-76)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (1)
NetworkSelectors(14-14)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
RouteAdvertisements(19-25)RouteAdvertisementsList(93-97)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (6)
ClusterUserDefinedNetworkSelector(71-77)NetworkSelector(21-47)NetworkAttachmentDefinitionSelector(104-116)PrimaryUserDefinedNetworkSelector(80-86)SecondaryUserDefinedNetworkSelector(89-101)NetworkSelectors(14-14)
🔇 Additional comments (18)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
8-11: OVN RouteAdvertisement imports look correct; verify scheme registration.Looks good. Please confirm the controller-runtime client has the RouteAdvertisement scheme attached (e.g., via AddToScheme/AttachScheme) somewhere in test setup; otherwise Create/Get/Delete will fail at runtime.
Also applies to: 32-32
833-861: BGPPeer builder and verification LGTM.disableMP, optional BFD/connectTime, and post‑create verification against FRR pods look good.
903-924: Don’t hard‑code OpenShift pod CIDR; use the provided expected prefixes.The helper ignores its parameter and assumes 10.128.0.0/14. Make it data‑driven as intended.
Apply:
func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { By("Verifying external FRR pod receives pod network routes from RouteAdvertisement") - By("Expected routes: cluster network 10.128.0.0/14 broken into /23 subnets per worker node") - - Eventually(func() string { - // Get BGP routes received by the external FRR pod - output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) - if err != nil { - By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" - } - - routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes (checking for cluster network routes): %s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring("10.128."), // Primary cluster network range (10.128.0.0/14) - ContainSubstring("10.129."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.130."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.131."), // Additional possible ranges within 10.128.0.0/14 - ), "External FRR pod should receive pod network routes (/23 subnets) from cluster network 10.128.0.0/14 via RouteAdvertisement") + Eventually(func() bool { + output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) + if err != nil { + By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) + return false + } + routes := output.String() + for _, prefix := range expectedPodNetworkPrefixes { + if strings.Contains(routes, prefix) { + return true + } + } + return false + }, 3*time.Minute, 15*time.Second).Should(BeTrue(), + fmt.Sprintf("External FRR pod should receive at least one of the expected pod network prefixes: %v", + expectedPodNetworkPrefixes)) }
106-106: Remove focused spec (FIt) before merge.Focused specs will skip the rest of the suite in CI.
Apply:
- FIt("Verify the FRR node only receives routes that are configured in the allowed prefixes", + It("Verify the FRR node only receives routes that are configured in the allowed prefixes",Run to ensure no other focused specs remain:
#!/bin/bash rg -nP '\bF(It|Describe|Context|When|Specify)\b' -g '!**/vendor/**' -C2vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocm/clusterv1/types.go (1)
145-167: Breaking surface change: ManagedClusterStatus fields/types removed — verify no in-repo usages.The removal of ManagedNamespaces (and related ManagedNamespaceConfig/ClusterSetManagedNamespaceConfig types per PR context) is a compile-time breaking change if any code references them. Runtime decoding of legacy JSON will silently drop unknown fields, but builds will fail if symbols are used.
Run to confirm no usages outside vendor and that deepcopy code no longer references the removed symbols:
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.go (2)
42-43: Doc-only change for Placement looks good.No functional impact. Comment is clear and accurate.
391-396: Reordering public struct fields can break unkeyed composite literals.JSON tags are unchanged (runtime API remains stable), but consumers using positional struct literals may compile‑break or misassign values. Please verify we don’t instantiate
StorageDeviceSetunkeyed in this repo.Run to inspect call sites (exclude vendor) and eyeball for keyed vs unkeyed literals:
Expected: all literals use keyed fields (contain
FieldName:in the snippet).vendor/modules.txt (2)
1172-1173: RouteAdvertisement schemes are vendored.Good to see the OVN routeadvertisement scheme paths present; aligns with the client scheme registration.
1017-1019: Forked eco-goinfra and a nonstandard “go 1.24.5” line.The vendor entry pins a fork (gkopels/eco-goinfra) and declares go 1.24.5. Verify CI/tooling uses a compatible Go version and that the fork pin is intentional and tracked.
Run:
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/clients/clients.go (1)
28-28: Importing OVN RouteAdvertisement scheme.Import alias looks good and keeps scheme wiring local to clients.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.go (2)
34-38: Single options guard is clear.Good input validation and error message.
74-83: Avoids range-var address pitfall.Copying the item before taking its address is correct. LGTM.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
27-31: CEL validation reads well.The PodNetwork/nodeSelector mutual-exclusion rule is clear.
29-30: Confirm nested field names referenced in CEL.The rule references i.networkSelectionType; verify the JSON name matches the underlying type in ovn/types.NetworkSelector. If it differs, the CEL will never match.
I can scan the repo and confirm the field/tag if you want.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.go (1)
21-141: LGTM: autogenerated deepcopy matches types and handles slices and nested structs correctly.No issues spotted in generated code.
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
129-134: Confirm scope: add Namespace to Get() key if the resource is namespaced.If RouteAdvertisements are namespaced, pass Namespace in client.ObjectKey; otherwise this Get will never find objects outside default scope.
Would you confirm the CRD scope? If namespaced, update to:
- err := builder.apiClient.Get(context.TODO(), client.ObjectKey{ - Name: builder.Definition.Name, - }, routeAdvertisement) + err := builder.apiClient.Get(context.TODO(), client.ObjectKey{ + Namespace: builder.Definition.Namespace, + Name: builder.Definition.Name, + }, routeAdvertisement)
190-203: Fix Exists(): treat only successful GET as existence; don’t mask other errors.Current logic returns true for any non-NotFound error, which can:
- Make Create() skip creation on transient/auth errors.
- Break PullRouteAdvertisement by setting Definition to nil after a failed Get.
Apply:
func (builder *RouteAdvertisementBuilder) Exists() bool { @@ - var err error - builder.Object, err = builder.Get() - - return err == nil || !errors.IsNotFound(err) + var err error + builder.Object, err = builder.Get() + if err != nil { + if !errors.IsNotFound(err) { + glog.Warningf("Exists(): unexpected error fetching RouteAdvertisement %q: %v", builder.Definition.Name, err) + } + return false + } + return true }vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.go (1)
21-150: LGTM: autogenerated deepcopy covers all selector variants and slice elements.No issues spotted in generated code.
| // PullRouteAdvertisement pulls existing RouteAdvertisement from cluster. | ||
| func PullRouteAdvertisement(apiClient client.Client, name string) (*RouteAdvertisementBuilder, error) { | ||
| glog.V(100).Infof("Pulling existing RouteAdvertisement name %s from cluster", name) | ||
|
|
||
| if apiClient == nil { | ||
| glog.V(100).Infof("The apiClient cannot be nil") | ||
|
|
||
| return nil, fmt.Errorf("RouteAdvertisement 'apiClient' cannot be nil") | ||
| } | ||
|
|
||
| builder := RouteAdvertisementBuilder{ | ||
| apiClient: apiClient, | ||
| Definition: &ovnv1.RouteAdvertisements{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if name == "" { | ||
| glog.V(100).Infof("The name of the RouteAdvertisement is empty") | ||
|
|
||
| return nil, fmt.Errorf("RouteAdvertisement 'name' cannot be empty") | ||
| } | ||
|
|
||
| if !builder.Exists() { | ||
| return nil, fmt.Errorf("RouteAdvertisement object %s does not exist", name) | ||
| } | ||
|
|
||
| builder.Definition = builder.Object | ||
|
|
||
| return &builder, nil | ||
| } |
There was a problem hiding this comment.
PullRouteAdvertisement should call Get() and propagate errors accurately.
Using Exists() here can misreport network/auth failures as “does not exist” and can leave Definition nil. Prefer a direct Get:
func PullRouteAdvertisement(apiClient client.Client, name string) (*RouteAdvertisementBuilder, error) {
@@
- if !builder.Exists() {
- return nil, fmt.Errorf("RouteAdvertisement object %s does not exist", name)
- }
-
- builder.Definition = builder.Object
+ obj, err := builder.Get()
+ if errors.IsNotFound(err) {
+ return nil, fmt.Errorf("RouteAdvertisement object %s does not exist", name)
+ }
+ if err != nil {
+ return nil, fmt.Errorf("failed to get RouteAdvertisement %s: %w", name, err)
+ }
+ builder.Object = obj
+ builder.Definition = obj📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // PullRouteAdvertisement pulls existing RouteAdvertisement from cluster. | |
| func PullRouteAdvertisement(apiClient client.Client, name string) (*RouteAdvertisementBuilder, error) { | |
| glog.V(100).Infof("Pulling existing RouteAdvertisement name %s from cluster", name) | |
| if apiClient == nil { | |
| glog.V(100).Infof("The apiClient cannot be nil") | |
| return nil, fmt.Errorf("RouteAdvertisement 'apiClient' cannot be nil") | |
| } | |
| builder := RouteAdvertisementBuilder{ | |
| apiClient: apiClient, | |
| Definition: &ovnv1.RouteAdvertisements{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| }, | |
| } | |
| if name == "" { | |
| glog.V(100).Infof("The name of the RouteAdvertisement is empty") | |
| return nil, fmt.Errorf("RouteAdvertisement 'name' cannot be empty") | |
| } | |
| if !builder.Exists() { | |
| return nil, fmt.Errorf("RouteAdvertisement object %s does not exist", name) | |
| } | |
| builder.Definition = builder.Object | |
| return &builder, nil | |
| } | |
| // PullRouteAdvertisement pulls existing RouteAdvertisement from cluster. | |
| func PullRouteAdvertisement(apiClient client.Client, name string) (*RouteAdvertisementBuilder, error) { | |
| glog.V(100).Infof("Pulling existing RouteAdvertisement name %s from cluster", name) | |
| if apiClient == nil { | |
| glog.V(100).Infof("The apiClient cannot be nil") | |
| return nil, fmt.Errorf("RouteAdvertisement 'apiClient' cannot be nil") | |
| } | |
| builder := RouteAdvertisementBuilder{ | |
| apiClient: apiClient, | |
| Definition: &ovnv1.RouteAdvertisements{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| }, | |
| } | |
| if name == "" { | |
| glog.V(100).Infof("The name of the RouteAdvertisement is empty") | |
| return nil, fmt.Errorf("RouteAdvertisement 'name' cannot be empty") | |
| } | |
| obj, err := builder.Get() | |
| if errors.IsNotFound(err) { | |
| return nil, fmt.Errorf("RouteAdvertisement object %s does not exist", name) | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get RouteAdvertisement %s: %w", name, err) | |
| } | |
| builder.Object = obj | |
| builder.Definition = obj | |
| return &builder, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go
around lines 87-119, replace the Exists() check with a direct Get() call on the
apiClient to fetch the RouteAdvertisement into builder.Definition, validating
apiClient and name first; if Get returns a not-found error return a clear "does
not exist" error, otherwise return the underlying Get error so network/auth
problems are propagated, and only after a successful Get set builder.Definition
(or builder.Object) and return the builder.
cdebbff to
524ce57
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)
124-124: Remove focused test (FIt) before merge.The FIt on line 124 will cause Ginkgo to run only this test and skip others in CI. This was flagged in previous reviews but is still present.
Apply this diff:
- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker",
834-872: Fix return type mismatch.The function returns
*ovn.RouteAdvertisementBuilderbutCreate()returns the CR object (not the builder). Additionally, the caller at line 156 discards the return value. Make this function void.Apply this diff:
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { By(fmt.Sprintf("Creating OVN RouteAdvertisement '%s' for PodNetwork", name)) // Define advertisement types advertisements := []ovnv1.AdvertisementType{ovnv1.PodNetwork} // Create selectors to match YAML spec nodeSelector := metav1.LabelSelector{} frrConfigurationSelector := metav1.LabelSelector{} networkSelectors := types.NetworkSelectors{ {NetworkSelectionType: types.DefaultNetwork}, } routeAdv := ovn.NewRouteAdvertisementBuilder( APIClient, name, advertisements, nodeSelector, frrConfigurationSelector, networkSelectors) Expect(routeAdv).ToNot(BeNil(), "RouteAdvertisementBuilder should be created successfully") By("Creating RouteAdvertisement resource") - createdRouteAdv, err := routeAdv.Create() + _, err := routeAdv.Create() if err != nil { By(fmt.Sprintf("RouteAdvertisement creation failed: %v", err)) } else { By(fmt.Sprintf("RouteAdvertisement '%s' created successfully", name)) } Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement") - Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") By("Skipping automated status verification - manually confirmed RouteAdvertisement is 'Accepted'") - // Status verification skipped due to CLI tools not available in test container - // Manual verification confirmed: oc get routeadvertisements.k8s.ovn.org shows STATUS: Accepted - - return createdRouteAdv }
922-943: Fix function to use expectedPodNetworkPrefixes parameter instead of hardcoded values.The function accepts
expectedPodNetworkPrefixesbut completely ignores it, hardcoding OpenShift's default pod CIDR instead. This was flagged in previous reviews but remains unfixed.Apply this diff to make the function data-driven:
func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { By("Verifying external FRR pod receives pod network routes from RouteAdvertisement") - By("Expected routes: cluster network 10.128.0.0/14 broken into /23 subnets per worker node") - Eventually(func() string { + Eventually(func() bool { // Get BGP routes received by the external FRR pod output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) if err != nil { By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" + return false } routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes (checking for cluster network routes): %s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring("10.128."), // Primary cluster network range (10.128.0.0/14) - ContainSubstring("10.129."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.130."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.131."), // Additional possible ranges within 10.128.0.0/14 - ), "External FRR pod should receive pod network routes (/23 subnets) from cluster network 10.128.0.0/14 via RouteAdvertisement") + for _, prefix := range expectedPodNetworkPrefixes { + if strings.Contains(routes, prefix) { + By(fmt.Sprintf("Found expected pod network route with prefix: %s", prefix)) + return true + } + } + return false + }, 3*time.Minute, 15*time.Second).Should(BeTrue(), + fmt.Sprintf("External FRR pod should receive at least one of the expected pod network prefixes: %v", expectedPodNetworkPrefixes)) }
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
778-806: Consider consolidating with createOVNKCompatibleBGPPeer to eliminate duplication.This function duplicates much of the logic in
createOVNKCompatibleBGPPeer(lines 874-889). Both create BGP peers withDisableMP=true. Consider consolidating into a single function that accepts all necessary parameters.Suggested consolidation:
-func createFRRCompatibleBGPPeerAndVerifyIfItsReady( - name, peerIP, bfdProfileName string, remoteAsn uint32, eBgpMultiHop bool, connectTime int, - frrk8sPods []*pod.Builder) { +func createBGPPeerWithDisableMP( + name, peerIP, bfdProfileName string, remoteAsn uint32, eBgpMultiHop bool, connectTime int, + frrk8sPods []*pod.Builder, verifyProtocol bool) { By("Creating FRR-compatible BGP Peer with disableMP=true") bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword).WithEBGPMultiHop(eBgpMultiHop). WithDisableMP(true) if bfdProfileName != "" { bgpPeer.WithBFDProfile(bfdProfileName) } if connectTime != 0 { bgpPeer.WithConnectTime(metav1.Duration{Duration: time.Duration(connectTime) * time.Second}) } _, err := bgpPeer.Create() Expect(err).ToNot(HaveOccurred(), "Failed to create FRR-compatible BGP peer") - By("Verifying if BGP protocol configured") - - for _, frrk8sPod := range frrk8sPods { - Eventually(frr.IsProtocolConfigured, - time.Minute, tsparams.DefaultRetryInterval).WithArguments(frrk8sPod, "router bgp"). - Should(BeTrue(), "BGP is not configured on the Speakers") + if verifyProtocol { + By("Verifying if BGP protocol configured") + for _, frrk8sPod := range frrk8sPods { + Eventually(frr.IsProtocolConfigured, + time.Minute, tsparams.DefaultRetryInterval).WithArguments(frrk8sPod, "router bgp"). + Should(BeTrue(), "BGP is not configured on the Speakers") + } } }Then remove
createOVNKCompatibleBGPPeerand update all call sites.
📜 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 956c7a5791ef8174f2e645529140da9dab1d0317 and 524ce57d0f100f12ee4484de4d47141feee97fb5.
⛔ Files ignored due to path filters (12)
go.sumis excluded by!**/*.sumvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement_test.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist_test.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (3)
go.mod(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(6 hunks)tests/cnf/core/network/sriov/tests/lacp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgppeer.go (2)
NewBPGPeerBuilder(33-85)PullBGPPeer(183-228)tests/cnf/core/network/metallb/internal/tsparams/consts.go (2)
LocalBGPASN(79-79)BGPPassword(21-21)tests/cnf/core/network/metallb/internal/frr/frr.go (1)
IsProtocolConfigured(366-380)tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
DefaultRetryInterval(76-76)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
RouteAdvertisementBuilder(20-29)NewRouteAdvertisementBuilder(32-94)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(79-79)PodNetwork(83-83)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)
🔇 Additional comments (3)
tests/cnf/core/network/sriov/tests/lacp.go (1)
111-111: Type compatibility confirmed; note the semantic change in environment variable sourcing.Both
GetSwitchInterfaces()andGetPrimarySwitchInterfaces()return[]stringwith identical validation logic (enforcing 4 interfaces), making them type-compatible. The LACP test's requirement for at least 2 interfaces is satisfied by both.However, this change is semantic, not just refactoring: the method now reads from
ECO_CNF_CORE_NET_SWITCH_INTERFACESinstead ofECO_CNF_CORE_NET_PRIMARY_SWITCH_INTERFACES. Ensure the environment configuration is updated accordingly to provide the correct interface list for LACP testing.tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
26-26: LGTM!The new imports are appropriate for the OVN RouteAdvertisement functionality being added.
Also applies to: 30-32
912-920: LGTM!Clean implementation using Eventually for polling. The timeout values are reasonable for cluster API calls.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
524ce57 to
7c08a88
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
124-124: CRITICAL: Focused test still present despite previous fix claim.The FIt (focused test) on line 124 remains in the code, even though a past review comment indicates this was addressed in commit 489b3d4. This will cause Ginkgo to skip all other tests in the suite during CI runs.
Apply this diff:
- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker",
139-140: Replace hardcoded 30-second sleep with Eventually-based verification.The sleep on line 140 is fragile and slows tests. The BGP session verification on line 142 should handle the wait.
Consider removing the sleep entirely since line 142 already verifies the BGP session state:
createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN, frrk8sPods) - time.Sleep(30 * time.Second) By("Checking that BGP session is established and up")If the verification needs more time, the
verifyMetalLbBGPSessionsAreUPOnFrrPodfunction should be updated to use Eventually internally.
154-155: CRITICAL: 20-minute sleep and "default" RouteAdvertisement name still present.Despite previous critical review comments, this code still has multiple severe issues:
- The 20-minute sleep makes this test completely unusable in CI
- The hard-coded "default" name may conflict with existing cluster RouteAdvertisements
- No cleanup is performed, causing resource leakage
Apply this approach:
- _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") - time.Sleep(20 * time.Minute) + routeAdvName := fmt.Sprintf("test-podnetwork-%d-%d", GinkgoParallelProcess(), time.Now().Unix()) + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + By("Deleting RouteAdvertisement") + if routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName); err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + }) + + By("Verifying external FRR pod receives pod network routes") + verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod, []string{"10.128.", "10.129.", "10.130.", "10.131."})Note: The verification function should use Eventually internally, removing the need for an external sleep.
832-870: Function return type mismatch - should be void.The function signature declares it returns
*ovn.RouteAdvertisementBuilder, butrouteAdv.Create()returns the created CR object. Since the return value is discarded at line 154, make this function void.Apply this diff:
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { By(fmt.Sprintf("Creating OVN RouteAdvertisement '%s' for PodNetwork", name)) // ... rest of function ... Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement") Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") - By("Skipping automated status verification - manually confirmed RouteAdvertisement is 'Accepted'") - // Status verification skipped due to CLI tools not available in test container - // Manual verification confirmed: oc get routeadvertisements.k8s.ovn.org shows STATUS: Accepted - - return createdRouteAdv }
872-887: Use existing WithDisableMP builder method instead of manual field assignment.The comment on line 879 states "no method exists yet", but
WithDisableMP(true)is already used on line 783 in this same file. Use the builder method for consistency.Apply this diff:
bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword) - - // Manually set DisableMP field since no method exists yet - bgpPeer.Definition.Spec.DisableMP = true + peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword).WithDisableMP(true)
889-908: Use existing WithBGPNeighborDisableMP builder method.The comment on line 901 is incorrect. The
WithBGPNeighborDisableMP(disableMP bool, routerIndex, neighborIndex uint)builder method exists in eco-goinfra. Use it instead of manual field assignment.Apply this diff:
frrConfig := metallb.NewFrrConfigurationBuilder(APIClient, name, NetConfig.Frrk8sNamespace). WithBGPRouter(tsparams.LocalBGPASN). WithBGPNeighbor(bgpPeerIP, remoteAS, 0). WithToReceiveModeAll(0, 0). WithBGPPassword("bgp-test", 0, 0). - WithPort(179, 0, 0) - - // Manually set DisableMP field on the neighbor since no method exists yet - frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP = true + WithPort(179, 0, 0). + WithBGPNeighborDisableMP(true, 0, 0)
920-941: Function ignores expectedPodNetworkPrefixes parameter and hard-codes pod CIDR.Despite a past review comment marked as addressed in commit e2ac446, this function still ignores its
expectedPodNetworkPrefixesparameter and hard-codes OpenShift default values (10.128.x). This won't work on clusters with different pod CIDRs.Make the function data-driven:
func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { By("Verifying external FRR pod receives pod network routes from RouteAdvertisement") - By("Expected routes: cluster network 10.128.0.0/14 broken into /23 subnets per worker node") - Eventually(func() string { + Eventually(func() bool { // Get BGP routes received by the external FRR pod output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) if err != nil { By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" + return false } routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes (checking for cluster network routes): %s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring("10.128."), // Primary cluster network range (10.128.0.0/14) - ContainSubstring("10.129."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.130."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.131."), // Additional possible ranges within 10.128.0.0/14 - ), "External FRR pod should receive pod network routes (/23 subnets) from cluster network 10.128.0.0/14 via RouteAdvertisement") + for _, prefix := range expectedPodNetworkPrefixes { + if strings.Contains(routes, prefix) { + return true + } + } + return false + }, 3*time.Minute, 15*time.Second).Should(BeTrue(), + fmt.Sprintf("External FRR pod should receive at least one of the expected pod network prefixes: %v", + expectedPodNetworkPrefixes)) }
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
776-804: Consider removing unused function or document its intended future use.The function
createFRRCompatibleBGPPeerAndVerifyIfItsReadyis not called anywhere in this file. If it's intended for future use or use in other files, add a comment explaining this. Otherwise, remove it to avoid code maintenance burden.
📜 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 524ce57d0f100f12ee4484de4d47141feee97fb5 and 7c08a880294410d9ca6281c37ebd4b1eb84f0970.
⛔ Files ignored due to path filters (11)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement_test.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist_test.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**
📒 Files selected for processing (3)
go.mod(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(6 hunks)tests/cnf/core/network/sriov/tests/lacp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cnf/core/network/sriov/tests/lacp.go
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (8)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPPassword(21-21)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgppeer.go (2)
NewBPGPeerBuilder(33-85)PullBGPPeer(183-228)tests/cnf/core/network/metallb/internal/frr/frr.go (1)
IsProtocolConfigured(366-380)tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
DefaultRetryInterval(76-76)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
RouteAdvertisementBuilder(20-29)NewRouteAdvertisementBuilder(32-94)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(79-79)PodNetwork(83-83)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go (1)
NewFrrConfigurationBuilder(30-76)
🔇 Additional comments (3)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)
26-32: LGTM!The new imports are appropriate for the OVN RouteAdvertisement integration being added.
149-149: LGTM!The FRR configuration creation call is appropriate for the OVN-compatible flow.
910-918: LGTM!The BGP peer verification helper is well-structured and uses Eventually with appropriate timeout values.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
7c08a88 to
475b2b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (5)
139-143: Replace fixed sleep with poll; assert BGP readiness.Avoid time.Sleep; poll until FRR reports router bgp configured.
- createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN, frrk8sPods) - time.Sleep(30 * time.Second) - By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) + By("Checking that BGP session is established and up") + Eventually(frr.IsProtocolConfigured, 60*time.Second, tsparams.DefaultRetryInterval). + WithArguments(frrPod, "router bgp"). + Should(BeTrue(), "BGP session should be established")
154-159: Avoid hardcoded RouteAdvertisement name and dead sleep; add cleanup and verification.Use a unique name, delete on teardown, and verify routes instead of sleeping.
- _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") - //time.Sleep(20 * time.Minute) + routeAdvName := fmt.Sprintf("ovnk-podnetwork-%d", GinkgoParallelProcess()) + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName) + if err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + }) + // Verify external FRR sees pod network routes announced by OVN-K. + verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod, []string{"10.128.", "10.129.", "10.130.", "10.131."})
124-124: Remove focused spec (FIt).Focused tests will skip the rest in CI. Convert FIt to It.
- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker",
832-870: Fix wrong return type; make helper void and correct call sites.routeAdv.Create() returns the CR object, not a builder. The function needn’t return a value.
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { @@ - Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") + Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") @@ - return createdRouteAdv + return }Update the caller (Line 154) per your other change to simply call without assignment (see separate diff).
920-941: Make verification data‑driven; remove hardcoded 10.128/14; fix lints.Use the expectedPodNetworkPrefixes parameter, return boolean for Eventually, wrap long message, and add blank lines before returns.
-func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { - By("Verifying external FRR pod receives pod network routes from RouteAdvertisement") - By("Expected routes: cluster network 10.128.0.0/14 broken into /23 subnets per worker node") - - Eventually(func() string { - // Get BGP routes received by the external FRR pod - output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) - if err != nil { - By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" - } - - routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes (checking for cluster network routes): %s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring("10.128."), // Primary cluster network range (10.128.0.0/14) - ContainSubstring("10.129."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.130."), // Additional possible ranges within 10.128.0.0/14 - ContainSubstring("10.131."), // Additional possible ranges within 10.128.0.0/14 - ), "External FRR pod should receive pod network routes (/23 subnets) from cluster network 10.128.0.0/14 via RouteAdvertisement") +func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { + By("Verifying external FRR pod receives pod network routes from RouteAdvertisement.") + By(fmt.Sprintf("Expected pod network prefixes: %v", expectedPodNetworkPrefixes)) + + Eventually(func() bool { + // Get BGP routes received by the external FRR pod. + output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) + if err != nil { + By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) + + return false + } + + routes := output.String() + By("Checking external FRR pod BGP routes for expected pod network prefixes.") + for _, prefix := range expectedPodNetworkPrefixes { + if strings.Contains(routes, prefix) { + return true + } + } + + return false + }, 3*time.Minute, 15*time.Second).Should(BeTrue(), + "External FRR pod should receive at least one of the expected pod network prefixes.") }
🧹 Nitpick comments (5)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (5)
776-804: Remove unused helper or use it.createFRRCompatibleBGPPeerAndVerifyIfItsReady is unused and flagged by linters.
- func createFRRCompatibleBGPPeerAndVerifyIfItsReady( - name, peerIP, bfdProfileName string, remoteAsn uint32, eBgpMultiHop bool, connectTime int, - frrk8sPods []*pod.Builder) { - By("Creating FRR-compatible BGP Peer with disableMP=true") - - bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword).WithEBGPMultiHop(eBgpMultiHop). - WithDisableMP(true) - - if bfdProfileName != "" { - bgpPeer.WithBFDProfile(bfdProfileName) - } - - if connectTime != 0 { - // Convert connectTime int to time.Duration in seconds - bgpPeer.WithConnectTime(metav1.Duration{Duration: time.Duration(connectTime) * time.Second}) - } - - _, err := bgpPeer.Create() - Expect(err).ToNot(HaveOccurred(), "Failed to create FRR-compatible BGP peer") - - By("Verifying if BGP protocol configured") - - for _, frrk8sPod := range frrk8sPods { - Eventually(frr.IsProtocolConfigured, - time.Minute, tsparams.DefaultRetryInterval).WithArguments(frrk8sPod, "router bgp"). - Should(BeTrue(), "BGP is not configured on the Speakers") - } - }
872-887: Drop unused parameter and use builder API to disable multipath.frrk8sPods is unused; also prefer builder method to set DisableMP.
-// createOVNKCompatibleBGPPeer creates a BGP peer with multipath disabled for RouteAdvertisement compatibility -func createOVNKCompatibleBGPPeer(name, peerIP string, remoteAsn uint32, frrk8sPods []*pod.Builder) { +// createOVNKCompatibleBGPPeer creates a BGP peer with multipath disabled for RouteAdvertisement compatibility. +func createOVNKCompatibleBGPPeer(name, peerIP string, remoteAsn uint32) { By("Creating RouteAdvertisement-compatible BGP Peer") bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword) - - // Manually set DisableMP field since no method exists yet - bgpPeer.Definition.Spec.DisableMP = true + peerIP, tsparams.LocalBGPASN, remoteAsn). + WithPassword(tsparams.BGPPassword). + WithDisableMP(true) _, err := bgpPeer.Create() Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement-compatible BGP peer") By("Verify that BGP Peer was successfully created") verifyBGPPeerExistsOnCluster(name, NetConfig.MlbOperatorNamespace) }And update the only call at Line 139 (see earlier diff).
889-909: Use builder method for neighbor DisableMP; fix comment punctuation.Avoid manual deep struct mutation; the builder supports this.
-// createOVNKCompatibleFRRConfiguration creates an FRR configuration with multipath disabled for RouteAdvertisement compatibility +// createOVNKCompatibleFRRConfiguration creates an FRR configuration with multipath disabled for RouteAdvertisement compatibility. func createOVNKCompatibleFRRConfiguration(name, bgpPeerIP string, remoteAS uint32) { By("Creating RouteAdvertisement-compatible FRR Configuration") @@ - // Manually set DisableMP field on the neighbor since no method exists yet - frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP = true + // Disable multipath on neighbor for RouteAdvertisement compatibility. + frrConfig.WithBGPNeighborDisableMP(true, 0, 0)
910-918: nlreturn: add blank line before return inside Eventually func.Silences linter and improves readability.
Eventually(func() error { - _, err := metallb.PullBGPPeer(APIClient, name, namespace) - return err + _, err := metallb.PullBGPPeer(APIClient, name, namespace) + + return err }, 30*time.Second, 2*time.Second).Should(Succeed(), "BGP peer should exist in cluster")
155-156: Fix lint on commented code or remove it.Either add a space after // or delete the commented sleep. Deleting is preferred.
- //time.Sleep(20 * time.Minute) + // time.Sleep(20 * time.Minute)(If applying the earlier RA verification diff, remove this line entirely.)
📜 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 7c08a880294410d9ca6281c37ebd4b1eb84f0970 and 475b2b446399381968472aed37f6981a5bad6dd0.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (3)
go.mod(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(6 hunks)tests/cnf/core/network/sriov/tests/lacp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- tests/cnf/core/network/sriov/tests/lacp.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPPassword(21-21)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgppeer.go (2)
NewBPGPeerBuilder(33-85)PullBGPPeer(183-228)tests/cnf/core/network/metallb/internal/frr/frr.go (1)
IsProtocolConfigured(366-380)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.go (2)
RouteAdvertisementBuilder(20-29)NewRouteAdvertisementBuilder(32-95)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.go (2)
AdvertisementType(79-79)PodNetwork(83-83)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.go (3)
NetworkSelectors(14-14)NetworkSelectionType(51-51)DefaultNetwork(55-55)vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/frr.go (1)
NewFrrConfigurationBuilder(30-76)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 155-155: commentFormatting: put a space between // and comment text (gocritic)
🪛 GitHub Check: build
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 155-155:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 776-776:
func createFRRCompatibleBGPPeerAndVerifyIfItsReady is unused (unused)
[failure] 873-873:
createOVNKCompatibleBGPPeer - frrk8sPods is unused (unparam)
[failure] 934-934:
return with no blank line before (nlreturn)
[failure] 929-929:
return with no blank line before (nlreturn)
[failure] 916-916:
return with no blank line before (nlreturn)
[failure] 940-940:
The line is 129 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 910-910:
Comment should end in a period (godot)
[failure] 889-889:
Comment should end in a period (godot)
[failure] 872-872:
Comment should end in a period (godot)
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
475b2b4 to
3c9117d
Compare
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
3c9117d to
69282d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (5)
140-162: Address test reliability and cleanup issues.Several issues in this test block:
- Line 141: The 30-second sleep is fragile. Use
Eventuallypolling instead.- Line 155: The hardcoded name
"default"may conflict with existing cluster RouteAdvertisements. Use a unique test-scoped name.- Missing cleanup: No
DeferCleanupto remove the RouteAdvertisement after the test.By("Creating BGP Peers") createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN, frrk8sPods) - time.Sleep(30 * time.Second) By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + Eventually(func() error { + return verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established")- _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + routeAdvName := fmt.Sprintf("test-podnetwork-%d", GinkgoParallelProcess()) + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName) + if err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + })
125-125: Remove focused test (FIt) before merge.This test still uses
FItwhich will cause Ginkgo to run only this test and skip all others in CI.- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker",
876-891: UseWithDisableMP(true)builder method instead of manual field assignment.The comment on line 883 claims "no method exists yet," but
WithDisableMP(true)is already used at line 787 increateFRRCompatibleBGPPeerAndVerifyIfItsReady. Use the builder method for consistency.bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword) - - // Manually set DisableMP field since no method exists yet - bgpPeer.Definition.Spec.DisableMP = true + peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword).WithDisableMP(true)Consider consolidating this function with
createFRRCompatibleBGPPeerAndVerifyIfItsReady(lines 780-808) to reduce duplication.
893-912: UseWithBGPNeighborDisableMPbuilder method.The comment on line 905 is incorrect—the builder method
WithBGPNeighborDisableMP(disableMP bool, routerIndex, neighborIndex uint)exists. Replace the manual field assignment:frrConfig := metallb.NewFrrConfigurationBuilder(APIClient, name, NetConfig.Frrk8sNamespace). WithBGPRouter(tsparams.LocalBGPASN). WithBGPNeighbor(bgpPeerIP, remoteAS, 0). WithToReceiveModeAll(0, 0). WithBGPPassword("bgp-test", 0, 0). - WithPort(179, 0, 0) - - // Manually set DisableMP field on the neighbor since no method exists yet - frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP = true + WithPort(179, 0, 0). + WithBGPNeighborDisableMP(true, 0, 0)
924-945: Use theexpectedPodNetworkPrefixesparameter instead of hardcoded values.The function ignores the
expectedPodNetworkPrefixesparameter and hardcodes10.128.xchecks. Either use the parameter or remove it to avoid confusion.func verifyExternalFRRPodReceivesPodNetworkRoutes(frrPod *pod.Builder, expectedPodNetworkPrefixes []string) { By("Verifying external FRR pod receives pod network routes from RouteAdvertisement") - By("Expected routes: cluster network 10.128.0.0/14 broken into /23 subnets per worker node") - Eventually(func() string { + Eventually(func() bool { output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp json"}) if err != nil { By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" + return false } - routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes (checking for cluster network routes): %s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring("10.128."), - ContainSubstring("10.129."), - ContainSubstring("10.130."), - ContainSubstring("10.131."), - ), "External FRR pod should receive pod network routes (/23 subnets) from cluster network 10.128.0.0/14 via RouteAdvertisement") + for _, prefix := range expectedPodNetworkPrefixes { + if strings.Contains(routes, prefix) { + return true + } + } + return false + }, 3*time.Minute, 15*time.Second).Should(BeTrue(), + fmt.Sprintf("External FRR pod should receive at least one expected prefix: %v", expectedPodNetworkPrefixes)) }
📜 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 3c9117df08f1679e08213dc2cc7ba723b35abb9e and 69282d61068ddcb42c9da0ebec0366d9628bf286.
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 31-31: Command 'make lint' failed: could not import github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn (tests/cnf/core/network/metallb/tests/frrk8-tests.go:31:2): cannot find module providing package github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn; import lookup disabled by -mod=vendor
🪛 GitHub Check: build
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 846-846:
missing type in composite literal (typecheck)
🔇 Additional comments (3)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)
780-808: LGTM!This function correctly uses the builder pattern with
WithDisableMP(true)and includes proper verification of BGP protocol configuration.
947-971: LGTM!Good approach to dynamically retrieve the cluster pod network CIDR from the network operator configuration rather than hardcoding values.
845-847: Fix missing type in composite literal.The static analysis reports a missing type at line 846. The slice element needs an explicit type.
networkSelectors := types.NetworkSelectors{ - {NetworkSelectionType: types.DefaultNetwork}, + types.NetworkSelector{NetworkSelectionType: types.DefaultNetwork}, }Verify the correct type name (
NetworkSelectoror similar) from thetypespackage definition.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
69282d6 to
58bc4c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
31-33: Critical: Verify OVN package dependencies are vendored.Based on past review comments, these OVN package imports previously failed with vendor module errors. Ensure you've run
go mod tidyandgo mod vendorto properly vendor the eco-goinfra OVN dependencies.
125-125: Critical: Focused test (FIt) must be removed before merge.This was marked as addressed in a previous review but the
FItis still present. Focused specs cause Ginkgo to run only this test and skip all others in CI, breaking the test suite.Apply this diff:
- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker",
140-141: Replace hardcoded 30-second sleep with Eventually-based verification.The 30-second sleep is fragile and unnecessarily slows down the test. Replace with a polling loop that waits for the BGP session to reach the expected state.
Consider this pattern:
By("Creating BGP Peers") createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) - time.Sleep(30 * time.Second) + By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + Eventually(func() error { + return verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established")
155-155: Critical: Use test-unique RouteAdvertisement name and add cleanup.The hardcoded "default" name risks conflicting with existing cluster-level RouteAdvertisements. Use a test-unique name and add proper cleanup.
Apply this approach:
+ routeAdvName := fmt.Sprintf("test-podnetwork-%d", GinkgoParallelProcess()) - _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + By("Deleting RouteAdvertisement") + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName) + if err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + })
806-844: Critical: Fix return type mismatch.The function returns
*ovn.RouteAdvertisementBuilderbutCreate()returns the created CR object, not the builder. Since the caller discards the return value (line 155), make this function void.Apply this diff:
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { By(fmt.Sprintf("Creating OVN RouteAdvertisement '%s' for PodNetwork", name)) // Define advertisement types @@ -836,7 +836,5 @@ } Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement") - Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") By("Skipping automated status verification - manually confirmed RouteAdvertisement is 'Accepted'") - - return createdRouteAdv }
847-861: Use builder method instead of manual field assignment.The comment claims no method exists, but
WithDisableMP(bool)is available in the BGPPeer builder. Use it instead of manually setting the field.Apply this diff:
func createOVNKCompatibleBGPPeer(name, peerIP string, remoteAsn uint32) { By("Creating RouteAdvertisement-compatible BGP Peer") bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword) - - // Manually set DisableMP field since no method exists yet - bgpPeer.Definition.Spec.DisableMP = true + peerIP, tsparams.LocalBGPASN, remoteAsn). + WithPassword(tsparams.BGPPassword). + WithDisableMP(true) _, err := bgpPeer.Create()
931-966: Major: CIDR prefix matching incomplete for /14 networks.The logic at line 947 only extracts the first two octets (
10.128.), but a/14network spans multiple second-octet values. For10.128.0.0/14, valid routes can be in10.128.x.xthrough10.131.x.x. The current check will miss routes in the10.129,10.130, and10.131ranges.Use
SatisfyAnyto check for all valid second-octet prefixes within the /14 range:// For a /14 network like 10.128.0.0/14, valid ranges are 10.128-131.x.x // We'll look for BGP routes that match the first two octets basePrefix := fmt.Sprintf("%s.%s.", ipParts[0], ipParts[1]) Eventually(func() string { output, err := frrPod.ExecCommand([]string{"vtysh", "-c", "show ip bgp"}) if err != nil { By(fmt.Sprintf("Failed to get BGP routes from external FRR pod: %v", err)) - return "" } routes := output.String() - By(fmt.Sprintf("External FRR pod BGP routes:\n%s", routes)) - return routes - }, 3*time.Minute, 15*time.Second).Should(ContainSubstring(basePrefix), + }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( + ContainSubstring(fmt.Sprintf("%s.%s.", ipParts[0], ipParts[1])), + ContainSubstring(fmt.Sprintf("%s.%s.", ipParts[0], "129")), + ContainSubstring(fmt.Sprintf("%s.%s.", ipParts[0], "130")), + ContainSubstring(fmt.Sprintf("%s.%s.", ipParts[0], "131")), + ), fmt.Sprintf("External FRR pod should receive pod network routes (/%d subnets) "+ "from cluster network %s via OVN-K RouteAdvertisement", hostPrefix, clusterCIDR))
📜 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 69282d61068ddcb42c9da0ebec0366d9628bf286 and 58bc4c08645f1b8dfb93e9564251134fcece1d41.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 876-876: golangci-lint SA1019: frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP is deprecated: DisableMP is deprecated in favor of dualStackAddressFamily. +optional +kubebuilder:default:=false (staticcheck)
🪛 GitHub Check: build
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 876-876:
SA1019: frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP is deprecated: DisableMP is deprecated in favor of dualStackAddressFamily. +optional +kubebuilder:default:=false (staticcheck)
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
58bc4c0 to
876530f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
125-163: Fix focused test, replace sleep, and avoid hard‑coded RouteAdvertisement nameWithin this test block there are several issues that should be addressed before merge:
FItat Line 125 will focus the spec and cause CI to run only this test. Change it back toIt.time.Sleep(30 * time.Second)at Line 141 is brittle; use anEventuallythat callsverifyMetalLbBGPSessionsAreUPOnFrrPodinstead, so the test proceeds as soon as sessions are up and fails if they never come up.createOVNKRouteAdvertisementAndVerifyIfItsReady("default")at Line 155 hard‑codes a potentially shared RouteAdvertisement name and performs no cleanup. Use a test‑unique name (e.g.,fmt.Sprintf("ovnk-podnetwork-%d", GinkgoParallelProcess())) and register aDeferCleanupthat pulls and deletes the created RouteAdvertisement to avoid conflicts and leaks.Example sketch:
- FIt("Verify that when the allow all mode is configured all routes are received on the FRR speaker", + It("Verify that when the allow all mode is configured all routes are received on the FRR speaker", @@ - createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) - time.Sleep(30 * time.Second) + createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) + Eventually(func() error { + return verifyMetalLbBGPSessionsAreUPOnFrrPod( + frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established") @@ - _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + routeAdvName := fmt.Sprintf("ovnk-podnetwork-%d", GinkgoParallelProcess()) + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName) + if err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + })
🧹 Nitpick comments (2)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
806-844: Align helper name, behavior, and return type for RouteAdvertisement creation
createOVNKRouteAdvertisementAndVerifyIfItsReadycurrently:
- Only verifies successful
Create, but does not actually wait for or assert any “ready/accepted” condition.- Returns the created object, yet all current call sites ignore the return value.
To reduce confusion and simplify usage, consider either:
- Making the function truly “ready‑aware” by adding an
Eventuallythat polls the RouteAdvertisement status for anAcceptedcondition; or- Renaming it (e.g.,
createOVNKRouteAdvertisement) and changing the signature to return nothing if you only need a fire‑and‑forget creator.Example minimal cleanup if you opt for no return:
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { @@ - createdRouteAdv, err := routeAdv.Create() + createdRouteAdv, err := routeAdv.Create() @@ - return createdRouteAdv + return }
923-968: CIDR‑based route verification is simplistic and comment mismatches behavior
verifyOVNKPodNetworkRoutesReceivedcurrently:
- Assumes an IPv4 CIDR and splits on
".", which will not work for IPv6 pod networks.- Derives
basePrefixfrom the first two octets only (e.g.,"10.128.") and assertsContainSubstring(basePrefix), even though the comment suggests checking"10.128.","10.129.","10.130.","10.131."for a/14range.To better match the intent and handle
/14(or other) ranges, consider either:
- Using
net.ParseCIDRonclusterCIDRand parsing BGP route prefixes, checking membership withipNet.Contains; or- If you prefer string checks, computing all expected second‑octet prefixes based on the CIDR and asserting that at least one of them appears in the BGP output.
At minimum, align the comment with the actual behavior or extend the logic to match the documented multi‑prefix check.
📜 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 58bc4c08645f1b8dfb93e9564251134fcece1d41 and 876530f6866989a84ebad9dbc7587dfc22cc0515.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPPassword(21-21)tests/internal/cluster/config.go (1)
GetOCPNetworkOperatorConfig(58-72)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 854-854: golangci-lint: directive //nolint:staticcheck // DisableMP is deprecated but required for RouteAdvertisement compatibility is unused for linter "staticcheck" (nolintlint)
🪛 GitHub Check: build
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 854-854:
directive //nolint:staticcheck // DisableMP is deprecated but required for RouteAdvertisement compatibility is unused for linter "staticcheck" (nolintlint)
🔇 Additional comments (4)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
25-33: OVN and cluster imports look consistent with new helpersThe added imports for
cluster,ovn,ovnv1,types, andmetav1align with the new RouteAdvertisement and cluster CIDR helpers; no issues from a readability or structure standpoint.
886-895: BGPPeer existence verification helper looks solid
verifyBGPPeerExistsOnClusteruses anEventuallyloop withPullBGPPeerand returns the error toShould(Succeed()), which is an idiomatic pattern for waiting on resource creation. Timeout and interval values (30s / 2s) are reasonable for tests.
897-921: Cluster pod CIDR helper is reasonable for single‑CIDR clusters
getClusterPodNetworkCIDRcorrectly:
- Uses the shared
clusterhelper to fetch the network operator config.- Guards against missing ClusterNetwork entries.
- Returns the first cluster network’s
CIDRandHostPrefixwith clear error messages andBylogging.For typical OCP setups with a single
ClusterNetworkentry this is fine; if you ever support multiple entries, you may want to make the index configurable, but that’s not required for this PR.
864-884: Verify builder method availability and DisableMP deprecation statusUnable to verify the review comment due to inaccessible codebase. The file and referenced code could not be located in the repository. To properly evaluate this suggestion, confirm:
- Whether
WithBGPNeighborDisableMPmethod exists in the metallb builder- Current deprecation status of the
DisableMPfield- Whether the builder method produces equivalent behavior to the manual field assignment
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
876530f to
566e00f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (6)
140-143: Avoid hard‑coded 30s sleep; gate on BGP session with EventuallyA fixed
time.Sleep(30 * time.Second)slows the test and is fragile; this was previously flagged and reappeared. Wrap the BGP session check in anEventuallyinstead so the test proceeds as soon as sessions are up and fails deterministically if they never come up.Example pattern (adapt to the actual signature of
verifyMetalLbBGPSessionsAreUPOnFrrPod):- By("Creating BGP Peers") - createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) - time.Sleep(30 * time.Second) - By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + By("Creating BGP Peers") + createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) + + By("Checking that BGP session is established and up") + Eventually(func() error { + return verifyMetalLbBGPSessionsAreUPOnFrrPod( + frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established")
149-162: Do not use a shared “default” RouteAdvertisement name and add cleanupCreating a cluster‑scoped RouteAdvertisement named
"default"risks colliding with an existing RA and leaves it behind after the test. Use a test‑unique name and register deferred cleanup, then call the helper without discarding the return.Within this test:
- By("Create a frrconfiguration allow all") - createOVNKCompatibleFRRConfiguration(frrCongigAllowAll, ipv4metalLbIPList[0], tsparams.LocalBGPASN) + By("Create a frrconfiguration allow all") + createOVNKCompatibleFRRConfiguration(frrCongigAllowAll, ipv4metalLbIPList[0], tsparams.LocalBGPASN) @@ - _ = createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + routeAdvName := fmt.Sprintf("ovnk-podnetwork-%d", GinkgoParallelProcess()) + createOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName) + DeferCleanup(func() { + By("Deleting RouteAdvertisement") + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, routeAdvName) + if err == nil { + _ = routeAdv.DeleteAndWait(30 * time.Second) + } + }) @@ - By("Verify external FRR pod receives OVN-K pod network routes via RouteAdvertisement") - verifyOVNKPodNetworkRoutesReceived(frrPod) + By("Verify external FRR pod receives OVN-K pod network routes via RouteAdvertisement") + verifyOVNKPodNetworkRoutesReceived(frrPod)(You’ll also need to add
routeAdvNameonly in this test scope; no global var required.)
806-844: Fix RouteAdvertisement helper return type; make it void
createOVNKRouteAdvertisementAndVerifyIfItsReadyis declared to return*ovn.RouteAdvertisementBuilder, but it returns the result ofrouteAdv.Create(), whose type is the created CR object, and the only call site discards the return. This is both misleading and (depending on the eco‑goinfra API) a compile‑time type mismatch.Given the return value is unused, simplify the helper to return nothing:
-func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) *ovn.RouteAdvertisementBuilder { +func createOVNKRouteAdvertisementAndVerifyIfItsReady(name string) { @@ - createdRouteAdv, err := routeAdv.Create() + createdRouteAdv, err := routeAdv.Create() @@ - Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") + Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") @@ - return createdRouteAdv + return }After this change, update call sites to drop the
_ =assignment and just callcreateOVNKRouteAdvertisementAndVerifyIfItsReady(routeAdvName).Confirm the return type of RouteAdvertisementBuilder.Create() in eco-goinfra’s ovn package (does it return the CR object or the builder?), and ensure the helper’s signature matches actual usage.
846-855: Use BGPPeer builder’s WithDisableMP instead of direct field assignment
createOVNKCompatibleBGPPeermanually sets the deprecatedDisableMPfield on the BGPPeer spec. eco‑goinfra already exposes a builder method for this, and direct struct mutation is brittle.Replace the manual assignment with the builder API:
- bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, - peerIP, tsparams.LocalBGPASN, remoteAsn).WithPassword(tsparams.BGPPassword) - - // Manually set DisableMP field since no method exists yet - bgpPeer.Definition.Spec.DisableMP = true + bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, + peerIP, tsparams.LocalBGPASN, remoteAsn). + WithPassword(tsparams.BGPPassword). + WithDisableMP(true)This aligns with the rest of the builder usage and avoids touching deprecated fields directly.
Check the eco-goinfra metallb BGPPeerBuilder documentation for the `WithDisableMP(bool)` method and its semantics.
863-877: Avoid deprecated neighbor DisableMP field; use WithBGPNeighborDisableMPHere you directly write to
frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMPand suppressstaticcheckwith a nolint. eco‑goinfra provides a dedicated builder method, and static analysis currently fails on this deprecated field.Refactor to use the builder and drop the manual assignment and nolint:
- frrConfig := metallb.NewFrrConfigurationBuilder(APIClient, name, - NetConfig.Frrk8sNamespace). - WithBGPRouter(tsparams.LocalBGPASN). - WithBGPNeighbor(bgpPeerIP, remoteAS, 0). - WithToReceiveModeAll(0, 0). - WithBGPPassword("bgp-test", 0, 0). - WithPort(179, 0, 0) - - // Manually set DisableMP field on the neighbor since no method exists yet - //nolint:staticcheck // DisableMP is deprecated but required for RouteAdvertisement compatibility - frrConfig.Definition.Spec.BGP.Routers[0].Neighbors[0].DisableMP = true + frrConfig := metallb.NewFrrConfigurationBuilder(APIClient, name, + NetConfig.Frrk8sNamespace). + WithBGPRouter(tsparams.LocalBGPASN). + WithBGPNeighbor(bgpPeerIP, remoteAS, 0). + WithToReceiveModeAll(0, 0). + WithBGPPassword("bgp-test", 0, 0). + WithPort(179, 0, 0). + WithBGPNeighborDisableMP(true, 0, 0)This should clear the
SA1019staticcheck warning and keeps all config via the builder.Verify that eco-goinfra’s FrrConfigurationBuilder exposes `WithBGPNeighborDisableMP(disableMP bool, routerIndex, neighborIndex uint)` and that it sets the same field the test currently writes directly.
922-967: CIDR matching only checks one second‑octet; incomplete for /14 ranges
verifyOVNKPodNetworkRoutesReceivedbuilds a singlebasePrefix("<octet0>.<octet1>.") and asserts the FRR routes contain that substring. For a/14like10.128.0.0/14, valid ranges include10.128.x.xthrough10.131.x.x; checking only10.128.may miss valid pod routes in the higher second‑octet ranges and cause false negatives.Consider generalizing the prefix check to cover all expected second‑octet values derived from the CIDR. For example, for
/14:secondOctet, err := strconv.Atoi(ipParts[1]) Expect(err).ToNot(HaveOccurred(), "invalid second octet in CIDR base IP") expectedPrefixes := []string{ fmt.Sprintf("%s.%d.", ipParts[0], secondOctet), fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+1), fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+2), fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+3), }and then:
Eventually(func() string { // ... existing vtysh call ... return routes }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( ContainSubstring(expectedPrefixes[0]), ContainSubstring(expectedPrefixes[1]), ContainSubstring(expectedPrefixes[2]), ContainSubstring(expectedPrefixes[3]), ), fmt.Sprintf("External FRR pod should receive pod network routes (/%d subnets) from cluster network %s via OVN-K RouteAdvertisement", hostPrefix, clusterCIDR))(or compute the exact prefix set from the CIDR mask). This will better match the full pod‑network span.
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
896-920: Cluster pod CIDR helper is fine; consider multi‑network clusters in future
getClusterPodNetworkCIDRcleanly wraps the network‑operator lookup and validates presence of at least oneClusterNetworkentry; that’s fine for typical single‑pod‑network clusters.If you expect multi‑cluster‑network setups, you may eventually want to either:
- Select the specific
ClusterNetworkby name/role, or- Iterate all entries and verify routes for each.
For current use cases this helper is reasonable as‑is.
📜 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 876530f6866989a84ebad9dbc7587dfc22cc0515 and 566e00fbae95c721d1d3d1224b00dddfff5e9cba.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(4 hunks)
🔇 Additional comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
885-894: BGPPeer existence verification helper looks good
verifyBGPPeerExistsOnClusterusesEventuallyovermetallb.PullBGPPeerwith a short polling interval, which is appropriate for handling controller reconciliation lag without flaking the test.
|
@gkopels Could you provide test execution log for changes in the PR? |
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
4 times, most recently
from
491eb52 to
3488dc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
142-142: Replace hardcoded sleep with verification polling.The 30-second sleep is still present despite being flagged in past reviews. This unnecessarily slows down test execution. Replace with a lightweight verification that polls for BGP session readiness.
Apply this diff:
- time.Sleep(30 * time.Second) By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + Eventually(func() error { + // Attempt to verify BGP sessions + verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + return nil + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established")Note: This assumes
verifyMetalLbBGPSessionsAreUPOnFrrPoduses assertions. If it doesn't return an error, wrap the verification logic to return an error on failure instead.
🧹 Nitpick comments (2)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
807-843: Consider adding programmatic status verification.Line 842 skips automated status verification, relying on manual confirmation. While the resource creation is verified, checking the
Acceptedstatus programmatically would make the test more robust and catch status-related issues automatically.Consider replacing the skip message with actual status verification:
- By("Skipping automated status verification - manually confirmed RouteAdvertisement is 'Accepted'") + By("Verifying RouteAdvertisement status") + Eventually(func() bool { + ra, err := ovn.PullRouteAdvertisement(APIClient, name) + if err != nil { + return false + } + // Check if status indicates the RouteAdvertisement is accepted/ready + return ra.Object.Status.Conditions != nil && len(ra.Object.Status.Conditions) > 0 + }, 60*time.Second, 5*time.Second).Should(BeTrue(), + "RouteAdvertisement should reach accepted status")Note: Adjust the status check based on the actual OVN RouteAdvertisement status fields.
870-870: Use constant for BGP password for consistency.Line 870 uses the string literal
"bgp-test"while the same password is referenced viatsparams.BGPPasswordincreateOVNKCompatibleBGPPeer(line 851) and is defined as a constant in the codebase. Using the constant improves maintainability.Apply this diff:
- WithBGPPassword("bgp-test", 0, 0). + WithBGPPassword(tsparams.BGPPassword, 0, 0).
📜 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 edef634887689b6298ffff0dc4da9e813d7a12b8 and 3488dc5a6dd3a312faab5a50e8d64218b62b887e.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPPassword(21-21)
| createOVNKRouteAdvertisementAndVerifyIfItsReady("default") | ||
|
|
||
| By("Validate both external BGP routes are received by FRR-K8s speakers") | ||
| verifyReceivedRoutes(frrk8sPods, externalAdvertisedIPv4Routes[0]) | ||
| verifyReceivedRoutes(frrk8sPods, externalAdvertisedIPv4Routes[1]) | ||
|
|
||
| By("Verify external FRR pod receives OVN-K pod network routes via RouteAdvertisement") | ||
| verifyOVNKPodNetworkRoutesReceived(frrPod) |
There was a problem hiding this comment.
Add cleanup for RouteAdvertisement resource.
The RouteAdvertisement created with name "default" is never cleaned up, which can cause conflicts in subsequent test runs or other tests. Add deferred cleanup immediately after creation.
Apply this diff:
createOVNKRouteAdvertisementAndVerifyIfItsReady("default")
+ DeferCleanup(func() {
+ By("Deleting RouteAdvertisement")
+ routeAdv, err := ovn.PullRouteAdvertisement(APIClient, "default")
+ if err == nil {
+ _, _ = routeAdv.Delete()
+ }
+ })
By("Validate both external BGP routes are received by FRR-K8s speakers")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/cnf/core/network/metallb/tests/frrk8-tests.go around lines 156 to 163,
after the createOVNKRouteAdvertisementAndVerifyIfItsReady("default") call add an
immediate deferred cleanup that deletes the RouteAdvertisement named "default"
(use the test suite's k8s client/delete helper) and assert the delete succeeds;
ensure the defer runs regardless of test outcome so the "default"
RouteAdvertisement cannot leak into subsequent tests.
| tsparams.LocalBGPASN, false, 0, frrk8sPods) | ||
|
|
||
| createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) | ||
| time.Sleep(30 * time.Second) |
| routeAdv := ovn.NewRouteAdvertisementBuilder( | ||
| APIClient, | ||
| name). | ||
| WithAdvertisements(advertisements). | ||
| WithNodeSelector(nodeSelector). | ||
| WithFRRConfigurationSelector(frrConfigurationSelector). | ||
| WithNetworkSelectors(networkSelectors) | ||
|
|
||
| Expect(routeAdv).ToNot(BeNil(), "RouteAdvertisementBuilder should be created successfully") | ||
|
|
||
| By("Creating RouteAdvertisement resource") | ||
|
|
||
| createdRouteAdv, err := routeAdv.Create() | ||
| if err != nil { | ||
| By(fmt.Sprintf("RouteAdvertisement creation failed: %v", err)) | ||
| } else { | ||
| By(fmt.Sprintf("RouteAdvertisement '%s' created successfully", name)) | ||
| } |
There was a problem hiding this comment.
ovn.NewRouteAdvertisementBuilder(
APIClient,
name).
WithAdvertisements(advertisements).
WithNodeSelector(nodeSelector).
WithFRRConfigurationSelector(frrConfigurationSelector).
WithNetworkSelectors(networkSelectors).Create()
| Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement") | ||
| Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") | ||
|
|
||
| By("Skipping automated status verification - manually confirmed RouteAdvertisement is 'Accepted'") |
There was a problem hiding this comment.
it was debugging removed
| } | ||
|
|
||
| Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement") | ||
| Expect(createdRouteAdv).ToNot(BeNil(), "Created RouteAdvertisement should not be nil") |
There was a problem hiding this comment.
No need. I beleive it's part of goinfra validation
| bgpPeer := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace, | ||
| peerIP, tsparams.LocalBGPASN, remoteAsn). | ||
| WithPassword(tsparams.BGPPassword). | ||
| WithDisableMP(true) | ||
|
|
||
| _, err := bgpPeer.Create() | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement-compatible BGP peer") |
There was a problem hiding this comment.
metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace,
peerIP, tsparams.LocalBGPASN, remoteAsn).
WithPassword(tsparams.BGPPassword).
WithDisableMP(true).create()
| // verifyBGPPeerExistsOnCluster verifies that a BGP peer exists in the cluster. | ||
| func verifyBGPPeerExistsOnCluster(name, namespace string) { | ||
| By(fmt.Sprintf("Verifying BGP peer '%s' exists in namespace '%s'", name, namespace)) | ||
|
|
||
| Eventually(func() error { | ||
| _, err := metallb.PullBGPPeer(APIClient, name, namespace) | ||
|
|
||
| return err | ||
| }, 30*time.Second, 2*time.Second).Should(Succeed(), "BGP peer should exist in cluster") | ||
| } |
There was a problem hiding this comment.
We have more validations that bgppeer confiurared correctly
https://github.com/rh-ecosystem-edge/eco-gotests/blob/main/tests/cnf/core/network/metallb/tests/common.go#L322
| if err != nil { | ||
| return "", 0, fmt.Errorf("failed to get network operator config: %w", err) | ||
| } | ||
|
|
||
| networkObj, err := clusterNetwork.Get() | ||
| if err != nil { | ||
| return "", 0, fmt.Errorf("failed to get network object: %w", err) | ||
| } | ||
|
|
||
| if len(networkObj.Spec.ClusterNetwork) == 0 { | ||
| return "", 0, fmt.Errorf("no cluster networks defined in network operator") | ||
| } | ||
|
|
||
| clusterCIDR := networkObj.Spec.ClusterNetwork[0].CIDR | ||
| hostPrefix := networkObj.Spec.ClusterNetwork[0].HostPrefix | ||
|
|
||
| By(fmt.Sprintf("Cluster pod network CIDR: %s with host prefix: /%d", clusterCIDR, hostPrefix)) | ||
|
|
||
| return clusterCIDR, hostPrefix, nil |
| func mustAtoi(s string) int { | ||
| val, err := strconv.Atoi(s) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to convert %q to int: %v", s, err)) |
There was a problem hiding this comment.
you are right changed to Expect
| } | ||
|
|
||
| // mustAtoi converts a string to an integer, panicking on error. | ||
| func mustAtoi(s string) int { |
There was a problem hiding this comment.
Why do we need this func?
|
|
||
| "github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn" | ||
| ovnv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1" | ||
| "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types" |
There was a problem hiding this comment.
| "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types" | |
| ovntypes "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types" |
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
3488dc5 to
114f8d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)
143-146: Replace hardcoded sleep with Eventually-based verification.The 30-second
time.Sleepat line 144 was flagged in a previous review. Consider replacing with an Eventually block that polls for BGP session establishment.By("Creating BGP Peers") createOVNKCompatibleBGPPeer(tsparams.BgpPeerName1, ipv4metalLbIPList[0], tsparams.LocalBGPASN) - time.Sleep(30 * time.Second) By("Checking that BGP session is established and up") - verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + Eventually(func() error { + return verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList)) + }, 60*time.Second, 5*time.Second).Should(Succeed(), "BGP session should be established")Note: This requires
verifyMetalLbBGPSessionsAreUPOnFrrPodto return an error. If it uses internalExpectcalls, wrap the check differently or adjust the function signature.
158-165: Add cleanup for RouteAdvertisement resource.The RouteAdvertisement created with name "default" is never cleaned up. This was flagged in a previous review. Add deferred cleanup to prevent resource leakage and test interference.
createOVNKRouteAdvertisementAndVerifyIfItsReady("default") + DeferCleanup(func() { + By("Deleting RouteAdvertisement") + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, "default") + if err == nil { + _, _ = routeAdv.Delete() + } + }) By("Validate both external BGP routes are received by FRR-K8s speakers")Additionally, consider using a unique name instead of "default" to avoid conflicts with existing cluster-level RouteAdvertisements, as noted in past reviews.
973-994: CIDR range calculation is still hardcoded for /14 networks.The logic checking
secondOctetthroughsecondOctet+3is correct only for/14CIDRs. As flagged in past reviews, this won't work for/16(single second-octet) or/12(16 second-octets). evgenLevin asked to review this.Consider calculating the range dynamically:
+ // Calculate the number of second-octet values based on CIDR prefix + cidrMaskBits := mustAtoi(cidrParts[1]) + var numRanges int + if cidrMaskBits <= 8 { + numRanges = 4 // Cap for practical matching + } else if cidrMaskBits <= 16 { + numRanges = 1 << (16 - cidrMaskBits) + if numRanges > 4 { + numRanges = 4 + } + } else { + numRanges = 1 + } + secondOctet := mustAtoi(ipParts[1]) + + matchers := make([]types.GomegaMatcher, numRanges) + for i := 0; i < numRanges; i++ { + matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) + } Eventually(func() string { // ... existing code ... - }, 3*time.Minute, 15*time.Second).Should(SatisfyAny( - ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet)), - ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+1)), - ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+2)), - ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+3)), - ), ...) + }, 3*time.Minute, 15*time.Second).Should(SatisfyAny(matchers...), ...)
🧹 Nitpick comments (4)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
33-35: Consider aliasing thetypesimport to avoid ambiguity.The unqualified
typesimport can conflict with other commontypespackages. As suggested in a previous review, consider using an alias."github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn" ovnv1 "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1" - "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types" + ovntypes "github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types"Then update line 848 accordingly:
networkSelectors := ovntypes.NetworkSelectors{ {NetworkSelectionType: ovntypes.DefaultNetwork}, }
877-891: Consider consolidating with existingcreateBGPPeerAndVerifyIfItsReadyfunction.As raised by evgenLevin, this function duplicates
createBGPPeerAndVerifyIfItsReadywith the only difference beingWithDisableMP(true). Consider adding an optional parameter to the existing function instead of creating a new one.Option 1: Add a parameter to the existing function:
func createBGPPeerAndVerifyIfItsReady(name, peerIP, secret string, remoteAsn uint32, ebgp bool, holdTime int, frrk8sPods []*pod.Builder, disableMP bool) {Option 2: If
frrk8sPodsverification is not needed for OVNK scenarios, document why this simpler variant exists.
923-947: Consider using Expect instead of error returns for consistency.As noted by evgenLevin, this function returns errors while other test helpers use
Expectdirectly. For consistency with the test helper pattern used elsewhere in this file, consider usingExpectassertions.-func getClusterPodNetworkCIDR() (string, uint32, error) { +func getClusterPodNetworkCIDR() (string, uint32) { By("Getting cluster network configuration from network operator") clusterNetwork, err := cluster.GetOCPNetworkOperatorConfig(APIClient) - if err != nil { - return "", 0, fmt.Errorf("failed to get network operator config: %w", err) - } + Expect(err).ToNot(HaveOccurred(), "Failed to get network operator config") networkObj, err := clusterNetwork.Get() - if err != nil { - return "", 0, fmt.Errorf("failed to get network object: %w", err) - } + Expect(err).ToNot(HaveOccurred(), "Failed to get network object") - if len(networkObj.Spec.ClusterNetwork) == 0 { - return "", 0, fmt.Errorf("no cluster networks defined in network operator") - } + Expect(len(networkObj.Spec.ClusterNetwork)).To(BeNumerically(">", 0), + "No cluster networks defined in network operator") clusterCIDR := networkObj.Spec.ClusterNetwork[0].CIDR hostPrefix := networkObj.Spec.ClusterNetwork[0].HostPrefix By(fmt.Sprintf("Cluster pod network CIDR: %s with host prefix: /%d", clusterCIDR, hostPrefix)) - return clusterCIDR, hostPrefix, nil + return clusterCIDR, hostPrefix }Update the caller at line 954-955 accordingly.
999-1007: Replace panic with Expect for consistency with test framework.As noted by evgenLevin, panicking in test helper code is unusual. Use
Expectfor consistency with the rest of the test framework, which provides better error messages.// mustAtoi converts a string to an integer, panicking on error. func mustAtoi(s string) int { val, err := strconv.Atoi(s) - if err != nil { - panic(fmt.Sprintf("failed to convert %q to int: %v", s, err)) - } + Expect(err).ToNot(HaveOccurred(), + fmt.Sprintf("Failed to convert %q to int", s)) return val }Alternatively, if this is only used for parsing known-good IP octets, consider removing the error handling entirely with
strconv.Atoiand accepting the risk (not recommended), or inlining the conversion where needed.
📜 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 3488dc5a6dd3a312faab5a50e8d64218b62b887e and 114f8d7c16d795bd727fe6438e37e73b3cbbf5d6.
📒 Files selected for processing (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
BgpPeerName1(32-32)LocalBGPASN(79-79)BGPPassword(21-21)tests/internal/cluster/config.go (1)
GetOCPNetworkOperatorConfig(58-72)
🔇 Additional comments (2)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
893-910: LGTM - uses builder method for DisableMP.The implementation correctly uses
WithBGPNeighborDisableMP(true, 0, 0)as suggested in past reviews, avoiding deprecated field access.
912-921: Consider reusing existing comprehensive BGP peer validation from common.go if applicable.If
common.gocontains more thorough BGP peer configuration validation (as suggested in previous reviews), evaluate whether that validation logic should be reused here or clearly document why this simplified existence check is sufficient for the RouteAdvertisement test scenarios.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
114f8d7 to
f67624e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
978-986: Use Expect instead of panic for better test diagnostics.The
panicon conversion failure makes debugging harder and is less idiomatic for Ginkgo tests. Past reviewer questioned this design choice.Refactor to use Gomega assertions:
-// mustAtoi converts a string to an integer, panicking on error. +// mustAtoi converts a string to an integer, failing the test on error. func mustAtoi(s string) int { val, err := strconv.Atoi(s) - if err != nil { - panic(fmt.Sprintf("failed to convert %q to int: %v", s, err)) - } + Expect(err).ToNot(HaveOccurred(), "failed to convert %q to int", s) return val }This provides clearer error context in test output and follows Ginkgo/Gomega conventions.
📜 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 114f8d7c16d795bd727fe6438e37e73b3cbbf5d6 and f67624eccb334ba585a93c003f2d2a96c13393c2.
📒 Files selected for processing (2)
tests/cnf/core/network/metallb/tests/common.go(2 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🔇 Additional comments (6)
tests/cnf/core/network/metallb/tests/common.go (1)
300-331: LGTM! Clean extension for OVN RouteAdvertisement compatibility.The variadic
disableMPparameter is implemented correctly with proper length checks before indexing, and uses the builder methodWithDisableMPas expected. This change is backward compatible with existing callers.tests/cnf/core/network/metallb/tests/frrk8-tests.go (5)
3-37: LGTM! Imports correctly support OVN RouteAdvertisement integration.All new imports (
strconv,cluster,metav1, OVN packages) are used in the new helper functions and test flow. Based on past review comments, vendor dependencies were properly updated.
143-168: Test flow correctly integrates OVN RouteAdvertisement.The test properly:
- Disables multipath (
disableMP=true) for RouteAdvertisement compatibility- Creates OVN-compatible FRR configuration
- Creates and waits for RouteAdvertisement readiness
- Verifies both external BGP routes and OVN-K pod network routes
Based on past review comments, cleanup and naming concerns were addressed in previous iterations.
841-875: Well-structured RouteAdvertisement creation with proper readiness check.The function correctly:
- Creates RouteAdvertisement with PodNetwork advertisement type
- Uses empty selectors to match all nodes/configurations by default
- Implements proper Eventually-based polling for "Accepted" status
- Provides clear logging at each step
877-894: Correct use of builder method for DisableMP configuration.The function properly uses
WithBGPNeighborDisableMP(true, 0, 0)instead of deprecated direct field access. All configuration follows the builder pattern consistently, making this RouteAdvertisement-compatible configuration clean and maintainable.
896-913: Clean retrieval of cluster network configuration.The function correctly uses the cluster API to fetch network operator configuration and extracts the pod network CIDR with appropriate error handling. The assumption of at least one cluster network is reasonable for OpenShift environments.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
f67624e to
85d5b0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
160-167: Add cleanup for RouteAdvertisement to prevent resource leakage.The RouteAdvertisement named "default" is created but never cleaned up. This can cause test failures in subsequent runs or conflicts with other tests. Past review comments flagged this issue.
Apply this diff to add cleanup:
createOVNKRouteAdvertisementAndWaitUntilReady("default") + DeferCleanup(func() { + By("Deleting RouteAdvertisement") + routeAdv, err := ovn.PullRouteAdvertisement(APIClient, "default") + if err == nil { + _, _ = routeAdv.Delete() + } + }) By("Validate both external BGP routes are received by FRR-K8s speakers")Additionally, consider using a test-unique name instead of "default" to avoid potential conflicts with cluster-level RouteAdvertisements (as flagged in past reviews).
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
915-974: Dynamic CIDR range matching is well-implemented.The function addresses past review feedback:
- Uses proper
Expect().To(HaveLen(...))for validation instead of the incorrectExpect(fmt.Errorf(...))pattern- Dynamically calculates the number of /16 ranges based on the CIDR mask bits
The cap at 4 ranges (lines 941-943) is reasonable for typical cluster networks but would need adjustment for very large CIDRs like /8. Consider adding a comment explaining the rationale for the cap.
numRanges := 1 << (16 - cidrMaskBits) // 2^(16 - maskBits) gives number of /16 ranges + // Cap at 4 to avoid excessive matchers for very large CIDRs; typical OpenShift clusters use /14 or /16 if numRanges > 4 { numRanges = 4 // Cap at 4 for practical checking }
📜 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 f67624eccb334ba585a93c003f2d2a96c13393c2 and 85d5b0ffd5a38c3c73b0398795e4baac633884aa.
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
tests/cnf/core/network/metallb/tests/common.go(2 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (2)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (1)
LocalBGPASN(79-79)tests/internal/cluster/config.go (1)
GetOCPNetworkOperatorConfig(58-72)
🔇 Additional comments (5)
tests/cnf/core/network/metallb/tests/common.go (1)
300-331: LGTM! Clean extension for OVN RouteAdvertisement compatibility.The variadic boolean parameter maintains backward compatibility while allowing callers to opt-in to disabling multiprotocol BGP. The pattern
len(disableMP) > 0 && disableMP[0]is idiomatic for optional boolean parameters.tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
1-37: Imports are well-organized and all appear to be used.The OVN-related imports follow the naming convention suggested in past reviews.
841-875: Well-structured RouteAdvertisement creation with proper readiness verification.The function correctly uses the builder pattern, sets all required selectors, and waits for the "Accepted" status using
Eventually. This addresses past review feedback about skipping readiness checks.
877-894: Correctly uses builder method for DisableMP configuration.The implementation properly uses
WithBGPNeighborDisableMP(true, 0, 0)instead of direct field assignment, addressing past review feedback about the deprecatedDisableMPfield.
896-913: Clean implementation for retrieving cluster pod network CIDR.The function properly leverages the existing
cluster.GetOCPNetworkOperatorConfigutility and extracts the required network configuration with appropriate error handling.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
85d5b0f to
3eba3e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
1058-1073: Network operator patching is correct; consider polling instead of fixed sleep.The function properly patches the Network CR to enable RouteAdvertisements using a merge patch. The 30-second sleep (line 1072) allows the OVN-K operator to reconcile the configuration change.
Optional improvement: If the network operator exposes a status field indicating when RouteAdvertisements are fully reconciled, replace the sleep with an Eventually poll for that status. However, if no such status exists, the current 30-second wait is a reasonable approach for cluster-wide configuration changes.
📜 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 85d5b0ffd5a38c3c73b0398795e4baac633884aa and 3eba3e0.
📒 Files selected for processing (2)
tests/cnf/core/network/metallb/tests/common.go(2 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
tests/internal/cluster/config.go (1)
GetOCPNetworkOperatorConfig(58-72)
🔇 Additional comments (8)
tests/cnf/core/network/metallb/tests/common.go (1)
300-331: LGTM! Clean extension for optional multipath disabling.The variadic
disableMPparameter allows callers to optionally disable multipath on the BGP peer without breaking existing call sites. The implementation correctly checks both the presence and value of the parameter before applyingWithDisableMP(true).tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
4-39: Imports are correctly added for new OVN RouteAdvertisement functionality.All new imports are utilized by the new helper functions and test context. The additions support OVN RouteAdvertisement CRD operations, cluster network configuration access, and dynamic CIDR range calculations.
629-702: Well-structured test context with proper setup and cleanup.The new "OVN-K RouteAdvertisements" context follows the established test patterns in this file. The BeforeAll/AfterAll hooks manage cluster-wide RouteAdvertisement configuration, while AfterEach ensures per-test resource cleanup. The test flow logically validates that OVN-K RouteAdvertisements correctly propagate pod network routes to external FRR instances.
914-952: RouteAdvertisement creation and readiness check are correctly implemented.The function properly constructs the RouteAdvertisement CR with appropriate selectors and waits for the "Accepted" status using an Eventually block with reasonable timeout/polling intervals. The empty nodeSelector correctly matches all nodes, and the frrConfigurationSelector ensures only labeled FRRConfigurations are matched.
954-976: FRR configuration correctly uses builder API and proper labeling.The function properly constructs an FRRConfiguration compatible with RouteAdvertisement by using
WithBGPNeighborDisableMP(true, 0, 0)(avoiding the deprecated field) and adds the required label for selector matching. All BGP parameters are correctly configured through the builder pattern.
978-995: Cluster network CIDR retrieval is correctly implemented.The function properly accesses the network operator configuration and extracts the cluster pod network CIDR and host prefix. The validation ensures ClusterNetwork is populated before accessing its first element, which is the standard location for the primary cluster network configuration.
1075-1106: Cleanup functions properly handle resource teardown.
disableOVNKRouteAdvertisementscorrectly patches the network operator to disable RouteAdvertisements, mirroring the enable function.cleanupRouteAdvertisementslists and deletes all RouteAdvertisement resources with graceful error handling, ensuring test cleanup even if some deletions fail. Both functions are appropriately called in AfterAll/AfterEach hooks to prevent resource leakage.
997-1056: Excellent dynamic CIDR range verification implementation.The function correctly addresses past review concerns by dynamically calculating the number of /16 ranges based on the CIDR mask (lines 1019-1032) rather than hard-coding values. The calculation
1 << (16 - cidrMaskBits)properly computes how many second-octet ranges are covered by the CIDR (e.g., /14 → 4 ranges, /16 → 1 range).Minor: Verify Go version compatibility.
Line 1036 uses
for i := range numRangessyntax introduced in Go 1.22. Confirm your project's minimum Go version ingo.modsupports this. If targeting Go < 1.22, replace withfor i := 0; i < numRanges; i++.
| Expect(err).ToNot(HaveOccurred(), "Failed to enable RouteAdvertisements on network operator") | ||
|
|
||
| By("Waiting for OVN-K to reconcile RouteAdvertisements setting") | ||
| time.Sleep(30 * time.Second) |
There was a problem hiding this comment.
Can we replace Sleep by eventually?
There was a problem hiding this comment.
replaced with Eventually
|
|
||
| patchData := []byte(`{"spec":{"defaultNetwork":{"ovnKubernetesConfig":{"routeAdvertisements":"Disabled"}}}}`) | ||
|
|
||
| err := APIClient.Patch(context.TODO(), &operatorv1.Network{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "cluster", | ||
| }, | ||
| }, client.RawPatch(apimachinerytype.MergePatchType, patchData)) | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to disable RouteAdvertisements on network operator") |
There was a problem hiding this comment.
Can we make it via eco-goinfra methods?
There was a problem hiding this comment.
| By("Cleaning up RouteAdvertisement resources") | ||
|
|
||
| raList, err := ovn.ListRouteAdvertisements(APIClient) | ||
| if err != nil { |
There was a problem hiding this comment.
Why don't we vaildate the errors and just login? Also can we use eco-goinfra to remove it?
There was a problem hiding this comment.
validated the error
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
2 times, most recently
from
a2ef0c8 to
fcdd031
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
1079-1096: Consider improving error visibility in cleanup routine.The cleanup function properly uses eco-goinfra methods (
ListRouteAdvertisementsandDelete), addressing the past review comment. However, errors during cleanup are only logged viaBy()statements and the function continues silently.For better observability, consider collecting cleanup failures:
func cleanupRouteAdvertisements() { By("Cleaning up RouteAdvertisement resources") raList, err := ovn.ListRouteAdvertisements(APIClient) if err != nil { By(fmt.Sprintf("Warning: Failed to list RouteAdvertisements: %v", err)) return } var cleanupErrors []string for _, ra := range raList { err := ra.Delete() if err != nil { cleanupErrors = append(cleanupErrors, fmt.Sprintf("RouteAdvertisement %s: %v", ra.Definition.Name, err)) } } if len(cleanupErrors) > 0 { By(fmt.Sprintf("Warning: %d cleanup errors occurred:\n%s", len(cleanupErrors), strings.Join(cleanupErrors, "\n"))) } }This makes cleanup failures more visible in test output while still continuing to clean up all resources.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
go.sumis excluded by!**/*.sumvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/kmm/bootmoduleconfig.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgpsessionstate.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/bgpsessionstatelist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/servicebgpstatus.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/metallb/servicebgpstatuslist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/network/operator.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/argocd/argocdoperatorcommon/defaults.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/kmm/v1beta1/bootmoduleconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/kmm/v1beta1/modulebuildsignconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/kmm/v1beta1/moduleimagesconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/kmm/v1beta1/nodemodulesconfig_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/kmm/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/nvidiagpu/nvidiagputypes/clusterpolicy_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/storagecluster_types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ocs/operatorv1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ptp/v1/ptpconfig_webhook.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ptp/v1/ptpoperatorconfig_webhook.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ptp/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/siteconfig/v1alpha1/clusterinstance_conditions.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/siteconfig/v1alpha1/clusterinstance_types.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (3)
go.mod(1 hunks)tests/cnf/core/network/metallb/tests/common.go(2 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/metallb/tests/common.go
🔇 Additional comments (8)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (7)
5-5: LGTM: Import additions support OVN RouteAdvertisement integration.The new imports are necessary and properly used throughout the test:
strconvfor CIDR mask parsingoperatorv1andnetworkfor operator-level RouteAdvertisement configurationovn,ovnv1,ovntypesfor RouteAdvertisement resource creationmetav1for label selectorsgomegatypesfor dynamic matcher constructionAlso applies to: 11-11, 14-14, 29-29, 31-31, 35-38
628-701: LGTM: Well-structured test context with proper lifecycle management.The new "OVN-K RouteAdvertisements" test context follows best practices:
- BeforeAll/AfterAll properly enable and disable RouteAdvertisements at the operator level
- AfterEach ensures cleanup of MetalLB and RouteAdvertisement resources between tests
- Test case uses clear, descriptive By() statements for each step
- Properly integrates with existing test infrastructure (BGPPeer, FRR configuration)
The use of "default" as the RouteAdvertisement name is acceptable here since cleanup is guaranteed in AfterEach.
913-951: LGTM: Properly implements RouteAdvertisement creation with status verification.The function correctly:
- Uses the builder pattern with proper selector configuration
- Employs Eventually to wait for asynchronous status updates
- Validates the "Accepted" status with appropriate timeout
- Handles errors gracefully within the Eventually block
The FRR configuration selector label
routeadvertisement.k8s.ovn.org: enabledproperly matches the label applied increateOVNKCompatibleFRRConfiguration(line 968).
953-975: LGTM: Correctly uses builder method for DisableMP configuration.The function properly addresses past review feedback by:
- Using
WithBGPNeighborDisableMP(true, 0, 0)builder method instead of direct field assignment- Adding the selector label that matches the RouteAdvertisement's FRR configuration selector
- Following the builder pattern consistently for all configuration options
The DisableMP setting is required for RouteAdvertisement compatibility as documented.
977-994: LGTM: Clean implementation for retrieving cluster pod network configuration.The function:
- Uses the appropriate cluster package helper to fetch network operator config
- Validates that ClusterNetwork is not empty before accessing the first entry
- Extracts both CIDR and host prefix needed for route verification
- Includes informative By() logging for test output
996-1055: LGTM: Excellent dynamic CIDR range calculation addressing past feedback.This function properly resolves previous concerns about hardcoded /14 network assumptions:
- Dynamically calculates the number of /16 ranges based on the actual CIDR mask:
numRanges = 1 << (16 - cidrMaskBits)- For /14 networks (like 10.128.0.0/14): generates 4 matchers for 10.128-131.x.x
- For /16 networks: generates 1 matcher for the single range
- Uses proper validation with
Expect(...).To(HaveLen(...))instead of error creation- Builds matchers dynamically using
gomegatypes.GomegaMatchersliceThe Eventually block appropriately waits up to 3 minutes for BGP route propagation with reasonable polling intervals.
1057-1077: LGTM: Proper operator-level RouteAdvertisement lifecycle management.Both functions correctly:
- Use the network operator API to configure RouteAdvertisements cluster-wide
- Apply the appropriate state (Enabled/Disabled) with proper operator enum values
- Include a reasonable 5-minute timeout for operator reconciliation
- Handle errors with Expect assertions
These are properly called in the test context's BeforeAll/AfterAll hooks to ensure clean test isolation.
go.mod (1)
298-298: Verify eco-goinfra version supports OVN RouteAdvertisement APIs.The updated eco-goinfra version should contain the OVN RouteAdvertisement builders and types used in the test. The version timestamp (2025-12-20) is current, but direct verification of the commit hash
b4fb5fe43957and its contents requires repository access.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
fcdd031 to
55ff88a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
1080-1096: Improve error handling in cleanup function.The early return on line 1088 when
ListRouteAdvertisementsfails prevents any cleanup attempt. For a cleanup function, it's better to log the error and continue with best-effort cleanup, or use alternative methods to locate resources.Additionally, past review comment from evgenLevin suggests exploring eco-goinfra methods for resource cleanup. Consider whether the namespace cleanup methods could be used here.
Suggested improvement
func cleanupRouteAdvertisements() { By("Cleaning up RouteAdvertisement resources") raList, err := ovn.ListRouteAdvertisements(APIClient) if err != nil { - By(fmt.Sprintf("Failed to list RouteAdvertisements: %v", err)) - - return + By(fmt.Sprintf("Warning: Failed to list RouteAdvertisements: %v - cleanup may be incomplete", err)) + // Continue with best-effort cleanup or return if list is critical } for _, ra := range raList { err := ra.Delete() if err != nil { By(fmt.Sprintf("Failed to delete RouteAdvertisement %s: %v", ra.Definition.Name, err)) } } }Based on learnings, ... past review suggests investigating whether eco-goinfra namespace cleanup methods could handle RouteAdvertisement resources more robustly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (3)
go.modtests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (4)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (1)
LocalBGPASN(79-79)tests/cnf/core/network/metallb/internal/metallbenv/metallbenv.go (1)
CreateNewMetalLbDaemonSetAndWaitUntilItsRunning(66-154)tests/cnf/core/network/internal/cmd/cmd.go (1)
RemovePrefixFromIPList(181-190)tests/internal/cluster/config.go (1)
GetOCPNetworkOperatorConfig(58-72)
🔇 Additional comments (7)
tests/cnf/core/network/metallb/tests/common.go (1)
299-319: LGTM: Clean implementation of optional disableMP parameter.The variadic
disableMP ...boolparameter provides backward compatibility while allowing callers to optionally disable multipath BGP. The conditional logic correctly appliesWithDisableMP(true)only when explicitly requested (whendisableMP[0]is true). When the parameter is not provided or is false, the method is not called, which is the intended behavior.tests/cnf/core/network/metallb/tests/frrk8-tests.go (6)
5-38: LGTM: Imports are appropriate for OVN-K integration.All new imports are used by the OVN-K RouteAdvertisement integration:
strconvfor CIDR parsingoperatorv1for RouteAdvertisement enablementnetworkandclusterpackages for cluster configuration retrievalovnpackages for RouteAdvertisement resource managementgomegatypesfor dynamic matcher construction
692-692: Verify the use of hardcoded "default" name for RouteAdvertisement.Multiple past review comments flagged using a hardcoded "default" name as a potential conflict with existing cluster-level RouteAdvertisements. While cleanup is implemented in
AfterEach(line 656), using a test-unique name (e.g.,fmt.Sprintf("test-podnetwork-%d", GinkgoParallelProcess())) would eliminate collision risks during parallel test execution or with existing cluster resources.If "default" is intentional for this test scenario, please confirm this design choice.
913-951: LGTM: Proper readiness verification implemented.The function correctly creates the RouteAdvertisement and waits for it to reach "Accepted" status using an Eventually block. The polling logic handles errors gracefully by returning an empty string to trigger retry, and the timeout/interval values (1 minute / 5 seconds) are appropriate for this operation.
954-975: LGTM: Correctly uses builder method for DisableMP configuration.The function properly uses
WithBGPNeighborDisableMP(true, 0, 0)instead of directly setting the deprecatedDisableMPfield, addressing past review concerns. The label addition for RouteAdvertisement selector matching is correctly implemented.
998-1055: LGTM: Dynamic CIDR range calculation correctly addresses past review concerns.The function dynamically calculates the number of IP ranges based on the CIDR mask (lines 1018-1028) instead of hardcoding /14 logic. The implementation:
- Parses and validates CIDR format properly
- Calculates
numRanges = 1 << (16 - cidrMaskBits)to determine how many /16 ranges exist- Caps at 4 for practical checking
- Builds matchers dynamically (lines 1034-1037)
This makes the verification work correctly for any cluster CIDR configuration.
1034-1037: The project's go.mod specifiesgo 1.25, which fully supports the integer range syntaxfor i := range numRangesintroduced in Go 1.22. No compatibility concerns.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
55ff88a to
03f2104
Compare
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
03f2104 to
d9f9964
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @tests/cnf/core/network/metallb/tests/frrk8-tests.go:
- Around line 1079-1091: cleanupRouteAdvertisements currently deletes every
RouteAdvertisement returned by ovn.ListRouteAdvertisements(APIClient), which
removes all cluster resources; change it to only delete test-created resources
by filtering the list (e.g., by a test-specific label or name prefix). Modify
the call site around cleanupRouteAdvertisements to ensure created
RouteAdvertisements are labeled (or recorded) and then update
cleanupRouteAdvertisements to call ovn.ListRouteAdvertisements(APIClient) and
iterate only over entries matching that label/name pattern before invoking
ra.Delete(); preserve the existing Expect checks and include ra.Definition.Name
in error messages to aid debugging.
- Around line 1035-1037: The loop uses the Go 1.22+ "range" over an integer (for
i := range numRanges) which breaks older Go versions; change it to a traditional
indexed loop such as for i := 0; i < numRanges; i++ (keeping references to
numRanges, matchers, ipParts, and secondOctet) so matchers[i] =
ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) works across
Go versions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
tests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
🔇 Additional comments (1)
tests/cnf/core/network/metallb/tests/common.go (1)
299-330: LGTM! Clean implementation of optional DisableMP parameter.The variadic parameter pattern maintains backward compatibility while allowing callers to optionally disable multipath BGP. The conditional logic safely checks the slice length before accessing the first element.
| for i := range numRanges { | ||
| matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) | ||
| } |
There was a problem hiding this comment.
Fix Go version compatibility issue in range loop.
Line 1035 uses for i := range numRanges, which is Go 1.22+ syntax. This will fail to compile on older Go versions.
🔎 Proposed fix
- // Build matchers dynamically based on calculated range
- matchers := make([]gomegatypes.GomegaMatcher, numRanges)
- for i := range numRanges {
- matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i))
- }
+ // Build matchers dynamically based on calculated range
+ matchers := make([]gomegatypes.GomegaMatcher, numRanges)
+ for i := 0; i < numRanges; i++ {
+ matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := range numRanges { | |
| matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) | |
| } | |
| // Build matchers dynamically based on calculated range | |
| matchers := make([]gomegatypes.GomegaMatcher, numRanges) | |
| for i := 0; i < numRanges; i++ { | |
| matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) | |
| } |
🤖 Prompt for AI Agents
In @tests/cnf/core/network/metallb/tests/frrk8-tests.go around lines 1035 -
1037, The loop uses the Go 1.22+ "range" over an integer (for i := range
numRanges) which breaks older Go versions; change it to a traditional indexed
loop such as for i := 0; i < numRanges; i++ (keeping references to numRanges,
matchers, ipParts, and secondOctet) so matchers[i] =
ContainSubstring(fmt.Sprintf("%s.%d.", ipParts[0], secondOctet+i)) works across
Go versions.
d9f9964 to
b235231
Compare
| _, err := frrConfig.Create() | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to create RouteAdvertisement-compatible FRR configuration") | ||
|
|
||
| By(fmt.Sprintf("RouteAdvertisement-compatible FRR configuration '%s' created successfully", name)) |
| clusterCIDR := networkObj.Spec.ClusterNetwork[0].CIDR | ||
| hostPrefix := networkObj.Spec.ClusterNetwork[0].HostPrefix | ||
|
|
||
| By(fmt.Sprintf("Cluster pod network CIDR: %s with host prefix: /%d", clusterCIDR, hostPrefix)) |
There was a problem hiding this comment.
No need. use logging instead of By
| clusterCIDR := networkObj.Spec.ClusterNetwork[0].CIDR | ||
| hostPrefix := networkObj.Spec.ClusterNetwork[0].HostPrefix |
There was a problem hiding this comment.
is [0] index always IPv4 ?
clusterNetwork:
- cidr: 10.128.0.0/14
hostPrefix: 23
- cidr: fd02::/48
hostPrefix: 64
May be it is better to loop over it and fetch the ipv4 address. what do you think ?
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/cnf/core/network/metallb/tests/frrk8-tests.go:
- Around line 996-1055: The for loop in verifyOVNKPodNetworkRoutesReceived
incorrectly uses Go 1.22+ "for i := range numRanges" (numRanges is an int),
causing older Go toolchains to fail; change the loop to iterate with an index
over the integer count (e.g., for i := 0; i < numRanges; i++) and allocate
matchers accordingly so matchers[i] = ContainSubstring(fmt.Sprintf("%s.%d.",
ipParts[0], secondOctet+i)) still works; ensure matchers is created with
make([]gomegatypes.GomegaMatcher, numRanges) before the loop and update any
related references to i if needed.
- Around line 1079-1091: The cleanupRouteAdvertisements function currently
removes every RouteAdvertisement found via
ovn.ListRouteAdvertisements(APIClient) which can delete non-test resources;
update cleanupRouteAdvertisements to only target test-created
RouteAdvertisements by filtering listRA before deletion — e.g., check
ra.Definition.Name for the test-specific prefix/suffix or a test label (ensure
tests create RouteAdvertisements with a unique name pattern or label), then call
ra.Delete() only for matching entries; keep using ovn.ListRouteAdvertisements
and APIClient but add the filter logic and/or a registry of created names to
avoid removing cluster-wide resources.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisement.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/ovn/routeadvertisementlist.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/register.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/types.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/routeadvertisement/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/doc.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/networkselector.gois excluded by!vendor/**vendor/github.com/rh-ecosystem-edge/eco-goinfra/pkg/schemes/ovn/types/zz_generated.deepcopy.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (2)
tests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/metallb/tests/common.go
🔇 Additional comments (6)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (6)
5-39: LGTM: Import additions support OVN-K RouteAdvertisement integration.The new imports (strconv, operatorv1, network, metav1, gomega types, OVN packages) are appropriate for the RouteAdvertisement functionality being added.
157-165: LGTM: Minor clarifications to existing test.The changes make the function parameters explicit and update the comment for clarity. No issues detected.
913-951: LGTM: RouteAdvertisement creation and readiness verification implemented correctly.The function properly creates a RouteAdvertisement with appropriate selectors and polls until it reaches "Accepted" status. The Eventually pattern with proper timeout and error handling is well-implemented.
953-975: LGTM: FRR configuration builder correctly uses WithBGPNeighborDisableMP.The function properly creates a RouteAdvertisement-compatible FRR configuration using the builder method
WithBGPNeighborDisableMP(true, 0, 0)and adds the necessary label for selector matching. Good use of the builder pattern throughout.
977-994: LGTM: Cluster network CIDR retrieval is clean and well-validated.The function correctly retrieves the cluster network configuration and extracts the CIDR and host prefix. Proper error handling and validation that ClusterNetwork is not empty.
1057-1077: LGTM: Enable/disable RouteAdvertisements functions are clean and correct.Both functions properly pull the network operator and toggle the RouteAdvertisements setting with appropriate timeouts and error handling. Clear and maintainable implementation.
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
b235231 to
9722122
Compare
| fmt.Sprintf("External FRR pod should receive pod network routes (/%d subnets) "+ | ||
| "from cluster network %s via OVN-K RouteAdvertisement", hostPrefix, clusterCIDR)) | ||
|
|
||
| By(fmt.Sprintf("Successfully verified pod network routes from %s are received", clusterCIDR)) |
Add test case 74273 to verify OVN-K RouteAdvertisements work with FRR-K8s. - Add enableOVNKRouteAdvertisements and disableOVNKRouteAdvertisements functions - Create RouteAdvertisement-compatible BGPPeer and FRRConfiguration - Add verifyOVNKPodNetworkRoutesReceived to validate pod network routes
gkopels
force-pushed
the
add-ovnk-routeadvertisement-step
branch
from
9722122 to
fc13b64
Compare
4 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.