Improve IdentityStore Invalidate performance #27184
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
| // bucketLocalAlias. | ||
| var memDBLocalAlias *identity.Alias | ||
| for i, localAlias := range memDBLocalAliases { | ||
| if localAlias.ID == bucketLocalAlias.ID { |
There was a problem hiding this comment.
Discussed out of band about the possibility of creating a new MemDBMapIDToEntityByBucketKeyInTxn (yuck). It would be really nice if we could build up a map[ID]identity.Entity and map[ID]identity.Alias and use those to trim duplicates by direct lookup, thus replacing the inner loop.
I think this could potentially be a significant improvement in clarity and performance (though the current patch has already proven a significant improvement over the prior implementation).
Since this code has already been tested and provides the results we want, we can defer this change as follow-up work after the Code Freeze.
There was a problem hiding this comment.
Looks good to me @marcboudreau! I know we've done significant testing and validation of the improvement, which helps us have confidence in the change.
The comment wording nit and the map optimization we discussed can be addressed as future work so I'll go ahead an approve!
edit: It looks like we need a godoc to pass the linter check. Aside from that CI appears to be happy.
| // We've considered the use of github.com/google/go-cmp here, | ||
| // but opted for sticking with reflect.DeepEqual because go-cmp | ||
| // is intended for testing and is able to panic in some | ||
| // situations. |
There was a problem hiding this comment.
| // We've considered the use of github.com/google/go-cmp here, | |
| // but opted for sticking with reflect.DeepEqual because go-cmp | |
| // is intended for testing and is able to panic in some | |
| // situations. | |
| // Though DeepEqual relies on == equality for underlying comparison, | |
| // this is perfectly safe for all compared fields. Timestamps are all in | |
| // unix epoch time and embedded structs contain no `.Equals`. |
marcboudreau
deleted the
marcboudreau/VAULT-27060/identity-store-invalidation
branch
* improve identitystore invalidate performance * add changelog * adding test to cover invalidation of entity bucket keys within IdentityStore * minor clean ups * adding tests * add missing godoc for tests
…/1.16.x (#27230) * Improve IdentityStore Invalidate performance (#27184) * improve identitystore invalidate performance * add changelog * adding test to cover invalidation of entity bucket keys within IdentityStore * minor clean ups * adding tests * add missing godoc for tests * fix incorrect merge resolution --------- Co-authored-by: Marc Boudreau <[email protected]>
The Invalidate method of the IdentityStore struct was using a simplistic algorithm to synchronize the MemDB records (entities, groups, local entity aliases) with those from the storage bucket. This simplistic algorithm would result in a large number of MemDB operations within a single transaction whenever the storage bucket contained a large number or records. This large number of operations led to using a much slower comparer function within MemDB which caused the Invalidate function to take a long time to complete and could lead the node to fall so far behind in processing WALs sent over by the primary cluster that the replication state would transition to
merkle-sync.The simplistic approach basically consisted of deleting everything from MemDB that was associated with the invalidated storage bucket and re-inserting those resources using state contained in the storage bucket. Since invalidations usually occur to signal a single resource has changed, been added, or been deleted; when a large number of unchanged resources also exist in the storage bucket, a lot of unnecessary work was being done (deleting and re-adding).
These changes replace the simplistic approach for the handling of entities and local entity aliases since they are the more likely resource to exist in large numbers where this problem occurs.
The new approach consists of comparing the contents of the invalidated storage bucket with the set of resources from MemDB associated that storage bucket. Resources that match in both systems are left alone, and only differences are rectified in MemDB.