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

[Mark Michelson]

Thank you very much, Ben!

On 06/25/2018 04:22 PM, Ben Pfaff wrote:
> Done.
> 
> On Mon, Jun 25, 2018 at 08:08:02AM -0400, Mark Michelson wrote:
>> 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.
>>>
>>