<br><br><div class="gmail_quote">On Tue, Jun 18, 2013 at 2:35 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br><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 02:24:47PM -0700, Gurucharan Shetty wrote:<br>
> On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>> wrote:<br>
><br>
> > 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<br>
> > 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>
> > OK.<br>
> ><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>
> > I guess that it is existing code, so it is better not to change it,<br>
> > especially in an unrelated patch.<br>
> ><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>
> > 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>
> ><br>
> > new_bridges = {}<br>
> > for row in idl.tables["Bridge"].rows.itervalues():<br>
> > 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 =<br>
> > 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>
> ><br>
> So in this case, we will not be setting the external_ids:bridge_id as "",<br>
> if get_single_bridge_id() does not get correct records from xapi and we<br>
> will simply retain the old value in the database. I guess, this was not<br>
> your intent?<br>
<br>
</div></div>If our connection to XAPI is malfunctioning in some way, then I don't<br>
know whether it is better to clear the database fields or retain them.<br>
If you think that it is better to clear them, then I think something<br>
very similar would work, maybe like this:<br>
<div class="im"><br>
new_bridges = {}<br>
for row in idl.tables["Bridge"].rows.itervalues():<br>
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>
</div> set_external_id(row, "bridge-id", bridge_id)<br>
<div class="im"><br>
if bridge_id is not None:<br>
</div><div class="im"> new_bridges[<a href="http://row.name" target="_blank">row.name</a>] = bridge_id<br>
bridges = new_bridges<br>
<br>
</div>That is, move the set_external_id call out of the final "if" (and get<br>
rid of the 'split' because one cannot split None).<br></blockquote><div>Okay. I am fine with this. Should I apply the original patch and you will send a code re-org patch as a separate one? Or should I do that?</div>
<div><br></div><div>Thanks,</div><div>Gruu </div></div><br>