-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add poller autoscaler #1184
Add poller autoscaler #1184
Conversation
b1e5751
to
cb857fe
Compare
Pull Request Test Coverage Report for Build 0182dc0d-f5a2-4033-bffd-2b4b85a25e34
💛 - Coveralls |
want: Resource(12), | ||
}, | ||
{ | ||
name: "over utilized, scale up", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicates test above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
want: Resource(10), | ||
}, | ||
{ | ||
name: "under utilized, scale down", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a test where usage is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
go func() { | ||
for { | ||
select { | ||
case <-p.ctx.Done(): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
}() | ||
} | ||
wg.Wait() | ||
time.Sleep(time.Millisecond * 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of sleeping yourself, better would be to use assert.Eventually() - less chance of race condition and should test faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. Thank you for the advice. I've added optional hook functions in the pollerAutoscaler implementation to simply the testing.
internal/common/autoscaler/types.go
Outdated
|
||
type ( | ||
// Resource is the unit of scalable resources | ||
Resource uint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be called ResourceCount, or something that signifies that it's an int / unit? The way "Resource" reads to me makes it sound like its more than an int.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I was more ambitious in thinking a single autoscaler can handle multiple resources. But it doesn't seem we will need it any time soon. So yeah, I've changed it to ResourceUnit.
internal/common/autoscaler/types.go
Outdated
Resource uint | ||
|
||
// UsageInMilli is the unit of Resource usage times 1000 | ||
UsageInMilli uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just to clarify the naming of this - its got nothing to do with milliseconds, right? As far as I can tell, its just resource usage * 1000. Milli in the name makes it sound like milliseconds, but maybe thats just me. Would this not work as a float?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed to MilliUsage similar to how kubernetes autoscaler named Millicores. https://sourcegraph.com/github.com/kubernetes/autoscaler@master/-/blob/vertical-pod-autoscaler/pkg/recommender/logic/recommender.go#L27:5
I was trying to avoid using float to avoid negative numbers from the beginning. Otherwise, I'll need to handle negative cases in a lot of places unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a valid reason not to use floats - I definitely prefer simplicity in this case too. How about calling it Millicores or MilliSomething?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for making those changes!
* add poller autoscaler * add tests * address comments
* add poller autoscaler * add tests * address comments
What changed?
Why?
Pollers constantly poll the cadence-frontend for tasks even though there are no tasks.
To reduce unnecessary polling, an autoscaler is used to limit the number of concurrent polls. This PR addresses the implementation of polleraAutoscaler and there will be a follow-up PR to add the autoscaler on pollers.
As for the design of autoscaler, a resizable semaphore is used to limit concurrency.
How did you test it?
unit tests
Potential risks