On Tue, Jun 18, 2013 at 02:45:46PM -0700, Gurucharan Shetty wrote:
> On Tue, Jun 18, 2013 at 2:35 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > On Tue, Jun 18, 2013 at 02:24:47PM -0700, Gurucharan Shetty wrote:
> > > On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff <b...@nicira.com> wrote:
> > >
> > > > On Tue, Jun 18, 2013 at 01:30:28PM -0700, Gurucharan Shetty wrote:
> > > > > On Tue, Jun 18, 2013 at 11:05 AM, Ben Pfaff <b...@nicira.com> wrote:
> > > > >
> > > > > > On Mon, Jun 17, 2013 at 01:25:01AM -0700, Gurucharan Shetty wrote:
> > > > > > > Currently we connect to xapi in case there are multiple
> > > > > > > external_ids:xs-network-uuids to get the single bridge id
> > everytime
> > > > > > > we have a change in the database for all the interested columns
> > in
> > > > > > > ovs-xapi-sync. The xs-network-uuids value can also change
> > whenever
> > > > > > > new VLANs are added or deleted, which is a common use case. The
> > > > > > > disadvantage with this approach is that we query XAPI more often
> > > > > > > and set the bridge-id as "" if we don't get a valid response for
> > > > > > > our query. This can take down the logical connectivity for all
> > the
> > > > > > > VMs on that xenserver.
> > > > > > >
> > > > > > > Instead of looking at the PIF records for all the
> > xs-network-uuids,
> > > > > > > we can instead just look at the xapi record which has the same
> > bridge
> > > > > > > name as the OVS bridge name and then cache its uuid. This value
> > will
> > > > > > > hold true till the OVS bridge is recreated in which case we will
> > > > re-read
> > > > > > > the value.
> > > > > > >
> > > > > > > Signed-off-by: Gurucharan Shetty <gshe...@nicira.com>
> > > > > >
> > > > > > I think that the tolerance for XAPI failures is incomplete,
> > because we
> > > > > > call update_fail_mode(), update_in_band_mgmt(), and get_bridge_id()
> > > > > > only once for a bridge, even if XAPI fails to respond on the first
> > > > > > attempt.
> > > > > >
> > > > > Yes. We can make some improvements. Do you mind, if I come up with a
> > > > > separate patch
> > > > > for this, since the current one talks about caching non
> > nicira-bridge-id.
> > > > > (get_bridge_id() gets
> > > > > the nicira-bridge-id)
> > > >
> > > > OK.
> > > >
> > > > > > I am not sure why the set_external_id() call splits bridge_id on
> > ';'.
> > > > > > Can bridge_id contain ';' at this point?
> > > > > >
> > > > > The case wherein bridge-id can have ";" is if nicira-bridge-id has a
> > ";".
> > > > > If you feel, that is not a valid use case, I can get rid of it.
> > > >
> > > > I guess that it is existing code, so it is better not to change it,
> > > > especially in an unrelated patch.
> > > >
> > > > > > I am not sure why bridge_id and bridge_id_cache are different
> > > > > > variables. When do they have different values?
> > > > > >
> > > > > In case get_single_bridge_id() gets us a "", we don't want to cache
> > it.
> > > > > Hence 2 separate variables.
> > > >
> > > > I see. "" != None even though Python treats both as false.
> > > >
> > > > This code is really confusing. Every time I look at it, I get more
> > > > confused.
> > > >
> > > > I am probably doing something stupid here again, but how about this
> > > > version:
> > > >
> > > > new_bridges = {}
> > > > for row in idl.tables["Bridge"].rows.itervalues():
> > > > bridge_id = bridges.get(row.name)
> > > > if bridge_id is None:
> > > > # Configure the new bridge.
> > > > update_fail_mode(row)
> > > > update_in_band_mgmt(row)
> > > >
> > > > # Get the correct bridge_id, if we can.
> > > > bridge_id = get_bridge_id(row.name)
> > > > if bridge_id is None:
> > > > xs_network_uuids =
> > > > row.external_ids.get("xs-network-uuids")
> > > > if xs_network_uuids:
> > > > bridge_ids = xs_network_uuids.split(";")
> > > > if len(bridge_ids) == 1:
> > > > bridge_id = bridge_ids[0]
> > > > else:
> > > > bridge_id =
> > get_single_bridge_id(bridge_ids,
> > > > row.name)
> > > >
> > > 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?
> >
> > If our connection to XAPI is malfunctioning in some way, then I don't
> > know whether it is better to clear the database fields or retain them.
> > If you think that it is better to clear them, then I think something
> > very similar would work, maybe like this:
> >
> > new_bridges = {}
> > for row in idl.tables["Bridge"].rows.itervalues():
> > bridge_id = bridges.get(row.name)
> > if bridge_id is None:
> > # Configure the new bridge.
> > update_fail_mode(row)
> > update_in_band_mgmt(row)
> >
> > # Get the correct bridge_id, if we can.
> > bridge_id = get_bridge_id(row.name)
> > if bridge_id is None:
> > xs_network_uuids =
> > row.external_ids.get("xs-network-uuids")
> > if xs_network_uuids:
> > bridge_ids = xs_network_uuids.split(";")
> > if len(bridge_ids) == 1:
> > bridge_id = bridge_ids[0]
> > else:
> > bridge_id = get_single_bridge_id(bridge_ids,
> > row.name)
> > set_external_id(row, "bridge-id", bridge_id)
> >
> > if bridge_id is not None:
> > new_bridges[row.name] = bridge_id
> > bridges = new_bridges
> >
> > That is, move the set_external_id call out of the final "if" (and get
> > rid of the 'split' because one cannot split None).
> >
> 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?
Whichever you prefer is fine.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
