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

Force the planner to use a certain instance type #42

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Force the planner to use a certain instance type #42

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b03b37
Force the planner to use a certain instance type
Feb 9, 2023
cb39a50
Split _allow_hw to handle instances and drives seperately
Feb 14, 2023
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
18 changes: 14 additions & 4 deletions service_capacity_modeling/capacity_planner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,21 @@ def _allow_hardware(
lifecycle: Lifecycle,
allowed_names: Sequence[str],
allowed_lifecycles: Sequence[Lifecycle],
exact_match: bool
) -> bool:

# If the user has explicitly asked for particular families instead
# of all lifecycles filter based on that
if allowed_names:
if name not in allowed_names:
if exact_match:
for allowed_name in allowed_names:
if name == allowed_name:
return True
return False
else:
if name not in allowed_names:
return False
return True
# Otherwise consider lifecycle (default)
else:
if lifecycle not in allowed_lifecycles:
Expand Down Expand Up @@ -365,16 +374,17 @@ def _plan_certain(

plans = []
for instance in hardware.instances.values():
exact_match = instance.family_separator in instance.family
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always return true false. I think you want to do the for loop like I suggested inside the method against the strings in instance_families

Copy link
Author

Choose a reason for hiding this comment

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

@jolynch - Does it make sense to have separate functions?
def _allow_instance
and
def _allow_drives

We use this function for the disk drives below on line 377. Should we keep this change specific to instance?
https://github.com/Netflix-Skunkworks/service-capacity-modeling/blob/main/service_capacity_modeling/capacity_planner.py#L377

if not _allow_hardware(
instance.family, instance.lifecycle, instance_families, lifecycles
instance.family, instance.lifecycle, instance_families, lifecycles, exact_match
):
continue

if per_instance_mem > instance.ram_gib:
continue

for drive in hardware.drives.values():
if not _allow_hardware(drive.name, drive.lifecycle, drives, lifecycles):
if not _allow_hardware(drive.name, drive.lifecycle, drives, lifecycles, False):
continue

plan = self._models[model_name].capacity_plan(
Expand Down Expand Up @@ -578,4 +588,4 @@ def _sub_models(


planner = CapacityPlanner()
planner.register_group(netflix.models)
planner.register_group(netflix.models)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: add back in a newline (UNIX mode not Windows)