RoutesQuestion: Add more match criteria #7714
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ratulm)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 176 at r2 (raw file):
mainRib
Is any of this specific to the main RIB? Can we rename to rib?
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 202 at r2 (raw file):
case LONGEST_PREFIX_MATCH: // handled separately default: throw new IllegalArgumentException("Illegal PrefixMatchType " + prefixMatchType);
To make our lives easier if/when we add more match types, I would distinguish between illegal (LPM) and unimplemented new match types.
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 60 at r2 (raw file):
all prefixes equal or longer than the input network; can return multiple prefixes
only matches prefixes contained by the input network?
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 62 at r2 (raw file):
all prefixes equal or shorter than the input network; can return multiple prefixes
only matches prefixes that contain the input network?
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 68 at r2 (raw file):
only for main RIB
why doesn't this work for other RIBs?
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 945 at r2 (raw file):
testPrefixMatches()
please add test cases for longer/shorter prefixes without containment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anothermattbrown)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 176 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
mainRibIs any of this specific to the main RIB? Can we rename to
rib?
This code is not specific. So, renamed. But it will be called only on the main RIB.
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 60 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
all prefixes equal or longer than the input network; can return multiple prefixesonly matches prefixes contained by the input network?
Done.
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 62 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
all prefixes equal or shorter than the input network; can return multiple prefixesonly matches prefixes that contain the input network?
Done.
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 68 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
only for main RIBwhy doesn't this work for other RIBs?
FWICT, protocol RIBs do not appear to be stored as RIB but as set of routes. So, LPM on that is extra work.
There was a problem hiding this comment.
Reviewed 2 of 6 files at r3.
Reviewable status: 9 of 13 files reviewed, 2 unresolved discussions (waiting on @anothermattbrown and @ratulm)
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 68 at r2 (raw file):
Previously, ratulm wrote…
FWICT, protocol RIBs do not appear to be stored as RIB but as set of routes. So, LPM on that is extra work.
it should be easy/efficient enough to create a PrefixTrieMultiMap containing the matching routes and then call longestPrefixMatch(Ip address, int maxPrefixLength).
There was a problem hiding this comment.
Reviewed 1 of 6 files at r3.
Reviewable status: 10 of 13 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown and @ratulm)
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 68 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
it should be easy/efficient enough to create a
PrefixTrieMultiMapcontaining the matching routes and then calllongestPrefixMatch(Ip address, int maxPrefixLength).
or just do it directly -- find the max length of matching networks, then do exact match. doesn't seem like that much extra work
There was a problem hiding this comment.
Reviewable status: 10 of 13 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown)
projects/question/src/main/java/org/batfish/question/routes/RoutesQuestion.java, line 68 at r2 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
or just do it directly -- find the max length of matching networks, then do exact match. doesn't seem like that much extra work
sure, on it.
There was a problem hiding this comment.
Reviewed 2 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown)
Codecov Report
@@ Coverage Diff @@
## master #7714 +/- ##
============================================
+ Coverage 73.81% 73.82% +0.01%
- Complexity 41814 41873 +59
============================================
Files 3282 3290 +8
Lines 164631 164798 +167
Branches 19782 19793 +11
============================================
+ Hits 121515 121661 +146
- Misses 33623 33636 +13
- Partials 9493 9501 +8
|
There was a problem hiding this comment.
Reviewed 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ratulm)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 687 at r3 (raw file):
BgpRouteStatus.
why remove qualification here but not elsewhere?
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 305 at r5 (raw file):
* <p>It the prefix match type is LONGEST_PREFIX_MATCH, the returned prefix is decided based on * LPM on the best routes table.
this is based on the invariant that the backup table won't contain any longer prefixes (because they would not be backing anything up)? would be helpful to document that.
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 334 at r5 (raw file):
PrefixMatchType.EXACT
why hard-code exact here?
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 1159 at r5 (raw file):
EXACT
this block says it's testing LPM
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 1194 at r5 (raw file):
multi-prefix matcher
for LONGER_PREFIXES, we should test that equal prefixes are included and shorter prefixes are excluded (and similarly for shorter).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @anothermattbrown)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 687 at r3 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
BgpRouteStatus.why remove qualification here but not elsewhere?
IDE ...
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 305 at r5 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
* <p>It the prefix match type is LONGEST_PREFIX_MATCH, the returned prefix is decided based on * LPM on the best routes table.this is based on the invariant that the backup table won't contain any longer prefixes (because they would not be backing anything up)? would be helpful to document that.
Done.
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 334 at r5 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
PrefixMatchType.EXACTwhy hard-code exact here?
ooooh. BUG!
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 1159 at r5 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
EXACTthis block says it's testing LPM
Done.
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 1194 at r5 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
multi-prefix matcherfor LONGER_PREFIXES, we should test that equal prefixes are included and shorter prefixes are excluded (and similarly for shorter).
That testing is there in testPrefixMatches. The function being tested here delegates to prefixMatches. You see value in doing it here too?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ratulm)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 324 at r6 (raw file):
* A consequence of these semantics is that if the user asks only for backup routes (via * routeStatus), nothing may be reported in cases where the LPM-based matching on the best table * leads to prefix P1 but P1 is not present in the backup table.
This seems like a weird UX, though I'm not sure what the use case of LPM matching over only backup routes would be...
projects/question/src/test/java/org/batfish/question/routes/RoutesAnswererUtilTest.java, line 1194 at r5 (raw file):
Previously, ratulm wrote…
That testing is there in
testPrefixMatches. The function being tested here delegates toprefixMatches. You see value in doing it here too?
nope that sounds right
There was a problem hiding this comment.
Reviewable status:
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswererUtil.java, line 324 at r6 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
* A consequence of these semantics is that if the user asks only for backup routes (via * routeStatus), nothing may be reported in cases where the LPM-based matching on the best table * leads to prefix P1 but P1 is not present in the backup table.This seems like a weird UX, though I'm not sure what the use case of LPM matching over only backup routes would be...
FWIW, a similar issue is happening with protocol filters: do LPM first, and then filter. This semantics is consistent with what is happening there.
We can revisit if it seems counter-intuitive to users. Need to lookup what routers do as well.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status:
For the main RIB only.