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

Add per-tenant provisioning throttling #1074

Merged
merged 7 commits into from
Mar 11, 2025

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Mar 7, 2025

Description

Adds settings to control per-tenant limits on simultaneous provision, reprovision, and deprovision.

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • [ ] API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • [ ] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions github-actions bot added the backport 2.x backport PRs to 2.x branch label Mar 7, 2025
@dbwiddis dbwiddis removed the backport 2.x backport PRs to 2.x branch label Mar 7, 2025
@dbwiddis dbwiddis linked an issue Mar 7, 2025 that may be closed by this pull request
@dbwiddis dbwiddis marked this pull request as ready for review March 7, 2025 07:37
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 69.01408% with 22 lines in your changes missing coverage. Please review.

Project coverage is 76.32%. Comparing base (0a19823) to head (6fbf718).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../transport/ReprovisionWorkflowTransportAction.java 9.09% 8 Missing and 2 partials ⚠️
...rk/transport/ProvisionWorkflowTransportAction.java 30.00% 5 Missing and 2 partials ⚠️
.../transport/DeprovisionWorkflowTransportAction.java 33.33% 1 Missing and 3 partials ⚠️
...ensearch/flowframework/util/TenantAwareHelper.java 97.05% 0 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (69.01%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1074      +/-   ##
============================================
+ Coverage     75.57%   76.32%   +0.75%     
- Complexity     1050     1075      +25     
============================================
  Files           101      101              
  Lines          5212     5276      +64     
  Branches        498      503       +5     
============================================
+ Hits           3939     4027      +88     
+ Misses         1035     1001      -34     
- Partials        238      248      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 7, 2025

tests failing because of jar hell caused by opensearch-project/ml-commons#3475 cc @owaiskazi19

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis force-pushed the tenant-provision-throttle branch from cae1242 to b4a6fd9 Compare March 11, 2025 04:09
Copy link
Member

@owaiskazi19 owaiskazi19 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 overall with comments

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Copy link
Member Author

@owaiskazi19 @amitgalitz Thanks for your review!

I've addressed some comments in 6fbf718:

  • used computeIfPresent() to remove from the map when it hits 0
  • used ActionListener.runBefore() in the deprovision case; it is a lot simpler

I'd like to handle changing the value of settings/limits and defaults in another PR (followed by a discussion issue).

I'll consider refactoring all of the multitenant ITs at a later date.

Other comments I've responded that I don't think changes are needed; I've added some comments in code explaining why. In particular I don't think there's a easy way to replace the increment/rollback logic with compareAndSet without really overcomplicating things.

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments

@dbwiddis dbwiddis merged commit c262bce into opensearch-project:main Mar 11, 2025
22 of 23 checks passed
@dbwiddis dbwiddis deleted the tenant-provision-throttle branch March 11, 2025 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants