Improve deduplication of syntactic differences across devices for ComparePeerGroupPolicies

[nickgian]

This change groups together syntactic differences (current snapshot policy, reference snapshot policy) from different devices allowing us to reduce the number of calls to CRP.

Syntactic differences across devices are considered the same if the definitions (lists) of the two devices are equal, the policies in current-snapshot are syntactically identical, and the policies in the reference-snapshot are syntactically identical.

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #8884 (de5ff89) into master (d9b6865) will decrease coverage by 0.01%.
Report is 25 commits behind head on master.
The diff coverage is 71.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8884      +/-   ##
==========================================
- Coverage   72.51%   72.51%   -0.01%     
==========================================
  Files        3319     3319              
  Lines      169433   169467      +34     
  Branches    19878    19890      +12     
==========================================
+ Hits       122861   122881      +20     
- Misses      37426    37430       +4     
- Partials     9146     9156      +10     

Files	Coverage Δ
.../comparepeergrouppolicies/SyntacticDifference.java	96.87% <95.83%> (+5.96%)	⬆️
...arepeergrouppolicies/RoutingPolicyContextDiff.java	82.45% <28.57%> (-17.55%)	⬇️

... and 12 files with indirect coverage changes

[millstein]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nickgian)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifference.java line 95 at r1 (raw file):

   *     contexts. If all of these are equal, the two differences are equal. Otherwise, we simply
   *     order the differences by name and hostname. Note that we consider the full routing context
   *     of the device, even if some definitions are not used in this routing policy. This might

It seems like you already have a method in RoutingPolicyContextDiff (called differ) that can scope the difference to a particular routing policy, so why not use that to make this comparison more precise?

---

projects/minesweeper/src/test/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifferenceTest.java line 31 at r1 (raw file):

import projects.minesweeper.src.main.java.org.batfish.minesweeper.question.comparepeergrouppolicies.SyntacticDifference;

public class SyntacticDifferenceTest {

A few other tests seem useful: 1) testing that routingPolicySyntaxEq properly tests equality of all the callees (finding when they are equal and also when they are not); 2) testing that compareTo orders the policies in the right way when it does not return 0.

[nickgian]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @millstein)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifference.java line 95 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

It seems like you already have a method in RoutingPolicyContextDiff (called differ) that can scope the difference to a particular routing policy, so why not use that to make this comparison more precise?

We probably can. It felt a bit more work at first, and potentially a bit expensive computationally as you have to construct a SyntacticCompare object for every comparison (which involves, walking the routing policy and tracking the definitions it uses).
I think I should be able to prototype this quickly, so let me give it a shot and report back.

---

projects/minesweeper/src/test/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifferenceTest.java line 31 at r1 (raw file):

Previously, millstein (Todd Millstein) wrote…

A few other tests seem useful: 1) testing that routingPolicySyntaxEq properly tests equality of all the callees (finding when they are equal and also when they are not); 2) testing that compareTo orders the policies in the right way when it does not return 0.

Added tests for 1, not sure about 2, we don't really have any ordering in mind, other than that they should not be considered equal. I suppose, I can strengthen a test that shows lexicographical ordering of policy-name/hostname "wins".

[nickgian]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @millstein)

---

projects/minesweeper/src/main/java/org/batfish/minesweeper/question/comparepeergrouppolicies/SyntacticDifference.java line 95 at r1 (raw file):

Previously, nickgian (Nick Giannarakis) wrote…

We probably can. It felt a bit more work at first, and potentially a bit expensive computationally as you have to construct a SyntacticCompare object for every comparison (which involves, walking the routing policy and tracking the definitions it uses).
I think I should be able to prototype this quickly, so let me give it a shot and report back.

actually, the problem is SyntacticCompare works on top of configurations that concern the same device. We would have to redesign this class to apply stripStatements to every routing policy it touches. It's not infeasible, but the benefits to doing this refactoring in an efficient way seem small to me at this point. I suggest we go ahead with this slightly imprecise solution and refactor if performance becomes an issue in the future.

[millstein]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgian)