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

fix: Ensure that resource limits that exactly equal capacity are allowed #200

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

willthames
Copy link
Contributor

Fixes #198

Description
Change the definition of ExceededBy to allow exact resource matches (this can happen if a Provisioner has
spec.resources.limits.cpu set to a value identical to a node's CPU count (or a multiple of a node's CPU count))

How was this change tested?

I wasn't able to find an existing test suite that handled node provisioning (I did find a pod provisioning suite but couldn't write a test that would fail before this change and would work after this change). i.e. this change is as yet untested

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@willthames willthames requested a review from a team as a code owner February 2, 2023 06:09
@sftim
Copy link

sftim commented Feb 2, 2023

AIUI, this can happen even without:

spec.resources.limits.cpu set to a value identical to a node's CPU count (or a multiple of a node's CPU count))

For example: eligible nodes come in sizes of 12, 20, 32 or 40 CPUs, and the limit is set to 112 CPU. That should be able to be reached exactly even though the limit isn't a multiple of any eligible node's CPU count.

@jonathan-innis
Copy link
Member

@willthames Do you mind adding a unit test alongside your change to catch this in the future?

@jonathan-innis
Copy link
Member

Otherwise, LGTM! 🚀

@coveralls
Copy link

coveralls commented Feb 2, 2023

Pull Request Test Coverage Report for Build 4079333176

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 12 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 78.355%

Files with Coverage Reduction New Missed Lines %
pkg/controllers/deprovisioning/controller.go 3 76.54%
pkg/controllers/deprovisioning/events/events.go 9 80.0%
Totals Coverage Status
Change from base Build 4069455737: 0.0%
Covered Lines: 6219
Relevant Lines: 7937

💛 - Coveralls

@jonathan-innis jonathan-innis self-assigned this Feb 2, 2023
Change the definition of ExceededBy to allow exact resource
matches (this can happen if a Provisioner has
`spec.resources.limits.cpu` set to a value identical to a node's
CPU count (or a multiple of a node's CPU count))
Copy link
Member

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis merged commit 2018a5e into kubernetes-sigs:main Feb 2, 2023
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.

Deprovisioning fails when node resources exactly equal to provisioner resources
4 participants