-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement test server referential integrity #375
Conversation
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 <jcaamano@redhat.com>
Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
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 <jcaamano@redhat.com>
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 <jcaamano@redhat.com>
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 <jcaamano@redhat.com>
Add the isRoot flag to the schema which will be used for referential integrity Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
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 <jcaamano@redhat.com>
Provides coverage for the interaction of reference garbage collection with index constraint checks. Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
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 <jcaamano@redhat.com>
Checked this implementation in our downstream use, providing expected results |
Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <jcaamano@redhat.com>
Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
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 ! |
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