Refactor cache to support storing resources in memory #52210
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rosstimothy
added
the
no-changelog
rosstimothy
force-pushed
the
tross/refactor_cache
branch
4 times, most recently
from
3be09f7 to
9baa177
Compare
fspmarshall
reviewed
Feb 18, 2025
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
fe33466 to
b28680c
Compare
This is a mechanical operation to rename the existing collections to legacy collections. These collections will slowly be converted over to a new variant that holds resources in memory instead of relying on the memory backend and storage implementation in lib/services/local.
Introduces an alternate to the legacyCollections and genericCollection that relies on caching resource in memory. The new collection[T] takes inspiration from the legacy collection types but aims to both simplify and break reliance on using the local storage services. The two collection mechanisms will and can coexist until all types are converted to the new mechanism. A single implementation of the new collection exists for the StaticTokens. Additionally, during the conversion process some refactoring will be done to try to better organize and reduce the size of cache.go. In this PR all of the code specific to static tokens was moved out of cache.go and legacy_collections.go and into static_tokens.go. This will allow separating concerns and making it much easier to identify where specific content lives.
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
88fdb27 to
ab060e3
Compare
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
ab060e3 to
63618f5
Compare
fspmarshall
reviewed
Feb 20, 2025
Comment on lines
+127
to
+142
| // Always perform the delete if this is not a singleton, otherwise | ||
| // only perform the delete if the singleton wasn't found | ||
| // or the resource kind isn't cached in the current generation. | ||
| if !c.singleton || deleteSingleton || !cacheOK { | ||
| if err := c.store.clear(); err != nil { | ||
| if !trace.IsNotFound(err) { | ||
| return trace.Wrap(err) | ||
| } | ||
| } | ||
| } | ||
| // If this is a singleton and we performed a deletion, return here | ||
| // because we only want to update or delete a singleton, not both. | ||
| // Also don't continue if the resource kind isn't cached in the current generation. | ||
| if c.singleton && deleteSingleton || !cacheOK { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Looking over this and the old collection logic that this was derived from, I'm not sure we should be caring about whether or not the underlying type is a singleton. As far as I can see, it seems like if it just always clear and always apply any resources in the slice we should achieve the exact same end-effect with much less control-flow.
Comment on lines
+96
to
+98
| for idx, transform := range s.indexes { | ||
| s.cache.Delete(idx, transform(t)) | ||
| } |
There was a problem hiding this comment.
nit: SortCache deletes the item across all indexes if it is deleted on any index, so only one call to Delete is required (though maybe just leaving as-is is easier than deciding which one to use lol).
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
970ff84 to
39d9934
Compare
fspmarshall
reviewed
Feb 21, 2025
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
a3fd56f to
dd3a9c2
Compare
rosstimothy
force-pushed
the
tross/refactor_cache
branch
from
f2d8fe6 to
fb1947b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While the current cache storage is also in memory, it leverages the memory backend which requires converting resources to and from json. Marshaling json is suboptimal and is often a source of CPU and memory consumption. The biggest problem with json marshaling though is that it requires calling CheckAndSetDefaults. When validations enforced in CASD become stricter it can leave caches unable to become healthy when there are mixed versions within a cluster as they all might have a slightly different view of what should be allowed.
As a means to solve both problems, this changes the cache to store resources received from the upstream directly in memory without doing any conversion to json. There are two gotchas with this approach, caching is no longer "free" and cloning of resources must be done diligently to avoid races. Historically the cache relied on the storage implementation in
services/localto manage persisting resources in the cache'sbackend.Memory. However, that will no longer be the case as the cache needs to support storage directly in memory. In order to reduce the burden this may pose on developers the new collection machinery is simpler, and helpers will be added on top ofsortcache.SortCacheto make a second storage implementation trivial.The changes here are two fold, mark the existing collections machinery as legacy and introduce new machinery to operate entirely in memory. This includes an initial implementation of caching for static tokens, users, and cert authorities that leverages the new collection machinery. Additionally, all of the new resource specific code was moved to
lib/cache/static_tokens.go,lib/cache/users.go, andlib/cache/cert_authority.go. This follows the blue print some of the newer cached resources use to make it easier to identify where code for a specific resource lives and to reduce the size of the cache.go and collections.go files.Once this lands I plan to copy the same pattern used on other cached resource until all of the legacy collections are gone.