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

readonly cluster configs #43422

Merged
merged 1 commit into from
Jul 2, 2024
Merged

readonly cluster configs #43422

merged 1 commit into from
Jul 2, 2024

Conversation

fspmarshall
Copy link
Contributor

@fspmarshall fspmarshall commented Jun 24, 2024

This PR aims to reduce excess CPU/memory usage caused by large numbers of concurrent loads of certain cluster configuration resources. Most notably, values such as auth preference that are loaded for basically any RBAC check performed by a teleport instance. This is typically an inconsequential cost, but on auth servers handling many thousands of concurrent requests, the resource consumption of constantly deserializing these values can be non-trivial.

This PR moves a number of hot paths over to using shared in-memory values rather than loading a separate copy per goroutine. In order to facilitate doing this sharing safely, a new package readonly has been added which provides readonly subsets of certain common cluster configuration interfaces as well as a basic ttl-cache that stores readonly copies in-memory.

Currently, the implementation of readonly.Cache is fairly tightly coupled with the needs of lib/authz and lib/auth/clusterconfig. These two packages were selected as the starting point/proof of concept for this idea. In the long run, we'll likely either want to have a family of different specialized readonly caches, or a way to toggle on and off which resources the cache is configured to handle (much like how the primary cache in lib/cache currently works).

changelog: reduced CPU usage in auth servers experiencing very high concurrent request load.

@fspmarshall fspmarshall force-pushed the fspmarshall/singleton-caching branch 3 times, most recently from dd92225 to 44f245b Compare June 24, 2024 17:56
@fspmarshall fspmarshall marked this pull request as ready for review June 24, 2024 18:28
@github-actions github-actions bot requested review from greedy52 and probakowski June 24, 2024 18:28
@github-actions github-actions bot added database-access Database access related issues and PRs size/md labels Jun 24, 2024
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

lib/auth/grpcserver.go Outdated Show resolved Hide resolved
lib/services/readonly/readonly.go Outdated Show resolved Hide resolved
Comment on lines +32 to +35
// AuthPreference is a read-only subset of types.AuthPreference used on certain hot paths
// to ensure that we do not modify the underlying AuthPreference as it may be shared across
// multiple goroutines.
type AuthPreference interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, the readonly interfaces could be put in api/types and let AuthPreference inherits it. Otherwise, it is easy to miss this interface when adding new functions to AuthPreference. Though even if you miss it, it won't break anything unless you really need the new function. I don't have a strong opinion on this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered this, but opted against it for the time being mostly because once this is in api it has to obey major version compatibility. This is a somewhat experimental pattern. I don't want it leaking into api until we're confident that this is the right way to handle this kind of problem going forward.

@fspmarshall fspmarshall force-pushed the fspmarshall/singleton-caching branch 2 times, most recently from 501b810 to 7b519d5 Compare June 28, 2024 22:12
@fspmarshall fspmarshall enabled auto-merge June 28, 2024 22:12
@fspmarshall fspmarshall force-pushed the fspmarshall/singleton-caching branch 2 times, most recently from f18d000 to 08d308b Compare July 1, 2024 17:46
@fspmarshall fspmarshall force-pushed the fspmarshall/singleton-caching branch from 08d308b to ee888f1 Compare July 2, 2024 17:02
@fspmarshall fspmarshall added this pull request to the merge queue Jul 2, 2024
Merged via the queue into master with commit 1742c46 Jul 2, 2024
38 checks passed
@fspmarshall fspmarshall deleted the fspmarshall/singleton-caching branch July 2, 2024 17:41
@public-teleport-github-review-bot

@fspmarshall See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Failed
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants