Multiple OSPF processes in VI datamodel #3799
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #3799 +/- ##
============================================
+ Coverage 73.93% 73.96% +0.03%
- Complexity 24121 24149 +28
============================================
Files 2038 2038
Lines 98013 98090 +77
Branches 11754 11758 +4
============================================
+ Hits 72464 72551 +87
+ Misses 20272 20256 -16
- Partials 5277 5283 +6
|
There was a problem hiding this comment.
Reviewed 3 of 29 files at r1, 1 of 8 files at r2.
Reviewable status: 4 of 35 files reviewed, 2 unresolved discussions (waiting on @corinaminer, @haverma, and @progwriter)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Interface.java, line 1247 at r2 (raw file):
@JsonIgnore public OspfArea getOspfArea() {
@Nullable for the return type ?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 96 at r2 (raw file):
private IsisProcess _isisProcess; private SortedSet<KernelRoute> _kernelRoutes; @Nonnull private Map<String, OspfProcess> _ospfProcesses;
The reference type should be SortedMap or NavigableMap. Map is too general, when the content is actually a sorted map. applies to getter also
There was a problem hiding this comment.
Reviewable status: 4 of 35 files reviewed, 2 unresolved discussions (waiting on @haverma and @progwriter)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Interface.java, line 1247 at r2 (raw file):
Previously, haverma (Harsh Verma) wrote…
@Nullablefor the return type ?
Done.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 96 at r2 (raw file):
Previously, haverma (Harsh Verma) wrote…
The reference type should be
SortedMaporNavigableMap.Mapis too general, when the content is actually a sorted map. applies to getter also
Done.
| "ospfDeadInterval" : 0, | ||
| "ospfEnabled" : false, | ||
| "ospfEnabled" : true, |
There was a problem hiding this comment.
Was this change expected ? or the diff is messed up ?
There was a problem hiding this comment.
Reviewed 24 of 29 files at r1, 7 of 8 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @corinaminer and @progwriter)
projects/question/src/main/java/org/batfish/question/ospfinterface/OspfInterfaceConfigurationAnswerer.java, line 155 at r2 (raw file):
private static Row getRow( String nodeName, @Nullable String ospfProcessId,
this annotation is not needed
There was a problem hiding this comment.
Reviewed 25 of 29 files at r1, 6 of 8 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @corinaminer)
projects/batfish/src/main/java/org/batfish/symbolic/Graph.java, line 241 at r3 (raw file):
// TODO Support multiple OSPF processes String exp = conf.getDefaultVrf().getOspfProcesses().values().iterator().next().getExportPolicy();
how is this guaranteed to be deterministic? Just the sorted order of the map keys? (same applies below)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 291 at r3 (raw file):
@JsonProperty(PROP_OSPF_PROCESS) public void setOspfProcess(@Nonnull OspfProcess process) {
These non-null annotations on setters are wrong given that they are also @JsonProperty annotated.
Also, I'm guessing you're keeping this method for json backwards-compatibility, in which case it's best to make it private and document it as such. We likely don't want legit conversion code calling this?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 300 at r3 (raw file):
} public void setOspfProcesses(@Nonnull Stream<OspfProcess> processes) {
that's a strange setter to have.
- why? 2. if you want to keep it
@JsonIgnoreit for clarity, until this class gets a proper jsoncreator.
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/VrfMatchers.java, line 69 at r3 (raw file):
* OSPF processes. * * <p>TODO: Require a process name in order to test more precisely
javadoc todos? (ಥ﹏ಥ)
Make a static function that takes a name and does hasEntry(equalTo(id), submatcher)
There was a problem hiding this comment.
Reviewable status: 17 of 37 files reviewed, 4 unresolved discussions (waiting on @haverma and @progwriter)
projects/batfish/src/main/java/org/batfish/symbolic/Graph.java, line 241 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
how is this guaranteed to be deterministic? Just the sorted order of the map keys? (same applies below)
Yes, is there a better way?
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 291 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
These non-null annotations on setters are wrong given that they are also
@JsonPropertyannotated.Also, I'm guessing you're keeping this method for json backwards-compatibility, in which case it's best to make it private and document it as such. We likely don't want legit conversion code calling this?
Good points. Started a JsonCreator to deal with the nullability and backwards compatibility.
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 300 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
that's a strange setter to have.
- why? 2. if you want to keep it
@JsonIgnoreit for clarity, until this class gets a proper jsoncreator.
- Why not? It's more convenient in several places. If you think it obfuscates the fact that OSPF processes are stored as a map, i can toss it.
- Done.
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/VrfMatchers.java, line 69 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
javadoc todos? (ಥ﹏ಥ)
Make a static function that takes a name and does
hasEntry(equalTo(id), submatcher)
Was going to save for a separate PR, but since you insist, done
projects/question/src/main/java/org/batfish/question/ospfinterface/OspfInterfaceConfigurationAnswerer.java, line 155 at r2 (raw file):
Previously, haverma (Harsh Verma) wrote…
this annotation is not needed
done
tests/parsing-tests/unit-tests-nodes.ref, line 11150 at r3 (raw file):
Previously, haverma (Harsh Verma) wrote…
Was this change expected ? or the diff is messed up ?
certainly expecting changes since the VI model now has ospfProcesses instead of ospfProcess. I skimmed the diff and it looked okay. Did you see something that seemed wrong?
There was a problem hiding this comment.
Reviewable status: 16 of 37 files reviewed, 3 unresolved discussions (waiting on @haverma and @progwriter)
tests/parsing-tests/unit-tests-nodes.ref, line 11150 at r3 (raw file):
Previously, corinaminer (Corina Miner) wrote…
certainly expecting changes since the VI model now has
ospfProcessesinstead ofospfProcess. I skimmed the diff and it looked okay. Did you see something that seemed wrong?
Well, now that this ref is gone, I wouldn't worry about it unless other refs look off
There was a problem hiding this comment.
Reviewed 1 of 4 files at r4, 22 of 30 files at r5, 4 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @progwriter)
There was a problem hiding this comment.
Reviewed 1 of 29 files at r1, 1 of 4 files at r4, 19 of 30 files at r5, 4 of 4 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @corinaminer)
projects/batfish/src/main/java/org/batfish/symbolic/Graph.java, line 241 at r3 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Yes, is there a better way?
Generally I'd try to be explicit as to which representative you're picking and explain the criteria.
Looking around this code, there doesn't seem to be an obvious winner of a criterion, so just clear comments that this will result in a process with lexicographically-lowest process id will likely suffice for now
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 300 at r3 (raw file):
Previously, corinaminer (Corina Miner) wrote…
- Why not? It's more convenient in several places. If you think it obfuscates the fact that OSPF processes are stored as a map, i can toss it.
- Done.
- Fine, but then please add javadoc explaining that a map will be constructed with process ID as the key.
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/VrfTest.java, line 20 at r6 (raw file):
ImmutableSortedMap.of( "ospf", OspfProcess.builder().setProcessId("ospf").setReferenceBandwidth(1d).build())); assertThat(BatfishObjectMapper.clone(v, Vrf.class), equalTo(v));
I do not know if mixing @JsonCreators with @JsonProperty setters is allowed. can you add some other non-empty fields to the VRF to ensure that round-tripping happens correctly?
projects/minesweeper/src/main/java/org/batfish/minesweeper/Graph.java, line 288 at r6 (raw file):
if (proto.isOspf()) { for (OspfProcess ospf : conf.getDefaultVrf().getOspfProcesses().values()) {
this seems inconsistent with behavior elsewhere: you're picking only one export policy and only one router id, but all of a sudden this node originates networks using all the processes? same below for areas.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @progwriter)
projects/batfish/src/main/java/org/batfish/symbolic/Graph.java, line 241 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
Generally I'd try to be explicit as to which representative you're picking and explain the criteria.
Looking around this code, there doesn't seem to be an obvious winner of a criterion, so just clear comments that this will result in a process with lexicographically-lowest process id will likely suffice for now
done, see other comment for details
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Vrf.java, line 300 at r3 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
- Fine, but then please add javadoc explaining that a map will be constructed with process ID as the key.
Done
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/VrfTest.java, line 20 at r6 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
I do not know if mixing
@JsonCreatorswith@JsonPropertysetters is allowed. can you add some other non-empty fields to the VRF to ensure that round-tripping happens correctly?
oh, you're going to love this
projects/minesweeper/src/main/java/org/batfish/minesweeper/Graph.java, line 288 at r6 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
this seems inconsistent with behavior elsewhere: you're picking only one export policy and only one router id, but all of a sudden this node originates networks using all the processes? same below for areas.
Yeah, I did this because it seems like the right behavior for supporting multiple OSPF processes in minesweeper; didn't do it for export policy/router id because the code there isn't easily modifiable to include all OSPF processes.
In retrospect it's probably safer to consistently pretend there's only one OSPF process, and moreover to touch minesweeper as little as possible. I added a getFirstOspfProcess() method with a TODO and an explanation of how the first process is chosen, and basically reverted the rest of this class back to the version in master, with vrf.getOspfProcess() replaced by getFirstOspfProcess(vrf).
There was a problem hiding this comment.
Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corinaminer)
projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/VrfTest.java, line 20 at r6 (raw file):
Previously, corinaminer (Corina Miner) wrote…
oh, you're going to love this
😢
I... will just fix this later
projects/minesweeper/src/main/java/org/batfish/minesweeper/Graph.java, line 288 at r6 (raw file):
Previously, corinaminer (Corina Miner) wrote…
Yeah, I did this because it seems like the right behavior for supporting multiple OSPF processes in minesweeper; didn't do it for export policy/router id because the code there isn't easily modifiable to include all OSPF processes.
In retrospect it's probably safer to consistently pretend there's only one OSPF process, and moreover to touch minesweeper as little as possible. I added a
getFirstOspfProcess()method with a TODO and an explanation of how the first process is chosen, and basically reverted the rest of this class back to the version in master, withvrf.getOspfProcess()replaced bygetFirstOspfProcess(vrf).
Sweet.
May require some follow-up if there are OSPF questions whose answers don't currently include OSPF process ID. Such questions should still work fine, but it may be ambiguous which OSPF process is associated with a given row of an answer.