Add support for autotune limits in service_shard_update #3830
Conversation
| "min_instances": args.min_instance_count, | ||
| "max_instances": args.prod_max_instance_count | ||
| if deploy_prefix == "prod" | ||
| else args.non_prod_max_instance_count, |
There was a problem hiding this comment.
@nemacysts (from #3829) how would you suggest testing paasta's behavior with no max/min instances?
There was a problem hiding this comment.
@karmeleon sorry, yesterday was a busy day!
i don't think we've setup the local paasta playground for autoscaling, but we should be able to create an instance in infrastage (or any test cluster) and see what happens
lemme do that real fast, actually :)
There was a problem hiding this comment.
whoops - replied in the wrong place: #3829 (comment)
There was a problem hiding this comment.
(i.e., we'd need to tweak paasta to set a default min/max instances if one is not set explicitly or by autotune)
There was a problem hiding this comment.
thanks for checking! to clarify, we're totally okay with min/max instances both being at 1 for brand new shards, since they typically see traffic ramp up gradually enough for autotune to deal with it. do you think it's safe to ship this branch now, or should we wait for that ticket to be completed?
There was a problem hiding this comment.
i'm trying to verify that autotune will do the "right" thing and setup min/max instances - but when i ran the rightsizer scripts locally it didn't seem to...that said, i'm not sure if it's cause i need to wait for splunk to have more data
There was a problem hiding this comment.
no worries, think we're gonna go in and manually update live configs to add autotune min/maxes to stabilize things a bit
There was a problem hiding this comment.
whoops, sorry - forgot about this: i think i hallucinated that autotune would fixup the autoscaling config: my test instance has not had autotune add min/max instances correctly :(
There was a problem hiding this comment.
(i.e., we'll either want to revert the {min,max}_instances changes here or tackle PAASTA-18283)
| "min_instances": args.min_instance_count, | ||
| "max_instances": args.prod_max_instance_count | ||
| if deploy_prefix == "prod" | ||
| else args.non_prod_max_instance_count, |
There was a problem hiding this comment.
(i.e., we'll either want to revert the {min,max}_instances changes here or tackle PAASTA-18283)
| parser.add_argument( | ||
| "-d", | ||
| "--dry-run", | ||
| help="Do not commit changes to git", | ||
| action="store_true", | ||
| dest="dry_run", | ||
| ) |
There was a problem hiding this comment.
++
(i'm also a fan of --for-real flags so that dry-run is the default, but that would be a little more annoying to rollout here)
| args.autotune_max_disk, | ||
| ) | ||
| ): | ||
| instance_config["autotune"] = {} |
There was a problem hiding this comment.
| instance_config["autotune"] = {} | |
| instance_config["autotune_limits"] = {} |
is the key we want instead
might also be nice to do something like:
limit_config = instance_config["autotune_limits"]
...
limit_config["cpus"]["min"] = args.autotune_min_cpus
...so that we don't need to repeat instance_config["autotune_limits"] so many times :)
There was a problem hiding this comment.
i think maybe we could trim this code down by doing something like:
limit_config["cpus"] = {
"min": args.autotune_min_cpus,
"max": args.autotune_max_cpus
}
limit_config["mem"] = {
"min": args.autotune_min_mem,
"max": args.autotune_max_mem
}
limit_config["disk"] = {
"min": args.autotune_min_disk,
"max": args.autotune_max_disk
}and then adding a processing step that removes any min/max keys with None values - and any resource (e.g., cpus, mem, disk) keys with no subkeys
(that said, maybe the code to do that processing step wouldn't look too different and doing this would be a waste of time :p)
Added to PaaSTA at large in #3744. Also pulls in commit from #3829 to make min/max instance values optional and adds a new
--dry-runparam that skips committing and pushing changes to yelpsoa-configs.Confirmed that the new kwargs do as expected, generating valid configs. If anyone knows a way to do this that doesn't involve a mess of nested if statements, I'm all ears lol