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

Clean-up reconcileAutoscaler and improve it's unittest coverage #53728

Closed
MaciekPytel opened this Issue Oct 11, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@MaciekPytel
Contributor

MaciekPytel commented Oct 11, 2017

We recently had 2 serious bugs caused by non-trivial logic calculating desiredReplicas in hpa reconcileAutoscaler method (#48997, #53690).

I think we should extract this part of logic (starting around https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/podautoscaler/horizontal.go#L404) to a separate function and make sure it's covered with unittests for different possible edge condition. The logic itself takes a few ints as input (calculated desiredReplicas, min/max replicas, currentReplicas, ...) to produce a single integer value and set some status conditions, making it very easily unittestable if we extract it to a separate function.

WDYT @mwielgus, @DirectXMan12?

@mwielgus

This comment has been minimized.

Show comment
Hide comment
@mwielgus

mwielgus Oct 11, 2017

Contributor

+100. It would be super unfortunate if we had yet another issue in this area of the code.

Contributor

mwielgus commented Oct 11, 2017

+100. It would be super unfortunate if we had yet another issue in this area of the code.

@DirectXMan12

This comment has been minimized.

Show comment
Hide comment
@DirectXMan12

DirectXMan12 Oct 11, 2017

Contributor

agreed. @mattjmcnaughton you've been going over these pieces of code recently, you want to tackle this?

Contributor

DirectXMan12 commented Oct 11, 2017

agreed. @mattjmcnaughton you've been going over these pieces of code recently, you want to tackle this?

@mattjmcnaughton

This comment has been minimized.

Show comment
Hide comment
@mattjmcnaughton

mattjmcnaughton Oct 11, 2017

Contributor
Contributor

mattjmcnaughton commented Oct 11, 2017

hh pushed a commit to ii/kubernetes that referenced this issue Nov 20, 2017

Merge pull request #54701 from mattjmcnaughton/mattjmcnaughton/53728-…
…clean-up-reconcileAutoscaling

Automatic merge from submit-queue (batch tested with PRs 55974, 54701). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Refactor `reconcileAutoscaler` method in hpa

**What this PR does / why we need it**:

There have been a couple of recent bugs in the "normalizing" part of the
`reconcileAutoscaler` method. This part of the code base is responsible
for, among other things, taking the suggested desired replicas based on
the metrics, ensuring it conforms to certain conditions, and updating it
if it does not. Isolate the part that converts the desired replicas
based on a given set of rules into its own function.

We are refactoring this part of the code base to make the logic simpler
and to make it easier to write unit tests.

**Which issue this PR fixes**: 
Fixes #53728

**Release note**:
```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment