[ovs-discuss] OpenStack profiling with networking-ovn - port creation is slow

[Ben Pfaff]

On Thu, Feb 15, 2018 at 01:38:04PM -0800, Han Zhou wrote:
> On Thu, Feb 15, 2018 at 12:56 PM, Ben Pfaff <blp at ovn.org> wrote:
> >
> > On Thu, Feb 15, 2018 at 06:50:15PM +0000, Lucas Alvares Gomes wrote:
> > > Hi all,
> > >
> > > We currently have a problem with Address_Set in networking-ovn (and
> > > potentially other technologies using OVN as backend) which *maybe*
> > > could be fixed together with this idea of a new "port set" (a.k.a
> > > macro set).
> > >
> > > The problem is bit tricky but it shows as a race condition between
> > > creating and deleting ports in Neutron. That is, deleting a port in
> > > Neutron will result in the IP address of that port to be removed from
> > > an address set, but at the same time, if another request to create a
> > > port overtaking the same IP address of the port being deleted is
> > > issued, depending on which order things are committed it could result
> > > in the address set to not contain the new port IP address.
> > >
> > > Here's a bug ticket describing the problem in more detail:
> > > https://bugs.launchpad.net/networking-ovn/+bug/1611852
> > >
> > > The reason why it happens is because we don't have a direct
> > > relationship between the addresses in an address set and the ports
> > > that those addresses belongs to. So, if we create this relation we
> > > might end up having both ports and addresses present in the
> > > Address_Set table which then can be used to fix both of our problems.
> >
> > It seems very odd to me that Neutron can commit things out of order.  If
> > the OVSDB schema for Address_Set were slightly different, this would
> > even be a constraint violation that would cause the create operation to
> > fail if it were executed before the delete operation.  I wouldn't be
> > surprised if other operations could be reordered in a way that would
> > cause such a failure.  I'll be disappointed if we solve this specific
> > problem and then multiple other examples of the same general problem
> > appear.
> >
> > Here is the best idea that has come to my mind so far: use key-value
> > pairs in the "external-ids" column to indicate which port owns which
> > address.  At time of port insertion, Neutron add the key; at time of
> > port deletion it only removes an address if the deleted port owned it.
> > (This is doable in an atomic fashion with the existing OVSDB protocol,
> > or at least it is possible to abort the transaction if the deleted port
> > does not own it.)
> >
> The external-ids approach would require going through the whole
> external-ids list to see if there is any other owners for the same address
> when we are trying to delete one, which may be inefficient. Also it seems
> to introduce too much redundant data, since we are repeating the addresses
> in external-ids.

I don't think that this argument about inefficiency is likely to be
true; it's simply an in-memory search.  I find the latter argument more
persuasive, since redundancy often indicates weakness in a design.

> > > Here's some ideas:
> > >
> > > # 1. A new map column
> > >
> > > We could add a "map" type column called "ports" in the Address_Set
> > > table that would look like this in the database:
> > >
> > > "Address_Set": {
> > >   "ports": {"port-1-name": {"ipv4_addresses": [...],
> > >                                      "ipv6_addresses": [...}}
> > >   ...
> > > }
> >
> > This particular solution seems to me like it solves a very specific
> > problem.  I'd rather solve a more general problem if we can.
> >
> > > # 2: Add a new way to populate the address set:
> > >
> > > Instead of directly populating the addresses in an address set where
> > > the port relationship isn't clear, we could add two list of ports
> > > references (one for each IP version) and have the addresses
> > > automatically populated.
> > >
> > > For example:
> > >
> > > "Address_Set": {
> > >   "columns": {
> > >     "ipv4_ports": {"type": {"key": {"type": "uuid",
> > >                                     "refTable": "Logical_Switch_Port",
> > >                                     "refType": "weak"},
> > >                             "min": 0,
> > >                             "max": "unlimited"}}
> > >     "ipv6_ports": {"type": {"key": {"type": "uuid",
> > >                                     "refTable": "Logical_Switch_Port",
> > >                                     "refType": "weak"},
> > >                             "min": 0,
> > >                             "max": "unlimited"}}
> > >      ...
> > > }
> > >
> > > The problem here is that we would pull all addresses from those ports
> > > into the address set.
> > >
> > > The good part is that since it's a weak reference, deleting a port
> > > would automatically remove it from the address set.
> >
> > This is creative and I appreciate it.  It also seems very specific to
> > this particular problem.
> >
> #1 and #2 make the assumption that all addresses in a set belong to a
> lport, which is not always true (it may be true in current Neutron,
> though). We should not prevent user from using address set without creating
> lport for the addresses.

Yes, I agree.

> > > # 3: Allow duplicated addresses in the list
> > >
> > > If the above options sounds too complicated, maybe we could keep the
> > > idea of this email of creating a "Macro_Set" that could be used for
> > > both ports and addresses types [0]. But, when the type is set to
> > > "address" we could allow duplicated items in the list of elements that
> > > way we won't have a problem if one transaction removes one duplicated
> > > element in the list.
> > >
> > > [0]
> https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/077118.html
> >
> > I am closer to being comfortable with this solution.  The specific
> > solution cannot work because OVSDB doesn't support duplicates in its
> > "set" types.
> >
> > We could extend the current Address_Set to resemble this solution by
> > removing the constraint that the address set's name be unique and making
> > ovn-northd and ovn-controller merge addresses from rows with duplicate
> > names.  Neutron could use the "external-ids" column to track which port
> > owns which row.
> 
> This sounds better to me, too. Would this mean we actually use each row to
> store only one address, but it will just be backward compatible to support
> multiple addresses in a row?

I guess that would be the common case, yes.

> Another way is to change the addresses column to map instead of set. The
> key is address, and value is a reference counter to the address. By default
> the counter is 1, but if there are N owners for the same address the
> counter will be N. How does this sound?

I guess I like a reference list better than a reference count for this
application, because for debugging and troubleshooting a reference count
is opaque and difficult to verify.

This change would break backward compatibility.