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

Ben Pfaff blp at ovn.org
Fri Jun 15 23:27:29 UTC 2018


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.


More information about the dev mailing list