On Thu, 14 May 2015, David Lamparter wrote:
That's why I said "making out of", it needs to be hooked up with the GR
bits.
Well, if my patches can be NACKed because it doesn't do what that patch
does, I guess I should NACK that other one for not fixing the external
protocol side of things?
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.
Right, you already mentioned that to me and I though I sent a small patch
to address that so that R=1 will only apply to peers read from the config
file. Last weekend? Did you see that?
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.
Right, which is what it does with the last patch.
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.
No, it doesn't do the right thing, no more than mine does. :)
It addresses one aspect of the right thing.
My patch set addresses another aspect.
Each is fixing stuff, neither fixes everything. If one can be NACKed for
not addressing it all, then the other can be too, and all the problems are
left.
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.
Well, as per a previous email in this thread ;), there's currently a race:
* bgpd starts up
a) timer fires b) OPEN is sent to a (startup) GR peer
At present, b can happen after a (e.g. bgpd overloaded, network problems
related to the thing that caused bgpd to be restarted such as surrounding
routers converging), and we can end sending R=0 to a peer we should send
R=1. Sending R=0 means we will not wait, and we may send transient churn
to the remote host (which may be stable and part of a stable segment of
the network) - causing needless churn on that remote host and potentially
needless flapping in the network elsewhere.
Timer-driven changes in behaviour are very often a hack indicating "it was
too much work to make this happen based on something sensible".
E.g., in the case of the current code, something more sensible than some
arbitrary timer would be "No connection has succeeded to that peer", where
"no connection has succeeded" may be "no previous connection, or if there
was it didn't get far enough to exchange all routes - as determined by
receiving EoRs".
Now, with that in place, you could make that event-driven determination
even better. E.g. by making it consider all EoRs - or hooking it into
other code that already tracks all-EoR state. But that's for another
patch. ;)
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.
Right, that should be "to a remote GR peer in the startup config". I can
fix the commit message.
regards,
--
Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A
Fortune:
When you dial a wrong number you never get a busy signal.
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
