Skip to content

PEREL-5643: support Gi/Mi strings in autotune_limits mem/disk clamping - #4348

Open
matt-ullmer wants to merge 6 commits into
masterfrom
perel-5643-autotune-limits-string-support
Open

PEREL-5643: support Gi/Mi strings in autotune_limits mem/disk clamping#4348
matt-ullmer wants to merge 6 commits into
masterfrom
perel-5643-autotune-limits-string-support

Conversation

@matt-ullmer

Copy link
Copy Markdown
Contributor

Summary

  • Add _resource_value_to_mib() to normalize resource values (int/float MiB or K8s strings like "4Gi"/"512Mi") to a common unit before comparing
  • Update _clamp_recommendations() to use it so Cassandra clusters can set mem/disk floors via autotune_limits without TypeError on string comparisons
  • Fix pre-existing log bug: max-clamping branch was logging min_value instead of max_value

Cassandra autotune recommendations use Gi strings for mem/disk. Direct </> comparisons against integer limits would raise TypeError. This is the paasta-side change; a follow-up yelpsoa-configs PR will update the cassandra_k8s/validate.py schema from integer to string pattern once this is deployed.

Jira: https://jira.yelpcorp.com/browse/PEREL-5643

Test plan

  • pytest tests/test_config_utils.py — all 37 tests pass including new parametrized test_resource_value_to_mib cases and test_auto_config_updater_merge_recommendations_limits_gi_strings
  • pre-commit clean on changed files

Add _resource_value_to_mib() to convert int/float MiB or K8s strings
("4Gi", "512Mi") to a common unit before comparing in _clamp_recommendations.
Cassandra autotune outputs Gi strings; direct comparison against integer
limits would TypeError, blocking mem/disk floor configuration.
- Union[int, float, str] instead of Any on _resource_value_to_mib
- Compute unclamped_mib once; avoid parsing the same value twice
- Fix pre-existing log that printed min_value when clamping to max
@matt-ullmer
matt-ullmer marked this pull request as ready for review July 10, 2026 17:51
@matt-ullmer
matt-ullmer requested a review from a team as a code owner July 10, 2026 17:51
@matt-ullmer
matt-ullmer requested a review from nemacysts July 10, 2026 19:31
Comment thread paasta_tools/config_utils.py Outdated
Comment thread paasta_tools/config_utils.py Outdated
Comment thread paasta_tools/config_utils.py
Comment thread paasta_tools/config_utils.py Outdated
Comment on lines +317 to +318
unclamped_mib = _resource_value_to_mib(unclamped_resource_value)
if min_value and unclamped_mib < _resource_value_to_mib(min_value):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmmm, the naming here is gonna be a bit confusing - this is used for all 3 resource types (cpu, mem, disk), but only applies to mem/disk

...not sure if we wanna branch of the type or if we want to just have a comment here explaining the weirdness

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@matt-ullmer i think this is the only outstanding comment now - i'm not really sure what's best though :p

i think another potential option might be something like

for limit_type, limits in config.get("autotune_limits", {}).items():
            log.debug(f"Processing {limit_type} autotune limits...")
            min_value = _normalized_value(limits.get("min"), limit_type)
            max_value = _normalized_value(limits.get("max"), limit_type)
            unclamped_resource_value = _normalized_value(clamped_recomendation.get(limit_type), limit_type)

where _normalized_value is something like:

NORMALIZABLE_VALUE_TYPES = {"mem", "disk", ...}

...

def _normalized_value(value: int | float | str, value_type: str) -> float: # XXX: maybe an enum/Literal/etc for value_type would work?
    """Normalize a resource value to MiB for comparison. For numeric types
    (e.g. cpus as float), this is a passthrough cast to float."""

    if isinstance(value, (int, float)):
        return float(value)
    elif value_type in NORMALIZABLE_VALUE_TYPES and isinstance(value, str):
        for suffix, factor in _RESOURCE_UNIT_TO_MIB.items():
            if value.endswith(suffix):
                return float(value.removesuffix(suffix)) * factor

    raise ValueError(f"Cannot parse {value_type} resource value: {value!r}")

(naming yolo'd - i am not tied to any of the names here and didn't think too hard about them)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea — went with something close to your sketch. Pushed 2a1b013: renamed to _normalize_resource_value(value, resource_type) with a _SUFFIX_RESOURCE_TYPES = {"mem", "disk"} guard. Numeric values pass through for any type, suffix parsing only fires for mem/disk, and strings for cpu now raise ValueError. Added a test for the cpu-string case too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we still want to remove the unclamped_mib naming since not all of these will have an mib unit value?

- int | float | str instead of Union[int, float, str]
- str.removesuffix() instead of manual slice indexing
- Add docstring clarifying the numeric passthrough for cpus
Rename _resource_value_to_mib to _normalize_resource_value and add a
resource_type param so suffix parsing (Gi/Mi/Ki) only fires for
mem/disk. Strings passed for cpu now raise ValueError.
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.

2 participants