SearchRoutePolicies: AS-path support #6168
Conversation
…zed it in preparation for handling AS path regexes
…zed it in preparation for handling AS path regexes
…path access lists
…rom the results of symbolic route analysis
…sfer-bdd-as-path-3
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6168 +/- ##
============================================
- Coverage 72.88% 72.81% -0.07%
+ Complexity 34859 34735 -124
============================================
Files 2818 2818
Lines 142254 141741 -513
Branches 17146 17003 -143
============================================
- Hits 103676 103205 -471
+ Misses 30404 30363 -41
+ Partials 8174 8173 -1
|
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @millstein)
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 233 at r1 (raw file):
It is useful when the goal is to produce concrete example routes from a BDDRoute, for instance.
The constraints don't always have to hold?
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 241 at r1 (raw file):
// at most one AS-path regex atomic predicate should be true, since by construction their // regexes are all pairwise disjoint
in this case, can you just allocate log(N) BDD variables instead of N, for N=number of APs? That's what we do in BDDFiniteDomain. In fact, maybe you can use BDDFiniteDomain? It still has a well-formedness constraint, since there may be extra values.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @anothermattbrown)
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 233 at r1 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
It is useful when the goal is to produce concrete example routes from a BDDRoute, for instance.The constraints don't always have to hold?
The constraints do always have to hold. But I don't think it suffices to enforce them during symbolic route analysis (TransferBDD). That analysis computes a BDD representing the input routes that are accepted by the route map. So ANDing the well-formedness constraints in there would ensure that any models of that BDD are feasible routes. But if what a client is interested in is routes that are denied by the route map, then simply negating that whole thing would be incorrect, because it would not ensure that the well-formedness constraints hold. So I think it's logically a separate notion.
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 241 at r1 (raw file):
Previously, anothermattbrown (Matt Brown) wrote…
// at most one AS-path regex atomic predicate should be true, since by construction their // regexes are all pairwise disjointin this case, can you just allocate log(N) BDD variables instead of N, for N=number of APs? That's what we do in BDDFiniteDomain. In fact, maybe you can use BDDFiniteDomain? It still has a well-formedness constraint, since there may be extra values.
Could do that, although I'm not sure if it's worth it given that I expect N to be small. Also I've found in other projects that the smallest encoding is not always the best performing one, because it introduces spurious correlations. For example, with the "unary" encoding after an AS-path regex match some subset of the choices will have the BDD 1 and the rest the BDD 0, while with the binary encoding there will be more complicated BDDs. Happy to discuss of course.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @millstein)
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 233 at r1 (raw file):
Previously, millstein (Todd Millstein) wrote…
The constraints do always have to hold. But I don't think it suffices to enforce them during symbolic route analysis (TransferBDD). That analysis computes a BDD representing the input routes that are accepted by the route map. So ANDing the well-formedness constraints in there would ensure that any models of that BDD are feasible routes. But if what a client is interested in is routes that are denied by the route map, then simply negating that whole thing would be incorrect, because it would not ensure that the well-formedness constraints hold. So I think it's logically a separate notion.
makes sense. these points would be worth including in the comment.
projects/minesweeper/src/main/java/org/batfish/minesweeper/bdd/BDDRoute.java, line 241 at r1 (raw file):
Previously, millstein (Todd Millstein) wrote…
Could do that, although I'm not sure if it's worth it given that I expect N to be small. Also I've found in other projects that the smallest encoding is not always the best performing one, because it introduces spurious correlations. For example, with the "unary" encoding after an AS-path regex match some subset of the choices will have the BDD 1 and the rest the BDD 0, while with the binary encoding there will be more complicated BDDs. Happy to discuss of course.
Yeah it'll be interesting to see how many APs we see in practice. The other benefit of the log(N) encoding is that it would eliminate the need for this n^2 constraint, since the AP identifiers would be mutually exclusive. In any case, it would be worth making a note of these trade-offs in the comment.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewable status:
Support AS-path filtering in the searchRoutePolicies question. Also support constraints on the AS paths of the input/output route announcements.