Arista: port NX-OS route-map logic #6465
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6465 +/- ##
============================================
- Coverage 73.28% 73.28% -0.01%
- Complexity 35451 35452 +1
============================================
Files 2821 2821
Lines 143320 143306 -14
Branches 17306 17299 -7
============================================
- Hits 105036 105023 -13
- Misses 29957 29959 +2
+ Partials 8327 8324 -3
|
dhalperi
commented
Dec 2, 2020
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @arifogel)
projects/batfish/src/main/java/org/batfish/representation/arista/AristaConfiguration.java, line 1903 at r1 (raw file):
// Continue: on match, change the default. if (action == LineAction.PERMIT) { trueStatements.add(Statements.SetLocalDefaultActionAccept.toStaticStatement());
codecov reveals we need new unit tests of continue statements
arifogel
reviewed
Dec 2, 2020
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion
Prior to this PR, Arista modeled route-maps as nested structures, so that if there were N terms, those terms would go into a recursive structure that was N terms deep. For large N (hundreds+) this can cause recursion limits in Java, e.g., in walking the route-map or in serializing it to disk. Fix this by adopting (neearly verbatim) the logic from NX-OS. Validated by existing unit tests + Batfish Enterprise compare view: routes are the same before and after in many real networks.
dhalperi
force-pushed
the
arista-rm-redo
branch
from
f522a52
to
eee8c83
Compare
dhalperi
commented
Dec 2, 2020
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
dhalperi
added a commit
to dhalperi/batfish
that referenced
this pull request
Dec 2, 2020
And add another missing test.
dhalperi
added a commit
to dhalperi/batfish
that referenced
this pull request
Dec 2, 2020
And add another missing test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Prior to this PR, Arista modeled route-maps as nested structures, so that if
there were N terms, those terms would go into a recursive structure that was N
terms deep. For large N (hundreds+) this can cause recursion limits in Java,
e.g., in walking the route-map or in serializing it to disk.
Fix this by adopting (nearly verbatim) the logic from NX-OS.
Validated by existing unit tests + Batfish Enterprise compare view: routes are
the same before and after in many real networks.