From f015e02002d184ba607b888c7b7a476d2439a554 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 4 Jul 2020 18:33:19 +0000 Subject: [PATCH 1/5] added scaling->min default discussion to group->count documentation --- website/pages/docs/job-specification/group.mdx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/website/pages/docs/job-specification/group.mdx b/website/pages/docs/job-specification/group.mdx index 89499497548a..644a977a155c 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 From 7f8176a18893d7530f926bb80954617768238c8d Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 4 Jul 2020 19:05:50 +0000 Subject: [PATCH 2/5] changes to make sure that Max is present and valid, to improve error messages * made api.Scaling.Max a pointer, so we can detect (and complain) when it is neglected * added checks to HCL parsing that it is present * when Scaling.Max is absent/invalid, don't return extraneous error messages during validation * tweak to multiregion handling to ensure that the count is valid on the interpolated regional jobs resolves #8355 --- api/scaling.go | 2 +- command/agent/scaling_endpoint.go | 7 ++++++- jobspec/parse_group.go | 3 +++ nomad/structs/structs.go | 16 +++++++++------- vendor/github.com/hashicorp/nomad/api/scaling.go | 2 +- 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/api/scaling.go b/api/scaling.go index 3a31abd89903..9f5f03de58fa 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/command/agent/scaling_endpoint.go b/command/agent/scaling_endpoint.go index df0df44bf5ad..3781ff6abc52 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 db78ec538c69..d5a495c64534 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/nomad/structs/structs.go b/nomad/structs/structs.go index b415fac59970..651db5152c0e 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5948,19 +5948,21 @@ 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 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 int64(tg.Count) < tg.Scaling.Min && !(j.IsMultiregion() && tg.Count == 0) { + } else if tg.Scaling.Max < int64(tg.Count) { mErr.Errors = append(mErr.Errors, - fmt.Errorf("Scaling policy invalid: task group count must not be less than minimum count in scaling policy")) + fmt.Errorf("Scaling policy invalid: task group count must not be greater than maximum count in scaling policy")) } - if tg.Scaling.Max < int64(tg.Count) { + 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 greater than maximum count in scaling policy")) + fmt.Errorf("Scaling policy invalid: task group count must not be less than minimum 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 3a31abd89903..9f5f03de58fa 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 From a05f3af419e4719153e271c6ab6cbdcfd7cd1b17 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 4 Jul 2020 19:10:24 +0000 Subject: [PATCH 3/5] added changelog entry for #8360 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbbedb2f8b29..9841f7fa17c7 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)] From 15a66e60a8d5daad6a0894f499943430449180f9 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 4 Jul 2020 19:23:58 +0000 Subject: [PATCH 4/5] fixed api tests for changes --- api/scaling_test.go | 4 ++-- api/tasks_test.go | 2 +- api/util_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/scaling_test.go b/api/scaling_test.go index 38f28a429bc4..415775e2fa09 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 a1296d01f530..9e15bd82d68e 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 b63cdf751389..1be4f60aad09 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 From b5f595a054895a6c3d2d3e827eb20a9e385db2f0 Mon Sep 17 00:00:00 2001 From: Chris Baker <1675087+cgbaker@users.noreply.github.com> Date: Sat, 4 Jul 2020 19:32:37 +0000 Subject: [PATCH 5/5] better testing of scaling parsing, fixed some broken tests by api changes --- jobspec/parse_test.go | 10 ++++++++-- .../test-fixtures/tg-scaling-policy-minimal.hcl | 4 +++- .../tg-scaling-policy-missing-max.hcl | 7 +++++++ nomad/structs/structs.go | 15 +++++++++------ 4 files changed, 27 insertions(+), 9 deletions(-) create mode 100644 jobspec/test-fixtures/tg-scaling-policy-missing-max.hcl diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 35a79287ba55..73491645060e 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 60aedac053fc..f145506c898e 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 000000000000..1d60684fdd54 --- /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 651db5152c0e..4996b3638e22 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -5952,12 +5952,15 @@ func (tg *TaskGroup) validateScalingPolicy(j *Job) error { if tg.Scaling.Max < 0 { mErr.Errors = append(mErr.Errors, 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")) - } else 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")) + } 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 && j.Region == "global") {