Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2E: add announcement tests #19

Merged
merged 3 commits into from
Jul 17, 2023
Merged

E2E: add announcement tests #19

merged 3 commits into from
Jul 17, 2023

Conversation

fedepaol
Copy link
Member

We add basic announcement tests where all the nodes announce the same
ips to all / some neighbors.

Signed-off-by: Federico Paolinelli <[email protected]>
@fedepaol fedepaol force-pushed the moretests branch 2 times, most recently from acb9fb1 to aa5ecc8 Compare July 11, 2023 09:25
ValidatePrefixesForNeighbor(f, nodes, "fc00:f853:ccd:e799::/64", "fc00:f853:ccd:e800::/64")
}
},
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a dual case at this point or not yet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment on lines +33 to +34
var notFound RouteNotFoundError
if errors.As(err, &notFound) {
return false, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the specific error is different from a conversion error or something not recoverable.
Even though routeForCIDR can only return that type of error, it's better to handle the specific error separately from the generic one, it will make the logic future proof.

return frr.Route{}, RouteNotFoundError(fmt.Sprintf("route %s not found", cidr))
}

func havePrefix(pod *v1.Pod, pairingFamily ipfamily.Family, prefix *net.IPNet, nextHop, vrf string) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this pr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I reworked a lot of the logic in the "incoming prefixes" PR and backported the files, this sneaked in. Removing

}
},
}),
ginkgo.Entry("DUAL - Advertise with mode allowall", params{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DUAL -> DUALSTACK as that is what we skip in the ci? (although this passes on these lanes)

We add basic announcement tests where all the nodes announce the same
ips to all / some neighbors.

Signed-off-by: Federico Paolinelli <[email protected]>
Copy link
Member

@oribon oribon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fedepaol fedepaol added this pull request to the merge queue Jul 17, 2023
Merged via the queue into metallb:main with commit 4853d14 Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants