Implement test server referential integrity #375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An optional atomic value is cleared with an empty set instead of a nil set. Thus use an empty set as an original empty value so that when an op that sets a value is merged with an op that clears the value, the comparison with an empty orignal value results in a noop. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
The number of results of a transaction should always have a minimum length of the number of operations passed to it even if there is an error. The recent introduction of the named UUID expansion in the transaction broke this requirement. Changed the error handling approach so that this is less likely to happen in the future. Also removed the panic when initializing the transaction cache. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
The main purpose of this change is to have a package for common database types and interfaces that can be used throughout the project without incurring in circular dependencies. To that end, the in-memory database implementation is moved to its own subpackage. The transaction implementation is also moved to its own subpackage given that it can work with any database implementing the Database interface. At the interface level though it should be expected that a transaction implementation could have a more tighter underlying coupling with a database implementation. So it makes sense to introduce a transaction interface, and for transactions to be provided by database implementations rather than being built directly by the server. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Once we introduce referential integrity, existing tests would start to fail as they are not accounting for it. So its better to remove the references from the test schema which is something that current tests are not leveraging, and then expand the schema with other references that the referenctial integrity tests would leverage. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Add the isRoot flag to the schema which will be used for referential integrity Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Implement referential integrity and garbage collection as described in RFC7047. This is achieved keeping track of inverse references. A reference tracker is used in the transaction to keep these references updated, to check for referential integrity related violations and to add additional updates to the transaction resulting from reference garbage collection. The inverse references are stored in the database on commit. The updates resulting from reference garbage collection are also sent to any monitoring clients as expected. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Provides coverage for the interaction of reference garbage collection with index constraint checks. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
While these tests could have been purely server tests, setting them up as integration tests against both the test server and the real OVSDB server allows us to contrast the implemented behavior. Also provides coverage for how the references are updated in the database as well as for how referential integrity updates are sent back to the client. Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
|
Checked this implementation in our downstream use, providing expected results |
Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
dave-tucker
approved these changes
Jan 18, 2024
There was a problem hiding this comment.
Thanks for this @jcaamano. The code is well written, well documented and easy to understand. I've not been deeply involved in libovsdb for a while but all of these changes look good to me and I appreciate that you've provided tests 🎉 and gone as far as testing this downstream in ovn-k8s! Overall, great work and LGTM 😎
Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
Signed-off-by: Jaime Caamaño Ruiz <[email protected]>
|
I also took a look at the PR. I'm not very familiar with the server part so I might miss something, but I did not spot any obvious error and was able to follow the implementation pretty well. Thanks @jcaamano |
|
Thanks for the reviews @dave-tucker @amorenoz ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement referential integrity and garbage collection as described in
RFC7047.
This is achieved keeping track of inverse references. A reference
tracker is used in the transaction to keep these references updated, to
check for referential integrity related violations and to add additional
updates to the transaction resulting from reference garbage collection.
The inverse references are stored in the database on commit.
The updates resulting from reference garbage collection are also sent to
any monitoring clients as expected.
The database package was split so that new types could be introduced
without incurring in circular dependencies.
Fixed a couple of issues found along the way.
As it usually happens, a good portion of the code is just testing.
This should only have code changes impacting the test server, no impacts
to the client.
Fixes: #219
Partially Fixes: #338