Skip to content

Configurable HorizontalPodAutoscaler: business logic change#2

Open
gliush wants to merge 15 commits intoconfigurable-hpa-limitsfrom
configurable-hpa-limits-logic-change
Open

Configurable HorizontalPodAutoscaler: business logic change#2
gliush wants to merge 15 commits intoconfigurable-hpa-limitsfrom
configurable-hpa-limits-logic-change

Conversation

@gliush
Copy link
Copy Markdown
Owner

@gliush gliush commented Oct 14, 2019

I introduce an algorithm-agnostic HPA object configuration that will configure each particular HPA scaling behavior.
This is the second PR with all the business logic changes.
The first PR with API changes and full explanation: kubernetes#74525

KEP PR: kubernetes/enhancements#883
KEP PR: kubernetes/enhancements#1234

What type of PR is this?
/kind feature

@gliush gliush force-pushed the configurable-hpa-limits branch from 2af47cc to 7f3f689 Compare October 15, 2019 10:05
@gliush gliush force-pushed the configurable-hpa-limits-logic-change branch from a362978 to d2bb60a Compare October 15, 2019 11:53
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This function should also be renamed in that case to calculateScaleUpLimitBehavior

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Renamed it to calculateScaleUpLimitWithScalingRules

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should generate the fully populated behavior object only one and use it in normalizeDesiredReplicas and in storeScaleEvent.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yep, agreed, done.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we want to store scale events for HPAs which don't have behavior. I would argue that we should simply wrap this function call in the behavior!=nil condition as well.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Agreed. Removed: b9b06eb

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

assert.Equal takes a list of arguments so the fmt.Sprintf is not required.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed.

@gliush
Copy link
Copy Markdown
Owner Author

gliush commented Oct 18, 2019

@arjunrn : could you have another look? I've fixed everything in my last commit: b9b06eb

@gliush gliush force-pushed the configurable-hpa-limits-logic-change branch from 52dad0d to dde049b Compare October 23, 2019 08:24
@gliush gliush force-pushed the configurable-hpa-limits branch from 877d2ff to 76342ba Compare October 30, 2019 12:03
- Uses capital letters for const
- Uses JSON names for fields
- Specified default values
@gliush gliush force-pushed the configurable-hpa-limits branch from 13f7ddc to c32de0c Compare October 31, 2019 09:26
@gliush gliush force-pushed the configurable-hpa-limits-logic-change branch from dde049b to fc130c7 Compare October 31, 2019 17:58
@gliush gliush force-pushed the configurable-hpa-limits-logic-change branch from fc130c7 to e05ef55 Compare November 1, 2019 09:24
@gliush gliush force-pushed the configurable-hpa-limits branch from 288e5de to b6b34cb Compare November 13, 2019 14:36
@gliush gliush force-pushed the configurable-hpa-limits branch 3 times, most recently from 72a1076 to 14bb2cd Compare November 14, 2019 18:00
@arjunrn arjunrn force-pushed the configurable-hpa-limits branch 3 times, most recently from 3ababc8 to 8b159e8 Compare December 9, 2019 07:06
@gliush gliush force-pushed the configurable-hpa-limits branch from 8b159e8 to bad0417 Compare December 10, 2019 16:42
@arjunrn arjunrn force-pushed the configurable-hpa-limits branch from bad0417 to 8ab2262 Compare December 10, 2019 17:09
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.

2 participants