Merge pull request #7474 from hashicorp/f-scaling-changes-from-review

more testing for scaling API

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,

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,

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

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
 	}
 }

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.

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) {

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

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,

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"

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
 }
 

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,
 		},
 		{

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(),

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",