Feature/frr Support set metric +/- in Cumulus route-maps.

[raveranj]

No description provided.

[batfish-bot]

This change is 

[arifogel]

Reviewed 6 of 11 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @raveranj)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrr_routemap.g4, line 11 at r2 (raw file):

int_expr
:
   (

nit: Unnecessary outer parens

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 188 at r2 (raw file):

;

DASH

Please move DASH and PLUS to complex tokens section

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 367 at r2 (raw file):

            .setNetwork(Prefix.parse("10.20.30.0/31"))
            .setTag(0L)
            .setMetric(1L)

I'd recommend starting with a number greater than 1. Otherwise you cannot tell if underflow correction is always applied, or just when result is less than 0.
Of course you'll have to update expectations below.

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 385 at r2 (raw file):

    assertThat(outputRoute2.getMetric(), equalTo(2L));
    assertThat(outputRoute3.getMetric(), equalTo(0L));
    assertThat(outputRoute4.getMetric(), equalTo(4294967295L));

More legible and harder to make a typo: 0xFFFFFFFFL

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/DecrementMetric.java, line 43 at r2 (raw file):

  public long evaluate(Environment environment) {
    long oldMetric = environment.getOriginalRoute().getMetric();
    long newVal = oldMetric - _subtrahend;

No need to reimplement max:
newVal = Math.max(environment.getOriginalRoute().getMetric() - _subtrahend, 0L)

(Also applies for overflow logic)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/IncrementMetric.java, line 15 at r2 (raw file):

public final class IncrementMetric extends LongExpr {
  private static final String PROP_ADDEND = "addend";
  private static final long MAX_INT_VALUE = (long) Math.pow(2, 32) - 1;

Just inline the result: 0xFFFFFFFFL

[raveranj]

Reviewable status: 6 of 12 files reviewed, 5 unresolved discussions (waiting on @arifogel and @raveranj)

---

projects/batfish/src/main/antlr4/org/batfish/grammar/cumulus_frr/CumulusFrrLexer.g4, line 188 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please move DASH and PLUS to complex tokens section

Done.

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 385 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

More legible and harder to make a typo: 0xFFFFFFFFL

Done.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/DecrementMetric.java, line 43 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

No need to reimplement max:
newVal = Math.max(environment.getOriginalRoute().getMetric() - _subtrahend, 0L)

(Also applies for overflow logic)

Done.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/routing_policy/expr/IncrementMetric.java, line 15 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Just inline the result: 0xFFFFFFFFL

Done.

[arifogel]

Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raveranj)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@e95ef11). Click here to learn what that means.
The diff coverage is 82.92%.

@@            Coverage Diff            @@
##             master    #5441   +/-   ##
=========================================
  Coverage          ?   73.44%           
  Complexity        ?    32052           
=========================================
  Files             ?     2620           
  Lines             ?   128972           
  Branches          ?    15532           
=========================================
  Hits              ?    94722           
  Misses            ?    26714           
  Partials          ?     7536

Impacted Files	Coverage Δ	Complexity Δ
...tfish/representation/cisco/CiscoConfiguration.java	85.49% <ø> (ø)	524 <0> (?)
...va/org/batfish/datamodel/IpWildcardSetIpSpace.java	85% <ø> (ø)	14 <0> (?)
...fish/representation/cumulus/RouteMapSetMetric.java	100% <ø> (ø)	3 <0> (?)
...rc/main/java/org/batfish/datamodel/AclIpSpace.java	85.08% <ø> (ø)	42 <0> (?)
...batfish/representation/cisco/CiscoConversions.java	87.23% <ø> (ø)	173 <0> (?)
...g/batfish/datamodel/acl/OriginatingFromDevice.java	100% <ø> (ø)	6 <0> (?)
...src/main/java/org/batfish/datamodel/IpIpSpace.java	88.88% <ø> (ø)	6 <0> (?)
.../java/org/batfish/datamodel/IpWildcardIpSpace.java	88.88% <ø> (ø)	6 <0> (?)
.../question/searchfilters/SearchFiltersQuestion.java	100% <ø> (ø)	25 <0> (?)
...n/java/org/batfish/datamodel/acl/NotMatchExpr.java	100% <ø> (ø)	7 <0> (?)
... and 111 more

[raveranj]

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 367 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

I'd recommend starting with a number greater than 1. Otherwise you cannot tell if underflow correction is always applied, or just when result is less than 0.
Of course you'll have to update expectations below.

I replaced the underflow check in DecrementMetric.java with Math.max(), which will always trigger the underflow check.

[arifogel]

Reviewable status: complete! all files reviewed, all discussions resolved

---

projects/batfish/src/test/java/org/batfish/grammar/cumulus_concatenated/CumulusConcatenatedGrammarTest.java, line 367 at r2 (raw file):

Previously, raveranj wrote…

I replaced the underflow check in DecrementMetric.java with Math.max(), which will always trigger the underflow check.

I fully believe that your code is correct. I'm simply pointing out that since there is no difference between the input/output pairs for RM_METRIC_MINUS and RM_METRIC_UNDERFLOW given your input metric of 1, this test in isolation does not prove that you are testing two different behaviors (subtraction with/without underflow).

In general, a test should be persuasive without having to look at the implementation of the function being tested.

My complaint is minor given that the implementation is simple and I have reviewed it in full.

You can fix as I described, or merge as is.

[arifogel]

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