Skip to content
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

feat(tenants): support for tenant storage migration #1747

Merged

Conversation

TimoGlastra
Copy link
Contributor

This PR adds support for storage migration on in the @credo-ts/tenants package. It adds checks to ensure an outdated tenants record is not used, and it also adds methods to update tenant storage. If autoUpdateStorageOnStartup is enabled it will automatically update a tenant when it is first opened (not on agent startup, but on tenant startup) to not have to update all tenants on startup.

You an also query all outdated tenants, as well as update each tenant manually. From 0.6 onwards I'd like to make all breaking chagnes to storage last 2 versions, so the framework code for e.g. 0.6 can handle storage version for 0.5 and 0.6. This means you can progressively update storage and even use the tenants still while you update the storage. As now you have to go offline for a bit (remove all 0.4 agents, then spin up one 0.5 agent and update the storage), while with the other approach you can keep the 0.4 running, add in one 0.5 agent, and then scale down the 0.4 agents.

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and clear! Just some typing issues preventing the CI to pass I think

* When this method is called we can be sure that we are in the mutex runExclusive lock and thus other sessions
* will not be able to open a session for this tenant until we're done.
*
* NOTE: We don't support multi-instance locking for now. That means you can only have a single instance open and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting note. I think this applies also for single-tenancy cases where we have multiple instances. It would be another reason of disabling automatic updating in server side deployments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed. It is something we can fix in a future version but you'd need something like redis to create the lock across all instances. Or we can use the askar record locking (forUpdate=true) but i'm not really sure yet how that works.

But you generally want all sessions closed when updating so if you have multiple instances in single tenancy it's similar to having one instance during the update.

Definitely room for improvement here.. Will try to tinker with it more

@@ -9,6 +9,16 @@ import type {
} from '../types'
import type { Buffer } from '../utils/buffer'

// Split up into WalletManager and Wallet instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah damn. No i thought i wanted to do that now, but it's a biggie and we should move this to 0.6

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra added this to the 0.5 milestone Feb 14, 2024
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra enabled auto-merge (squash) February 14, 2024 05:22
@TimoGlastra TimoGlastra merged commit 12c617e into openwallet-foundation:main Feb 14, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants