On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Tue, Jun 18, 2013 at 01:30:28PM -0700, Gurucharan Shetty wrote:<br>
&gt; On Tue, Jun 18, 2013 at 11:05 AM, Ben Pfaff &lt;<a href="mailto:blp@nicira.com">blp@nicira.com</a>&gt; wrote:<br>
&gt;<br>
&gt; &gt; On Mon, Jun 17, 2013 at 01:25:01AM -0700, Gurucharan Shetty wrote:<br>
&gt; &gt; &gt; Currently we connect to xapi in case there are multiple<br>
&gt; &gt; &gt; external_ids:xs-network-uuids to get the single bridge id everytime<br>
&gt; &gt; &gt; we have a change in the database for all the interested columns in<br>
&gt; &gt; &gt; ovs-xapi-sync. The xs-network-uuids value can also change whenever<br>
&gt; &gt; &gt; new VLANs are added or deleted, which is a common use case. The<br>
&gt; &gt; &gt; disadvantage with this approach is that we query XAPI more often<br>
&gt; &gt; &gt; and set the bridge-id as &quot;&quot; if we don&#39;t get a valid response for<br>
&gt; &gt; &gt; our query. This can take down the logical connectivity for all the<br>
&gt; &gt; &gt; VMs on that xenserver.<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; Instead of looking at the PIF records for all the xs-network-uuids,<br>
&gt; &gt; &gt; we can instead just look at the xapi record which has the same bridge<br>
&gt; &gt; &gt; name as the OVS bridge name and then cache its uuid. This value will<br>
&gt; &gt; &gt; hold true till the OVS bridge is recreated in which case we will re-read<br>
&gt; &gt; &gt; the value.<br>
&gt; &gt; &gt;<br>
&gt; &gt; &gt; Signed-off-by: Gurucharan Shetty &lt;<a href="mailto:gshetty@nicira.com">gshetty@nicira.com</a>&gt;<br>
&gt; &gt;<br>
&gt; &gt; I think that the tolerance for XAPI failures is incomplete, because we<br>
&gt; &gt; call update_fail_mode(), update_in_band_mgmt(), and get_bridge_id()<br>
&gt; &gt; only once for a bridge, even if XAPI fails to respond on the first<br>
&gt; &gt; attempt.<br>
&gt; &gt;<br>
&gt; Yes. We can make some improvements. Do you mind, if I come up with a<br>
&gt; separate patch<br>
&gt; for this, since the current one talks about caching non nicira-bridge-id.<br>
&gt; (get_bridge_id() gets<br>
&gt; the nicira-bridge-id)<br>
<br>
</div></div>OK.<br>
<div class="im"><br>
&gt; &gt; I am not sure why the set_external_id() call splits bridge_id on &#39;;&#39;.<br>
&gt; &gt; Can bridge_id contain &#39;;&#39; at this point?<br>
&gt; &gt;<br>
&gt; The case wherein bridge-id can have &quot;;&quot; is if nicira-bridge-id has a &quot;;&quot;.<br>
&gt; If you feel, that is not a valid use case, I can get rid of it.<br>
<br>
</div>I guess that it is existing code, so it is better not to change it,<br>
especially in an unrelated patch.<br>
<div class="im"><br>
&gt; &gt; I am not sure why bridge_id and bridge_id_cache are different<br>
&gt; &gt; variables.  When do they have different values?<br>
&gt; &gt;<br>
&gt; In case get_single_bridge_id() gets us a &quot;&quot;, we don&#39;t want to cache it.<br>
&gt; Hence 2 separate variables.<br>
<br>
</div>I see.  &quot;&quot; != None even though Python treats both as false.<br>
<br>
This code is really confusing.  Every time I look at it, I get more<br>
confused.<br>
<br>
I am probably doing something stupid here again, but how about this<br>
version:<br>
<div class="im"><br>
        new_bridges = {}<br>
        for row in idl.tables[&quot;Bridge&quot;].rows.itervalues():<br>
</div>            bridge_id = bridges.get(<a href="http://row.name" target="_blank">row.name</a>)<br>
            if bridge_id is None:<br>
                # Configure the new bridge.<br>
                update_fail_mode(row)<br>
                update_in_band_mgmt(row)<br>
<br>
                # Get the correct bridge_id, if we can.<br>
                bridge_id = get_bridge_id(<a href="http://row.name" target="_blank">row.name</a>)<br>
                if bridge_id is None:<br>
                    xs_network_uuids = row.external_ids.get(&quot;xs-network-uuids&quot;)<br>
                    if xs_network_uuids:<br>
                        bridge_ids = xs_network_uuids.split(&quot;;&quot;)<br>
                        if len(bridge_ids) == 1:<br>
                            bridge_id = bridge_ids[0]<br>
                        else:<br>
                            bridge_id = get_single_bridge_id(bridge_ids,<br>
                                                             <a href="http://row.name" target="_blank">row.name</a>)<br></blockquote><div>So in this case, we will not be setting the external_ids:bridge_id as &quot;&quot;, if get_single_bridge_id() does not get correct records from xapi and we will simply retain the old value in the database. I guess, this was not your intent? </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
            if bridge_id is not None:<br>
                set_external_id(row, &quot;bridge-id&quot;, bridge_id.split(&quot;;&quot;)[0])<br>
</div>                new_bridges[<a href="http://row.name" target="_blank">row.name</a>] = bridge_id<br>
        bridges = new_bridges<br>
</blockquote></div><br>