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): tenant lifecycle #942

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Jul 11, 2022

Adds lifecycle management to the tenants module (especially the TenantSessionCoordinator and the TenantAgentContextProvider).

There's an internal registry of open sessions (TenantSessionMutex). This is configured with a max number of sessions (currently set to 10.000, I hope to add custom module config with modularization soon). It also adds a timeout for how long it can take to acquire a session (also hardcoded for now, will become configurable). It's important that sessions are always ended. You can either do that using withAgentContext which will end the session for you, or you need to call .destroy after you're done using the tenant agent. After a message has been processed the session will also be ended.

This doesn't include caching of agent context yet. So if the last session for a tenant is ended, the wallet will be closed. In the future we could look at keeping the wallet open for a while, but with RAW key derivation I have this process to be quite fast. I first wanted to make sure we can handle a lot of sessions for different and same tenants at the same time and don't break.

Some notes:

  • I've updated most of the codebase to work with multiple contexts, and making sure we aren't creating events based on contexts that may have been expired. The exception is the mediation recipient module. This will need more work, and as you probably won't use mediation as a tenant is lower priority for now.
  • max session count, session acquire timeout are hardcoded for now. Will be configurable soon
  • Added some e2e tests that open 5 sessions each for 20 tenants in parallel. WE should add some proper benchmark tests soon
  • I've updated the wallet to use RAW key derivation (we use generateWalletKey) which means the wallet key generation process has been sped up significantly.
  • There's improvements to be made in how we weigh sessions. Currently 10 sessions for 1 tenant weigh equal to 10 sessions spread over 10 tenants. In theory in the first case we only open 1 wallet, while in the latter we open 10. We could make the first session have 5 weight points, and the concurrent sessions 1. This means it's cheaper to have multiple sessions for the same tenant than sessions for different tenants. Server environments should make sure to use sticky sessions and route the same tenants to the same agent.

One thing we should still look at is that you can call tenantAgent.wallet.close() at any time which will corrupt all current sessions. I'm not sure if we can avoid this, and we should probably document this.

Also see: https://hackmd.io/vGLVlxLvQR6jsEEjzNcL8g?view#Tenant-Lifecycle

Usage:

// manual: FAILURE TO CALL .destroy() CAN LEAD TO DEADLOCKS. SECOND APPROACH ADVISED
const tenantAgent = await agent.tenants.getTenantAgent({ tenantId: 'tenant-id' })
const invitation = await tenantAgent.oob.createInvitation()
await tenantAgent.destroy()

// auto
const tenantAgent = await agent.tenants.withTenantAgent({ tenantId: 'tenant-id' }, async tenantAgent => {
	const invitation = await tenantAgent.oob.createInvitation()
})

@TimoGlastra TimoGlastra requested a review from a team as a code owner July 11, 2022 14:46
@TimoGlastra TimoGlastra added the multitenancy Tasks related to multi-tenancy label Jul 11, 2022
@karimStekelenburg
Copy link
Contributor

What do you exactly mean with the 10k session limit? Does that mean that we can have 10k sessions simultaneously? That sounds like a lot!

Indeed a good idea do some stress testing. Would also be good to benchmark the default session limit we will be using (currently 10k, if I understand it correctly) on a machine with 'average specs'. Just to see if we encounter any memory issues or substantial increases in response times.

Regarding session weights: I think we need to think this over carefully. I can imagine use cases where you'd want to set a certain limit of the amount of concurrent sessions a certain tenant can have. For instance: if I'm a vendor who offers SSI services using a multi-tenant agent, I can imagine offering my clients different tiers. A free tier might include 5 concurrent sessions, while a 'pro tier' might offer a 100. This also raises the question about what to do if the limit is exceeded. You could just return an error, another options is to queue requests and process them whenever a session is freed up.

For the sake of simplicity we could follow a 'all tenants are created equal' philosophy for now, meaning we maintain the same limit for all tenants.

NOTE: this suggestion isn't intended to replace the 'global session cap'. We definitely need that as well to make sure we don't run out of resources.

That being said, nice work @TimoGlastra! This is really starting to take form. I will review this in more detail soon.

@TimoGlastra
Copy link
Contributor Author

What do you exactly mean with the 10k session limit? Does that mean that we can have 10k sessions simultaneously? That sounds like a lot!

Yes! But it is just an arbitrary number. I have no idea what is reasonable here yet. I was able to run 10k sessions for a single tenant (so all using the same wallet). I was also able to run 1k sessions spread over 100 tenants. However I didn't do much with the sessions. If we start to create 10k credential definitions simultaneously there'll probably be some issues.

That's why we need the stress testing and make it configureable.

Regarding session weights: I think we need to think this over carefully. I can imagine use cases where you'd want to set a certain limit of the amount of concurrent sessions a certain tenant can have. For instance: if I'm a vendor who offers SSI services using a multi-tenant agent, I can imagine offering my clients different tiers. A free tier might include 5 concurrent sessions, while a 'pro tier' might offer a 100. This also raises the question about what to do if the limit is exceeded. You could just return an error, another options is to queue requests and process them whenever a session is freed up.

Currently there's two things we track: total session count (across all tenants) and number of sessions for a specific tenant. What I was mainly referring to with weighted sessions is not from an external PoV (so building a product around AFJ), but more on balancing the total resources used. Having 10 sessions for a single tenant is different than having 10 sessions spread across 10 tenants. If we also want to take these other factors into account it should probably be a pluggable interface that people can implement as they wish (and would give them a way to keep track of sessions per tenant etc...)

@TimoGlastra TimoGlastra enabled auto-merge (squash) July 13, 2022 20:45
@TimoGlastra TimoGlastra disabled auto-merge July 13, 2022 20:45
@TimoGlastra TimoGlastra merged commit 310185b into openwallet-foundation:0.3.0-pre Jul 13, 2022
@TimoGlastra TimoGlastra deleted the feat/tenants-lifecycle branch July 13, 2022 20:46
TimoGlastra added a commit that referenced this pull request Aug 11, 2022
* fix: make event listeners tenant aware
* chore(deps): update tsyringe
* feat: add agent context disposal
* feat(tenants): with tenant agent method
* test(tenants): add tests for session mutex
* feat(tenants): use RAW key derivation
* test(tenants): add e2e session tests
* feat(tenants): destroy and end session

Signed-off-by: Timo Glastra <[email protected]>
TimoGlastra added a commit that referenced this pull request Aug 26, 2022
* fix: make event listeners tenant aware
* chore(deps): update tsyringe
* feat: add agent context disposal
* feat(tenants): with tenant agent method
* test(tenants): add tests for session mutex
* feat(tenants): use RAW key derivation
* test(tenants): add e2e session tests
* feat(tenants): destroy and end session

Signed-off-by: Timo Glastra <[email protected]>
TimoGlastra added a commit that referenced this pull request Aug 26, 2022
* fix: make event listeners tenant aware
* chore(deps): update tsyringe
* feat: add agent context disposal
* feat(tenants): with tenant agent method
* test(tenants): add tests for session mutex
* feat(tenants): use RAW key derivation
* test(tenants): add e2e session tests
* feat(tenants): destroy and end session

Signed-off-by: Timo Glastra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multitenancy Tasks related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants