[quagga-dev 12294] Re: [PATCH 1/2] bgpd: GR Restart-bit should be stateful rather than timer from startup

David Lamparter david at opensourcerouting.org
Thu May 14 17:20:03 BST 2015


On Thu, May 14, 2015 at 04:47:37PM +0100, Paul Jakma wrote:
> On Thu, 14 May 2015, David Lamparter wrote:
> > Indeed, I'd appreciate if we could work on making full GR out of
> > https://github.com/CumulusNetworks/quagga/commit/2ca83e1825158650546b82791834b87c2bb829fa
> > instead of doing this ;)
> 
> Hmm, do you perhaps mean this one?:
> 
> https://github.com/CumulusNetworks/quagga/commit/3e2a4c3513b1c896f92085bbb8f2bfb60c16f1e2

Yes, sorry, wrong link.

> That patch is somewhat orthogonal to what this patch-set is doing. The GR 
> patches I posted are fixing external-protocol related things. The cumulus 
> update-delay patch doesn't touch that at all, its addressing internal 
> issues.

That's why I said "making out of", it needs to be hooked up with the GR
bits.

> > This should indeed fix the issue I pointed out on IRC, however this 
> > leaves another issue (if I'm brain-executing the code correctly):
> >
> > If we start up with N peers (i.e. PEER_STATUS_GR_SEND_R_BIT), and one of 
> > them is down/unreachable/plain doesn't exist anymore, we will forever 
> > stay in GR=1 status, since bgp_update_all_eor_recvd includes that peer 
> > and thus will never be true.
> 
> Only for that peer. The flag is peer-specific, it can't affect any other 
> peer. bgp_update_all_eor_recvd touches only peer-specific state.

[...]

> > [Unless I misread the code and above description doesn't match the code:
> > NAK on all patches in this thread.]
> 
> I think you've misread the code maybe?

Ah, yes, I misread the loop in bgp_update_all_eor_recvd.  For some
reason I thought it was walking all peers.

That does leave a different situation where this code doesn't do the
right thing.  If we have N peers started up correctly, a few weeks pass,
then another peer restarts after having been offline.  That peer
implements GR.  Your patch will send R=1, which means that other peer
won't wait for us to send our entire table.

This will result in additional churn, especially if that peer only has
other Quaggas as peers, all sending R=1.

The correct behaviour is to send R=0.  The R bit is tied to "do we have
a stable operational table", which is global state.  I guess I'm saying
the bit shouldn't be per-peer after all.

I agree that the fixed timer from startup is not the best thing to do.
This is why I'm suggesting we pick up the update-delay patch you posted;
the logic added by that patch does the right thing.


-David


P.S.: you're operating with the theory "GR Restart-bit should be
stateful rather than timer from startup".  I don't think I agree with
that.  I do think we could start the timer later (at first session-up),
but I'm missing a rationale for pulling this apart further into the
per-peer state machine you're suggesting.  I would really like to see an
argument for that.

Also, your patch description says "The GR restart bit should be set on
the OPEN of at least the first connection to a remote GR peer since
startup."  That is not correct at least in the situation I described
above where our table is settled & stable.




More information about the Quagga-dev mailing list