Incorrect resources allocation #2224
Merged
Conversation
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
…ved to always be negative.
…yResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it.
…limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible).
This was referenced Aug 25, 2025
…n data preparation
… flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible)
…pecial definition of resource, it will take its default resource instead to be more concise in calculation of requirements
…s for calculations dependant on the requests or limits & requests (for missing limit)
…ube-HPC/hkube into incorrect_resources_allocation
golanha
previously approved these changes
Aug 31, 2025
Member
golanha
left a comment
golanha
left a comment
There was a problem hiding this comment.
@golanha reviewed 14 of 14 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Adir111)
hkube-ci
pushed a commit
that referenced
this pull request
Aug 31, 2025
* refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci]
hkube-ci
pushed a commit
that referenced
this pull request
Aug 31, 2025
* refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci]
RonShvarz
pushed a commit
that referenced
this pull request
Sep 7, 2025
* refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version
RonShvarz
pushed a commit
that referenced
this pull request
Sep 7, 2025
* refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci]
RonShvarz
pushed a commit
that referenced
this pull request
Sep 7, 2025
* refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci]
RonShvarz
added a commit
that referenced
this pull request
Sep 8, 2025
* Incorrect resources allocation (#2224) * refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version * Incorrect resources allocation (#2224) * refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci] * Incorrect resources allocation (#2224) * refactored names and added jsdoc * fixed bug where workers stats under node were not calculated correctly * fixed bug - bad calculation, as long free is negative it causes reserved to always be negative. * added new method to get default value for resources, via limitrange * added stub data for test * fixed bug - should apply workerResourceRequests only if settings.applyResources is true, plus fixed merged method to not include fields that are empty or null, since null will fail k8s job and empty will just be odd in the job spec, better of without it. * refactor a bit, and changed behavior to take max out of requests and limits if useResouceLimits is on (some pods have no limit, which can cause limits to be lower than requests, which is impossible). * jsdoc was added and small refactor * now taking worker resources in account when calculating resources per algortihm * extracted default container resources out of the reconcile, and now in data preparation * fix all graph calculations errors * Now takes maximum between request and limit in case useResourceLimits flag is true, in case many pods have no limit and causes limit to be lower than request (which is impossible) * Passing default resources, so when calculating a worker without any special definition of resource, it will take its default resource instead to be more concise in calculation of requirements * Now sending actual request to other always, while keeping the requests for calculations dependant on the requests or limits & requests (for missing limit) * fixed free&other calculation after changes in task-executor * . * added unit tests to test the method after it changes. * update k8s client version .... bump version [skip ci] * moved size into condition (should verify requestedAlgorithm) (#2231) * moved size into condition (should verify requestedAlgorithm) (#2231) .... bump version [skip ci] * moved size into condition (should verify requestedAlgorithm) (#2231) .... bump version [skip ci] * Trigger CI --------- Co-authored-by: Adir David <[email protected]> Co-authored-by: GitHub Action <[email protected]>
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.
IMPORTANT - This PR cannot be merged until this one: kube-HPC/kubernetes-client.hkube#39 is merged, and package.lock file is updated with the new version of our k8s client. Also, this PR should be merged before as well: #2224
Related issue - #2185
This change is