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

[Cluster launcher] [aws] Bump max keys per account from 60 to 600 #35512

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

architkulkarni
Copy link
Contributor

Why are these changes needed?

Followup to #27506. This PR bumps it to 600.

Context from the previous PR copied below:

This is a minor QoL improvement to bump the hardcoded limit for number of aws keys per account. The limit is arbitrary and has been bumped before. AFAICT the fundamental aws limit is a 5000 key per region limit which we are not close to.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Archit Kulkarni <[email protected]>
Copy link
Contributor

@wuisawesome wuisawesome left a comment

Choose a reason for hiding this comment

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

k i think we need to draw a hard limit at around 1k here since these keys are a global resource and we don't want to interfere with other applications.

btw i wonder if we should keep the default smaller and configure the constant via env var? I assume most users don't have a legitimate use case for so many keys.

@architkulkarni
Copy link
Contributor Author

Oh good point, making it configurable via env var sounds good to me. Is the use case of a company account with many developers using the same account unusual? Maybe companies with >>5000 employees have multiple accounts. (5000 keys per region means 5000 keys per region per account, right?)

@wuisawesome
Copy link
Contributor

I'm honestly not sure what the best practice is for that use case, but whatever the solution, I don't think that 5k key max is a quota that can easily be bumped.

@architkulkarni
Copy link
Contributor Author

Failed test are unrelated. Will merge this for now. If we hit the limit again, we can make it configurable via env var or figure out another solution

@architkulkarni architkulkarni merged commit f2e867f into ray-project:master Jun 8, 2023
2 checks passed
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
Followup to ray-project#27506. This PR bumps it to 600.

Context from the previous PR copied below:

This is a minor QoL improvement to bump the hardcoded limit for number of aws keys per account. The limit is arbitrary and has been bumped before. AFAICT the fundamental aws limit is a 5000 key per region limit which we are not close to.

Signed-off-by: Archit Kulkarni <[email protected]>
Signed-off-by: e428265 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants