<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; color: rgb(0, 0, 0); font-size: 14px; font-family: Calibri, sans-serif; "><div>Hello,</div><span id="OLK_SRC_BODY_SECTION"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; font-size: 14px; font-family: Calibri, sans-serif; color: rgb(0, 0, 0); "><div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">During an effort to improve BGP bring up time, we came across the code that deals with 'conflict resolution' as BGP from both ends of a peering starts to open connections.</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">I tried my best and this is how I interpreted this code:</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">&nbsp;1. Conflict resolution was not implemented completely as per the RFC.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; Because the code in quagga does a transfer from the realpeer(the peer where BGP does connect) right away to the accepted peer, even if the real-peer may not have gone through OpenSent and beyond. To be considered in the conflict_resolution routine.</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">2. Then, because of the behavior caused by (1), looks like there was some observed issue and a workaround fix as noted in the comment preceded by '<span style="color: rgb(255, 0, 0); ">XXX: This is an awful problem'.</span></div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">We are hitting this condition and it seems to delay the bring-up.</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">Given that, we were wondering if we should just fix (1), by letting the FSM for the realpeer proceed until it goes to OpenConfirm and be considered properly in the conflict resolution routine.</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">Before proceeding, we would like to know if there was an effort to fix (1) as per the RFC ?</div></div></div></span><div>And if such efforts were blocked because of a serious code/design limitation in quagga BGP. Code comment does hint a bit about how it may require a significant change in the FSM.</div><span id="OLK_SRC_BODY_SECTION"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; font-size: 14px; font-family: Calibri, sans-serif; color: rgb(0, 0, 0); "><div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">If &nbsp;you happen to have any history or insight about this, please share. So we can avoid making a change that was tried and had hit some show-stopper in the past.</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">Regards,</div><div style="color: rgb(0, 0, 0); ">Vipin</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">bgpd/bgp_packet.c:</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); "><div>static int</div><div>bgp_open_receive (struct peer *peer, bgp_size_t size)</div><div>{</div><div><br></div><div>&#8230;</div><div>&#8230;</div><div><br></div></div><div style="color: rgb(0, 0, 0); "><div>&nbsp;/* Lookup peer from Open packet. */</div><div>&nbsp; if (CHECK_FLAG (peer-&gt;sflags, PEER_STATUS_ACCEPT_PEER))</div><div>&nbsp; &nbsp; {</div><div>&nbsp; &nbsp; &nbsp; int as = 0;</div><div><br></div><div>&nbsp; &nbsp; &nbsp; realpeer = peer_lookup_with_open (&amp;peer-&gt;su, remote_as, &amp;remote_id, &amp;as);</div><div><br></div><div>&#8230;</div><div>&#8230;</div></div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); "><div>&nbsp;/* When collision is detected and this peer is closed. &nbsp;Retrun</div><div>&nbsp; &nbsp; &nbsp;immidiately. */</div><div>&nbsp; ret = bgp_collision_detect (peer, remote_id);</div><div>&nbsp; if (ret &lt; 0)</div><div>&nbsp; &nbsp; return ret;</div><div><br></div><div>&nbsp; /* Hack part. */</div><div>&nbsp; if (CHECK_FLAG (peer-&gt;sflags, PEER_STATUS_ACCEPT_PEER))</div><div>&nbsp; &nbsp; {</div><div>&nbsp; if (realpeer-&gt;status == Established</div><div>&nbsp; &nbsp; &nbsp; &amp;&amp; CHECK_FLAG (realpeer-&gt;sflags, PEER_STATUS_NSF_MODE))</div><div>&nbsp; {</div><div>&nbsp; &nbsp; realpeer-&gt;last_reset = PEER_DOWN_NSF_CLOSE_SESSION;</div><div>&nbsp; &nbsp; SET_FLAG (realpeer-&gt;sflags, PEER_STATUS_NSF_WAIT);</div><div>&nbsp; }</div><div>&nbsp; else if (ret == 0 &amp;&amp; realpeer-&gt;status != Active</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;&amp;&amp; realpeer-&gt;status != OpenSent</div><div>&nbsp; &nbsp; &nbsp;&amp;&amp; realpeer-&gt;status != OpenConfirm</div><div>&nbsp; &nbsp; &nbsp;&amp;&amp; realpeer-&gt;status != Connect)</div><div>&nbsp; {</div></div><div><div style="color: rgb(0, 0, 0); "><br></div><div><font color="#ff0000">&nbsp; &nbsp; /* XXX: This is an awful problem..</font></div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* According to the RFC we should just let this connection (of the</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* accepted 'peer') continue on to Established if the other</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* connection (the 'realpeer' one) is in state Connect, and deal</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* with the more larval FSM as/when it gets far enough to receive</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* an Open. We don't do that though, we instead close the (more</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* developed) accepted connection.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* This means there's a race, which if hit, can loop:</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; FSM for A &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;FSM for B</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp;realpeer &nbsp; &nbsp; accept-peer &nbsp; &nbsp; &nbsp; realpeer &nbsp; &nbsp; accept-peer</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp;Connect &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;Connect</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; Active</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; OpenSent &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;OpenSent</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &lt;arrive here,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; Notify, delete&gt;</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; Idle &nbsp; &nbsp; &nbsp; &nbsp; Active</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; OpenSent &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; OpenSent</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;&lt;arrive here,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;Notify, delete&gt;</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; Idle</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; &lt;wait&gt; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;&lt;wait&gt;</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* &nbsp; Connect &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; Connect</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* If both sides are Quagga, they're almost certain to wait for</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* the same amount of time of course (which doesn't preclude other</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* implementations also waiting for same time). The race is</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* exacerbated by high-latency (in bgpd and/or the network).</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* The reason we do this is because our FSM is tied to our peer</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* structure, which carries our configuration information, etc.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* I.e. we can't let the accepted-peer FSM continue on as it is,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* cause it's not associated with any actual peer configuration -</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* it's just a dummy.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* It's possible we could hack-fix this by just bgp_stop'ing the</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* realpeer and continueing on with the 'transfer FSM' below.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* Ideally, we need to seperate FSMs from struct peer.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;* Setting one side to passive avoids the race, as a workaround.</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp;*/</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; if (BGP_DEBUG (events, EVENTS))</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; zlog_debug ("%s peer status is %s close connection",</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; realpeer-&gt;host, LOOKUP (bgp_status_msg,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; realpeer-&gt;status));</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; bgp_notify_send (peer, BGP_NOTIFY_CEASE,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;BGP_NOTIFY_CEASE_CONNECT_REJECT);</div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; return -1;</div><div style="color: rgb(0, 0, 0); ">&nbsp; }</div></div><div style="color: rgb(0, 0, 0); "><br></div><div><div style="color: rgb(0, 0, 0); "><br></div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; if (BGP_DEBUG (events, EVENTS))</div><div style="color: rgb(0, 0, 0); ">&nbsp; zlog_debug ("%s [Event] Transfer accept BGP peer to real (state %s)",</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; &nbsp;peer-&gt;host,</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; &nbsp;LOOKUP (bgp_status_msg, realpeer-&gt;status));</div><div style="color: rgb(0, 0, 0); "><br></div><div>&nbsp; &nbsp; <font color="#ff0000">&nbsp; bgp_stop (realpeer);</font></div><div><font color="#ff0000"><br></font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; /* Transfer file descriptor. */</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; realpeer-&gt;fd = peer-&gt;fd;</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; peer-&gt;fd = -1;</font></div><div><font color="#ff0000"><br></font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; /* Transfer input buffer. */</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; stream_free (realpeer-&gt;ibuf);</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; realpeer-&gt;ibuf = peer-&gt;ibuf;</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; realpeer-&gt;packet_size = peer-&gt;packet_size;</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; peer-&gt;ibuf = NULL;</font></div><div><font color="#ff0000"><br></font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; /* Transfer status. */</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; realpeer-&gt;status = peer-&gt;status;</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; bgp_stop (peer);</font></div><div><font color="#ff0000"><br></font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; /* peer pointer change. Open packet send to neighbor. */</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; peer = realpeer;</font></div><div><font color="#ff0000">&nbsp; &nbsp; &nbsp; bgp_open_send (peer);</font></div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; if (peer-&gt;fd &lt; 0)</div><div style="color: rgb(0, 0, 0); ">&nbsp; {</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; zlog_err ("bgp_open_receive peer's fd is negative value %d",</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; &nbsp; peer-&gt;fd);</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; return -1;</div><div style="color: rgb(0, 0, 0); ">&nbsp; }</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; &nbsp; BGP_READ_ON (peer-&gt;t_read, bgp_read, peer-&gt;fd);</div><div style="color: rgb(0, 0, 0); ">&nbsp; &nbsp; }</div></div></div><div style="color: rgb(0, 0, 0); "><br></div></div></span></body></html>