-
Notifications
You must be signed in to change notification settings - Fork 64
Add CRDB schema changes and queries required for trust quorum #9486
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm going to continue iterating on this and adding in db-queries to use this SQL. But please take a look at the SQL. |
davepacheco
left a comment
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.
The SQL changes look good to me!
|
There are a few things remaining on this PR that I wanted to document here before I wrap up this work:
Furthermore, this PR contains DB migrations from LRTQ to trust-quorum. It's likely that we'll want to separate those migrations and related code into a separate PR that we only merge once the rest of the TQ and LRTQ upgrade code is ready. This way if we ship a release before TQ and LRTQ upgrade are ready we won't have stale data around in the DB that may not correspond to existing sleds we want to migrate. |
5af5d07 to
40dcef9
Compare
|
This PR has significantly changed since I opened it originally. The SQL has been simplified and a set of DB queries have been added to utilize the SQL. It's expected that new configurations will be added and aborted via an end-user API and then background tasks will read the configurations and drive them to completion. That will come in follow up PRs. I believe this is now ready for review. |
|
Of note to reviewers, this PR also collapses the duplicate |
|
After I started using this in a background task, I realized that it's missing a query. Namely, it's missing the ability to retrieve the latest configuration for all racks. I also realized, more importantly for this PR, that not having an efficient mechanism in the database to know when Nexus is no longer required to operate on the trust quorum makes everything more expensive. For instance, in the common case, all the latest epochs would always have to be retrieved and the filtering of the state of each member as committed would have to be done inside the background task. Therefore, once Nexus has seen acked commits for all members it should change from the I'm open to other alternatives. |
|
Discussed in chat, but I'm going to remove the LRTQ table altogether and make the upgrade from LRTQ an operator operation because it can fail. |
Relatedly, I"m going to add a state for this operation as well to |
jgallagher
left a comment
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.
I haven't been following the TQ work very closely, so all my comments are basically nits on naming / clarity. I'd feel more comfortable if someone with more familiarity approved too, but I'll give it my stamp from a purely "reviewing the Rust/SQL implementation" point of view.
We don't want these until the rest of the LRTQ migration code is in. Otherwise we can end up with stale data. A separate PR will be opened to include this later.
* Remove lrtq-members * Add new states Queries and tests were updated to reflect these changes More tests TBD
651c2dd to
9847448
Compare
EDIT: I added a bunch of usage for the new SQL. It should be fairly complete.
I tested the migrations in CRDB SQL shell. This structure matches the property based tests and I think is all that is required for Trust Quorum. It may require small tweaks as I build out the implementation. I can either make those tweaks in a follow up migration or keep this open and push it in with some nexus changes. Either is fine with me.
I'm mainly opening this to get eyes on it since I rarely touch SQL.