diff --git a/CHANGELOG.md b/CHANGELOG.md index fbbedb2f8b2..9841f7fa17c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ IMPROVEMENTS: * core: Support for persisting previous task group counts when updating a job [[GH-8168](https://github.com/hashicorp/nomad/issues/8168)] * core: Block Job.Scale actions when the job is under active deployment [[GH-8187](https://github.com/hashicorp/nomad/issues/8187)] +* api: Better error messages around Scaling->Max [[GH-8360](https://github.com/hashicorp/nomad/issues/8360)] * api: Persist previous count with scaling events [[GH-8167](https://github.com/hashicorp/nomad/issues/8167)] * api: Support querying for jobs and allocations across all namespaces [[GH-8192](https://github.com/hashicorp/nomad/issues/8192)] * api: New `/agent/host` endpoint returns diagnostic information about the host [[GH-8325](https://github.com/hashicorp/nomad/pull/8325)] diff --git a/api/scaling.go b/api/scaling.go index 3a31abd8990..9f5f03de58f 100644 --- a/api/scaling.go +++ b/api/scaling.go @@ -56,7 +56,7 @@ type ScalingPolicy struct { Namespace string Target map[string]string Min *int64 - Max int64 + Max *int64 Policy map[string]interface{} Enabled *bool CreateIndex uint64 diff --git a/api/scaling_test.go b/api/scaling_test.go index 38f28a429bc..415775e2fa0 100644 --- a/api/scaling_test.go +++ b/api/scaling_test.go @@ -23,7 +23,7 @@ func TestScalingPolicies_ListPolicies(t *testing.T) { // Register a job with a scaling policy job := testJob() job.TaskGroups[0].Scaling = &ScalingPolicy{ - Max: 100, + Max: int64ToPtr(100), } _, _, err = jobs.Register(job, nil) require.NoError(err) @@ -75,7 +75,7 @@ func TestScalingPolicies_GetPolicy(t *testing.T) { policy := &ScalingPolicy{ Enabled: boolToPtr(true), Min: int64ToPtr(1), - Max: 1, + Max: int64ToPtr(1), Policy: map[string]interface{}{ "key": "value", }, diff --git a/api/tasks_test.go b/api/tasks_test.go index a1296d01f53..9e15bd82d68 100644 --- a/api/tasks_test.go +++ b/api/tasks_test.go @@ -444,7 +444,7 @@ func TestTaskGroup_Canonicalize_Scaling(t *testing.T) { Count: nil, Scaling: &ScalingPolicy{ Min: nil, - Max: 10, + Max: int64ToPtr(10), Policy: nil, Enabled: nil, CreateIndex: 0, diff --git a/api/util_test.go b/api/util_test.go index b63cdf75138..1be4f60aad0 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: 1, + Max: int64ToPtr(1), Enabled: boolToPtr(true), } return job diff --git a/command/agent/scaling_endpoint.go b/command/agent/scaling_endpoint.go index df0df44bf5a..3781ff6abc5 100644 --- a/command/agent/scaling_endpoint.go +++ b/command/agent/scaling_endpoint.go @@ -78,10 +78,15 @@ func (s *HTTPServer) scalingPolicyQuery(resp http.ResponseWriter, req *http.Requ func ApiScalingPolicyToStructs(count int, ap *api.ScalingPolicy) *structs.ScalingPolicy { p := structs.ScalingPolicy{ Enabled: *ap.Enabled, - Max: ap.Max, Policy: ap.Policy, Target: map[string]string{}, } + if ap.Max != nil { + p.Max = *ap.Max + } else { + // catch this in Validate + p.Max = -1 + } if ap.Min != nil { p.Min = *ap.Min } else { diff --git a/jobspec/parse_group.go b/jobspec/parse_group.go index db78ec538c6..d5a495c6453 100644 --- a/jobspec/parse_group.go +++ b/jobspec/parse_group.go @@ -364,6 +364,9 @@ func parseScalingPolicy(out **api.ScalingPolicy, list *ast.ObjectList) error { if err := dec.Decode(m); err != nil { return err } + if result.Max == nil { + return fmt.Errorf("missing 'max'") + } // If we have policy, then parse that if o := listVal.Filter("policy"); len(o.Items) > 0 { diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 35a79287ba5..73491645060 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -1320,7 +1320,7 @@ func TestParse(t *testing.T) { Name: helper.StringToPtr("group"), Scaling: &api.ScalingPolicy{ Min: helper.Int64ToPtr(5), - Max: 100, + Max: helper.Int64ToPtr(100), Policy: map[string]interface{}{ "foo": "bar", "b": true, @@ -1345,7 +1345,7 @@ func TestParse(t *testing.T) { Name: helper.StringToPtr("group"), Scaling: &api.ScalingPolicy{ Min: nil, - Max: 0, + Max: helper.Int64ToPtr(10), Policy: nil, Enabled: nil, }, @@ -1355,6 +1355,12 @@ func TestParse(t *testing.T) { false, }, + { + "tg-scaling-policy-missing-max.hcl", + nil, + true, + }, + { "tg-scaling-policy-multi-policy.hcl", nil, diff --git a/jobspec/test-fixtures/tg-scaling-policy-minimal.hcl b/jobspec/test-fixtures/tg-scaling-policy-minimal.hcl index 60aedac053f..f145506c898 100644 --- a/jobspec/test-fixtures/tg-scaling-policy-minimal.hcl +++ b/jobspec/test-fixtures/tg-scaling-policy-minimal.hcl @@ -1,5 +1,7 @@ job "elastic" { group "group" { - scaling {} + scaling { + max = 10 + } } } diff --git a/jobspec/test-fixtures/tg-scaling-policy-missing-max.hcl b/jobspec/test-fixtures/tg-scaling-policy-missing-max.hcl new file mode 100644 index 00000000000..1d60684fdd5 --- /dev/null +++ b/jobspec/test-fixtures/tg-scaling-policy-missing-max.hcl @@ -0,0 +1,7 @@ +job "elastic" { + group "group" { + scaling { + // required: max = ... + } + } +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index b415fac5997..4996b3638e2 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5948,21 +5948,26 @@ func (tg *TaskGroup) validateScalingPolicy(j *Job) error { var mErr multierror.Error - if tg.Scaling.Min > tg.Scaling.Max { + // was invalid or not specified; don't bother testing anything else involving max + if tg.Scaling.Max < 0 { mErr.Errors = append(mErr.Errors, - fmt.Errorf("Scaling policy invalid: maximum count must not be less than minimum count")) + fmt.Errorf("Scaling policy invalid: maximum count must be specified and non-negative")) + } else { + if tg.Scaling.Max < tg.Scaling.Min { + mErr.Errors = append(mErr.Errors, + fmt.Errorf("Scaling policy invalid: maximum count must not be less than minimum count")) + } + if tg.Scaling.Max < int64(tg.Count) { + mErr.Errors = append(mErr.Errors, + fmt.Errorf("Scaling policy invalid: task group count must not be greater than maximum count in scaling policy")) + } } - if int64(tg.Count) < tg.Scaling.Min && !(j.IsMultiregion() && tg.Count == 0) { + if int64(tg.Count) < tg.Scaling.Min && !(j.IsMultiregion() && tg.Count == 0 && j.Region == "global") { mErr.Errors = append(mErr.Errors, fmt.Errorf("Scaling policy invalid: task group count must not be less than minimum count in scaling policy")) } - if tg.Scaling.Max < int64(tg.Count) { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("Scaling policy invalid: task group count must not be greater than maximum count in scaling policy")) - } - return mErr.ErrorOrNil() } diff --git a/vendor/github.com/hashicorp/nomad/api/scaling.go b/vendor/github.com/hashicorp/nomad/api/scaling.go index 3a31abd8990..9f5f03de58f 100644 --- a/vendor/github.com/hashicorp/nomad/api/scaling.go +++ b/vendor/github.com/hashicorp/nomad/api/scaling.go @@ -56,7 +56,7 @@ type ScalingPolicy struct { Namespace string Target map[string]string Min *int64 - Max int64 + Max *int64 Policy map[string]interface{} Enabled *bool CreateIndex uint64 diff --git a/website/pages/docs/job-specification/group.mdx b/website/pages/docs/job-specification/group.mdx index 89499497548..644a977a155 100644 --- a/website/pages/docs/job-specification/group.mdx +++ b/website/pages/docs/job-specification/group.mdx @@ -36,8 +36,10 @@ job "docs" { node attribute or metadata. See the [Nomad spread reference](/docs/job-specification/spread) for more details. -- `count` `(int: 1)` - Specifies the number of the task groups that should - be running under this group. This value must be non-negative. +- `count` `(int)` - Specifies the number of instances that should be running + under for this group. This value must be non-negative. This defaults to the + `min` value specified in the [`scaling`](/docs/job-specification/scaling) + block, if present; otherwise, this defaults to `1`. - `ephemeral_disk` ([EphemeralDisk][]: nil) - Specifies the ephemeral disk requirements of the group. Ephemeral disks can be marked as