[ovs-dev] [PATCH] ovs-xapi-sync: Cache the bridge-id value for non nicira-bridge-id too.

Gurucharan Shetty shettyg at nicira.com
Tue Jun 18 21:24:47 UTC 2013


On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff <blp at 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 <blp at 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 <gshetty at 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 bridge_id is not None:
>                 set_external_id(row, "bridge-id", bridge_id.split(";")[0])
>                 new_bridges[row.name] = bridge_id
>         bridges = new_bridges
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130618/ee3c50d6/attachment-0003.html>


More information about the dev mailing list