Repository navigation
fix: accept a fractional CPU limit on the task container - #61
Open
eastagiletracker wants to merge 2 commits into
Open
eastagiletracker wants to merge 2 commits into
eastagiletracker wants to merge 2 commits into
Conversation
A task_resource_requirements.limits.cpu of "1.5" or "0.5" is a valid Kubernetes quantity and passes the CRD, but cpu_string_to_decimal ran it through int() and raised ValueError, so the settings ConfigMap never rendered and every reconcile of the deployment failed. Parse the quantity as a Decimal, keep rounding down to whole CPUs as before, and raise the intended AnsibleFilterError for anything that is not a CPU quantity. That error path never worked either: __ERROR_MSG inside the class is name-mangled and raised NameError instead.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unbounded decimal exponents can cause excessive controller resource consumption during integer conversion.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates CPU parsing to support fractional Kubernetes CPU limits while preserving whole-core rounding.
Changes:
- Parses CPU values using
Decimal. - Corrects filter error handling for invalid values.
| File | Description |
|---|---|
roles/installer/filter_plugins/cpu_string_to_decimal.py |
Adds fractional CPU parsing and validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| raise AnsibleFilterError("%s: %r" % (ERROR_MSG, cpu_string)) | ||
|
|
||
| return cpu | ||
| return int(cpu) |
Decimal accepts an exponent of any size, so a limit such as "1e1000000" reached int() and spent seconds of controller CPU building an integer with a million digits. With an m suffix the division could instead raise decimal.Overflow, which the InvalidOperation handler did not catch. Work in millicores and reject anything negative or above int64 millicores, the largest CPU limit Kubernetes can apply, before converting. Catch every decimal signal, not only InvalidOperation. Valid limits render the same.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This PR proposes a fix so that a fractional CPU limit on the task container (for example
task_resource_requirements.limits.cpu: "1.5") no longer stops the installer from rendering the settings ConfigMap. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/625. You can sign in with your GitHub ID to claim ownership of the project.The defect
configmaps/config.yaml.j2passes the task container's CPU limit through thecpu_string_to_decimalfilter to sizeJOB_EVENT_WORKERS. The filter handles millicores ("2000m") and whole cores ("6"), but a fractional core count such as"1.5","2.5"or"0.5"goes straight intoint()and raisesValueError. Those values are valid Kubernetes quantities, the CRD accepts them (the field istype: string), and Ascender itself readsSYSTEM_TASK_ABS_CPU = '1.5'fine, so the only thing that breaks is the operator: the template fails, the ConfigMap is never applied, and every reconcile of that deployment fails. The filter's own error path is broken too:__ERROR_MSGis read inside the class, so Python name-mangles it to_FilterModule__ERROR_MSGand a non-string value raisesNameErrorinstead of the intended message.Reproduced at current
devel(649823e) by rendering the real template with the role defaults and the repo's filter plugin, passing the limit the way the operator does (as an extra var):render.ymlis a five-line play that loadsroles/common/defaults/main.ymlandroles/installer/defaults/main.ymland doeslookup('template', 'roles/installer/templates/configmaps/config.yaml.j2').The change
One file,
roles/installer/filter_plugins/cpu_string_to_decimal.py. The quantity is now parsed as aDecimal(millicores divided by 1000), and the result still rounds down to whole CPUs exactly as before, so every value that worked keeps its output. Anything that is not a CPU quantity ("abc","", a non-string,NaN) now raises the intendedAnsibleFilterErrornaming the bad value, with the message moved to module level so it resolves.Verification
Filter output before and after, for the same inputs:
100m,2000m,4500m6,81.5,2.5,0.5ValueError1500.5mValueErrorabcValueErrorAnsibleFilterError: Not a valid cpu value. Cannot process value: 'abc'6as an int,NoneNameError: name '_FilterModule__ERROR_MSG' is not definedAnsibleFilterErrorRendering the full ConfigMap after the change:
1.5,2.5and0.5render withSYSTEM_TASK_ABS_CPUand noJOB_EVENT_WORKERS(at or below the default of 4),5.5rendersJOB_EVENT_WORKERS = 5,6and6500mrenderJOB_EVENT_WORKERS = 6as before, and the default spec with no limits renders unchanged.yamllint rolesandansible-lint --offline -p roles/installerreport the same findings before and after the change, with nothing new. I did not run the molecule scenario, which needs a kind cluster; the table above can be replayed with justansible-corefrom the repo root.How this was managed
This fix was tracked as its own story on the Ascender Operator board, which was imported from this repository's 59 issues and pull requests and used to manage this work.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/