-
Notifications
You must be signed in to change notification settings - Fork 84
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
agent: support scaling cooldowns to aid oscillation migration. #117
Conversation
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.
A few comments, but nothing major. The most important one is the mutex for the ticker, I think we can have a race condition without it (but that was already true before this PR).
Cooldown is now supported within a scaling policy as a time duration. If the scaling policy omits the cooldown field, a default value will be used. The default value can be controlled by the CLI and has a default value of 5mins. In the event the agent successfully registers a scaling action, the policy manager will be instructed to place the policy eval handler in cooldown. The cooldown is enforced by performing a timed block using time.Timer. Target plugins can optionally return a meta entry to detail the last scaling event from their prespective. This allows the autoscaler to enforce cooldowns, even when scaling actions have been triggered manually by operators. The flow is similar to that mentioned above, the only difference being that the block period is based on the calculated remaning cooldown time from the last event timestamp.
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.
Much clear solution now. There's no state to keep track of anymore since the Go routine will block during cooldown, so it reduces complexity and mental loading when reasoning about the code.
I made a few suggestions but nothing major.
Good stuff 👍
Co-authored-by: Christian Winther <jippignu@gmail.com>
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.
some minor comments, no big changes.
Cooldown is now supported within a scaling policy as a time
duration. If the scaling policy omits the cooldown field, a
default value will be used. The default value can be controlled
by the CLI and has a default value of 5mins.
In the event the agent successfully registers a scaling action,
the policy manager will be instructed to place the policy eval
handler in cooldown by updating the ticker to the cooldown time
duration. The ticker is reset to its eval time duration on the
first tick out of cooldown.
Target plugins can optionally return a meta entry to detail the
last scaling event from their prespective. This allows the
autoscaler to enforce cooldowns, even when scaling actions have
been triggered manually by operators. The flow is similar to that
mentioned above, the only difference being that the ticker update
is based on the calculated remaning cooldown time from the last
event timestamp.
closes #12