[ovs-dev] [PATCH] ovsdb-idl: Correct singleton insert logic

[Mark Michelson]

Hi Ben,

This fixes an issue that is being experienced by users in version 2.9 as 
well. Can this please be backported there as well?

Thanks,
Mark Michelson

On 06/15/2018 07:27 PM, Ben Pfaff wrote:
> On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote:
>> Hi Ben,
>>
>> Thanks for having a look and providing an update. See my replies in-line
>> below.
>>
>> On 05/25/2018 06:31 PM, Ben Pfaff wrote:
>>> On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote:
>>>> When inserting data into a "singleton" table (one that has maxRows ==
>>>> 1), there is a check that ensures that the table is currently empty
>>>> before inserting the row. The intention is to prevent races where
>>>> multiple clients might attempt to insert rows at the same time.
>>>>
>>>> The problem is that this singleton check can cause legitimate
>>>> transactions to fail. Specifically, a transaction that attempts to
>>>> delete the current content of the table and insert new data will cause
>>>> the singleton check to fail since the table currently has data.
>>>>
>>>> This patch corrects the issue by keeping a count of the rows being
>>>> deleted and added to singleton tables. If the total is larger than zero,
>>>> then the net operation is attempting to insert rows. If the total is
>>>> less than zero, then the net operation is attempting to remove rows. If
>>>> the total is zero, then the operation is inserting and deleting an equal
>>>> number of rows (or is just updating rows). We only add the singleton
>>>> check if the total is larger than zero.
>>>>
>>>> This patch also includes a new test for singleton tables that ensures
>>>> that the maxRows constraint works as expected.
>>>>
>>>> Signed-off-by: Mark Michelson <mmichels at redhat.com>
>>>
>>> Good catch.  (It's kind of weird to delete and repopulate a singleton
>>> table, but we should support it.)
>>
>> The use case that brought this to our attention was the "set-ssl" commands
>> for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these
>> operations delete the current row and repopulate with a new row. The
>> existing check resulted in failure when when the transaction should have
>> been allowed.
>>
>>>
>>> I think that the following patch achieves the same end with less
>>> bookkeeping overhead.  What do you think?  It doesn't break anything in
>>> the testsuite, but I didn't check that it actually achieves the purpose.
>>>
>>
>> I did some testing and debugging and this looks like it has the expected
>> behavior, and I agree that this seems like a simpler approach. So
>>
>> Acked-by: Mark Michelson <mmichels at redhat.com>
> 
> Thanks.  I applied this to master.
>