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

Move expensive role building off transport thread #113020

Merged

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented Sep 17, 2024

This PR moves role building off the transport thread to the generic thread pool, since role building can be expensive depending on role structure.

Role building is CPU bound so this PR uses a ThrottledTaskRunner to limit the number of concurrent requests. I will explore adding a max queue limit in a follow up.

Resolves: ES-9505

@n1v0lg n1v0lg added >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Sep 17, 2024
@n1v0lg n1v0lg self-assigned this Sep 17, 2024
@n1v0lg n1v0lg changed the title [WIP] move app privilege automaton building off transport [WIP] move expensive role building off transport thread Sep 20, 2024
@n1v0lg n1v0lg changed the title [WIP] move expensive role building off transport thread Move expensive role building off transport thread Sep 20, 2024
@n1v0lg n1v0lg marked this pull request as ready for review September 20, 2024 15:01
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Sep 20, 2024
@n1v0lg n1v0lg requested review from slobodanadamovic and removed request for jakelandis September 23, 2024 07:34
Copy link
Contributor

@slobodanadamovic slobodanadamovic left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

I've left only non-blocking comments add a suggestion to add more coverage around forking logic. Approving, since this should not require another round of review.

@n1v0lg
Copy link
Contributor Author

n1v0lg commented Sep 24, 2024

@elasticmachine update branch

@n1v0lg n1v0lg added v8.16.0 auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge labels Sep 24, 2024
@elasticsearchmachine elasticsearchmachine merged commit 6cdd59b into elastic:main Sep 24, 2024
20 checks passed
@n1v0lg n1v0lg deleted the app-priv-automaton-off-transport branch September 24, 2024 08:22
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

n1v0lg added a commit to n1v0lg/elasticsearch that referenced this pull request Sep 24, 2024
This PR moves role building off the transport thread to the generic
thread pool, since role building can be expensive depending on role
structure. 

Role building is CPU bound so this PR uses a `ThrottledTaskRunner` to
limit the number of concurrent requests. I will explore adding a max
queue limit in a follow up. 

Resolves: ES-9505
@n1v0lg
Copy link
Contributor Author

n1v0lg commented Nov 19, 2024

💚 All backports created successfully

Status Branch Result
8.16

Questions ?

Please refer to the Backport tool documentation

n1v0lg added a commit to n1v0lg/elasticsearch that referenced this pull request Nov 19, 2024
This PR moves role building off the transport thread to the generic
thread pool, since role building can be expensive depending on role
structure.

Role building is CPU bound so this PR uses a `ThrottledTaskRunner` to
limit the number of concurrent requests. I will explore adding a max
queue limit in a follow up.

Resolves: ES-9505
(cherry picked from commit 6cdd59b)
n1v0lg added a commit that referenced this pull request Nov 19, 2024
# Backport

This will backport the following commits from `main` to `8.16`:
 - [Move expensive role building off transport thread (#113020)](#113020)
elasticsearchmachine pushed a commit that referenced this pull request Nov 20, 2024
This PR moves role building off the transport thread to the generic
thread pool, since role building can be expensive depending on role
structure. 

Role building is CPU bound so this PR uses a `ThrottledTaskRunner` to
limit the number of concurrent requests. I will explore adding a max
queue limit in a follow up. 

Resolves: ES-9505

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants