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

Improve node listing in tsh and tctl #39458

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Improve node listing in tsh and tctl #39458

merged 1 commit into from
Mar 19, 2024

Conversation

rosstimothy
Copy link
Contributor

Converts both tools to use the ListUnifiedResources RPC instead of the ListResources RPC to improve performance. While there may only be marginal gains when listing the entire set of nodes, there are substantial improvements when filtering via predicate, labels, or search.

tctl was also changed slightly to make use of server side filtering when tctl get node/foo is run. Prior to this change the entire node set was always retrieved and tctl performed the filter client side if the user provided a hostname or UUID.

Changelog: Improve performance when listing nodes with tsh or tctl

@rosstimothy
Copy link
Contributor Author

rosstimothy commented Mar 16, 2024

The tests below were run on a v15.1.4 cluster with 150_000 "fake" static nodes and one real node.

tsh ls

$ time ./releases/15.1.4/tsh ls --format=json | jq .[].metadata.name | wc -l
  150001

________________________________________________________
Executed in    3.80 secs    fish           external
   usr time    3.24 secs    0.20 millis    3.24 secs
   sys time    0.36 secs    2.66 millis    0.36 secs


$ time ~/src/teleport/build/tsh ls --format=json | jq .[].metadata.name | wc -l
  150001

________________________________________________________
Executed in    3.67 secs    fish           external
   usr time    3.12 secs    0.09 millis    3.12 secs
   sys time    0.29 secs    1.09 millis    0.29 secs

tsh ls --search='foo'

$ time ./releases/15.1.4/tsh ls --search='foo' --format=json | jq .[].metadata.name | wc -l
       1

________________________________________________________
Executed in  840.57 millis    fish           external
   usr time   61.72 millis    0.18 millis   61.54 millis
   sys time   64.86 millis    1.46 millis   63.41 millis

$ time ~/src/teleport/build/tsh ls --search='foo' --format=json | jq .[].metadata.name | wc -l
       1

________________________________________________________
Executed in  324.12 millis    fish           external
   usr time   64.45 millis    0.24 millis   64.20 millis
   sys time   31.73 millis    2.03 millis   29.70 millis

tctl get nodes

$ time ./releases/15.1.4/tctl get nodes --format=json | jq .[].metadata.name | wc -l
  150001

________________________________________________________
Executed in    4.27 secs    fish           external
   usr time    3.66 secs    0.20 millis    3.66 secs
   sys time    0.36 secs    1.80 millis    0.36 secs

$ time ~/src/teleport/build/tctl get nodes --format=json | jq .[].metadata.name | wc -l
  150001

________________________________________________________
Executed in    4.24 secs    fish           external
   usr time    3.64 secs  100.00 micros    3.64 secs
   sys time    0.32 secs  865.00 micros    0.32 secs

tctl get node/foo

$ time ./releases/15.1.4/tctl get node/foo --format=json | jq .[].spec.hostname
"foo"

________________________________________________________
Executed in    1.05 secs      fish           external
   usr time  384.21 millis   63.00 micros  384.15 millis
   sys time  144.39 millis  806.00 micros  143.58 millis


$ time ~/src/teleport/build/tctl get node/foo --format=json | jq .[].spec.hostname
"foo"

________________________________________________________
Executed in  322.17 millis    fish           external
   usr time   47.47 millis    0.07 millis   47.40 millis
   sys time   21.62 millis    1.27 millis   20.35 millis

@rosstimothy rosstimothy marked this pull request as ready for review March 16, 2024 16:00
@github-actions github-actions bot requested review from kimlisa and Tener March 16, 2024 16:00
@github-actions github-actions bot added size/sm tctl tctl - Teleport admin tool labels Mar 16, 2024
Copy link
Collaborator

@zmb3 zmb3 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.

Are there any good use cases for GetAllResources left, or should we mark it deprecated and point people to the unified resources RPC?

api/client/client.go Outdated Show resolved Hide resolved
lib/client/api.go Show resolved Hide resolved
tool/tctl/common/resource_command.go Outdated Show resolved Hide resolved
@rosstimothy
Copy link
Contributor Author

Are there any good use cases for GetAllResources left, or should we mark it deprecated and point people to the unified resources RPC?

We probably want to migrate all of our listing operations over to the unified resources API. I can follow up with a deprecation and subsequent PRs to migrate other resources over. This PR also does not modify tsh ls -R because the recursive listing mechanism is used for all resources and I was trying to scope this down to something that was just for nodes.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from kimlisa March 18, 2024 15:53
@rosstimothy
Copy link
Contributor Author

This depends on #39519 to address a bug exposed by these changes.

@rosstimothy rosstimothy force-pushed the tross/client_nodes branch 3 times, most recently from d9c2ce0 to 593dcba Compare March 19, 2024 13:39
Converts both tools to use the ListUnifiedResources RPC instead of
the ListResources RPC to improve performance. While there may only
be marginal gains when listing the entire set of nodes, there are
substantial improvements when filtering via predicate, labels, or
search.

tctl was also changed slightly to make use of server side filtering
when tctl get node/foo is run. Prior to this change the entire node
set was always retreived and tctl performed the filter client side
if the user provided a hostname or UUID.
@rosstimothy rosstimothy enabled auto-merge March 19, 2024 13:52
@rosstimothy rosstimothy added this pull request to the merge queue Mar 19, 2024
Merged via the queue into master with commit 8abd045 Mar 19, 2024
34 checks passed
@rosstimothy rosstimothy deleted the tross/client_nodes branch March 19, 2024 14:17
@public-teleport-github-review-bot

@rosstimothy See the table below for backport results.

Branch Result
branch/v14 Create PR
branch/v15 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