Changes for "additive" in "set community" for FRR.

[raveranj]

Fixed compile errors to enable "additive" in "set community" command.

Removed comments from RouteMapSetCommunity.java.

Removed commented code.

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #5327 into master will decrease coverage by 0.02%.
The diff coverage is 92.3%.

@@             Coverage Diff              @@
##             master    #5327      +/-   ##
============================================
- Coverage     73.33%    73.3%   -0.03%     
+ Complexity    32051    32036      -15     
============================================
  Files          2624     2624              
  Lines        129272   129304      +32     
  Branches      15557    15562       +5     
============================================
- Hits          94796    94787       -9     
- Misses        26953    26989      +36     
- Partials       7523     7528       +5

Impacted Files	Coverage Δ	Complexity Δ
...ar/cumulus_frr/CumulusFrrConfigurationBuilder.java	77.48% <100%> (+0.48%)	113 <0> (+2)	⬆️
...h/representation/cumulus/RouteMapSetCommunity.java	75% <90.9%> (ø)	5 <4> (+2)	⬆️
...tamodel/routing_policy/statement/SetCommunity.java	51.85% <0%> (-7.41%)	7% <0%> (-1%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java	53.33% <0%> (-6.67%)	5% <0%> (-1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java	59.55% <0%> (-4.5%)	13% <0%> (-2%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java	87.09% <0%> (-3.23%)	11% <0%> (-2%)
...org/batfish/datamodel/flow/BidirectionalTrace.java	81.81% <0%> (-2.28%)	14% <0%> (-1%)
...a/org/batfish/representation/cisco/BgpProcess.java	73.73% <0%> (-2.03%)	32% <0%> (-1%)
...org/batfish/representation/cisco/BgpPeerGroup.java	86.53% <0%> (-1.93%)	100% <0%> (-3%)
...batfish/representation/cisco/CiscoConversions.java	87.26% <0%> (-0.84%)	173% <0%> (-4%)
... and 9 more

[arifogel]

Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @raveranj)

a discussion (no related file):
The correct implementation of set community ... additive is non-trivial, and unfortunately not achieved here.

Fortunately, there are working examples that can be followed for other vendors.

See org.batfish.representation.cisco_nxos.RouteMapSetCommunity, and follow its usage throughout the project.

You can contact me directly on Batfish slack with any questions.

---

---

projects/batfish/src/main/java/org/batfish/grammar/cumulus_frr/CumulusFrrConfigurationBuilder.java, line 866 at r2 (raw file):

  public void exitRms_community(Rms_communityContext ctx) {
    RouteMapSetCommunity old = _currentRouteMapEntry.getSetCommunity();
    if (ctx.ADDITIVE() == null) {

The only thing that should happen here when additive appears on a set community line is that the RouteMapSetCommunity that gets created should be marked as additive. You should not modify the existing RouteMapSetCommunity - rather, it should be replaced entirely.

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetCommunity.java, line 21 at r2 (raw file):

  private boolean _isAdditive;

  public void set_isAdditive() {

nit: we use slouching camel case for property setters and getters.
Suggest: _additive, getAdditive, setAdditive.

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetCommunity.java, line 51 at r2 (raw file):

    ArrayList<StandardCommunity> commList =
        communities.stream().collect(Collectors.toCollection(ArrayList::new));
    if (get_isAdditive()) {

The behavior you have implemented here does not reflect the behavior of a Cumulus device when a set community xx:yy additive command is entered,
Whenever a set community line appears in a route-map, it replaces any existing one.

The meaning of additive is that the communities mentioned should be added to the communities of a route to be processed, rather than replacing the communities of the route to be processed.
This function should not be altered.

[arifogel]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @raveranj)

a discussion (no related file):
Need to add a test case to org.batfish.representation.cumiulus.RouteMapSetCommunityTest

---

[raveranj]

---

projects/batfish/src/main/java/org/batfish/representation/cumulus/RouteMapSetCommunity.java, line 51 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

The behavior you have implemented here does not reflect the behavior of a Cumulus device when a set community xx:yy additive command is entered,
Whenever a set community line appears in a route-map, it replaces any existing one.

The meaning of additive is that the communities mentioned should be added to the communities of a route to be processed, rather than replacing the communities of the route to be processed.
This function should not be altered.

Sync-ed up with Ari offline. Fixing up the behavior. Thanks Ari!

[dhalperi]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @raveranj)

a discussion (no related file):
Hi @raveranj - need anything to keep going here?

---

[raveranj]

Hey @dhalperi - Should have an updated PR today (latest tomorrow).

[arifogel]

Reviewed 2 of 6 files at r3, 5 of 5 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @raveranj)

[arifogel]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved