Add VRF route persistence test using vrf-name #1399
Conversation
Add e2e test to verify VRF routes specified by vrf-name persist across node reboots. This tests the new vrf-name parameter for routes that allows referencing VRF route tables by interface name instead of numeric table-id. The test includes: - Helper function vrfRouteWithVrfName() to create VRF routes using vrf-name - Basic verification test for vrf-name route configuration - Reboot persistence test to ensure routes survive node restart Related to upstream nmstate PR: nmstate/nmstate#3016 Co-Authored-By: Claude <[email protected]> Signed-off-by: Emilia Desch <[email protected]>
kubevirt-bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @emy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the network configuration testing suite by adding new end-to-end tests. These tests specifically validate the persistence of Virtual Routing and Forwarding (VRF) routes that are configured using the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds an e2e test to verify that VRF routes configured with vrf-name persist after a node reboot. The overall approach is good, and the new test covers the intended scenario. However, I've found a critical issue in the test helper function where the generated YAML for the network state is incorrectly indented, which would cause the test to fail. I've also suggested a refactoring in the test file to reduce code duplication and improve maintainability. Please see the detailed comments for suggestions.
| - destination: %s | ||
| metric: 150 | ||
| next-hop-address: %s | ||
| next-hop-interface: %s | ||
| vrf-name: %s |
There was a problem hiding this comment.
The YAML for the route configuration is incorrectly indented. The list item under config (the - on line 260) should be indented to be a child of config. Currently, it's at the same level, which is invalid YAML and will cause a parsing error when the state is applied.
| - destination: %s | |
| metric: 150 | |
| next-hop-address: %s | |
| next-hop-interface: %s | |
| vrf-name: %s | |
| - destination: %s | |
| metric: 150 | |
| next-hop-address: %s | |
| next-hop-interface: %s | |
| vrf-name: %s |
| It("should have the VRF interface and route configured", func() { | ||
| vrfForNodeInterfaceEventually(node, vrfID).Should(Equal(vrfID)) | ||
| ipAddressForNodeInterfaceEventually(node, firstSecondaryNic).Should(Equal(ipAddress)) | ||
| routeNextHopInterfaceWithTableID(node, destIPAddress, vrfID).Should(Equal(firstSecondaryNic)) | ||
| }) | ||
|
|
||
| It("should persist VRF route after node reboot", func() { | ||
| Byf("Verifying VRF interface and route before reboot") | ||
| vrfForNodeInterfaceEventually(node, vrfID).Should(Equal(vrfID)) | ||
| ipAddressForNodeInterfaceEventually(node, firstSecondaryNic).Should(Equal(ipAddress)) | ||
| routeNextHopInterfaceWithTableID(node, destIPAddress, vrfID).Should(Equal(firstSecondaryNic)) | ||
|
|
||
| restartNodeWithoutWaiting(node) | ||
|
|
||
| By("Wait for policy re-reconciled after node reboot") | ||
| policy.WaitForPolicyTransitionUpdate(TestPolicy) | ||
| policy.WaitForAvailablePolicy(TestPolicy) | ||
|
|
||
| Byf("Node %s was rebooted, verifying VRF route still exists", node) | ||
| vrfForNodeInterfaceEventually(node, vrfID).Should(Equal(vrfID)) | ||
| ipAddressForNodeInterfaceEventually(node, firstSecondaryNic).Should(Equal(ipAddress)) | ||
| routeNextHopInterfaceWithTableID(node, destIPAddress, vrfID).Should(Equal(firstSecondaryNic)) | ||
| }) |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can extract the repeated verification logic into a helper function. The assertions on lines 87-89, 94-96, and 105-107 are identical. A helper function will make the tests cleaner and easier to read.
verifyVrfRouteIsConfigured := func() {
vrfForNodeInterfaceEventually(node, vrfID).Should(Equal(vrfID))
ipAddressForNodeInterfaceEventually(node, firstSecondaryNic).Should(Equal(ipAddress))
routeNextHopInterfaceWithTableID(node, destIPAddress, vrfID).Should(Equal(firstSecondaryNic))
}
It("should have the VRF interface and route configured", func() {
verifyVrfRouteIsConfigured()
})
It("should persist VRF route after node reboot", func() {
Byf("Verifying VRF interface and route before reboot")
verifyVrfRouteIsConfigured()
restartNodeWithoutWaiting(node)
By("Wait for policy re-reconciled after node reboot")
policy.WaitForPolicyTransitionUpdate(TestPolicy)
policy.WaitForAvailablePolicy(TestPolicy)
Byf("Node %s was rebooted, verifying VRF route still exists", node)
verifyVrfRouteIsConfigured()
})
|
@emy: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Handler e2e fails because the required nmstate version is 2.2.55 and above. |
We need to see this test passing to merge it, although would be good to skip it if nmstate version is no the expected, the nmstate version can be check looking at the NNS |
3 participants
Is this a BUG FIX or a FEATURE ?:
/kind enhancement
What this PR does / why we need it:
Add e2e test to verify VRF routes specified by vrf-name persist across node reboots. This tests the new vrf-name parameter for routes that allows referencing VRF route tables by interface name instead of numeric table-id.
The test includes:
Special notes for your reviewer:
Depeds-on: nmstate 2.2.55
Release note: