[ovs-dev] [PATCH v2 0/2] ovsdb-idl: Preserve references for tracked deleted rows.

Dumitru Ceara dceara at redhat.com
Wed Mar 10 21:10:29 UTC 2021


On 3/10/21 9:23 PM, Ben Pfaff wrote:
> On Wed, Mar 10, 2021 at 08:43:40PM +0100, Dumitru Ceara wrote:
>>
>> On 3/10/21 7:46 PM, Ben Pfaff wrote:
>>> On Wed, Mar 03, 2021 at 03:39:27PM +0100, Dumitru Ceara wrote:
>>>> The second patch of the series fixes a problem with the IDL change
>>>> tracking code for deleted records which was causing OVN to crash due to
>>>> strong reference fields in IDL records not being preserved at row
>>>> deletion.
>>>
>>> There are many difficult cases here, both difficult to implement
>>> properly and difficult to conceptualize.  It might be worth spending
>>> some time thinking about whether it is possible and valuable to write a
>>> very general test, one that tries to transition from every possible
>>> state to every other possible state (within a limited but interesting
>>> state space) and verifies that the results are what is expected.  When
>>> I've been able to do this in other contexts, it found lots of bugs that
>>> I didn't expect and it protected against regression.
>>>
>>
>> Thanks for the suggestion.
>>
>> We have a few change tracking related tests in ovsdb-idl.at but, as you
>> said, they only deal with specific cases.  As a matter of fact, I'm adding 4
>> new similar tests for the bugs we identified so far.
>>
>> I can try to also add a single, more general, test to cover all relevant
>> transitions.  Do you have anything specific in mind or would a single,
>> large, OVSDB_CHECK_IDL_TRACK-based test, be good enough?
> 
> I was thinking of something systematic and general-purpose like this:
>          https://benpfaff.org/papers/explode.pdf
> or the code documented here:
>          https://git.savannah.gnu.org/cgit/pspp.git/diff/src/libpspp/model-checker.h?id=5060fdedfe17e843301ac0c738e12488af467378

Oh, I see now.  This is great, thanks for sharing!

> I built something equivalent for OVS at one point but it didn't ever
> make it into the tree; I could probably dig it up if anyone actually
> wanted to use it though.

If it's not too much trouble to dig up, I'd be interested in looking at 
it.  Otherwise the pspp model-checker code and its users are already a 
nice read.

> 
> It's a lot of effort so it is only worth it if the code is hard enough
> to get right.  I do not know whether that is the case here.  I cannot

I have a v3 almost ready that should cover all currently identified cases.

> devote the time to do it myself in this case, so it would only happen if
> someone felt themselves motivated to do it.
> 

As Han mentioned on the review for patch 2/2 of the series, it might be 
worth considering a refactor of the IDL change tracking code in general. 
  If we decide to go that way in the future, then I think the effort to 
add a general purpose test and a second ovsdb-idl checker implementation 
to be used by the test is worth it.



More information about the dev mailing list