Add support for setting lower/upper-bounds for autotuned resources #3744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At the moment this only supports a specific subset of top-level resource
keys:
* cpus
* mem
* disk
Supporting {min,max}_instances should be pretty trivial (but I'm not
quite sure when we'd want to use that yet, so I left those out of the
allowlist).
I'm explicitly not supporting these values for sidecars since:
a) it seemed like it would complicate things a bit and,
b) I'm not sure we have a usecase for this :p
nemacysts
force-pushed
the
luisp/COMPINFRA-3424-autotune-mins
branch
from
e1d8c9f to
aad1ce9
Compare
nemacysts
commented
Dec 12, 2023
| @@ -276,7 +279,7 @@ def get_recommendations_by_service_file( | |||
| result["cluster"], | |||
| ) # e.g. (foo, marathon-norcal-stagef) | |||
| instance_type = result["cluster"].split("-", 1)[0] | |||
| rec: Union[KubernetesRecommendation, CassandraRecommendation] | |||
| rec: Union[KubernetesRecommendation, CassandraRecommendation] = {} | |||
There was a problem hiding this comment.
not super necessary, but pyright was correctly calling this out as a potentially unbound variable below (and I figured this is innocuous enough to sneak in here :p)
nemacysts
requested review from
EvanKrall,
jfongatyelp,
cuza,
bobokun,
EmanElsaban and
KaspariK
EvanKrall
reviewed
Dec 13, 2023
paasta_tools/config_utils.py
Outdated
Comment on lines
295
to
297
| for limit_type, limits in ( | ||
| existing_configs[instance_name].get("autotune_limits", {}).items() | ||
| ): |
There was a problem hiding this comment.
Would it make sense to move this whole for loop into its own function, maybe called clamp_recommendations? This function is a bit long.
jfongatyelp
approved these changes
Dec 14, 2023
* Add docstring explaining types * Match return type * Update a test to make sure that we're doing the right thing with multiple instances in the same file
EvanKrall
approved these changes
Dec 14, 2023
EvanKrall
approved these changes
Dec 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment this only supports a specific subset of top-level resource keys:
Supporting {min,max}_instances should be pretty trivial (but I'm not quite sure when we'd want to use that yet, so I left those out of the allowlist).
I'm explicitly not supporting these values for sidecars since: a) it seemed like it would complicate things a bit and, b) I'm not sure we have a usecase for this :p