Multi-tenancy stale wallet clean up #1692

dbluhm · 2022-03-25T16:31:21Z

This PR is intended to supersede #928. Due to significant changes since the original PR was opened, I cherry-picked a commit but had to modify it. History has not been well preserved, unfortunately. Credit to @TimoGlastra for the original work on the ProfileCache especially and for assistance in finding the solution implemented here.

This PR adds a LRU cache for profiles opened by MultitenantManager (corresponding to wallet types indy and askar, but not askar-profile). weakref.finalize is used to ensure the profiles are closed when they fall out of scope. An ordered dictionary of profiles holds a (strong) reference to the profile until it is evicted. If that profile happens to still be in use at the time it is evicted, it will not be closed until other (strong) references to it expire, i.e. after processing of a message or admin request has finished. This exact scenario is unlikely but possible.

In addition to this profile caching mechanism, I also updated a few aspects of the BaseMultitenantManager and its subclasses to make them more consistent with conventions used in ACA-Py.

Also, after being very confused by handling of askar profiles (not to be confused with AskarProfiles) within the AskarProfileMultitenantManager and AskarProfile, I did some light updates to (I hope) improve clarity.

A brief listing of known limitations:

~~As implemented, the capacity of the LRU cache is statically defined as 100.~~
A temporally based system for evicting profiles from the cache is likely better than a max capacity based system.

Update:
I've taken a pass at improving the multitenancy-config argument. It will still accept json (read: this shouldn't be a breaking change) but it will now also accept key=value pairs. I'm still a bit iffy on whether these shouldn't just be their own separate arguments though...

Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

remove usage of self._instances in AskarProfileMultitenantManager add profile_id to AskarProfile init (to clearly differentiate between AskarProfiles where default profile is used and alternate profiles are used) base update_wallet only updates records; managers where profiles are held open are responsible for updating currently open profiles Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm · 2022-03-25T16:37:15Z

I'm still in the process of validating the cache behavior so I've opened this as a draft PR but I would appreciate feedback on what's here so far. Due to resourcing constraints, it's not likely that I'll be able to implement a time based cache (I haven't evaluated what that would look like, though, so if it ends up being super easy, maybe lol).

I would be particularly interested to hear feedback from @TimoGlastra and @andrewwhitehead since the two of you are the original participants on #928 but more than happy to get feedback from anyone.

swcurran · 2022-03-25T16:42:30Z

@PaulWen - this would seem a really good PR to test with the Load Generator. Do you have an easy way to run tests on a PR branch?

dbluhm · 2022-03-25T16:48:01Z

Odd how submitting a PR suddenly brings clarity lol; as implemented, if more than 100 (or whatever the capacity is defined as) is actively in use, this can result in rapid opening and closure of wallets and WalletAlreadyOpen errors 🤔 I'm probably not quite on the mark here yet...

PaulWen · 2022-03-25T20:23:52Z

This AcaPy repo is already integrated as a git submodule into the Load Generator repo to build AcaPy images with a debugger. Therefore, it should not be difficult to checkout any branch and build a docker image in a similar manner.

@dbluhm Could you describe to me what a suitable test flow would look like? Feel free to drop me a PM in Discord.

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

codecov-commenter · 2022-04-12T00:58:35Z

Codecov Report

Merging #1692 (df75214) into main (27b238a) will decrease coverage by 0.02%.
The diff coverage is 96.35%.

@@            Coverage Diff             @@
##             main    #1692      +/-   ##
==========================================
- Coverage   95.23%   95.21%   -0.03%     
==========================================
  Files         525      526       +1     
  Lines       33056    33143      +87     
==========================================
+ Hits        31482    31558      +76     
- Misses       1574     1585      +11

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm · 2022-04-12T01:16:40Z

I've sorted out most of the issues with this PR. I was having a heck of a time identifying where references to profiles were being held open but I've found the culprits (reponders held references to profiles/contexts and then profiles held references to responders...) and my rudimentary tests behave as expected.

I'll do my bit to cover new lines better for the sake of code coverage but I'm open to ideas for how or even if we should have tests exercising this functionality inside of ACA-Py.

Should have this ready for review soon.

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

And make tests follow pytest convention Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

andrewwhitehead · 2022-04-20T18:12:05Z

aries_cloudagent/askar/profile.py

+        def _finalize(opened: Optional[AskarOpenStore]):
+            if opened:
+                LOGGER.debug("Profile finalizer called; closing wallet")
+                asyncio.get_event_loop().create_task(opened.close())


