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.

Sign up for GitHub

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 support for autotune limits in service_shard_update #3830

Merged
merged 6 commits into from
Oct 14, 2024
Merged

Add support for autotune limits in service_shard_update #3830

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b92bc1
Add autotune min/max support to service_shard_update
karmeleon Apr 15, 2024
0a0e67c
optional min/max instance values instead of defaults
kedarv Apr 15, 2024
f771248
whoops pre-commit didn't install
karmeleon Apr 15, 2024
6ce5f17
Merge branch 'master' of github.com:Yelp/paasta into add_autotune_lim…
karmeleon Oct 11, 2024
08b222a
revert changes to min/max instances, clean up autotune limits code
karmeleon Oct 11, 2024
ef31617
fix iteration
karmeleon Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 89 additions & 4 deletions paasta_tools/contrib/service_shard_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ def parse_args():
action="store_true",
dest="verbose",
)
parser.add_argument(
"-d",
"--dry-run",
help="Do not commit changes to git",
action="store_true",
dest="dry_run",
)
Comment on lines +42 to +48
Copy link
Member

Choose a reason for hiding this comment

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

++

(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)

parser.add_argument(
"--source-id",
help="String to attribute the changes in the commit message.",
Expand Down Expand Up @@ -115,6 +122,48 @@ def parse_args():
type=int,
dest="timeout_server_ms",
)
parser.add_argument(
"--autotune-min-cpus",
help="Minimum number of CPUs Autotune should give the shard",
required=False,
type=float,
dest="autotune_min_cpus",
)
parser.add_argument(
"--autotune-max-cpus",
help="Maximum number of CPUs Autotune should give the shard",
required=False,
type=float,
dest="autotune_max_cpus",
)
parser.add_argument(
"--autotune-min-mem",
help="Minimum amount of memory Autotune should give the shard",
required=False,
type=int,
dest="autotune_min_mem",
)
parser.add_argument(
"--autotune-max-mem",
help="Maximum amount of memory Autotune should give the shard",
required=False,
type=int,
dest="autotune_max_mem",
)
parser.add_argument(
"--autotune-min-disk",
help="Minimum amount of disk Autotune should give the shard",
required=False,
type=int,
dest="autotune_min_disk",
)
parser.add_argument(
"--autotune-max-disk",
help="Maximum amount of disk Autotune should give the shard",
required=False,
type=int,
dest="autotune_max_disk",
)
return parser.parse_args()


Expand Down Expand Up @@ -195,9 +244,11 @@ def main(args):
instance_config = {
"deploy_group": f"{deploy_prefix}.{args.shard_name}",
"min_instances": args.min_instance_count,
"max_instances": args.prod_max_instance_count
if deploy_prefix == "prod"
else args.non_prod_max_instance_count,
"max_instances": (
args.prod_max_instance_count
if deploy_prefix == "prod"
else args.non_prod_max_instance_count
),
"env": {
"PAASTA_SECRET_BUGSNAG_API_KEY": "SECRET(bugsnag_api_key)",
},
Expand All @@ -217,6 +268,40 @@ def main(args):
instance_config["cpus"] = args.cpus
if args.mem is not None:
instance_config["mem"] = args.mem
if any(
(
args.autotune_min_cpus,
args.autotune_max_cpus,
args.autotune_min_mem,
args.autotune_max_mem,
args.autotune_min_disk,
args.autotune_max_disk,
)
):
limit_config = {}
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,
}

# remove any None values to keep the config clean
for resource in list(limit_config):
for key in list(limit_config[resource]):
Comment on lines +296 to +297
Copy link
Member

Choose a reason for hiding this comment

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

(these list()'s aren't necessary, but i'm assuming they're were helpful in some way while you were debugging and converting from a view to a list shouldn't really impact perf in a meaningful way for this script)

Copy link
Contributor Author

@karmeleon karmeleon Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

unfortunately they are necessary under py3 :( without them i get this

Traceback (most recent call last):
  File "paasta_tools/contrib/service_shard_update.py", line 350, in <module>
    main(args)
  File "paasta_tools/contrib/service_shard_update.py", line 297, in main
    for key in limit_config[resource]:
RuntimeError: dictionary changed size during iteration

Copy link
Member

Choose a reason for hiding this comment

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

oh d'oh - i'm dumb: we're mutating the dict

i blame friday-brain :p

if limit_config[resource][key] is None:
del limit_config[resource][key]
if len(limit_config[resource]) == 0:
del limit_config[resource]

if len(limit_config) > 0:
instance_config["autotune_limits"] = limit_config
# If the service config does not contain definitions for the shard in each ecosystem
# Add the missing definition and write to the corresponding config
if args.shard_name not in config_file.keys():
Expand Down Expand Up @@ -247,7 +332,7 @@ def main(args):
log.info(f"{args.shard_name} is in smartstack config already, skipping.")

# Only commit to remote if changes were made
if changes_made:
if changes_made and not args.dry_run:
updater.commit_to_remote()
trigger_deploys(args.service)
else:
Expand Down
Loading