Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
adding test cases for indirect next hops. #3659
base: main
Are you sure you want to change the base?
adding test cases for indirect next hops. #3659
Changes from all commits
cff9734
e755346
e66be74
f1c90b7
d6f52a7
be46d35
d7d7481
6f1c043
ce1275b
910c66b
5f47ede
4408124
a08b48f
b609c03
cc42810
fc255ee
e37574e
e476a0a
d89eb06
8447245
5596464
e32c54d
a7311d0
d124fc3
8e00c94
f6f8591
8ef4a58
e8c73df
dce47ea
493902d
5f886d4
c59fa14
630d308
d0fcf93
a7c89ae
8712872
44ad581
4bba7f5
79b29ff
69029ad
00e19b1
ddb8d70
ef5b393
abb4bea
0794030
8a76473
062a61c
42f5640
d40c4c4
bfe37ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to specify how to configure the addressing in this README -- since if you're expecting multiple BGP NHs, how does the DUT resolve these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need this scale for a base test? It seems like we're testing more than just than just base AFT exports work correctly here. Please consider breaking this test down into:
There can be signficant code sharing between these, but this will give you the "base" functionality that ensures that the right AFT exports are happening. Some of these could be added as test cases within BGP and IS-IS -- please discuss with @rohit-rp.
We can then start to say, "OK test negative cases":
We can then start to say, "OK, introduce scale" and add test cases that validate:
And then inject churn:
This way -- you know that the base export works as you expect and we test the contract there, then we know that the scale is OK in steady state, and then we start to introduce scale+churn to validate these together. The last cases are more complex and fragile than the first, but we don't want them to break because of functional gaps -- so the former cases help us validate that (and encode the contract in code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? We need to specify the routes that are going to be advertised over this session if we expect that this is multi-path -- since I assume that you mean some prefix must be resolved to >1 BGP NH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear why in this test case this is true -- this is why we need to spell out the topology more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not define what "tunnels" are -- and you removed the RSVP-TE config (which I agree with). Can we update the rest of the README to reflect this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please expand on this "verify" -- does this mean we check that the AFT prefixes are exactly what was injected?
Especially for "verify all other leaves mentioned in the path section", this seems very ambiguous. Please consider writing this README as though you are trying to plan what code you're going to write. We don't need to enumerate exactly the check in prose, but we do need to define the correctness criteria.