Skip to content

Commit

Permalink
Search Route Policies: Remove redundant atomic predicate computation (#…
Browse files Browse the repository at this point in the history 
…6284)

* Build the symbolic Graph, which computes atomic predicates for the community and AS-path regexes in a configuration, once per node rather than once per node per route policy. Since computing atomic predicates is potentially expensive, this may provide a useful performance improvement in some cases. 

* Declare almost all of the Graph's internal state 'final', to document its read-only nature, which allows it to be safely reused.
  • Loading branch information
@millstein
millstein committed Oct 8, 2020
1 parent 67ef9ef commit d0cbef2
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 68 deletions.
58 changes: 29 additions & 29 deletions projects/minesweeper/src/main/java/org/batfish/minesweeper/Graph.java
View file
Expand Up @@ -89,61 +89,61 @@ public enum BgpSendType {

public static final String BGP_COMMON_FILTER_LIST_NAME = "BGP_COMMON_EXPORT_POLICY";
private static final String NULL_INTERFACE_NAME = "null_interface";
private IBatfish _batfish;
private Set<String> _routers;
private Map<String, Configuration> _configurations;
private final IBatfish _batfish;
private final Set<String> _routers;
private final Map<String, Configuration> _configurations;
private final NetworkSnapshot _snapshot;
private Map<String, Set<Long>> _areaIds;
private Table2<String, String, List<StaticRoute>> _staticRoutes;
private Map<String, List<StaticRoute>> _nullStaticRoutes;
private Map<String, Set<String>> _neighbors;
private Map<String, List<GraphEdge>> _edgeMap;
private Set<GraphEdge> _allRealEdges;
private Set<GraphEdge> _allEdges;
private Map<GraphEdge, GraphEdge> _otherEnd;
private Map<GraphEdge, BgpActivePeerConfig> _ebgpNeighbors;
private Map<GraphEdge, BgpActivePeerConfig> _ibgpNeighbors;
private Map<String, String> _routeReflectorParent;
private Map<String, Set<String>> _routeReflectorClients;
private Map<String, Integer> _originatorId;
private Map<String, Integer> _domainMap;
private Map<Integer, Set<String>> _domainMapInverse;
private final Map<String, Set<Long>> _areaIds;
private final Table2<String, String, List<StaticRoute>> _staticRoutes;
private final Map<String, List<StaticRoute>> _nullStaticRoutes;
private final Map<String, Set<String>> _neighbors;
private final Map<String, List<GraphEdge>> _edgeMap;
private final Set<GraphEdge> _allRealEdges;
private final Set<GraphEdge> _allEdges;
private final Map<GraphEdge, GraphEdge> _otherEnd;
private final Map<GraphEdge, BgpActivePeerConfig> _ebgpNeighbors;
private final Map<GraphEdge, BgpActivePeerConfig> _ibgpNeighbors;
private final Map<String, String> _routeReflectorParent;
private final Map<String, Set<String>> _routeReflectorClients;
private final Map<String, Integer> _originatorId;
private final Map<String, Integer> _domainMap;
private final Map<Integer, Set<String>> _domainMapInverse;

/**
* The SMT- and BDD-based analyses (see the corresponding smt and bdd packages) handle communities
* differently and make different assumptions about these communities, and in the future might
* diverge further. Hence we use this flag to build the appropriate Graph object for the given
* analysis.
*/
private boolean _bddBasedAnalysis;
private final boolean _bddBasedAnalysis;

/**
* A graph with a static route with a dynamic next hop cannot be encoded to SMT, so some of the
* Minesweeper analyses will fail. Compression is still possible though.
*/
private boolean _hasStaticRouteWithDynamicNextHop;

private Set<CommunityVar> _allCommunities;
private final Set<CommunityVar> _allCommunities;

/**
* Keys are all REGEX vars, and values are lists of EXACT or OTHER vars. This field is only used
* by the SMT-based analyses.
*/
private SortedMap<CommunityVar, List<CommunityVar>> _communityDependencies;
private final SortedMap<CommunityVar, List<CommunityVar>> _communityDependencies;

private Map<String, String> _namedCommunities;
private final Map<String, String> _namedCommunities;

/**
* In order to track community literals and regexes in the BDD-based analysis, we compute a set of
* "atomic predicates" for them.
*/
private RegexAtomicPredicates<CommunityVar> _communityAtomicPredicates;
private final RegexAtomicPredicates<CommunityVar> _communityAtomicPredicates;

/**
* We also compute a set of atomic predicates for the AS-path regexes that appear in the given
* configurations.
*/
private RegexAtomicPredicates<SymbolicAsPathRegex> _asPathRegexAtomicPredicates;
private final RegexAtomicPredicates<SymbolicAsPathRegex> _asPathRegexAtomicPredicates;

/**
* Create a graph, loading configurations from the given {@link IBatfish}.
Expand Down Expand Up @@ -226,13 +226,12 @@ public Graph(
_originatorId = new HashMap<>();
_domainMap = new HashMap<>();
_domainMapInverse = new HashMap<>();
_configurations = configs;
_allCommunities = new HashSet<>();
_communityDependencies = new TreeMap<>();
_snapshot = snapshot;
_bddBasedAnalysis = bddBasedAnalysis;

if (_configurations == null) {
if (configs == null) {
// Since many functions that use the graph mutate the configurations, we must clone them
// before that happens.
// A simple way to do this is to create a deep clone of each entry using Java serialization.
Expand All @@ -244,6 +243,8 @@ public Graph(
.collect(toMap(Entry::getKey, entry -> SerializationUtils.clone(entry.getValue())));

_configurations = clonedConfigs;
} else {
_configurations = configs;
}
_routers = _configurations.keySet();

Expand Down Expand Up @@ -280,8 +281,10 @@ public Graph(
.collect(ImmutableSet.toImmutableSet());
_communityAtomicPredicates = new RegexAtomicPredicates<>(comms, CommunityVar.ALL_COMMUNITIES);
} else {
_communityAtomicPredicates = null;
initCommDependencies();
}
_namedCommunities = new HashMap<>();
initNamedCommunities();
_asPathRegexAtomicPredicates =
new RegexAtomicPredicates<>(
Expand Down Expand Up @@ -464,8 +467,6 @@ private void initGraph(Topology topology) {
routerIfaceMap.put(router, ifacePairs);
}

_neighbors = new HashMap<>();

for (Entry<String, Set<NodeInterfacePair>> entry : routerIfaceMap.entrySet()) {
String router = entry.getKey();
Set<NodeInterfacePair> nips = entry.getValue();
Expand Down Expand Up @@ -974,7 +975,6 @@ private void initCommDependencies() {
* easier to provide intuitive counter examples.
*/
private void initNamedCommunities() {
_namedCommunities = new HashMap<>();
for (Configuration conf : getConfigurations().values()) {
for (Entry<String, CommunityList> entry : conf.getCommunityLists().entrySet()) {
String name = entry.getKey();
Expand Down
74 changes: 35 additions & 39 deletions ...ava/org/batfish/minesweeper/question/searchroutepolicies/SearchRoutePoliciesAnswerer.java
View file
Expand Up @@ -15,17 +15,14 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultiset;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Multiset;
import com.google.common.collect.Ordering;
import com.google.common.collect.Range;
import dk.brics.automaton.Automaton;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.SortedSet;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
Expand All @@ -42,7 +39,6 @@
import org.batfish.common.plugin.IBatfish;
import org.batfish.datamodel.AsPath;
import org.batfish.datamodel.Bgpv4Route;
import org.batfish.datamodel.Configuration;
import org.batfish.datamodel.Ip;
import org.batfish.datamodel.LongSpace;
import org.batfish.datamodel.OriginType;
Expand Down Expand Up @@ -308,15 +304,6 @@ private Optional<Result> constraintsToResult(
}
}

private SortedSet<RoutingPolicyId> resolvePolicies(SpecifierContext context) {
return _nodeSpecifier.resolve(context).stream()
.flatMap(
node ->
_policySpecifier.resolve(node, context).stream()
.map(policy -> new RoutingPolicyId(node, policy.getName())))
.collect(ImmutableSortedSet.toImmutableSortedSet(Ordering.natural()));
}

private BDD prefixSpaceToBDD(PrefixSpace space, BDDRoute r, boolean complementPrefixes) {
BDDFactory factory = r.getPrefix().getFactory();
if (space.isEmpty()) {
Expand Down Expand Up @@ -436,18 +423,15 @@ private BDD routeConstraintsToBDD(BgpRouteConstraints constraints, BDDRoute r, G
return result;
}

private Optional<Result> searchPolicy(RoutingPolicy policy) {
/**
* Search a particular route policy for behaviors of interest.
*
* @param policy the routing policy
* @param g a Graph object providing information about the policy's owner configuration
* @return an optional result, if a behavior of interest was found
*/
private Optional<Result> searchPolicy(RoutingPolicy policy, Graph g) {
TransferReturn result;
Graph g =
new Graph(
_batfish,
_batfish.getSnapshot(),
null,
ImmutableSet.of(policy.getOwner().getHostname()),
_communityRegexes.stream()
.map(RegexCommunitySet::new)
.collect(ImmutableSet.toImmutableSet()),
_asPathRegexes);
try {
TransferBDD tbdd = new TransferBDD(g, policy.getOwner(), policy.getStatements());
result = tbdd.compute(ImmutableSet.of()).getReturnValue();
Expand All @@ -474,15 +458,37 @@ private Optional<Result> searchPolicy(RoutingPolicy policy) {
return constraintsToResult(intersection, outputRoute, policy, g);
}

/**
* Search all of the route policies of a particular node for behaviors of interest.
*
* @param node the node
* @param policies all route policies in that node
* @return all results from analyzing those route policies
*/
private Stream<Result> searchPoliciesForNode(String node, Set<RoutingPolicy> policies) {
Graph g =
new Graph(
_batfish,
_batfish.getSnapshot(),
null,
ImmutableSet.of(node),
_communityRegexes.stream()
.map(RegexCommunitySet::new)
.collect(ImmutableSet.toImmutableSet()),
_asPathRegexes);

return policies.stream()
.map(policy -> searchPolicy(policy, g))
.filter(Optional::isPresent)
.map(Optional::get);
}

@Override
public AnswerElement answer(NetworkSnapshot snapshot) {
SpecifierContext context = _batfish.specifierContext(snapshot);
SortedSet<RoutingPolicyId> policies = resolvePolicies(context);
Multiset<Row> rows =
getPolicies(context, policies)
.map(this::searchPolicy)
.filter(Optional::isPresent)
.map(Optional::get)
_nodeSpecifier.resolve(context).stream()
.flatMap(node -> searchPoliciesForNode(node, _policySpecifier.resolve(node, context)))
.map(SearchRoutePoliciesAnswerer::toRow)
.collect(ImmutableMultiset.toImmutableMultiset());

Expand All @@ -491,16 +497,6 @@ public AnswerElement answer(NetworkSnapshot snapshot) {
return answerElement;
}

@Nonnull
private Stream<RoutingPolicy> getPolicies(
SpecifierContext context, SortedSet<RoutingPolicyId> policies) {
Map<String, Configuration> configs = context.getConfigs();
return policies.stream()
.map(
policyId ->
configs.get(policyId.getNode()).getRoutingPolicies().get(policyId.getPolicy()));
}

@Nullable
private static org.batfish.datamodel.questions.BgpRoute toQuestionsBgpRoute(
@Nullable Bgpv4Route dataplaneBgpRoute) {
Expand Down

0 comments on commit d0cbef2

Please sign in to comment.