diff --git a/acl/policy.go b/acl/policy.go index 887ee95a8..cdb1aca80 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -11,11 +11,11 @@ const ( // The following levels are the only valid values for the `policy = "read"` stanza. // When policies are merged together, the most privilege is granted, except for deny // which always takes precedence and supercedes. - PolicyDeny = "deny" - PolicyRead = "read" - PolicyList = "list" - PolicyWrite = "write" - PolicyAutoscaler = "autoscaler" + PolicyDeny = "deny" + PolicyRead = "read" + PolicyList = "list" + PolicyWrite = "write" + PolicyScale = "scale" ) const ( @@ -126,7 +126,7 @@ type PluginPolicy struct { // isPolicyValid makes sure the given string matches one of the valid policies. func isPolicyValid(policy string) bool { switch policy { - case PolicyDeny, PolicyRead, PolicyWrite, PolicyAutoscaler: + case PolicyDeny, PolicyRead, PolicyWrite, PolicyScale: return true default: return false @@ -192,7 +192,7 @@ func expandNamespacePolicy(policy string) []string { return read case PolicyWrite: return write - case PolicyAutoscaler: + case PolicyScale: return []string{ NamespaceCapabilityListScalingPolicies, NamespaceCapabilityReadScalingPolicy, diff --git a/acl/policy_test.go b/acl/policy_test.go index aff25356f..59ae88922 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -52,7 +52,7 @@ func TestParse(t *testing.T) { capabilities = ["deny", "read-logs"] } namespace "autoscaler" { - policy = "autoscaler" + policy = "scale" } agent { policy = "read" @@ -117,7 +117,7 @@ func TestParse(t *testing.T) { }, { Name: "autoscaler", - Policy: PolicyAutoscaler, + Policy: PolicyScale, Capabilities: []string{ NamespaceCapabilityListScalingPolicies, NamespaceCapabilityReadScalingPolicy, diff --git a/api/jobs_test.go b/api/jobs_test.go index 90a4d6434..348244193 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -1595,18 +1595,20 @@ func TestJobs_ScaleAction(t *testing.T) { require.Contains(err.Error(), "not found") // Register the job - _, wm, err := jobs.Register(job, nil) + regResp, wm, err := jobs.Register(job, nil) require.NoError(err) assertWriteMeta(t, wm) // Perform scaling action newCount := groupCount + 1 - resp1, wm, err := jobs.Scale(id, groupName, + scalingResp, wm, err := jobs.Scale(id, groupName, intToPtr(newCount), stringToPtr("need more instances"), nil, nil, nil) require.NoError(err) - require.NotNil(resp1) - require.NotEmpty(resp1.EvalID) + require.NotNil(scalingResp) + require.NotEmpty(scalingResp.EvalID) + require.NotEmpty(scalingResp.EvalCreateIndex) + require.Greater(scalingResp.JobModifyIndex, regResp.JobModifyIndex) assertWriteMeta(t, wm) // Query the job again @@ -1614,7 +1616,47 @@ func TestJobs_ScaleAction(t *testing.T) { require.NoError(err) require.Equal(*resp.TaskGroups[0].Count, newCount) - // TODO: check if reason is stored + // TODO: check that scaling event was persisted +} + +func TestJobs_ScaleAction_Noop(t *testing.T) { + t.Parallel() + require := require.New(t) + + c, s := makeClient(t, nil, nil) + defer s.Stop() + jobs := c.Jobs() + + id := "job-id/with\\troublesome:characters\n?&字\000" + job := testJobWithScalingPolicy() + job.ID = &id + groupName := *job.TaskGroups[0].Name + prevCount := *job.TaskGroups[0].Count + + // Register the job + regResp, wm, err := jobs.Register(job, nil) + require.NoError(err) + assertWriteMeta(t, wm) + + // Perform scaling action + scaleResp, wm, err := jobs.Scale(id, groupName, + nil, stringToPtr("no count, just informative"), nil, nil, nil) + + require.NoError(err) + require.NotNil(scaleResp) + require.Empty(scaleResp.EvalID) + require.Empty(scaleResp.EvalCreateIndex) + assertWriteMeta(t, wm) + + // Query the job again + resp, _, err := jobs.Info(*job.ID, nil) + require.NoError(err) + require.Equal(*resp.TaskGroups[0].Count, prevCount) + require.Equal(regResp.JobModifyIndex, scaleResp.JobModifyIndex) + require.Empty(scaleResp.EvalCreateIndex) + require.Empty(scaleResp.EvalID) + + // TODO: check that scaling event was persisted } // TestJobs_ScaleStatus tests the /scale status endpoint for task group count diff --git a/api/scaling.go b/api/scaling.go index 7f872cf36..9c92fc861 100644 --- a/api/scaling.go +++ b/api/scaling.go @@ -28,18 +28,12 @@ func (s *Scaling) GetPolicy(ID string, q *QueryOptions) (*ScalingPolicy, *QueryM return &policy, qm, nil } -func (p *ScalingPolicy) Canonicalize(tg *TaskGroup) { +func (p *ScalingPolicy) Canonicalize(taskGroupCount int) { if p.Enabled == nil { p.Enabled = boolToPtr(true) } if p.Min == nil { - var m int64 - if tg.Count != nil { - m = int64(*tg.Count) - } else { - // this should not be at this point, but safeguard here just in case - m = 0 - } + var m int64 = int64(taskGroupCount) p.Min = &m } } diff --git a/api/tasks.go b/api/tasks.go index d18d8a0b1..991244208 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -450,6 +450,9 @@ func (g *TaskGroup) Canonicalize(job *Job) { g.Count = intToPtr(1) } } + if g.Scaling != nil { + g.Scaling.Canonicalize(*g.Count) + } for _, t := range g.Tasks { t.Canonicalize(g, job) } @@ -548,10 +551,6 @@ func (g *TaskGroup) Canonicalize(job *Job) { for _, s := range g.Services { s.Canonicalize(nil, g, job) } - - if g.Scaling != nil { - g.Scaling.Canonicalize(g) - } } // Constrain is used to add a constraint to a task group. diff --git a/api/tasks_test.go b/api/tasks_test.go index 8217aee1f..d30834565 100644 --- a/api/tasks_test.go +++ b/api/tasks_test.go @@ -457,27 +457,35 @@ func TestTaskGroup_Canonicalize_Scaling(t *testing.T) { tg.Canonicalize(job) require.NotNil(tg.Count) require.NotNil(tg.Scaling.Min) - require.Equal(1, *tg.Count) - require.Equal(int64(*tg.Count), *tg.Scaling.Min) + require.EqualValues(1, *tg.Count) + require.EqualValues(*tg.Count, *tg.Scaling.Min) + // count == nil => count = Scaling.Min tg.Count = nil tg.Scaling.Min = int64ToPtr(5) - // count == nil => count = Scaling.Min tg.Canonicalize(job) require.NotNil(tg.Count) require.NotNil(tg.Scaling.Min) - require.Equal(5, *tg.Count) - require.Equal(int64(*tg.Count), *tg.Scaling.Min) + require.EqualValues(5, *tg.Count) + require.EqualValues(*tg.Count, *tg.Scaling.Min) // Scaling.Min == nil => Scaling.Min == count tg.Count = intToPtr(5) tg.Scaling.Min = nil - // count == nil => count = Scaling.Min tg.Canonicalize(job) require.NotNil(tg.Count) require.NotNil(tg.Scaling.Min) - require.Equal(int64(5), *tg.Scaling.Min) - require.Equal(*tg.Scaling.Min, int64(*tg.Count)) + require.EqualValues(5, *tg.Scaling.Min) + require.EqualValues(*tg.Scaling.Min, *tg.Count) + + // both present, both persisted + tg.Count = intToPtr(5) + tg.Scaling.Min = int64ToPtr(1) + tg.Canonicalize(job) + require.NotNil(tg.Count) + require.NotNil(tg.Scaling.Min) + require.EqualValues(1, *tg.Scaling.Min) + require.EqualValues(5, *tg.Count) } func TestTaskGroup_Merge_Update(t *testing.T) { diff --git a/api/utils.go b/api/utils.go index d56d889a5..9e54306f6 100644 --- a/api/utils.go +++ b/api/utils.go @@ -27,8 +27,8 @@ func uint64ToPtr(u uint64) *uint64 { } // int64ToPtr returns the pointer to a int64 -func int64ToPtr(u int64) *int64 { - return &u +func int64ToPtr(i int64) *int64 { + return &i } // stringToPtr returns the pointer to a string diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 5eda7063c..89fde3312 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1139,6 +1139,8 @@ func TestParse(t *testing.T) { { Name: helper.StringToPtr("group"), Scaling: &api.ScalingPolicy{ + Min: helper.Int64ToPtr(5), + Max: 100, Policy: map[string]interface{}{ "foo": "bar", "b": true, diff --git a/jobspec/test-fixtures/tg-scaling-policy.hcl b/jobspec/test-fixtures/tg-scaling-policy.hcl index 483e62a64..c5569d8c1 100644 --- a/jobspec/test-fixtures/tg-scaling-policy.hcl +++ b/jobspec/test-fixtures/tg-scaling-policy.hcl @@ -2,6 +2,8 @@ job "elastic" { group "group" { scaling { enabled = false + min = 5 + max = 100 policy { foo = "bar" diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 86ad10bcc..ec1b7d858 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -894,7 +894,7 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes return structs.NewErrRPCCoded(404, fmt.Sprintf("job %q not found", args.JobID)) } - index := job.ModifyIndex + jobModifyIndex := job.ModifyIndex if args.Count != nil { found := false for _, tg := range job.TaskGroups { @@ -917,22 +917,22 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes } // Commit this update via Raft - _, index, err = j.srv.raftApply(structs.JobRegisterRequestType, registerReq) + _, jobModifyIndex, err = j.srv.raftApply(structs.JobRegisterRequestType, registerReq) if err != nil { j.logger.Error("job register for scale failed", "error", err) return err } - } // Populate the reply with job information - reply.JobModifyIndex = index + reply.JobModifyIndex = jobModifyIndex + reply.Index = reply.JobModifyIndex // FINISH: // register the scaling event to the scaling_event table, once that exists // If the job is periodic or parameterized, we don't create an eval. - if job != nil && (job.IsPeriodic() || job.IsParameterized()) { + if job != nil && (job.IsPeriodic() || job.IsParameterized()) || args.Count == nil { return nil } @@ -946,7 +946,7 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes Type: structs.JobTypeService, TriggeredBy: structs.EvalTriggerScaling, JobID: args.JobID, - JobModifyIndex: index, + JobModifyIndex: jobModifyIndex, Status: structs.EvalStatusPending, CreateTime: now, ModifyTime: now, @@ -966,7 +966,7 @@ func (j *Job) Scale(args *structs.JobScaleRequest, reply *structs.JobRegisterRes // Populate the reply with eval information reply.EvalID = eval.ID reply.EvalCreateIndex = evalIndex - reply.Index = evalIndex + reply.Index = reply.EvalCreateIndex return nil } diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 62d0e4b75..c8a83f650 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -5369,7 +5369,7 @@ func TestJobEndpoint_Scale_ACL(t *testing.T) { { name: "autoscaler disposition should succeed", authToken: mock.CreatePolicyAndToken(t, state, 1005, "test-valid-autoscaler", - mock.NamespacePolicy(structs.DefaultNamespace, "autoscaler", nil)). + mock.NamespacePolicy(structs.DefaultNamespace, "scale", nil)). SecretID, }, { @@ -5568,7 +5568,7 @@ func TestJobEndpoint_GetScaleStatus_ACL(t *testing.T) { { name: "autoscaler disposition should succeed", authToken: mock.CreatePolicyAndToken(t, state, 1005, "test-valid-autoscaler", - mock.NamespacePolicy(structs.DefaultNamespace, "autoscaler", nil)). + mock.NamespacePolicy(structs.DefaultNamespace, "scale", nil)). SecretID, }, { diff --git a/nomad/mock/mock.go b/nomad/mock/mock.go index ccecb6d65..fff4c1dca 100644 --- a/nomad/mock/mock.go +++ b/nomad/mock/mock.go @@ -1256,7 +1256,9 @@ func ACLManagementToken() *structs.ACLToken { func ScalingPolicy() *structs.ScalingPolicy { return &structs.ScalingPolicy{ - ID: uuid.Generate(), + ID: uuid.Generate(), + Min: 1, + Max: 100, Target: map[string]string{ structs.ScalingTargetNamespace: structs.DefaultNamespace, structs.ScalingTargetJob: uuid.Generate(), diff --git a/nomad/scaling_endpoint_test.go b/nomad/scaling_endpoint_test.go index 9fd00de4a..57f15a742 100644 --- a/nomad/scaling_endpoint_test.go +++ b/nomad/scaling_endpoint_test.go @@ -104,7 +104,7 @@ func TestScalingEndpoint_GetPolicy_ACL(t *testing.T) { { name: "autoscaler disposition should succeed", authToken: mock.CreatePolicyAndToken(t, state, 1005, "test-valid-autoscaler", - mock.NamespacePolicy(structs.DefaultNamespace, "autoscaler", nil)).SecretID, + mock.NamespacePolicy(structs.DefaultNamespace, "scale", nil)).SecretID, }, { name: "list-jobs+read-job capability should succeed", @@ -208,7 +208,7 @@ func TestScalingEndpoint_ListPolicies_ACL(t *testing.T) { { name: "autoscaler disposition should succeed", authToken: mock.CreatePolicyAndToken(t, state, 1005, "test-valid-autoscaler", - mock.NamespacePolicy(structs.DefaultNamespace, "autoscaler", nil)).SecretID, + mock.NamespacePolicy(structs.DefaultNamespace, "scale", nil)).SecretID, }, { name: "list-scaling-policies capability should succeed",