From ebd28c527c42905c949ff76298d47be35e46152a Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Fri, 8 Jan 2021 19:24:36 +0000 Subject: [PATCH 1/4] in Job.Scale, ensure that new count is within [min,max] configured in scaling policy resolves #9758 --- nomad/job_endpoint.go | 12 +++++++++++ nomad/job_endpoint_test.go | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 379c759d4c4..01cc4343759 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1039,6 +1039,18 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes prevCount := found.Count if args.Count != nil { + // if there is a scaling policy, check that the new count is within bounds + if found.Scaling != nil { + if *args.Count < found.Scaling.Min { + return structs.NewErrRPCCoded(400, + fmt.Sprintf("new scaling count cannot be less than the scaling policy minimum")) + } + if found.Scaling.Max < *args.Count { + return structs.NewErrRPCCoded(400, + fmt.Sprintf("new scaling count cannot be greater than the scaling policy maximum")) + } + } + // Lookup the latest deployment, to see whether this scaling event should be blocked d, err := snap.LatestDeploymentByJobID(ws, namespace, args.JobID) if err != nil { diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 684557060f6..c255f8cfb95 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -6705,6 +6705,50 @@ func TestJobEndpoint_Scale_Invalid(t *testing.T) { require.Contains(err.Error(), "should not contain count if error is true") } +func TestJobEndpoint_Scale_OutOfBounds(t *testing.T) { + t.Parallel() + require := require.New(t) + + s1, cleanupS1 := TestServer(t, nil) + defer cleanupS1() + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + state := s1.fsm.State() + + job, pol := mock.JobWithScalingPolicy() + pol.Min = 3 + pol.Max = 10 + job.TaskGroups[0].Count = 5 + + // register the job + err := state.UpsertJob(structs.MsgTypeTestSetup, 1000, job) + require.Nil(err) + + var resp structs.JobRegisterResponse + scale := &structs.JobScaleRequest{ + JobID: job.ID, + Target: map[string]string{ + structs.ScalingTargetGroup: job.TaskGroups[0].Name, + }, + Count: helper.Int64ToPtr(pol.Max + 1), + Message: "too high", + PolicyOverride: false, + WriteRequest: structs.WriteRequest{ + Region: "global", + Namespace: job.Namespace, + }, + } + err = msgpackrpc.CallWithCodec(codec, "Job.Scale", scale, &resp) + require.Error(err) + require.Contains(err.Error(), "cannot be greater than") + + scale.Count = helper.Int64ToPtr(pol.Min - 1) + scale.Message = "too low" + err = msgpackrpc.CallWithCodec(codec, "Job.Scale", scale, &resp) + require.Error(err) + require.Contains(err.Error(), "cannot be less than") +} + func TestJobEndpoint_Scale_NoEval(t *testing.T) { t.Parallel() require := require.New(t) From 9087e0be99a88cc7a91e2f9475c6b2542ad627b6 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Fri, 8 Jan 2021 19:26:42 +0000 Subject: [PATCH 2/4] changelog for 9761 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8401d8e3e0f..5e26e66058d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ BUG FIXES: * consul/connect: Fixed a bug where in-place upgrade of Nomad client running Connect enabled jobs would panic [[GH-9738](https://github.com/hashicorp/nomad/issues/9738)] * template: Fixed multiple issues in template src/dest and artifact dest interpolation [[GH-9671](https://github.com/hashicorp/nomad/issues/9671)] * template: Fixed a bug where dynamic secrets did not trigger the template `change_mode` after a client restart. [[GH-9636](https://github.com/hashicorp/nomad/issues/9636)] + * scaling: Fixed a bug where job scaling endpoint did not enforce scaling policy min/max [[GH-9761](https://github.com/hashicorp/nomad/issues/9761)] * server: Fixed a bug where new servers may bootstrap prematurely when configured with `bootstrap_expect = 0`. ## 1.0.1 (December 16, 2020) From 59271c668e7e79e9b2e9a0783912fce87812e417 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Fri, 8 Jan 2021 19:38:25 +0000 Subject: [PATCH 3/4] appease the linter and fix an incorrect test --- api/util_test.go | 2 +- nomad/job_endpoint.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/util_test.go b/api/util_test.go index 5c0fd524f71..7e5f2e1b575 100644 --- a/api/util_test.go +++ b/api/util_test.go @@ -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), Enabled: boolToPtr(true), } return job diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 01cc4343759..146fa30d4e8 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1043,11 +1043,11 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes if found.Scaling != nil { if *args.Count < found.Scaling.Min { return structs.NewErrRPCCoded(400, - fmt.Sprintf("new scaling count cannot be less than the scaling policy minimum")) + "new scaling count cannot be less than the scaling policy minimum") } if found.Scaling.Max < *args.Count { return structs.NewErrRPCCoded(400, - fmt.Sprintf("new scaling count cannot be greater than the scaling policy maximum")) + "new scaling count cannot be greater than the scaling policy maximum") } } From cb1c0181be8d5de155ba35494c8e77171d609b13 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Fri, 8 Jan 2021 21:13:29 +0000 Subject: [PATCH 4/4] nicer error message --- nomad/job_endpoint.go | 6 ++++-- nomad/job_endpoint_test.go | 9 ++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 146fa30d4e8..333071ad6af 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1043,11 +1043,13 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes if found.Scaling != nil { if *args.Count < found.Scaling.Min { return structs.NewErrRPCCoded(400, - "new scaling count cannot be less than the scaling policy minimum") + fmt.Sprintf("group count was less than scaling policy minimum: %d < %d", + *args.Count, found.Scaling.Min)) } if found.Scaling.Max < *args.Count { return structs.NewErrRPCCoded(400, - "new scaling count cannot be greater than the scaling policy maximum") + fmt.Sprintf("group count was greater than scaling policy maximum: %d > %d", + *args.Count, found.Scaling.Max)) } } diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index c255f8cfb95..62bad2c33eb 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -6731,7 +6731,7 @@ func TestJobEndpoint_Scale_OutOfBounds(t *testing.T) { structs.ScalingTargetGroup: job.TaskGroups[0].Name, }, Count: helper.Int64ToPtr(pol.Max + 1), - Message: "too high", + Message: "out of bounds", PolicyOverride: false, WriteRequest: structs.WriteRequest{ Region: "global", @@ -6740,13 +6740,12 @@ func TestJobEndpoint_Scale_OutOfBounds(t *testing.T) { } err = msgpackrpc.CallWithCodec(codec, "Job.Scale", scale, &resp) require.Error(err) - require.Contains(err.Error(), "cannot be greater than") + require.Contains(err.Error(), "group count was greater than scaling policy maximum: 11 > 10") - scale.Count = helper.Int64ToPtr(pol.Min - 1) - scale.Message = "too low" + scale.Count = helper.Int64ToPtr(2) err = msgpackrpc.CallWithCodec(codec, "Job.Scale", scale, &resp) require.Error(err) - require.Contains(err.Error(), "cannot be less than") + require.Contains(err.Error(), "group count was less than scaling policy minimum: 2 < 3") } func TestJobEndpoint_Scale_NoEval(t *testing.T) {