bdp oscillation recovery mechanism with tie-breaking fixes #585
Conversation
arifogel
commented
Oct 16, 2017
•
arifogel
commented
- allows convergence in certain cases where no unique fixed point exists
- fixes to BGP tie-breaking
- fix VirtualRouter instantiation arguments in some tests
- redo BGP debugging settings to allow finely-tuned recovery, use sane defaults
arifogel
changed the title
- allows convergence in certain cases where no unique fixed point exists
- clean up bdp debugging settings and references therof - fix switched start/end iterations passed to handleOscillation - add setting to limit max oscillation recovery attempts - re-add BdpOscillationException and associated test - re-add bdp iteration debugging information when exception is thrown - record and dump iteration hash codes for each recovery attempt
arifogel
force-pushed
the
ari-oscillation-repair
branch
from
702b0a9
to
c72c5ad
Compare
arifogel
changed the title
There was a problem hiding this comment.
Some basic thoughts. Suggest @progwriter does another readthrough with an eye towards the tests (which he probably understands better at this junction).
| vr._prevOspfExternalType2Rib = vr._ospfExternalType2Rib; | ||
| vr._ospfExternalType2Rib = new OspfExternalType2Rib(vr); | ||
|
|
||
| vr._prevBgpRib = vr._bgpMultipathRib; |
There was a problem hiding this comment.
should this be named _prevBgpMultipathRib?
| vr._bgpMultipathRib = new BgpMultipathRib(vr); | ||
|
|
||
| vr._prevBgpBestPathRib = vr._bgpBestPathRib; | ||
| vr._prevBgpBestPathRib._prev = null; |
There was a problem hiding this comment.
add comment: BestPathRibs save a pointer to the previous RIB in order to implement oldest-route-wins. Once a RIB moves from the "current" RIB to the "previous" RIB, null out its "previous" RIB to not keep full history of RIBs and enable GC.
Consider whether the nulling logic has a better place (e.g., in the importRib function, .... or somewhere else.)
| vr._ibgpBestPathRib = new BgpBestPathRib(vr, vr._prevIbgpBestPathRib); | ||
| vr.importRib(vr._ibgpBestPathRib, vr._baseIbgpRib); | ||
|
|
||
| vr._prevIbgpRib = vr._ibgpMultipathRib; |
| @@ -309,12 +759,14 @@ private void computeFibs(Map<String, Node> nodes) { | |||
| }); | |||
| } | |||
|
|
|||
| private void computeFixedPoint( | |||
| private boolean computeFixedPoint( | |||
There was a problem hiding this comment.
what's the return value? (javadoc)
| @@ -264,6 +270,43 @@ public boolean computeInterAreaSummaries() { | |||
| return changed; | |||
| } | |||
|
|
|||
| /** | |||
| * Whether or not a given remote neighbor is allowed to advertise to this neighbor this iteration. | |||
| * Lexicographically-lower neighbors are allowed to advertise in odd iterations, while | |||
There was a problem hiding this comment.
you say this comment is wrong. allowed vs priority.
| * @return Whether or not a given remote neighbor is allowed to advertise to this neighbor this | ||
| * iteration. | ||
| */ | ||
| private boolean hasAdvertisementPriority( |
There was a problem hiding this comment.
consider renaming function something like hasAdvPriorityDuringRecovery?
| setDefaultProperty(BfConsts.ARG_BDP_DEBUG_MAX_RECORDED_ITERATIONS, 12); | ||
| setDefaultProperty(BfConsts.ARG_BDP_DEBUG_REPEAT_ITERATIONS, false); | ||
| setDefaultProperty(BfConsts.ARG_BDP_DETAIL, false); | ||
| setDefaultProperty(BfConsts.ARG_BDP_MAX_OSCILLATION_RECOVERY_ATTEMPTS, 1); |
There was a problem hiding this comment.
I would suggest making this default to 0 until we have more experience with recovery. Esp as we think we've fixed existing oscillation bugs in other ways.
| oscillatingPrefixes, | ||
| neighbor, | ||
| remoteBgpNeighbor)) { | ||
| synchronized (markedPrefixes) { |
There was a problem hiding this comment.
was wondering if we should move the synchronization into the markPrefix function.
