On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></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>
> On Tue, Jun 18, 2013 at 11:05 AM, Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>> wrote:<br>
><br>
> > On Mon, Jun 17, 2013 at 01:25:01AM -0700, Gurucharan Shetty wrote:<br>
> > > Currently we connect to xapi in case there are multiple<br>
> > > external_ids:xs-network-uuids to get the single bridge id everytime<br>
> > > we have a change in the database for all the interested columns in<br>
> > > ovs-xapi-sync. The xs-network-uuids value can also change whenever<br>
> > > new VLANs are added or deleted, which is a common use case. The<br>
> > > disadvantage with this approach is that we query XAPI more often<br>
> > > and set the bridge-id as "" if we don't get a valid response for<br>
> > > our query. This can take down the logical connectivity for all the<br>
> > > VMs on that xenserver.<br>
> > ><br>
> > > Instead of looking at the PIF records for all the xs-network-uuids,<br>
> > > we can instead just look at the xapi record which has the same bridge<br>
> > > name as the OVS bridge name and then cache its uuid. This value will<br>
> > > hold true till the OVS bridge is recreated in which case we will re-read<br>
> > > the value.<br>
> > ><br>
> > > Signed-off-by: Gurucharan Shetty <<a href="mailto:gshetty@nicira.com">gshetty@nicira.com</a>><br>
> ><br>
> > I think that the tolerance for XAPI failures is incomplete, because we<br>
> > call update_fail_mode(), update_in_band_mgmt(), and get_bridge_id()<br>
> > only once for a bridge, even if XAPI fails to respond on the first<br>
> > attempt.<br>
> ><br>
> Yes. We can make some improvements. Do you mind, if I come up with a<br>
> separate patch<br>
> for this, since the current one talks about caching non nicira-bridge-id.<br>
> (get_bridge_id() gets<br>
> the nicira-bridge-id)<br>
<br>
</div></div>OK.<br>
<div class="im"><br>
> > I am not sure why the set_external_id() call splits bridge_id on ';'.<br>
> > Can bridge_id contain ';' at this point?<br>
> ><br>
> The case wherein bridge-id can have ";" is if nicira-bridge-id has a ";".<br>
> 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>
> > I am not sure why bridge_id and bridge_id_cache are different<br>
> > variables. When do they have different values?<br>
> ><br>
> In case get_single_bridge_id() gets us a "", we don't want to cache it.<br>
> Hence 2 separate variables.<br>
<br>
</div>I see. "" != 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["Bridge"].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("xs-network-uuids")<br>
if xs_network_uuids:<br>
bridge_ids = xs_network_uuids.split(";")<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 "", 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, "bridge-id", bridge_id.split(";")[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>