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

Host user creation - Only update groups if needed #44991

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

atburke
Copy link
Contributor

@atburke atburke commented Aug 1, 2024

This change updates host user creation to only update a host user's groups when they have changed (in the role). This allows nodes to skip acquiring a user lock most of the time, which fixes performance issues when a Teleport user execs repeatedly.

Benchmarks from running tsh ssh 5 times concurrently are below.

Before:

Command Mean [s] Min [s] Max [s] Relative
seq 5 | parallel tsh ssh -l keeptemp node echo 9.338 ± 4.842 5.351 15.777 1.00

After:

Command Mean [ms] Min [ms] Max [ms] Relative
seq 5 | parallel tsh ssh -l keeptemp node echo 367.2 ± 26.9 342.7 505.5 1.00

Changelog: Improved tsh ssh performance for concurrent execs

Copy link
Contributor

@strideynet strideynet left a comment

Choose a reason for hiding this comment

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

Looks good - thanks for including some performance measurements. Definitely looks like it makes a big difference!

@atburke
Copy link
Contributor Author

atburke commented Aug 2, 2024

Friendly ping @fspmarshall

lib/srv/usermgmt.go Outdated Show resolved Hide resolved
@atburke
Copy link
Contributor Author

atburke commented Aug 5, 2024

Friendly ping @fspmarshall @rosstimothy

@rosstimothy
Copy link
Contributor

I'm working on a benchmark test to cover the case that identified this regression. Below is the comparison from running it on master and running it on this branch.

benchstat old.txt new.txt 
goos: linux
goarch: arm64
pkg: github.com/gravitational/teleport/lib/srv/regular
                                 │     old.txt     │               new.txt                │
                                 │     sec/op      │    sec/op     vs base                │
ExecCommand/no_user_creation-8       340.5m ±  17%   338.5m ± 10%        ~ (p=0.579 n=10)
ExecCommand/with_user_creation-8   50110.2m ± 110%   289.5m ± 20%  -99.42% (p=0.000 n=10)
geomean                               4.131          313.0m        -92.42%

@atburke atburke added this pull request to the merge queue Aug 6, 2024
Merged via the queue into master with commit d5c7ef7 Aug 6, 2024
38 checks passed
@atburke atburke deleted the atburke/host-group-check branch August 6, 2024 18:38
@public-teleport-github-review-bot

@atburke See the table below for backport results.

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

@rosstimothy
Copy link
Contributor

Regression originally introduced by #41919

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