<div dir="ltr"><div><div><br><br>On Thu, Feb 15, 2018 at 12:56 PM, Ben Pfaff &lt;<a href="mailto:blp@ovn.org">blp@ovn.org</a>&gt; wrote:<br>&gt;<br>&gt; On Thu, Feb 15, 2018 at 06:50:15PM +0000, Lucas Alvares Gomes wrote:<br>&gt; &gt; Hi all,<br>&gt; &gt;<br>&gt; &gt; We currently have a problem with Address_Set in networking-ovn (and<br>&gt; &gt; potentially other technologies using OVN as backend) which *maybe*<br>&gt; &gt; could be fixed together with this idea of a new &quot;port set&quot; (a.k.a<br>&gt; &gt; macro set).<br>&gt; &gt;<br>&gt; &gt; The problem is bit tricky but it shows as a race condition between<br>&gt; &gt; creating and deleting ports in Neutron. That is, deleting a port in<br>&gt; &gt; Neutron will result in the IP address of that port to be removed from<br>&gt; &gt; an address set, but at the same time, if another request to create a<br>&gt; &gt; port overtaking the same IP address of the port being deleted is<br>&gt; &gt; issued, depending on which order things are committed it could result<br>&gt; &gt; in the address set to not contain the new port IP address.<br>&gt; &gt;<br>&gt; &gt; Here&#39;s a bug ticket describing the problem in more detail:<br>&gt; &gt; <a href="https://bugs.launchpad.net/networking-ovn/+bug/1611852">https://bugs.launchpad.net/networking-ovn/+bug/1611852</a><br>&gt; &gt;<br>&gt; &gt; The reason why it happens is because we don&#39;t have a direct<br>&gt; &gt; relationship between the addresses in an address set and the ports<br>&gt; &gt; that those addresses belongs to. So, if we create this relation we<br>&gt; &gt; might end up having both ports and addresses present in the<br>&gt; &gt; Address_Set table which then can be used to fix both of our problems.<br>&gt;<br>&gt; It seems very odd to me that Neutron can commit things out of order.  If<br>&gt; the OVSDB schema for Address_Set were slightly different, this would<br>&gt; even be a constraint violation that would cause the create operation to<br>&gt; fail if it were executed before the delete operation.  I wouldn&#39;t be<br>&gt; surprised if other operations could be reordered in a way that would<br>&gt; cause such a failure.  I&#39;ll be disappointed if we solve this specific<br>&gt; problem and then multiple other examples of the same general problem<br>&gt; appear.<br>&gt;<br>&gt; Here is the best idea that has come to my mind so far: use key-value<br>&gt; pairs in the &quot;external-ids&quot; column to indicate which port owns which<br>&gt; address.  At time of port insertion, Neutron add the key; at time of<br>&gt; port deletion it only removes an address if the deleted port owned it.<br>&gt; (This is doable in an atomic fashion with the existing OVSDB protocol,<br>&gt; or at least it is possible to abort the transaction if the deleted port<br>&gt; does not own it.)<br>&gt;<br></div><div>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.<br></div><div><br>&gt; &gt; Here&#39;s some ideas:<br>&gt; &gt;<br>&gt; &gt; # 1. A new map column<br>&gt; &gt;<br>&gt; &gt; We could add a &quot;map&quot; type column called &quot;ports&quot; in the Address_Set<br>&gt; &gt; table that would look like this in the database:<br>&gt; &gt;<br>&gt; &gt; &quot;Address_Set&quot;: {<br>&gt; &gt;   &quot;ports&quot;: {&quot;port-1-name&quot;: {&quot;ipv4_addresses&quot;: [...],<br>&gt; &gt;                                      &quot;ipv6_addresses&quot;: [...}}<br>&gt; &gt;   ...<br>&gt; &gt; }<br>&gt;<br>&gt; This particular solution seems to me like it solves a very specific<br>&gt; problem.  I&#39;d rather solve a more general problem if we can.<br>&gt;<br>&gt; &gt; # 2: Add a new way to populate the address set:<br>&gt; &gt;<br>&gt; &gt; Instead of directly populating the addresses in an address set where<br>&gt; &gt; the port relationship isn&#39;t clear, we could add two list of ports<br>&gt; &gt; references (one for each IP version) and have the addresses<br>&gt; &gt; automatically populated.<br>&gt; &gt;<br>&gt; &gt; For example:<br>&gt; &gt;<br>&gt; &gt; &quot;Address_Set&quot;: {<br>&gt; &gt;   &quot;columns&quot;: {<br>&gt; &gt;     &quot;ipv4_ports&quot;: {&quot;type&quot;: {&quot;key&quot;: {&quot;type&quot;: &quot;uuid&quot;,<br>&gt; &gt;                                     &quot;refTable&quot;: &quot;Logical_Switch_Port&quot;,<br>&gt; &gt;                                     &quot;refType&quot;: &quot;weak&quot;},<br>&gt; &gt;                             &quot;min&quot;: 0,<br>&gt; &gt;                             &quot;max&quot;: &quot;unlimited&quot;}}<br>&gt; &gt;     &quot;ipv6_ports&quot;: {&quot;type&quot;: {&quot;key&quot;: {&quot;type&quot;: &quot;uuid&quot;,<br>&gt; &gt;                                     &quot;refTable&quot;: &quot;Logical_Switch_Port&quot;,<br>&gt; &gt;                                     &quot;refType&quot;: &quot;weak&quot;},<br>&gt; &gt;                             &quot;min&quot;: 0,<br>&gt; &gt;                             &quot;max&quot;: &quot;unlimited&quot;}}<br>&gt; &gt;      ...<br>&gt; &gt; }<br>&gt; &gt;<br>&gt; &gt; The problem here is that we would pull all addresses from those ports<br>&gt; &gt; into the address set.<br>&gt; &gt;<br>&gt; &gt; The good part is that since it&#39;s a weak reference, deleting a port<br>&gt; &gt; would automatically remove it from the address set.<br>&gt;<br>&gt; This is creative and I appreciate it.  It also seems very specific to<br>&gt; this particular problem.<br>&gt;<br>#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.<br><br>&gt; &gt; # 3: Allow duplicated addresses in the list<br>&gt; &gt;<br>&gt; &gt; If the above options sounds too complicated, maybe we could keep the<br>&gt; &gt; idea of this email of creating a &quot;Macro_Set&quot; that could be used for<br>&gt; &gt; both ports and addresses types [0]. But, when the type is set to<br>&gt; &gt; &quot;address&quot; we could allow duplicated items in the list of elements that<br>&gt; &gt; way we won&#39;t have a problem if one transaction removes one duplicated<br>&gt; &gt; element in the list.<br>&gt; &gt;<br>&gt; &gt; [0] <a href="https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/077118.html">https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/077118.html</a><br>&gt;<br>&gt; I am closer to being comfortable with this solution.  The specific<br>&gt; solution cannot work because OVSDB doesn&#39;t support duplicates in its<br>&gt; &quot;set&quot; types.<br>&gt;<br>&gt; We could extend the current Address_Set to resemble this solution by<br>&gt; removing the constraint that the address set&#39;s name be unique and making<br>&gt; ovn-northd and ovn-controller merge addresses from rows with duplicate<br>&gt; names.  Neutron could use the &quot;external-ids&quot; column to track which port<br>&gt; owns which row.<br><br></div>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?<br></div>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?<br><div><br><div><br></div></div></div>