On Thu, May 14, 2015 at 06:06:34PM +0100, Paul Jakma wrote: > 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?
I'm continuing to discuss your patch because I'm under the impression it makes things worse and doesn't work towards an actual GR implementation... > > 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 is for a peer configured on startup. > > 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. Again, this is for a peer configured on startup. It'll send R=1. Even if 2 weeks have passed, and we have a stable RIB. > > 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. I'm not sure it does. > 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", The R bit is not "no connection has succeeded to that peer". The R bit is "we don't have a stable RIB yet." If your argument is that the determination "we have a stable RIB" should be better than "timer expired", by all means implement something else... but splitting up the R-bit to be per-peer is not helpful in this regard. > 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. ;) None of this is an argument for making the state non-global. Why is "do we have a stable RIB?" a per-peer consideration? > > 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. Nope. My previous mail is about a GR peer in the startup config that isn't available for some time (for some external reason). -David _______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
