From 3c2c346d7249ab0e52f68d20ed8fc6eb89e4953f Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 13:34:22 +0100 Subject: [PATCH 01/15] Add job max and default priority in the agent part --- command/agent/agent.go | 17 ++++ command/agent/agent_test.go | 142 +++++++++++++++++++++++++++++ command/agent/config.go | 14 +++ command/agent/config_parse_test.go | 4 +- command/agent/config_test.go | 2 + command/agent/testdata/basic.hcl | 2 + command/agent/testdata/basic.json | 4 +- 7 files changed, 183 insertions(+), 2 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index d5703f72104..dc1adff5a55 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -326,6 +326,23 @@ func convertServerConfig(agentConfig *Config) (*nomad.Config, error) { } } + jobMaxPriority := int(structs.JobDefaultMaxPriority) + if agentConfig.Server.JobMaxPriority != nil && *agentConfig.Server.JobMaxPriority != 0 { + jobMaxPriority = *agentConfig.Server.JobMaxPriority + if jobMaxPriority < structs.JobDefaultMaxPriority || jobMaxPriority > structs.JobMaxPriority { + return nil, fmt.Errorf("job_max_priority cannot be %d. Must be between %d and %d", *agentConfig.Server.JobMaxPriority, structs.JobDefaultMaxPriority, structs.JobMaxPriority) + } + } + jobDefaultPriority := int(structs.JobDefaultPriority) + if agentConfig.Server.JobDefaultPriority != nil && *agentConfig.Server.JobDefaultPriority != 0 { + jobDefaultPriority = *agentConfig.Server.JobDefaultPriority + if jobDefaultPriority < structs.JobDefaultPriority || jobDefaultPriority >= jobMaxPriority { + return nil, fmt.Errorf("job_default_priority cannot be %d. Must be between %d and %d", *agentConfig.Server.JobDefaultPriority, structs.JobDefaultPriority, jobMaxPriority) + } + } + conf.JobMaxPriority = jobMaxPriority + conf.JobDefaultPriority = jobDefaultPriority + // Set up the bind addresses rpcAddr, err := net.ResolveTCPAddr("tcp", agentConfig.normalizedAddrs.RPC) if err != nil { diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 0e88061ebfd..15484ca2714 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -3,6 +3,7 @@ package agent import ( "fmt" "io/ioutil" + "math" "os" "path/filepath" "strings" @@ -1569,3 +1570,144 @@ func TestAgent_ProxyRPC_Dev(t *testing.T) { }) } + +func TestAgent_ServerConfig_JobMaxPriority_Ok(t *testing.T) { + ci.Parallel(t) + + cases := []struct { + maxPriority *int + jobMaxPriority int + }{ + { + maxPriority: nil, + jobMaxPriority: 100, + }, + + { + maxPriority: pointer.Of(0), + jobMaxPriority: 100, + }, + { + maxPriority: pointer.Of(100), + jobMaxPriority: 100, + }, + { + maxPriority: pointer.Of(200), + jobMaxPriority: 200, + }, + { + maxPriority: pointer.Of(32766), + jobMaxPriority: 32766, + }, + } + + for _, tc := range cases { + v := "default" + if tc.maxPriority != nil { + v = fmt.Sprintf("%v", *tc.maxPriority) + } + t.Run(v, func(t *testing.T) { + conf := DevConfig(nil) + require.NoError(t, conf.normalizeAddrs()) + + conf.Server.JobMaxPriority = tc.maxPriority + + serverConf, err := convertServerConfig(conf) + require.NoError(t, err) + + assert.Equal(t, tc.jobMaxPriority, serverConf.JobMaxPriority, "job max priority") + }) + } +} + +func TestAgent_ServerConfig_JobMaxPriority_Bad(t *testing.T) { + ci.Parallel(t) + + cases := []int{ + 99, + math.MaxInt16, + } + + for _, tc := range cases { + t.Run(fmt.Sprintf("%v", tc), func(t *testing.T) { + conf := DevConfig(nil) + require.NoError(t, conf.normalizeAddrs()) + + conf.Server.JobMaxPriority = &tc + + _, err := convertServerConfig(conf) + require.Error(t, err) + require.Contains(t, err.Error(), "job_max_priority cannot be") + }) + } +} +func TestAgent_ServerConfig_JobDefaultPriority_Ok(t *testing.T) { + ci.Parallel(t) + + cases := []struct { + defaultPriority *int + jobDefaultPriority int + }{ + { + defaultPriority: nil, + jobDefaultPriority: 50, + }, + + { + defaultPriority: pointer.Of(0), + jobDefaultPriority: 50, + }, + { + defaultPriority: pointer.Of(50), + jobDefaultPriority: 50, + }, + { + defaultPriority: pointer.Of(60), + jobDefaultPriority: 60, + }, + { + defaultPriority: pointer.Of(99), + jobDefaultPriority: 99, + }, + } + + for _, tc := range cases { + v := "default" + if tc.defaultPriority != nil { + v = fmt.Sprintf("%v", *tc.defaultPriority) + } + t.Run(v, func(t *testing.T) { + conf := DevConfig(nil) + require.NoError(t, conf.normalizeAddrs()) + + conf.Server.JobDefaultPriority = tc.defaultPriority + + serverConf, err := convertServerConfig(conf) + require.NoError(t, err) + + assert.Equal(t, tc.jobDefaultPriority, serverConf.JobDefaultPriority, "job default priority") + }) + } +} + +func TestAgent_ServerConfig_JobDefaultPriority_Bad(t *testing.T) { + ci.Parallel(t) + + cases := []int{ + 49, + 100, + } + + for _, tc := range cases { + t.Run(fmt.Sprintf("%v", tc), func(t *testing.T) { + conf := DevConfig(nil) + require.NoError(t, conf.normalizeAddrs()) + + conf.Server.JobDefaultPriority = &tc + + _, err := convertServerConfig(conf) + require.Error(t, err) + require.Contains(t, err.Error(), "job_default_priority cannot be") + }) + } +} diff --git a/command/agent/config.go b/command/agent/config.go index 29d357219a8..7e712292177 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -644,6 +644,12 @@ type ServerConfig struct { // forced to send an entire snapshot. The value passed here is the initial // setting used. This can be tuned during operation using a hot reload. RaftTrailingLogs *int `hcl:"raft_trailing_logs"` + + // JobDefaultPriority is the default Job priority if not specified. + JobDefaultPriority *int `hcl:"job_default_priority"` + + // JobMaxPriority is an upper bound on the Job priority. + JobMaxPriority *int `hcl:"job_max_priority"` } func (s *ServerConfig) Copy() *ServerConfig { @@ -669,6 +675,8 @@ func (s *ServerConfig) Copy() *ServerConfig { ns.RaftSnapshotInterval = pointer.Copy(s.RaftSnapshotInterval) ns.RaftSnapshotThreshold = pointer.Copy(s.RaftSnapshotThreshold) ns.RaftTrailingLogs = pointer.Copy(s.RaftTrailingLogs) + ns.JobDefaultPriority = pointer.Copy(s.JobDefaultPriority) + ns.JobMaxPriority = pointer.Copy(s.JobMaxPriority) return &ns } @@ -1867,6 +1875,12 @@ func (s *ServerConfig) Merge(b *ServerConfig) *ServerConfig { if b.JobGCThreshold != "" { result.JobGCThreshold = b.JobGCThreshold } + if b.JobDefaultPriority != nil { + result.JobDefaultPriority = pointer.Of(*b.JobDefaultPriority) + } + if b.JobMaxPriority != nil { + result.JobMaxPriority = pointer.Of(*b.JobMaxPriority) + } if b.EvalGCThreshold != "" { result.EvalGCThreshold = b.EvalGCThreshold } diff --git a/command/agent/config_parse_test.go b/command/agent/config_parse_test.go index 94d60cdf691..96ab98da40a 100644 --- a/command/agent/config_parse_test.go +++ b/command/agent/config_parse_test.go @@ -147,7 +147,9 @@ var basicConfig = &Config{ ServiceSchedulerEnabled: true, }, }, - LicensePath: "/tmp/nomad.hclic", + LicensePath: "/tmp/nomad.hclic", + JobDefaultPriority: pointer.Of(100), + JobMaxPriority: pointer.Of(200), }, ACL: &ACLConfig{ Enabled: true, diff --git a/command/agent/config_test.go b/command/agent/config_test.go index dc5d6f6468d..d90acc0c59d 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -358,6 +358,8 @@ func TestConfig_Merge(t *testing.T) { NodeThreshold: 100, NodeWindow: 11 * time.Minute, }, + JobMaxPriority: pointer.Of(200), + JobDefaultPriority: pointer.Of(100), }, ACL: &ACLConfig{ Enabled: true, diff --git a/command/agent/testdata/basic.hcl b/command/agent/testdata/basic.hcl index 55b9977b5b6..5257c9afcf5 100644 --- a/command/agent/testdata/basic.hcl +++ b/command/agent/testdata/basic.hcl @@ -133,6 +133,8 @@ server { raft_multiplier = 4 enable_event_broker = false event_buffer_size = 200 + job_default_priority = 100 + job_max_priority = 200 plan_rejection_tracker { enabled = true diff --git a/command/agent/testdata/basic.json b/command/agent/testdata/basic.json index 990400acf2d..8fffb94fb35 100644 --- a/command/agent/testdata/basic.json +++ b/command/agent/testdata/basic.json @@ -320,7 +320,9 @@ }] }], "upgrade_version": "0.8.0", - "license_path": "/tmp/nomad.hclic" + "license_path": "/tmp/nomad.hclic", + "job_default_priority": 100, + "job_max_priority": 200 } ], "syslog_facility": "LOCAL1", From 315cab8a562bcdd575bf8b0c45d17935047365a7 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 13:37:09 +0100 Subject: [PATCH 02/15] Make more obvious the default priority of job in the api --- api/jobs.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/jobs.go b/api/jobs.go index c9921c9fd3b..5a8b96b6478 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -31,6 +31,9 @@ const ( // DefaultNamespace is the default namespace. DefaultNamespace = "default" + // DefaultPriority is the default priority + DefaultPriority = 50 + // For Job configuration, GlobalRegion is a sentinel region value // that users may specify to indicate the job should be run on // the region of the node that the job was submitted to. @@ -938,7 +941,7 @@ func (j *Job) Canonicalize() { j.Namespace = pointerOf(DefaultNamespace) } if j.Priority == nil { - j.Priority = pointerOf(50) + j.Priority = pointerOf(DefaultPriority) } if j.Stop == nil { j.Stop = pointerOf(false) From 168f05907cf2b44bc9092362caba7fc9e8d8e767 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 13:52:40 +0100 Subject: [PATCH 03/15] Remove the hardcoded check of job priority in the nomad/structs/Job Validate func --- nomad/structs/structs.go | 13 ++++++------- nomad/structs/structs_test.go | 5 ++--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index baee5599f27..4079b36cadb 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4156,17 +4156,19 @@ const ( // JobMinPriority is the minimum allowed priority JobMinPriority = 1 - // JobDefaultPriority is the default priority if not - // not specified. + // JobDefaultPriority is the default priority if not specified. JobDefaultPriority = 50 + // JobDefaultMaxPriority is the default maximum allowed priority + JobDefaultMaxPriority = 100 + // JobMaxPriority is the maximum allowed priority - JobMaxPriority = 100 + JobMaxPriority = math.MaxInt16 - 1 // CoreJobPriority should be higher than any user // specified job so that it gets priority. This is important // for the system to remain healthy. - CoreJobPriority = JobMaxPriority * 2 + CoreJobPriority = math.MaxInt16 // JobTrackedVersions is the number of historic job versions that are // kept. @@ -4446,9 +4448,6 @@ func (j *Job) Validate() error { default: mErr.Errors = append(mErr.Errors, fmt.Errorf("Invalid job type: %q", j.Type)) } - if j.Priority < JobMinPriority || j.Priority > JobMaxPriority { - mErr.Errors = append(mErr.Errors, fmt.Errorf("Job priority must be between [%d, %d]", JobMinPriority, JobMaxPriority)) - } if len(j.Datacenters) == 0 && !j.IsMultiregion() { mErr.Errors = append(mErr.Errors, errors.New("Missing job datacenters")) } else { diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index a1cf101d02c..6dc38b716c7 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -30,7 +30,6 @@ func TestJob_Validate(t *testing.T) { "job region", "job type", "namespace", - "priority", "task groups", ) @@ -57,7 +56,7 @@ func TestJob_Validate(t *testing.T) { Namespace: "test", Name: "my-job", Type: JobTypeService, - Priority: 50, + Priority: JobDefaultPriority, Datacenters: []string{"*"}, TaskGroups: []*TaskGroup{ { @@ -370,7 +369,7 @@ func testJob() *Job { Namespace: "test", Name: "my-job", Type: JobTypeService, - Priority: 50, + Priority: JobDefaultPriority, AllAtOnce: false, Datacenters: []string{"*"}, Constraints: []*Constraint{ From 299950257ad4ae4bdbcf1a6dedb3510105f7809b Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 14:53:34 +0100 Subject: [PATCH 04/15] Move job priority bound checks in the jobValidate part --- nomad/config.go | 8 ++++++ nomad/job_endpoint.go | 4 +-- nomad/job_endpoint_hooks.go | 12 ++++++-- nomad/job_endpoint_test.go | 55 +++++++++++++++++++++++++++++++++++++ nomad/mock/job.go | 8 +++--- 5 files changed, 78 insertions(+), 9 deletions(-) diff --git a/nomad/config.go b/nomad/config.go index 5442e90dbeb..849b8c3d467 100644 --- a/nomad/config.go +++ b/nomad/config.go @@ -412,6 +412,12 @@ type Config struct { // DeploymentQueryRateLimit is in queries per second and is used by the // DeploymentWatcher to throttle the amount of simultaneously deployments DeploymentQueryRateLimit float64 + + // JobDefaultPriority is the default Job priority if not specified. + JobDefaultPriority int + + // JobMaxPriority is an upper bound on the Job priority. + JobMaxPriority int } func (c *Config) Copy() *Config { @@ -529,6 +535,8 @@ func DefaultConfig() *Config { }, }, DeploymentQueryRateLimit: deploymentwatcher.LimitStateQueriesPerSecond, + JobDefaultPriority: structs.JobDefaultPriority, + JobMaxPriority: structs.JobDefaultMaxPriority, } // Enable all known schedulers by default diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 1376d9c773d..a0cdd20a82f 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -76,7 +76,7 @@ func NewJobEndpoints(s *Server, ctx *RPCContext) *Job { jobExposeCheckHook{}, jobVaultHook{srv: s}, jobNamespaceConstraintCheckHook{srv: s}, - jobValidate{}, + &jobValidate{srv: s}, &memoryOversubscriptionValidate{srv: s}, }, } @@ -990,7 +990,7 @@ func (j *Job) BatchDeregister(args *structs.JobBatchDeregisterRequest, reply *st continue } - priority := structs.JobDefaultPriority + priority := j.srv.config.JobDefaultPriority jtype := structs.JobTypeService if job != nil { priority = job.Priority diff --git a/nomad/job_endpoint_hooks.go b/nomad/job_endpoint_hooks.go index 8c6c5c1bf66..d6b2c0d3f56 100644 --- a/nomad/job_endpoint_hooks.go +++ b/nomad/job_endpoint_hooks.go @@ -253,13 +253,15 @@ func mutateConstraint(matcher constraintMatcher, taskGroup *structs.TaskGroup, c // jobValidate validates a Job and task drivers and returns an error if there is // a validation problem or if the Job is of a type a user is not allowed to // submit. -type jobValidate struct{} +type jobValidate struct { + srv *Server +} -func (jobValidate) Name() string { +func (*jobValidate) Name() string { return "validate" } -func (jobValidate) Validate(job *structs.Job) (warnings []error, err error) { +func (v *jobValidate) Validate(job *structs.Job) (warnings []error, err error) { validationErrors := new(multierror.Error) if err := job.Validate(); err != nil { multierror.Append(validationErrors, err) @@ -287,6 +289,10 @@ func (jobValidate) Validate(job *structs.Job) (warnings []error, err error) { multierror.Append(validationErrors, fmt.Errorf("job can't be submitted with a payload, only dispatched")) } + if job.Priority < structs.JobMinPriority || job.Priority > v.srv.config.JobMaxPriority { + multierror.Append(validationErrors, fmt.Errorf("job priority must be between [%d, %d]", structs.JobMinPriority, v.srv.config.JobMaxPriority)) + } + return warnings, validationErrors.ErrorOrNil() } diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 9562167ad28..5e988f56efd 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -6651,6 +6651,61 @@ func TestJobEndpoint_ValidateJobUpdate_ACL(t *testing.T) { require.Equal("", validResp.Warnings) } +func TestJobEndpoint_ValidateJob_PriorityNotOk(t *testing.T) { + ci.Parallel(t) + + s1, cleanupS1 := TestServer(t, nil) + defer cleanupS1() + codec := rpcClient(t, s1) + testutil.WaitForLeader(t, s1.RPC) + + validateJob := func(j *structs.Job) error { + req := &structs.JobRegisterRequest{ + Job: j, + WriteRequest: structs.WriteRequest{ + Region: "global", + Namespace: j.Namespace, + }, + } + var resp structs.JobValidateResponse + if err := msgpackrpc.CallWithCodec(codec, "Job.Validate", req, &resp); err != nil { + return err + } + + if resp.Error != "" { + return errors.New(resp.Error) + } + + if len(resp.ValidationErrors) != 0 { + return errors.New(strings.Join(resp.ValidationErrors, ",")) + } + + if resp.Warnings != "" { + return errors.New(resp.Warnings) + } + + return nil + } + + t.Run("job with invalid min priority", func(t *testing.T) { + j := mock.Job() + j.Priority = 0 + + err := validateJob(j) + require.Error(t, err) + require.Contains(t, err.Error(), "job priority must be between") + }) + + t.Run("job with invalid max priority", func(t *testing.T) { + j := mock.Job() + j.Priority = 101 + + err := validateJob(j) + require.Error(t, err) + require.Contains(t, err.Error(), "job priority must be between") + }) +} + func TestJobEndpoint_Dispatch_ACL(t *testing.T) { ci.Parallel(t) require := require.New(t) diff --git a/nomad/mock/job.go b/nomad/mock/job.go index 3422c7e56f0..99b0265209e 100644 --- a/nomad/mock/job.go +++ b/nomad/mock/job.go @@ -15,7 +15,7 @@ func Job() *structs.Job { Name: "my-job", Namespace: structs.DefaultNamespace, Type: structs.JobTypeService, - Priority: 50, + Priority: structs.JobDefaultPriority, AllAtOnce: false, Datacenters: []string{"dc1"}, Constraints: []*structs.Constraint{ @@ -294,7 +294,7 @@ func BatchJob() *structs.Job { Name: "batch-job", Namespace: structs.DefaultNamespace, Type: structs.JobTypeBatch, - Priority: 50, + Priority: structs.JobDefaultPriority, AllAtOnce: false, Datacenters: []string{"dc1"}, TaskGroups: []*structs.TaskGroup{ @@ -360,7 +360,7 @@ func SystemJob() *structs.Job { ID: fmt.Sprintf("mock-system-%s", uuid.Generate()), Name: "my-job", Type: structs.JobTypeSystem, - Priority: 100, + Priority: structs.JobDefaultMaxPriority, AllAtOnce: false, Datacenters: []string{"dc1"}, Constraints: []*structs.Constraint{ @@ -437,7 +437,7 @@ func MaxParallelJob() *structs.Job { Name: "my-job", Namespace: structs.DefaultNamespace, Type: structs.JobTypeService, - Priority: 50, + Priority: structs.JobDefaultPriority, AllAtOnce: false, Datacenters: []string{"dc1"}, Constraints: []*structs.Constraint{ From c8dbd4364e44c43903764532333cbe3605fd0439 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 15:24:47 +0100 Subject: [PATCH 05/15] Add docs --- website/content/docs/configuration/server.mdx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/website/content/docs/configuration/server.mdx b/website/content/docs/configuration/server.mdx index 5b87695d23f..07a7ef9330b 100644 --- a/website/content/docs/configuration/server.mdx +++ b/website/content/docs/configuration/server.mdx @@ -249,6 +249,12 @@ server { - `search` ([search][search]: nil) - Specifies configuration parameters for the Nomad search API. +- `job_max_priority` `(int: 100)` - Specifies the maximum priority that can be assigned to a job. + A valid value must be between `100` and `32766` + +- `job_default_priority` `(int: 50)` - Specifies the default priority assigned to a job. + A valid value must be between `50` and `job_max_priority` + ### Deprecated Parameters - `retry_join` `(array: [])` - Specifies a list of server addresses to From 7d2fba08bcf02fe4dbd5bc19d1f5309b38b97805 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 16:38:05 +0100 Subject: [PATCH 06/15] use must pkg instead of require --- command/agent/agent_test.go | 26 +++++++++++++------------- nomad/job_endpoint_test.go | 8 ++++---- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 15484ca2714..20f941c4b07 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -1608,14 +1608,13 @@ func TestAgent_ServerConfig_JobMaxPriority_Ok(t *testing.T) { } t.Run(v, func(t *testing.T) { conf := DevConfig(nil) - require.NoError(t, conf.normalizeAddrs()) + must.NoError(t, conf.normalizeAddrs()) conf.Server.JobMaxPriority = tc.maxPriority serverConf, err := convertServerConfig(conf) - require.NoError(t, err) - - assert.Equal(t, tc.jobMaxPriority, serverConf.JobMaxPriority, "job max priority") + must.NoError(t, err) + must.Eq(t, tc.jobMaxPriority, serverConf.JobMaxPriority) }) } } @@ -1631,16 +1630,17 @@ func TestAgent_ServerConfig_JobMaxPriority_Bad(t *testing.T) { for _, tc := range cases { t.Run(fmt.Sprintf("%v", tc), func(t *testing.T) { conf := DevConfig(nil) - require.NoError(t, conf.normalizeAddrs()) + must.NoError(t, conf.normalizeAddrs()) conf.Server.JobMaxPriority = &tc _, err := convertServerConfig(conf) - require.Error(t, err) - require.Contains(t, err.Error(), "job_max_priority cannot be") + must.Error(t, err) + must.ErrorContains(t, err, "job_max_priority cannot be") }) } } + func TestAgent_ServerConfig_JobDefaultPriority_Ok(t *testing.T) { ci.Parallel(t) @@ -1678,14 +1678,14 @@ func TestAgent_ServerConfig_JobDefaultPriority_Ok(t *testing.T) { } t.Run(v, func(t *testing.T) { conf := DevConfig(nil) - require.NoError(t, conf.normalizeAddrs()) + must.NoError(t, conf.normalizeAddrs()) conf.Server.JobDefaultPriority = tc.defaultPriority serverConf, err := convertServerConfig(conf) - require.NoError(t, err) + must.NoError(t, err) - assert.Equal(t, tc.jobDefaultPriority, serverConf.JobDefaultPriority, "job default priority") + must.Eq(t, tc.jobDefaultPriority, serverConf.JobDefaultPriority) }) } } @@ -1701,13 +1701,13 @@ func TestAgent_ServerConfig_JobDefaultPriority_Bad(t *testing.T) { for _, tc := range cases { t.Run(fmt.Sprintf("%v", tc), func(t *testing.T) { conf := DevConfig(nil) - require.NoError(t, conf.normalizeAddrs()) + must.NoError(t, conf.normalizeAddrs()) conf.Server.JobDefaultPriority = &tc _, err := convertServerConfig(conf) - require.Error(t, err) - require.Contains(t, err.Error(), "job_default_priority cannot be") + must.Error(t, err) + must.ErrorContains(t, err, "job_default_priority cannot be") }) } } diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 5e988f56efd..c42a27ff11f 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -6692,8 +6692,8 @@ func TestJobEndpoint_ValidateJob_PriorityNotOk(t *testing.T) { j.Priority = 0 err := validateJob(j) - require.Error(t, err) - require.Contains(t, err.Error(), "job priority must be between") + must.Error(t, err) + must.ErrorContains(t, err, "job priority must be between") }) t.Run("job with invalid max priority", func(t *testing.T) { @@ -6701,8 +6701,8 @@ func TestJobEndpoint_ValidateJob_PriorityNotOk(t *testing.T) { j.Priority = 101 err := validateJob(j) - require.Error(t, err) - require.Contains(t, err.Error(), "job priority must be between") + must.Error(t, err) + must.ErrorContains(t, err, "job priority must be between") }) } From 958a7a8ad3f2a9de7a74f469d13e03c2ae65d634 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 16:38:38 +0100 Subject: [PATCH 07/15] remove unecessary value conversion --- command/agent/agent.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index dc1adff5a55..fde93b072f4 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -326,14 +326,14 @@ func convertServerConfig(agentConfig *Config) (*nomad.Config, error) { } } - jobMaxPriority := int(structs.JobDefaultMaxPriority) + jobMaxPriority := structs.JobDefaultMaxPriority if agentConfig.Server.JobMaxPriority != nil && *agentConfig.Server.JobMaxPriority != 0 { jobMaxPriority = *agentConfig.Server.JobMaxPriority if jobMaxPriority < structs.JobDefaultMaxPriority || jobMaxPriority > structs.JobMaxPriority { return nil, fmt.Errorf("job_max_priority cannot be %d. Must be between %d and %d", *agentConfig.Server.JobMaxPriority, structs.JobDefaultMaxPriority, structs.JobMaxPriority) } } - jobDefaultPriority := int(structs.JobDefaultPriority) + jobDefaultPriority := structs.JobDefaultPriority if agentConfig.Server.JobDefaultPriority != nil && *agentConfig.Server.JobDefaultPriority != 0 { jobDefaultPriority = *agentConfig.Server.JobDefaultPriority if jobDefaultPriority < structs.JobDefaultPriority || jobDefaultPriority >= jobMaxPriority { From 108cf6579ab595b3fc2e6bf06efd403fd1672edc Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 16:38:53 +0100 Subject: [PATCH 08/15] improve comments --- nomad/structs/structs.go | 2 +- website/content/docs/configuration/server.mdx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4079b36cadb..37f57ac4488 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4162,7 +4162,7 @@ const ( // JobDefaultMaxPriority is the default maximum allowed priority JobDefaultMaxPriority = 100 - // JobMaxPriority is the maximum allowed priority + // JobMaxPriority is the maximum allowed configuration value for maximum job priority JobMaxPriority = math.MaxInt16 - 1 // CoreJobPriority should be higher than any user diff --git a/website/content/docs/configuration/server.mdx b/website/content/docs/configuration/server.mdx index 07a7ef9330b..6e61ce14692 100644 --- a/website/content/docs/configuration/server.mdx +++ b/website/content/docs/configuration/server.mdx @@ -250,10 +250,10 @@ server { for the Nomad search API. - `job_max_priority` `(int: 100)` - Specifies the maximum priority that can be assigned to a job. - A valid value must be between `100` and `32766` + A valid value must be between `100` and `32766`. - `job_default_priority` `(int: 50)` - Specifies the default priority assigned to a job. - A valid value must be between `50` and `job_max_priority` + A valid value must be between `50` and `job_max_priority`. ### Deprecated Parameters From fc3cce4de5c31d857d4f175e67e4089f92f88147 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 16:42:40 +0100 Subject: [PATCH 09/15] add changelog --- .changelog/16084.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/16084.txt diff --git a/.changelog/16084.txt b/.changelog/16084.txt new file mode 100644 index 00000000000..c8b23bf25aa --- /dev/null +++ b/.changelog/16084.txt @@ -0,0 +1,3 @@ +```release-note:improvement +agent: Allow configurable range of Job priorities +``` From 1f1b455e8ac48c0901acd244ae4f2ebcae56746b Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 19:44:29 +0100 Subject: [PATCH 10/15] fix job default priority not correctly using the server config --- api/jobs.go | 5 +- command/agent/job_endpoint.go | 5 + command/agent/job_endpoint_test.go | 177 ++++++++++++++++++----------- 3 files changed, 114 insertions(+), 73 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 5a8b96b6478..55514b40efa 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -31,9 +31,6 @@ const ( // DefaultNamespace is the default namespace. DefaultNamespace = "default" - // DefaultPriority is the default priority - DefaultPriority = 50 - // For Job configuration, GlobalRegion is a sentinel region value // that users may specify to indicate the job should be run on // the region of the node that the job was submitted to. @@ -941,7 +938,7 @@ func (j *Job) Canonicalize() { j.Namespace = pointerOf(DefaultNamespace) } if j.Priority == nil { - j.Priority = pointerOf(DefaultPriority) + j.Priority = pointerOf(0) } if j.Stop == nil { j.Stop = pointerOf(false) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 407e782b660..27b9d9603b8 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -826,6 +826,11 @@ func (s *HTTPServer) apiJobAndRequestToStructs(job *api.Job, req *http.Request, sJob.Region = jobRegion writeReq.Region = requestRegion + // If the job priority is not set, we fallback on the defaults specified in the server config + if sJob.Priority == 0 { + sJob.Priority = s.agent.Server().GetConfig().JobDefaultPriority + } + queryNamespace := req.URL.Query().Get("namespace") namespace := namespaceForJob(job.Namespace, queryNamespace, writeReq.Namespace) sJob.Namespace = namespace diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index fa2a19a3ec6..1c466c85f68 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -14,8 +14,10 @@ import ( api "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/helper/pointer" + "github.com/hashicorp/nomad/nomad" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/shoenig/test/must" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1941,69 +1943,81 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { agentRegion := "agentRegion" cases := []struct { - name string - jobRegion string - multiregion *api.Multiregion - queryRegion string - queryNamespace string - queryToken string - apiRegion string - apiNamespace string - apiToken string - expectedRequestRegion string - expectedJobRegion string - expectedToken string - expectedNamespace string + name string + jobRegion string + multiregion *api.Multiregion + queryRegion string + queryNamespace string + queryToken string + apiRegion string + apiNamespace string + apiToken string + jobDefaultPriority int + expectedRequestRegion string + expectedJobRegion string + expectedToken string + expectedNamespace string + expectedJobDefaultPriority int }{ { - name: "no region provided at all", - jobRegion: "", - multiregion: nil, - queryRegion: "", - expectedRequestRegion: agentRegion, - expectedJobRegion: agentRegion, - expectedToken: "", - expectedNamespace: "default", + name: "no region provided at all", + jobRegion: "", + multiregion: nil, + queryRegion: "", + jobDefaultPriority: 0, + expectedRequestRegion: agentRegion, + expectedJobRegion: agentRegion, + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { - name: "no region provided but multiregion safe", - jobRegion: "", - multiregion: &api.Multiregion{}, - queryRegion: "", - expectedRequestRegion: agentRegion, - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", + name: "no region provided but multiregion safe", + jobRegion: "", + multiregion: &api.Multiregion{}, + queryRegion: "", + jobDefaultPriority: 0, + expectedRequestRegion: agentRegion, + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { - name: "region flag provided", - jobRegion: "", - multiregion: nil, - queryRegion: "west", - expectedRequestRegion: "west", - expectedJobRegion: "west", - expectedToken: "", - expectedNamespace: "default", + name: "region flag provided", + jobRegion: "", + multiregion: nil, + queryRegion: "west", + jobDefaultPriority: 0, + expectedRequestRegion: "west", + expectedJobRegion: "west", + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { - name: "job region provided", - jobRegion: "west", - multiregion: nil, - queryRegion: "", - expectedRequestRegion: "west", - expectedJobRegion: "west", - expectedToken: "", - expectedNamespace: "default", + name: "job region provided", + jobRegion: "west", + multiregion: nil, + queryRegion: "", + jobDefaultPriority: 0, + expectedRequestRegion: "west", + expectedJobRegion: "west", + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { - name: "job region overridden by region flag", - jobRegion: "west", - multiregion: nil, - queryRegion: "east", - expectedRequestRegion: "east", - expectedJobRegion: "east", - expectedToken: "", - expectedNamespace: "default", + name: "job region overridden by region flag", + jobRegion: "west", + multiregion: nil, + queryRegion: "east", + jobDefaultPriority: 0, + expectedRequestRegion: "east", + expectedJobRegion: "east", + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { name: "multiregion to valid region", @@ -2012,11 +2026,13 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { {Name: "west"}, {Name: "east"}, }}, - queryRegion: "east", - expectedRequestRegion: "east", - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", + queryRegion: "east", + jobDefaultPriority: 0, + expectedRequestRegion: "east", + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, }, { name: "multiregion sent to wrong region", @@ -2025,25 +2041,47 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { {Name: "west"}, {Name: "east"}, }}, - queryRegion: "north", - expectedRequestRegion: "west", - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", + queryRegion: "north", + jobDefaultPriority: 0, + expectedRequestRegion: "west", + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: structs.JobDefaultPriority, + }, + { + name: "default job priority ", + jobRegion: "", + multiregion: nil, + queryRegion: "", + jobDefaultPriority: 100, + expectedRequestRegion: agentRegion, + expectedJobRegion: agentRegion, + expectedToken: "", + expectedNamespace: "default", + expectedJobDefaultPriority: 100, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + nomadServer, cleanup := nomad.TestServer(t, func(c *nomad.Config) { + c.JobDefaultPriority = structs.JobDefaultPriority + }) + defer cleanup() // we need a valid agent config but we don't want to start up // a real server for this srv := &HTTPServer{} - srv.agent = &Agent{config: &Config{Region: agentRegion}} + srv.agent = &Agent{ + server: nomadServer, + config: &Config{Region: agentRegion}, + } job := &api.Job{ Region: pointer.Of(tc.jobRegion), Multiregion: tc.multiregion, + Priority: pointer.Of(tc.jobDefaultPriority), } req, _ := http.NewRequest("POST", "/", nil) @@ -2066,11 +2104,12 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { } sJob, sWriteReq := srv.apiJobAndRequestToStructs(job, req, apiReq) - require.Equal(t, tc.expectedJobRegion, sJob.Region) - require.Equal(t, tc.expectedNamespace, sJob.Namespace) - require.Equal(t, tc.expectedNamespace, sWriteReq.Namespace) - require.Equal(t, tc.expectedRequestRegion, sWriteReq.Region) - require.Equal(t, tc.expectedToken, sWriteReq.AuthToken) + must.Eq(t, tc.expectedJobRegion, sJob.Region) + must.Eq(t, tc.expectedNamespace, sJob.Namespace) + must.Eq(t, tc.expectedNamespace, sWriteReq.Namespace) + must.Eq(t, tc.expectedRequestRegion, sWriteReq.Region) + must.Eq(t, tc.expectedToken, sWriteReq.AuthToken) + must.Eq(t, tc.expectedJobDefaultPriority, sJob.Priority) }) } } From acf4369f272be278797ba70d0b9960f9f91a94d3 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 21:36:53 +0100 Subject: [PATCH 11/15] fix ValidateJobRequest not picking up the default server config --- command/agent/job_endpoint.go | 5 +++++ command/agent/job_endpoint_test.go | 7 ++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 27b9d9603b8..e735f594d94 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -196,6 +196,11 @@ func (s *HTTPServer) ValidateJobRequest(resp http.ResponseWriter, req *http.Requ job := ApiJobToStructJob(validateRequest.Job) + // If the job priority is not set, we fallback on the defaults specified in the server config + if job.Priority == 0 { + job.Priority = s.agent.Server().GetConfig().JobDefaultPriority + } + args := structs.JobValidateRequest{ Job: job, WriteRequest: structs.WriteRequest{ diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 1c466c85f68..342ff7fa691 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -3585,16 +3585,17 @@ func TestHTTP_JobValidate_SystemMigrate(t *testing.T) { // Make the HTTP request req, err := http.NewRequest("PUT", "/v1/validate/job", buf) - require.NoError(t, err) + must.NoError(t, err) respW := httptest.NewRecorder() // Make the request obj, err := s.Server.ValidateJobRequest(respW, req) - require.NoError(t, err) + must.NoError(t, err) // Check the response resp := obj.(structs.JobValidateResponse) - require.Contains(t, resp.Error, `Job type "system" does not allow migrate block`) + must.StrContains(t, resp.Error, `Job type "system" does not allow migrate block`) + must.Len(t, 1, resp.ValidationErrors) }) } From 55177b6c5ef681195a806a75e79a7b69e9801dbd Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 22:15:57 +0100 Subject: [PATCH 12/15] modify docs of job --- website/content/docs/job-specification/job.mdx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/website/content/docs/job-specification/job.mdx b/website/content/docs/job-specification/job.mdx index 998fced726b..bc1195f1515 100644 --- a/website/content/docs/job-specification/job.mdx +++ b/website/content/docs/job-specification/job.mdx @@ -105,8 +105,10 @@ job "docs" { at fixed times, dates or intervals. - `priority` `(int: 50)` - Specifies the job priority which is used to - prioritize scheduling and access to resources. Must be between 1 and 100 - inclusively, with a larger value corresponding to a higher priority. + prioritize scheduling and access to resources. + Must be between 1 and [`server` block `job_max_priority`] inclusively, + with a larger value corresponding to a higher priority. + If value 0 is provided this will fallback to [`server` block `job_default_priority`]. Priority only has an effect when job preemption is enabled. It does not have an effect on which of multiple pending jobs is run first. @@ -257,3 +259,5 @@ $ VAULT_TOKEN="..." nomad job run example.nomad.hcl [task]: /nomad/docs/job-specification/task 'Nomad task Job Specification' [update]: /nomad/docs/job-specification/update 'Nomad update Job Specification' [vault]: /nomad/docs/job-specification/vault 'Nomad vault Job Specification' +[`server` block `job_max_priority`]: /nomad/docs/configuration/server#job_max_priority +[`server` block `job_default_priority`]: /nomad/docs/configuration/server#job_default_priority From ac2f0a2cae23f6b13d64f34cfec734b455fb18db Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 22:21:12 +0100 Subject: [PATCH 13/15] fix failing tests for job canonicalize --- api/jobs_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 27f79b17ad0..dd3a3bb6fb6 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -279,7 +279,7 @@ func TestJobs_Canonicalize(t *testing.T) { Namespace: pointerOf(DefaultNamespace), Type: pointerOf("service"), ParentID: pointerOf(""), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -374,7 +374,7 @@ func TestJobs_Canonicalize(t *testing.T) { Namespace: pointerOf(DefaultNamespace), Type: pointerOf("batch"), ParentID: pointerOf(""), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -452,7 +452,7 @@ func TestJobs_Canonicalize(t *testing.T) { Region: pointerOf("global"), Type: pointerOf("service"), ParentID: pointerOf("lol"), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -621,7 +621,7 @@ func TestJobs_Canonicalize(t *testing.T) { ID: pointerOf("example_template"), Name: pointerOf("example_template"), ParentID: pointerOf(""), - Priority: pointerOf(50), + Priority: pointerOf(0), Region: pointerOf("global"), Type: pointerOf("service"), AllAtOnce: pointerOf(false), @@ -791,7 +791,7 @@ func TestJobs_Canonicalize(t *testing.T) { Name: pointerOf("bar"), Region: pointerOf("global"), Type: pointerOf("service"), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -882,7 +882,7 @@ func TestJobs_Canonicalize(t *testing.T) { Region: pointerOf("global"), Type: pointerOf("service"), ParentID: pointerOf("lol"), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -1058,7 +1058,7 @@ func TestJobs_Canonicalize(t *testing.T) { Region: pointerOf("global"), Type: pointerOf("service"), ParentID: pointerOf("lol"), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), @@ -1229,7 +1229,7 @@ func TestJobs_Canonicalize(t *testing.T) { Region: pointerOf("global"), Type: pointerOf("service"), ParentID: pointerOf("lol"), - Priority: pointerOf(50), + Priority: pointerOf(0), AllAtOnce: pointerOf(false), ConsulToken: pointerOf(""), ConsulNamespace: pointerOf(""), From 0c42e44aa27849cc31388243730fd5451f0fdeb5 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 23:15:29 +0100 Subject: [PATCH 14/15] move the job priority = 0 logic inside jobCanonicalizer --- command/agent/job_endpoint.go | 11 -- command/agent/job_endpoint_test.go | 166 +++++++++++------------------ nomad/job_endpoint.go | 2 +- nomad/job_endpoint_hooks.go | 14 ++- nomad/job_endpoint_hooks_test.go | 49 +++++++++ nomad/job_endpoint_test.go | 2 +- 6 files changed, 126 insertions(+), 118 deletions(-) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index e735f594d94..2f68678d42e 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -195,12 +195,6 @@ func (s *HTTPServer) ValidateJobRequest(resp http.ResponseWriter, req *http.Requ } job := ApiJobToStructJob(validateRequest.Job) - - // If the job priority is not set, we fallback on the defaults specified in the server config - if job.Priority == 0 { - job.Priority = s.agent.Server().GetConfig().JobDefaultPriority - } - args := structs.JobValidateRequest{ Job: job, WriteRequest: structs.WriteRequest{ @@ -831,11 +825,6 @@ func (s *HTTPServer) apiJobAndRequestToStructs(job *api.Job, req *http.Request, sJob.Region = jobRegion writeReq.Region = requestRegion - // If the job priority is not set, we fallback on the defaults specified in the server config - if sJob.Priority == 0 { - sJob.Priority = s.agent.Server().GetConfig().JobDefaultPriority - } - queryNamespace := req.URL.Query().Get("namespace") namespace := namespaceForJob(job.Namespace, queryNamespace, writeReq.Namespace) sJob.Namespace = namespace diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 342ff7fa691..95c5e72c5cb 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -14,7 +14,6 @@ import ( api "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/helper/pointer" - "github.com/hashicorp/nomad/nomad" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/shoenig/test/must" @@ -1943,81 +1942,69 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { agentRegion := "agentRegion" cases := []struct { - name string - jobRegion string - multiregion *api.Multiregion - queryRegion string - queryNamespace string - queryToken string - apiRegion string - apiNamespace string - apiToken string - jobDefaultPriority int - expectedRequestRegion string - expectedJobRegion string - expectedToken string - expectedNamespace string - expectedJobDefaultPriority int + name string + jobRegion string + multiregion *api.Multiregion + queryRegion string + queryNamespace string + queryToken string + apiRegion string + apiNamespace string + apiToken string + expectedRequestRegion string + expectedJobRegion string + expectedToken string + expectedNamespace string }{ { - name: "no region provided at all", - jobRegion: "", - multiregion: nil, - queryRegion: "", - jobDefaultPriority: 0, - expectedRequestRegion: agentRegion, - expectedJobRegion: agentRegion, - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + name: "no region provided at all", + jobRegion: "", + multiregion: nil, + queryRegion: "", + expectedRequestRegion: agentRegion, + expectedJobRegion: agentRegion, + expectedToken: "", + expectedNamespace: "default", }, { - name: "no region provided but multiregion safe", - jobRegion: "", - multiregion: &api.Multiregion{}, - queryRegion: "", - jobDefaultPriority: 0, - expectedRequestRegion: agentRegion, - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + name: "no region provided but multiregion safe", + jobRegion: "", + multiregion: &api.Multiregion{}, + queryRegion: "", + expectedRequestRegion: agentRegion, + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", }, { - name: "region flag provided", - jobRegion: "", - multiregion: nil, - queryRegion: "west", - jobDefaultPriority: 0, - expectedRequestRegion: "west", - expectedJobRegion: "west", - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + name: "region flag provided", + jobRegion: "", + multiregion: nil, + queryRegion: "west", + expectedRequestRegion: "west", + expectedJobRegion: "west", + expectedToken: "", + expectedNamespace: "default", }, { - name: "job region provided", - jobRegion: "west", - multiregion: nil, - queryRegion: "", - jobDefaultPriority: 0, - expectedRequestRegion: "west", - expectedJobRegion: "west", - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + name: "job region provided", + jobRegion: "west", + multiregion: nil, + queryRegion: "", + expectedRequestRegion: "west", + expectedJobRegion: "west", + expectedToken: "", + expectedNamespace: "default", }, { - name: "job region overridden by region flag", - jobRegion: "west", - multiregion: nil, - queryRegion: "east", - jobDefaultPriority: 0, - expectedRequestRegion: "east", - expectedJobRegion: "east", - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + name: "job region overridden by region flag", + jobRegion: "west", + multiregion: nil, + queryRegion: "east", + expectedRequestRegion: "east", + expectedJobRegion: "east", + expectedToken: "", + expectedNamespace: "default", }, { name: "multiregion to valid region", @@ -2026,13 +2013,11 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { {Name: "west"}, {Name: "east"}, }}, - queryRegion: "east", - jobDefaultPriority: 0, - expectedRequestRegion: "east", - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, + queryRegion: "east", + expectedRequestRegion: "east", + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", }, { name: "multiregion sent to wrong region", @@ -2041,47 +2026,25 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { {Name: "west"}, {Name: "east"}, }}, - queryRegion: "north", - jobDefaultPriority: 0, - expectedRequestRegion: "west", - expectedJobRegion: api.GlobalRegion, - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: structs.JobDefaultPriority, - }, - { - name: "default job priority ", - jobRegion: "", - multiregion: nil, - queryRegion: "", - jobDefaultPriority: 100, - expectedRequestRegion: agentRegion, - expectedJobRegion: agentRegion, - expectedToken: "", - expectedNamespace: "default", - expectedJobDefaultPriority: 100, + queryRegion: "north", + expectedRequestRegion: "west", + expectedJobRegion: api.GlobalRegion, + expectedToken: "", + expectedNamespace: "default", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - nomadServer, cleanup := nomad.TestServer(t, func(c *nomad.Config) { - c.JobDefaultPriority = structs.JobDefaultPriority - }) - defer cleanup() // we need a valid agent config but we don't want to start up // a real server for this srv := &HTTPServer{} - srv.agent = &Agent{ - server: nomadServer, - config: &Config{Region: agentRegion}, - } + srv.agent = &Agent{config: &Config{Region: agentRegion}} job := &api.Job{ Region: pointer.Of(tc.jobRegion), Multiregion: tc.multiregion, - Priority: pointer.Of(tc.jobDefaultPriority), } req, _ := http.NewRequest("POST", "/", nil) @@ -2109,7 +2072,6 @@ func TestJobs_ParsingWriteRequest(t *testing.T) { must.Eq(t, tc.expectedNamespace, sWriteReq.Namespace) must.Eq(t, tc.expectedRequestRegion, sWriteReq.Region) must.Eq(t, tc.expectedToken, sWriteReq.AuthToken) - must.Eq(t, tc.expectedJobDefaultPriority, sJob.Priority) }) } } diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index a0cdd20a82f..2fd00933770 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -66,7 +66,7 @@ func NewJobEndpoints(s *Server, ctx *RPCContext) *Job { ctx: ctx, logger: s.logger.Named("job"), mutators: []jobMutator{ - jobCanonicalizer{}, + &jobCanonicalizer{srv: s}, jobConnectHook{}, jobExposeCheckHook{}, jobImpliedConstraints{}, diff --git a/nomad/job_endpoint_hooks.go b/nomad/job_endpoint_hooks.go index d6b2c0d3f56..f823a0f1991 100644 --- a/nomad/job_endpoint_hooks.go +++ b/nomad/job_endpoint_hooks.go @@ -126,14 +126,22 @@ func (j *Job) admissionValidators(origJob *structs.Job) ([]error, error) { // jobCanonicalizer calls job.Canonicalize (sets defaults and initializes // fields) and returns any errors as warnings. -type jobCanonicalizer struct{} +type jobCanonicalizer struct { + srv *Server +} -func (jobCanonicalizer) Name() string { +func (c *jobCanonicalizer) Name() string { return "canonicalize" } -func (jobCanonicalizer) Mutate(job *structs.Job) (*structs.Job, []error, error) { +func (c *jobCanonicalizer) Mutate(job *structs.Job) (*structs.Job, []error, error) { job.Canonicalize() + + // If the job priority is not set, we fallback on the defaults specified in the server config + if job.Priority == 0 { + job.Priority = c.srv.GetConfig().JobDefaultPriority + } + return job, nil, nil } diff --git a/nomad/job_endpoint_hooks_test.go b/nomad/job_endpoint_hooks_test.go index 67ee2971d0b..f16d39fb5c5 100644 --- a/nomad/job_endpoint_hooks_test.go +++ b/nomad/job_endpoint_hooks_test.go @@ -5,6 +5,7 @@ import ( "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/nomad/structs" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -717,3 +718,51 @@ func Test_jobImpliedConstraints_Mutate(t *testing.T) { }) } } + +func Test_jobCanonicalizer_Mutate(t *testing.T) { + ci.Parallel(t) + + serverJobDefaultPriority := 100 + + testCases := []struct { + name string + inputJob *structs.Job + expectedOutputJob *structs.Job + }{ + { + name: "no mutation", + inputJob: &structs.Job{ + Namespace: "default", + Datacenters: []string{"*"}, + Priority: 123, + }, + expectedOutputJob: &structs.Job{ + Namespace: "default", + Datacenters: []string{"*"}, + Priority: 123, + }, + }, + { + name: "when priority is 0 mutate using the value present in the server config", + inputJob: &structs.Job{ + Namespace: "default", + Datacenters: []string{"*"}, + Priority: 0, + }, + expectedOutputJob: &structs.Job{ + Namespace: "default", + Datacenters: []string{"*"}, + Priority: serverJobDefaultPriority, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + impl := jobCanonicalizer{srv: &Server{config: &Config{JobDefaultPriority: serverJobDefaultPriority}}} + actualJob, actualWarnings, actualError := impl.Mutate(tc.inputJob) + must.Eq(t, tc.expectedOutputJob, actualJob) + must.NoError(t, actualError) + must.Nil(t, actualWarnings) + }) + } +} diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index c42a27ff11f..272622e3c06 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -6689,7 +6689,7 @@ func TestJobEndpoint_ValidateJob_PriorityNotOk(t *testing.T) { t.Run("job with invalid min priority", func(t *testing.T) { j := mock.Job() - j.Priority = 0 + j.Priority = -1 err := validateJob(j) must.Error(t, err) From b2202cf7e233a6e86c16de8dbe31d9116fa7e6be Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Thu, 16 Feb 2023 23:16:46 +0100 Subject: [PATCH 15/15] enhance job doc --- website/content/docs/job-specification/job.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/website/content/docs/job-specification/job.mdx b/website/content/docs/job-specification/job.mdx index bc1195f1515..8b0b5b5985c 100644 --- a/website/content/docs/job-specification/job.mdx +++ b/website/content/docs/job-specification/job.mdx @@ -106,9 +106,9 @@ job "docs" { - `priority` `(int: 50)` - Specifies the job priority which is used to prioritize scheduling and access to resources. - Must be between 1 and [`server` block `job_max_priority`] inclusively, + Must be between 1 and [`job_max_priority`] inclusively, with a larger value corresponding to a higher priority. - If value 0 is provided this will fallback to [`server` block `job_default_priority`]. + If value 0 is provided this will fallback to [`job_default_priority`]. Priority only has an effect when job preemption is enabled. It does not have an effect on which of multiple pending jobs is run first. @@ -259,5 +259,5 @@ $ VAULT_TOKEN="..." nomad job run example.nomad.hcl [task]: /nomad/docs/job-specification/task 'Nomad task Job Specification' [update]: /nomad/docs/job-specification/update 'Nomad update Job Specification' [vault]: /nomad/docs/job-specification/vault 'Nomad vault Job Specification' -[`server` block `job_max_priority`]: /nomad/docs/configuration/server#job_max_priority -[`server` block `job_default_priority`]: /nomad/docs/configuration/server#job_default_priority +[`job_max_priority`]: /nomad/docs/configuration/server#job_max_priority +[`job_default_priority`]: /nomad/docs/configuration/server#job_default_priority