-
Notifications
You must be signed in to change notification settings - Fork 102
[sql-33] session: add migration code from kvdb to SQL #1051
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
[sql-33] session: add migration code from kvdb to SQL #1051
Conversation
449a2cc
to
1828ec4
Compare
1828ec4
to
a868a93
Compare
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.
Adding some review comments where input would be extra helpful :)!
session/sql_store.go
Outdated
macRecipe = &MacaroonRecipe{} | ||
if perms != nil { | ||
macRecipe.Permissions = unmarshalMacPerms(perms) | ||
} | ||
if caveats != nil { | ||
macRecipe.Caveats = unmarshalMacCaveats(caveats) |
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.
Let me know if you think we should rather fix the kvdb store implementation rather than the sql one. But as we've previously opted for not updating the store behaviour, and make the sql one mimic the behaviour until we've completed the migration, I opted for this version here.
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.
Alternatively, I could also override this in newly added overrideMacaroonRecipe
function before the migration deep equals check. But I kept this as is, as I do think it's correct behaviour that we don't set the Permissions
or Caveats
and keep them at nil
, if no value actually exists for them.
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 think in this case it is fine to fix on the kvdb side so that our sql side can stay neat?
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.
also commit message says fix kvdb&sql but this only touches sql
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.
Please see #1051 (comment)
session/sql_migration.go
Outdated
if !session.RevokedAt.IsZero() { | ||
err = tx.SetSessionRevokedAt( | ||
ctx, | ||
makeSetRevokedAtParams(sqlId, session.RevokedAt), | ||
) | ||
if err != nil { | ||
return err | ||
} | ||
} |
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.
Could potentially update the insert session sql query to also take an optional RevokedAt sql.NullTime
param, to not have to do this as a separate step. I opted for this version to keep things as is as much as possible, but if you think that'd be better, I'll update to do that instead :)
session/sql_migration_test.go
Outdated
{ | ||
"randomized sessions", | ||
randomizedSessions, | ||
}, |
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.
Opted for a test that uses the "standard" rand
lib here, as @bitromortac had a preference for that in #1047 (comment). But can also look into adding a test that uses rapid
if you'd prefer.
// We always have a caveat.Id, but the rest are | ||
// randomized if they exist or not. | ||
macCaveat.Id = randomBytes(rand.Intn(10) + 1) |
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.
Let me know if this is not true in for real world data, as migrations WILL fail if we don't set a caveat.Id
when the caveats
param of WithMacaroonRecipe
is not nil
.
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.
Could potentially override this as well in the new overrideMacaroonRecipe
as well. But I do not think this scenario can happen for real world data. But open to implementing that as well if you think it's worth doing it just to be sure.
session/sql_migration_test.go
Outdated
// "dest" state, without checking if the state transition is legal. | ||
// | ||
// NOTE: this function should only be used for testing purposes. | ||
func (db *BoltStore) shiftStateUnsafe(_ context.Context, id ID, |
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.
Not fully sure if we add test functions like this one that's a struct
member to the _test
file, or if we add it in the actual struct file?
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.
right, not too great, you could change to func shiftStateUnsafe(_ context.Context, db *BoltStore, id ID, dest State) error {
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.
Great work, very clean on first pass! 🔥
session/sql_migration.go
Outdated
// The remote public key is currently only set for autopilot sessions, | ||
// else it's an empty byte array. |
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.
nit: I think it should read else it's a nil byte array
, since we compare to nil in the code in other sites, which is also correctly set above with the var
statement
session/sql_migration_test.go
Outdated
require.NoError(t, err) | ||
|
||
t.Cleanup(func() { | ||
require.NoError(t, kvStore.DB.Close()) |
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.
tiny nit: the embedded DB can be removed
session/sql_migration_test.go
Outdated
// numberOfSessions is set to 100 to add enough sessions to get | ||
// enough variation between number of invoices and payments, but | ||
// kept low enough for the test not take too long to run, as the | ||
// test time increases drastically by the number of sessions we | ||
// migrate. |
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 doc mentions invoice and payments, which is probably a copy/paste
session/sql_migration_test.go
Outdated
// "dest" state, without checking if the state transition is legal. | ||
// | ||
// NOTE: this function should only be used for testing purposes. | ||
func (db *BoltStore) shiftStateUnsafe(_ context.Context, id ID, |
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.
right, not too great, you could change to func shiftStateUnsafe(_ context.Context, db *BoltStore, id ID, dest State) error {
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.
holding off on review here until the prep PR discussed in the accounts mig PR is in (since that will be used to test this one)
Now that #1047 has been merged, I'll update this PR to adhere to the feedback brought up in that PR, and then re-request reviews afterwards. |
@ViktorTigerstrom, remember to re-request review from reviewers when ready |
a868a93
to
4b3b39f
Compare
Updated the PR to also address feedback raised in #1047! Will address the small linting issue after next review round :) |
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.
LGTM, utACK 🎉
a48e9b2
to
8c970bf
Compare
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 a lot for the review @bitromortac 🎉!! I noticed that we can have real world data that sets a non-nil macaroon recipe, but with nil
perms
& caveats
. I updated the code to be able to handle that case as well.
}, | ||
}, | ||
{ | ||
name: "macaroon recipe with nil perms and caveats", |
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.
Note that this test is new, as this can actually happen for real world sessions.
session/sql_store.go
Outdated
macRecipe = &MacaroonRecipe{} | ||
if perms != nil { | ||
macRecipe.Permissions = unmarshalMacPerms(perms) | ||
} | ||
if caveats != nil { | ||
macRecipe.Caveats = unmarshalMacCaveats(caveats) |
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.
Alternatively, I could also override this in newly added overrideMacaroonRecipe
function before the migration deep equals check. But I kept this as is, as I do think it's correct behaviour that we don't set the Permissions
or Caveats
and keep them at nil
, if no value actually exists for them.
// We always have a caveat.Id, but the rest are | ||
// randomized if they exist or not. | ||
macCaveat.Id = randomBytes(rand.Intn(10) + 1) |
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.
Could potentially override this as well in the new overrideMacaroonRecipe
as well. But I do not think this scenario can happen for real world data. But open to implementing that as well if you think it's worth doing it just to be sure.
8c970bf
to
46873cb
Compare
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.
looking good!
session/sql_store.go
Outdated
macRecipe = &MacaroonRecipe{} | ||
if perms != nil { | ||
macRecipe.Permissions = unmarshalMacPerms(perms) | ||
} | ||
if caveats != nil { | ||
macRecipe.Caveats = unmarshalMacCaveats(caveats) |
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 think in this case it is fine to fix on the kvdb side so that our sql side can stay neat?
session/sql_store.go
Outdated
macRecipe = &MacaroonRecipe{} | ||
if perms != nil { | ||
macRecipe.Permissions = unmarshalMacPerms(perms) | ||
} | ||
if caveats != nil { | ||
macRecipe.Caveats = unmarshalMacCaveats(caveats) |
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.
also commit message says fix kvdb&sql but this only touches sql
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
interesting - are we not able to rely on the fact that sessions are stored in order in kvdb? ie, if we get a session with a group ID set, that will always either point to the session at hand or a session we've already handled?
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.
ie, i think we can just:
for s := sessions {
if s.groupID = s.ID {
<insert new session. no coupled group>
continue
}
groupID := db.GetGroupIDByAlias(s.groupID)
<insert new session. couple to group>
}
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.
or did you actually run into a scenario where these were not in order?
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.
Hmmm so the listSessions
for the kvdb returns the sessions sorted by the CreatedAt
field. So given that this field actually contains timestamps in the correct order, your suggestion should work. But I see 2 potential scenarios where that wouldn't be true:
- The user has managed to change the used time, so that linked sessions gets a
CreatedAt
timestamp prior to the initial session they are linking to. - Even though it's a real edge case, I believe that when we changed to using
clock.Clock
forlitd
, that we realised that this could have the side effect that the new timestamps would potentially get changed by +- a few hours unless I'm remembering this incorrectly? In that scenario, for some edge case users, they could have created the initial session before updating to the newlitd
version that added theclock.Clock
implementation, and then linked a session to it right after which could then have an earlierCreatedAt
timestamp than the initial session.
As I don't see any real cons with my current approach, other than it adding some more code complexity, I think it's a worth tradeoff than risking the migrations failing for such edge cases. Do you agree?
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.
Hmmm so the listSessions for the kvdb returns the sessions sorted by the CreatedAt field
I'd opt for not using public methods like ListAllSessions for the migration. Rather let the migration manually get to the buckets/sub-buckets that it needs. We want to be able to change how ListAllSessions works without affecting the migration.
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.
notice that the LND invoice migration does the same: takes a raw kvdb.Backend and *sqlc.Queries
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.
im also more in favour of doing it this way cause then the IDs that the sessions have will be more intuitive as then (as long as the user didnt do anything strange) , the created at times will mostly be increasing at the IDs increase
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.
Cool thanks! I updated this PR to instead pass a *bbolt.DB
and directly iterate over the buckets, rather than using the public functions of the BoltStore
. I've done the same for the accounts migration as well now with:
#1084
I've also looked through the previous migrations of the sessions kvdb as discussed offline, and none of them should be able to effect the order of the sessions entires in the kvdb store. However as also discussed offline, I didn't update the logic in the migrations, so that non-linked sessions are still migrated prior to linked sessions. I'm open to changing this though if you believe it's worth updating.
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.
However as also discussed offline, I didn't update the logic in the migrations, so that non-linked sessions are still migrated prior to linked sessions. I'm open to changing this though if you believe it's worth updating.
ah, i thought the offline conclusion was the opposite 😅 cause i think it is just nice to have that the order in the DB is (at least on a best effort basis) the same as the order of creation.
Not completely blocking but i'd defs prefer it.
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.
Sorry I should have been aware of this earlier! Since we insert the sessions in the kvdb store like this into the bucket:
if err := SerializeSession(&buf, session); err != nil {
return err
}
return bucket.Put(getSessionKey(session), buf.Bytes())
Where the bucket is the bucket for the sessionBucketKey
and the result of the getSessionKey
will be session.LocalPublicKey.SerializeCompressed()
, the order the sessions when iterating over the sessionBucketKey
will actually be the lexicographical order of the serialized LocalPublicKey
, and not in insertion order of the sessions.
Therefore there's no way to iterate over them in "insertion order", unless you know some way I'm unaware of :)? The closest we can get is to do what the kvdb store listSessions
function does, which sorts the sessions by the CreatedAt
timestamp, but that has the issue of what I brought up earlier with: #1051 (comment)
Therefore I think our best option that doesn't over complicate the code is to keep the migration code as what I've implemented today, i.e. first insert non-linked sessions, followed by inserting linked sessions?
session/sql_migration.go
Outdated
session.AccountID.WhenSome(func(alias accounts.AccountID) { | ||
// Check that the account exists in the SQL store, before |
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.
technically what we are doing here is just getting the account DB level ID.
we dont actually need to "check before link" since SQL does that for us.
session/sql_migration.go
Outdated
return | ||
} | ||
|
||
acctID = sql.NullInt64{ |
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.
sqldb.SQLInt64(accDBID)
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, addressed these and others 🙏!
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, addressed these and others 🙏!
session/sql_migration.go
Outdated
privacyFlagParams := sqlc.InsertSessionPrivacyFlagParams{ | ||
SessionID: sqlId, | ||
Flag: int32(privacyFlag), | ||
} | ||
|
||
err = tx.InsertSessionPrivacyFlag(ctx, privacyFlagParams) | ||
if err != nil { | ||
return err |
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.
this and many of the above can just be in-lined instead of having separate param vars
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.
Addressed this and others :)
46873cb
to
56f52c7
Compare
I added a commit 6b8c58f at the end of the PR to address the feedback of #1051 (comment), as well as a fixup commit I will apply if you think the approach of added commit is good @ellemouton. Would therefore appreciate feedback on if this approach is what you had in mind when you suggested that I should change the kvdb implementation? Note that this actually changes how the Note that I will fix the failing unit test tomorrow, that's due to my update of how kvdb implementation stores the |
c7901f0
to
eabf4de
Compare
Updated the PR by ensuring that the @ellemouton, please let me know if you prefer that approach, or the other approach I've also implemented that changes how the kvdb persists the |
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.
shweet! a few more comments.
Agreed with you that the override macaroon recipe approach is good 👍 i think i didnt understand correctly before that for bbolt we could still have the TLV actually be written with out any contents (right?) so yes, the overwrite makes sense rather than the "un-set"
session/interface.go
Outdated
// Close closes the underlying store. | ||
Close() error | ||
|
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 think you can drop this commit.
The interface should not expose details about the underlying store if possible. This exposes that the underlying store is something that needs to be closed. It also gives things we pass the interface to access to this Close method which we dont want.
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.
ah, i think you're doing it cause of test cleanup - rather just call t.Cleanup sooner?
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.
Cool thanks for the idea 🙏! Updated with a commit that calls t.Cleanup
sooner instead.
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
Hmmm so the listSessions for the kvdb returns the sessions sorted by the CreatedAt field
I'd opt for not using public methods like ListAllSessions for the migration. Rather let the migration manually get to the buckets/sub-buckets that it needs. We want to be able to change how ListAllSessions works without affecting the migration.
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
it adding some more code complexity
yeah i think it's simpler and we have to worry about fewer things if we just directly iterate over the buckets.
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
now looking back, i think we should also change the accounts migration to not use the public Accounts method. Rather iterate directly over buckets.
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
in other words, rather than passing in a BoltStore, just pass in a *bbolt.DB instead (ie, the equivalent to SQLQueries)
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
notice that the LND invoice migration does the same: takes a raw kvdb.Backend and *sqlc.Queries
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
im also more in favour of doing it this way cause then the IDs that the sessions have will be more intuitive as then (as long as the user didnt do anything strange) , the created at times will mostly be increasing at the IDs increase
eabf4de
to
313cce1
Compare
Thanks for the latest review @ellemouton 🎉! Addressed your feedback with the latest push. Added a new PR to address #1051 (comment), and rebased this PR on that though. This PR should be reviewable (but not mergeable) prior to that PR though, so just skip reviewing the first two commits when reviewing this. |
Also, re-requested a review from @bitromortac, as quite much has changed since your last review. Hope that's ok! |
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.
lgtm!
great work
accounts/sql_migration_test.go
Outdated
@@ -39,9 +39,6 @@ func TestAccountStoreMigration(t *testing.T) { | |||
*db.TransactionExecutor[SQLQueries]) { |
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.
fwiw for future, I dont think these 2 account commits needed to be in this PR as i dont think there is a dependency here. These can be reviewed and merged in parallel in whichever order
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.
Did it that way so this wouldn't get merged without closing the stores in the tests, but I agree that it wouldn't be strictly necessary for merging :)
// If sessions are linked to a group, we must insert the initial session | ||
// of each group before the other sessions in that group. This ensures | ||
// we can retrieve the SQL group ID when inserting the remaining | ||
// sessions. Therefore, we first insert all initial group sessions, |
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.
However as also discussed offline, I didn't update the logic in the migrations, so that non-linked sessions are still migrated prior to linked sessions. I'm open to changing this though if you believe it's worth updating.
ah, i thought the offline conclusion was the opposite 😅 cause i think it is just nice to have that the order in the DB is (at least on a best effort basis) the same as the order of creation.
Not completely blocking but i'd defs prefer it.
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.
Nice, LGTM 🎉, just a question and a nit.
In preparation for upcoming migration tests from a kvdb to an SQL store, this commit updates the NewTestDB function to return the Store interface rather than a concrete store implementation. This change ensures that migration tests can call NewTestDB under any build tag while receiving a consistent return type.
We add a helper function to the functions that creates the test SQL stores, in order to ensure that the store is properly closed when the test is cleaned up.
This commit introduces the migration logic for transitioning the sessions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
cb67dca
to
2007d53
Compare
Thanks for the reviews 🎉⚡! |
This PR introduces the migration logic for transitioning the sessions store from kvdb to SQL.
Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.
Part of #917