It's nice but not necessary to close the store this way, as the library has its own finalizers. With multiple profile instances potentially referencing the same Store it would probably not be correct to close the store when a single profile is closed, either.

andrewwhitehead · 2022-04-20T18:15:36Z

aries_cloudagent/core/profile.py

@@ -115,6 +116,18 @@ def inject_or(
    async def close(self):
        """Close the profile instance."""

+    def finalizer(self) -> Optional[finalize]:


I think the finalizer can just be set up in the profile's __init__ method when required. We don't have to keep a reference to it, and requiring the caller to initialize it seems error prone. (It could make testing a little harder.)

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm · 2022-05-03T15:52:26Z

Not realizing that Askar already had finalizers and considering @andrewwhitehead's comments regarding requiring the caller to initialize the finalizer being potentially error prone, I've simplified the finalizer additions. Essentially, finalizers for Indy SDK wallet profiles are always created. I don't think there's a scenario where it would be appropriate for the wallet to remain opened after the profile has gone out of scope so I think this is a valid simplification.

In other words, all profiles managed by the profile cache are now assumed to clean up their own resources after falling out of scope and it is up to the profile implementation to ensure this is the case.

aries_cloudagent/config/argparse.py

andrewwhitehead · 2022-05-18T18:25:08Z

aries_cloudagent/indy/sdk/profile.py

+        """
+
+        def _finalize(opened: Optional[IndyOpenWallet]):
+            if opened:


I don't think opened can be None because it receives the original IndyOpenWallet instance. Maybe if opened.handle?

This came as a recommendation from pyright which indicates that IndySdkProfile.opened is IndyOpenWallet | None. I think it derived this from the self.opened = None line in the close method.

Following that same vein, I think it was possible before for the finalizer to be created after close had been called. Now that it's only intended to be called in __init__, I don't think this is a problem any more. I'll fix that.

andrewwhitehead · 2022-05-18T18:26:59Z

aries_cloudagent/multitenant/askar_profile_manager.py

@@ -15,13 +16,23 @@ class AskarProfileMultitenantManager(BaseMultitenantManager):

    DEFAULT_MULTIENANT_WALLET_NAME = "multitenant_sub_wallet"


Maybe we could fix this variable name

andrewwhitehead · 2022-05-18T18:28:04Z

aries_cloudagent/multitenant/base.py

+    @abstractproperty
+    def open_profiles(self) -> Iterable[Profile]:
+        """Return iterator over open profiles."""
+        ...


This was to satisfy pyright more than anything else. Oddly, it seems fine if I do @property and @abstractmethod but I have to add the ellipsis if I use @abstractproperty. I'll remove this.

Apparently it's about the same as writing 'pass', I just haven't seen it much before.

andrewwhitehead · 2022-05-18T18:32:53Z

Added a couple small comments but it looks great otherwise, I'd like to get this merged.

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm · 2022-06-02T23:31:22Z

Digging into the unit test failure; it's not behaving quite the same way when I run run_tests_indy locally so a little perplexed. Should have more time to look at this shortly.

dbluhm added 9 commits March 22, 2022 23:02

feat: add caching for multi-tenant profiles

7d88b23

Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

feat: add finalizers to profile impls

0769351

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

refactor: conductor iterates through open_profiles

fdac67c

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: multitenant manager tests

22e05f6

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: askar profile tests

ac585c9

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: open_profiles should be property

6f613ea

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: askar profile tests, profile passed to sesh, txn

ce7ccf1

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

c3d9886

dbluhm added 6 commits March 28, 2022 20:38

feat: profile cache track open profiles via weakref

a2e306e

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: create task for profile finalizer

c32d907

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

d0e2a6d

style: black fixes

3528409

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: prevent circular references from breaking cache

f424dbe

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

a7b5f84

feat: rework multitenancy-config arg

a091c63

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm added 8 commits April 11, 2022 21:22

fix: parsing multiple multitenancy-config options

2ffd521

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: multitenancy config json parsing and tests

b5b0937

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

569a0ca

test: askar profile finalizer

1c21804

And make tests follow pytest convention Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

refactor: indy test profile by pytest convention

108ecab

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

test: indy sdk profile finalizer

2e80b29

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

test: cache_size parsing

9cfbcda

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

test: dispatcher responder context weak ref

0422d8a

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm added 3 commits April 14, 2022 15:55

test: rescuing still open profile from eviction

94b0c3e

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

test: askar profile manager open profiles

a9402f4

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

625b276

dbluhm marked this pull request as ready for review April 14, 2022 20:00

dbluhm added 2 commits April 14, 2022 17:23

fix: self.manager for mocking methods in tests

4a5f65d

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

test: admin responder profile expired

4bf26e5

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

andrewwhitehead reviewed Apr 20, 2022

View reviewed changes

dbluhm added 2 commits May 3, 2022 11:44

feat: askar profiles don't need finalizers

e84b3f8

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

Merge branch 'main' into feature/multitenant-profile-cache

96e715c

Merge branch 'main' into feature/multitenant-profile-cache

df75214

dbluhm commented May 17, 2022

View reviewed changes

aries_cloudagent/config/argparse.py Outdated Show resolved Hide resolved

andrewwhitehead mentioned this pull request May 18, 2022

fix: add a close statement to ensure session is closed on error #1777

Merged

andrewwhitehead reviewed May 18, 2022

View reviewed changes

dbluhm added 4 commits June 2, 2022 15:04

Merge branch 'main' into feature/multitenant-profile-cache

e3c8a10

fix: oddities and typos

20d753b

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: add key_derivation_method help text back in

da236fd

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

fix: missing async marks for pytest

9575d07

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>

dbluhm added 3 commits June 9, 2022 15:08

Merge branch 'main' into feature/multitenant-profile-cache

b018ba8

Merge branch 'main' into feature/multitenant-profile-cache

537425e

Merge branch 'main' into feature/multitenant-profile-cache

5af0ae2

dbluhm requested a review from andrewwhitehead June 22, 2022 19:20

andrewwhitehead approved these changes Jun 22, 2022

View reviewed changes

swcurran merged commit 814cd8d into openwallet-foundation:main Jun 22, 2022

dbluhm mentioned this pull request Jun 30, 2022

[multi-tenancy] add profile resource management #928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-tenancy stale wallet clean up #1692

Multi-tenancy stale wallet clean up #1692

dbluhm commented Mar 25, 2022 •

edited

Loading

dbluhm commented Mar 25, 2022

swcurran commented Mar 25, 2022

dbluhm commented Mar 25, 2022

PaulWen commented Mar 25, 2022

codecov-commenter commented Apr 12, 2022 •

edited

Loading

dbluhm commented Apr 12, 2022

andrewwhitehead Apr 20, 2022

andrewwhitehead Apr 20, 2022 •

edited

Loading

dbluhm commented May 3, 2022

andrewwhitehead May 18, 2022

dbluhm Jun 2, 2022

andrewwhitehead May 18, 2022

andrewwhitehead May 18, 2022

dbluhm Jun 2, 2022

andrewwhitehead Jun 2, 2022

andrewwhitehead commented May 18, 2022

dbluhm commented Jun 2, 2022

		@@ -15,13 +16,23 @@ class AskarProfileMultitenantManager(BaseMultitenantManager):

		DEFAULT_MULTIENANT_WALLET_NAME = "multitenant_sub_wallet"

Multi-tenancy stale wallet clean up #1692

Multi-tenancy stale wallet clean up #1692

Conversation

dbluhm commented Mar 25, 2022 • edited Loading

dbluhm commented Mar 25, 2022

swcurran commented Mar 25, 2022

dbluhm commented Mar 25, 2022

PaulWen commented Mar 25, 2022

codecov-commenter commented Apr 12, 2022 • edited Loading

Codecov Report

dbluhm commented Apr 12, 2022

andrewwhitehead Apr 20, 2022

Choose a reason for hiding this comment

andrewwhitehead Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

dbluhm commented May 3, 2022

andrewwhitehead May 18, 2022

Choose a reason for hiding this comment

dbluhm Jun 2, 2022

Choose a reason for hiding this comment

andrewwhitehead May 18, 2022

Choose a reason for hiding this comment

andrewwhitehead May 18, 2022

Choose a reason for hiding this comment

dbluhm Jun 2, 2022

Choose a reason for hiding this comment

andrewwhitehead Jun 2, 2022

Choose a reason for hiding this comment

andrewwhitehead commented May 18, 2022

dbluhm commented Jun 2, 2022

dbluhm commented Mar 25, 2022 •

edited

Loading

codecov-commenter commented Apr 12, 2022 •

edited

Loading

andrewwhitehead Apr 20, 2022 •

edited

Loading