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

feat(anta.tests): Optimize VerifyRoutingTableEntry by quering all routes for a vrf. #682

Conversation

dlobato
Copy link
Contributor

@dlobato dlobato commented May 16, 2024

Description

Reduce number of commands executed on VerifyRoutingTableEntry by querying all routes of a vrf in one go.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have run pre-commit for code linting and typing (pre-commit run)
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (tox -e testenv)

@dlobato dlobato force-pushed the feat/optimize-verify-routing-table-entry-test branch 2 times, most recently from fde582d to 93be1ef Compare May 16, 2024 11:19
@dlobato dlobato changed the title Optimize VerifyRoutingTableEntry by quering all routes for a vrf. feat: Optimize VerifyRoutingTableEntry by quering all routes for a vrf. May 16, 2024
Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

Hey David, thanks for the PR! Did you test this against a large routing table? I remember going back and forth myself when I wrote this test. If I remember correctly I ran into issues with large routing tables and I think it was more efficient sending multiple commands.

@dlobato
Copy link
Contributor Author

dlobato commented May 17, 2024

Not really, that would be an interesting test. Maybe we can sync and test. Guillaume mentioned this and he proposed to have a parameter collect: [all|unique] that could control the behavior.

@gmuloc gmuloc added this to the v1.1.0 milestone May 17, 2024
@carl-baillargeon
Copy link
Contributor

Not really, that would be an interesting test. Maybe we can sync and test. Guillaume mentioned this and he proposed to have a parameter collect: [all|unique] that could control the behavior.

Tested this with a full routing table and ANTA is in fact crashing. I need to investigate more but when doing the same command with curl or using HTTPX outside of ANTA, it works fine. We can probably reproduce easily with other commands that generate a very large output so that's a separate issue in my opinion.

If we implement a knob to control that behavior, we open the door for users to hit that bug. @gmuloc Thoughts?

Copy link

sonarcloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@carl-baillargeon
Copy link
Contributor

Not really, that would be an interesting test. Maybe we can sync and test. Guillaume mentioned this and he proposed to have a parameter collect: [all|unique] that could control the behavior.

Tested this with a full routing table and ANTA is in fact crashing. I need to investigate more but when doing the same command with curl or using HTTPX outside of ANTA, it works fine. We can probably reproduce easily with other commands that generate a very large output so that's a separate issue in my opinion.

If we implement a knob to control that behavior, we open the door for users to hit that bug. @gmuloc Thoughts?

Update: I was able to successfuly test the full routing table with ANTA 1.0 but we should still implement a knob in the inputs [ all | unique ] to cover the use case where we need to query a single route against a full routing table.

@dlobato dlobato changed the title feat: Optimize VerifyRoutingTableEntry by quering all routes for a vrf. feat(anta.tests): Optimize VerifyRoutingTableEntry by quering all routes for a vrf. Jul 19, 2024
@dlobato dlobato force-pushed the feat/optimize-verify-routing-table-entry-test branch 3 times, most recently from 5dfcf09 to 4ea6296 Compare July 19, 2024 09:14
Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

I don't see improvements with a full routing table. Makes sense since every indiviual route is unique, not hitting the cache.

Also, we are spending a lot of time waiting on the actual HTTP response since the output is super big. Let's implement the knob for maximum flexibility.

…r: one=a route per command, all=all routes in vrf
@dlobato dlobato force-pushed the feat/optimize-verify-routing-table-entry-test branch from bef22e5 to 0094cb3 Compare July 26, 2024 14:20
Copy link

sonarcloud bot commented Jul 26, 2024

@carl-baillargeon carl-baillargeon merged commit af58310 into aristanetworks:main Aug 14, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants