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.

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

feat: Validate k8s resource requirements for CPU, Memory and Ephemeral Volumes #944

Merged
merged 11 commits into from
Jan 29, 2024

Conversation

KengoA
Copy link
Contributor

@KengoA KengoA commented Jan 27, 2024

Pull Request Checklist

Description of PR

Currently, resource requirements are only partially validated for binary units with hera.workflows.validators.validate_storage_units() function for string values. The current implementaion misses some edge cases where the input is missing numerial values and only the unit is provided (i.e. "Ki" or "Mi"), and the comparison between request and limit is limited to int values for cpu_request and cpu_limit.

This PR

  • Refactors validation for binary units
    • Fixes float values not compared for CPU requests/limits
  • Adds validation for decimal units representation of CPU requests/limits in string format
  • Adds converters from decimal and binary units to float values
  • Adds comparison of decimal units (CPU) and binary units (Memory and Ephemeral Volumes) representations of resource requirements
  • Adds tests for hera/workflows/resources.py

…al units

Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
…y units for cpu, compare limits and requests for ephemeral and memory

Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
@KengoA KengoA changed the title Validate k8s resource requirements for CPU, Memory and Ephemeral Volumes feat: Validate k8s resource requirements for CPU, Memory and Ephemeral Volumes Jan 27, 2024
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Copy link

codecov bot commented Jan 27, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b3fc378) 80.7% compared to head (154d9fe) 81.0%.

Files Patch % Lines
src/hera/workflows/resources.py 78.9% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #944     +/-   ##
=======================================
+ Coverage   80.7%   81.0%   +0.2%     
=======================================
  Files         50      51      +1     
  Lines       3908    3942     +34     
  Branches     793     801      +8     
=======================================
+ Hits        3157    3195     +38     
+ Misses       564     561      -3     
+ Partials     187     186      -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
@@ -43,30 +43,47 @@ def validate_name(name: str, max_length: Optional[int] = None, generate_name: bo
return name


def validate_storage_units(value: str) -> None:
"""Validates the units of the given value.
def validate_binary_units(value: str) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed from validate_storage_units to validate_binary_units as this applies to memory and ephemeral volumes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this helper is public and we don't know who might use it outside of the core of Hera. Any chance we can leave validate_storage_units(value: str) -> None as public and internally it passes the value to validate_binary_units? That way we get the new function with improved functionality while maintaining backwards compatibility

Copy link
Contributor Author

@KengoA KengoA Jan 27, 2024

Choose a reason for hiding this comment

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

Thanks for your quick review!

In the commit below I kept the public function and used alias to refer to the _validate_binary_units, which is now made private so that we can change the implementaion later if necessary.

To avoid confusion and also to make the function names consistent, other function names are now named more explicitly for their use cases (validate_storage_units, validate_cpu_units, validate_memory_units) as well as converters.

31184f8

Raises:
------
ValueError
When the identified unit is not a supported one. The supported units are ['m', 'k', 'M', 'G', 'T', 'P', 'E'].
Copy link
Collaborator

Choose a reason for hiding this comment

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

The supported units are ['m', 'k', 'M', 'G', 'T', 'P', 'E'].

Small - maybe this can go into the docstring of value

Copy link
Contributor Author

@KengoA KengoA Jan 27, 2024

Choose a reason for hiding this comment

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

Good point! I added supported units in docstrings for validators and converters in the value section 154d9fe.

Copy link
Collaborator

@flaviuvadan flaviuvadan left a comment

Choose a reason for hiding this comment

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

Thanks for the first time contribution @KengoA! This is really great :) Left some small feedback on backwards compatibility and docs. The rest looks great

@flaviuvadan flaviuvadan added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Jan 27, 2024
@flaviuvadan flaviuvadan linked an issue Jan 27, 2024 that may be closed by this pull request
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
Signed-off-by: KengoA <20113339+KengoA@users.noreply.github.com>
@elliotgunton elliotgunton added semver:minor A change requiring a minor version bump and removed semver:patch A change requiring a patch version bump labels Jan 29, 2024
Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @KengoA 🚀

@elliotgunton elliotgunton enabled auto-merge (squash) January 29, 2024 10:56
@elliotgunton elliotgunton merged commit d267da3 into argoproj-labs:main Jan 29, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Robust validation for k8s resource requirements
3 participants