-
Notifications
You must be signed in to change notification settings - Fork 2k
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
in Job.Scale, ensure that new count is within [min,max] configured in scaling policy #9761
Conversation
… scaling policy resolves #9758
@@ -53,7 +53,7 @@ func testJobWithScalingPolicy() *Job { | |||
job.TaskGroups[0].Scaling = &ScalingPolicy{ | |||
Policy: map[string]interface{}{}, | |||
Min: int64ToPtr(1), | |||
Max: int64ToPtr(1), | |||
Max: int64ToPtr(5), |
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.
fixing the bug meant that a test which had previously (incorrectly) passed started to fail. this makes room for that test to properly succeed.
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.
LGTM 👍
$ nomad job scale example 1
Error submitting scaling request: Unexpected response code: 400 (new scaling count cannot be less than the scaling policy minimum)
$ nomad job scale example 100
Error submitting scaling request: Unexpected response code: 400 (new scaling count cannot be greater than the scaling policy maximum)
A small suggestion would be to have the min/max value in the error message (and maybe call it group count
instead of scaling count
?)
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
the
Job.Scale
endpoint did not enforce min/max from the group scaling policy. this PR addresses that with a simple check. unit tested the RPC; I had to make a small tweak to one of theapi
scaling tests because it was previously (incorrectly) scaling outside of bounds.resolves #9758