From 68a95030298b0d0faeaca9126fcb1ddc3cbaea0d Mon Sep 17 00:00:00 2001 From: Lang Martin Date: Tue, 21 May 2019 15:37:19 -0400 Subject: [PATCH] api use job.update as the default for taskgroup.update --- api/jobs.go | 10 ++-- api/jobs_test.go | 4 +- command/agent/job_endpoint.go | 43 +++++++++++++----- command/agent/job_endpoint_test.go | 73 ++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 20 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 6a708ca92488..4f73da10c6eb 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -375,14 +375,15 @@ type UpdateStrategy struct { MinHealthyTime *time.Duration `mapstructure:"min_healthy_time"` HealthyDeadline *time.Duration `mapstructure:"healthy_deadline"` ProgressDeadline *time.Duration `mapstructure:"progress_deadline"` + Canary *int `mapstructure:"canary"` AutoRevert *bool `mapstructure:"auto_revert"` AutoPromote *bool `mapstructure:"auto_promote"` - Canary *int `mapstructure:"canary"` } // DefaultUpdateStrategy provides a baseline that can be used to upgrade // jobs with the old policy or for populating field defaults. func DefaultUpdateStrategy() *UpdateStrategy { + // boolPtr fields are omitted to avoid masking an unconfigured nil return &UpdateStrategy{ Stagger: timeToPtr(30 * time.Second), MaxParallel: intToPtr(1), @@ -392,7 +393,6 @@ func DefaultUpdateStrategy() *UpdateStrategy { ProgressDeadline: timeToPtr(10 * time.Minute), AutoRevert: boolToPtr(false), Canary: intToPtr(0), - AutoPromote: boolToPtr(false), } } @@ -487,6 +487,8 @@ func (u *UpdateStrategy) Merge(o *UpdateStrategy) { func (u *UpdateStrategy) Canonicalize() { d := DefaultUpdateStrategy() + // boolPtr fields are omitted to avoid masking an unconfigured nil + if u.MaxParallel == nil { u.MaxParallel = d.MaxParallel } @@ -518,10 +520,6 @@ func (u *UpdateStrategy) Canonicalize() { if u.Canary == nil { u.Canary = d.Canary } - - if u.AutoPromote == nil { - u.AutoPromote = d.AutoPromote - } } // Empty returns whether the UpdateStrategy is empty or has user defined values. diff --git a/api/jobs_test.go b/api/jobs_test.go index 62fb39a214ca..d55351214f0a 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -323,7 +323,7 @@ func TestJobs_Canonicalize(t *testing.T) { ProgressDeadline: timeToPtr(10 * time.Minute), AutoRevert: boolToPtr(false), Canary: intToPtr(0), - AutoPromote: boolToPtr(false), + AutoPromote: nil, }, TaskGroups: []*TaskGroup{ { @@ -358,7 +358,7 @@ func TestJobs_Canonicalize(t *testing.T) { ProgressDeadline: timeToPtr(10 * time.Minute), AutoRevert: boolToPtr(false), Canary: intToPtr(0), - AutoPromote: boolToPtr(false), + AutoPromote: nil, }, Migrate: DefaultMigrateStrategy(), Tasks: []*Task{ diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 7057264b113c..d81fcbd3b114 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -655,7 +655,7 @@ func ApiJobToStructJob(job *api.Job) *structs.Job { j.TaskGroups = make([]*structs.TaskGroup, l) for i, taskGroup := range job.TaskGroups { tg := &structs.TaskGroup{} - ApiTgToStructsTG(taskGroup, tg) + apiTgToStructsTG(taskGroup, tg, job.Update) j.TaskGroups[i] = tg } } @@ -663,7 +663,7 @@ func ApiJobToStructJob(job *api.Job) *structs.Job { return j } -func ApiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup) { +func apiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup, jobUpdate *api.UpdateStrategy) { tg.Name = *taskGroup.Name tg.Count = *taskGroup.Count tg.Meta = taskGroup.Meta @@ -710,17 +710,32 @@ func ApiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup) { } } - if taskGroup.Update != nil { + // Convert the UpdateStrategy, using defaults from the job + if taskGroup.Update != nil || jobUpdate != nil { + var au *api.UpdateStrategy + if jobUpdate != nil { + au = jobUpdate.Copy() + au.Merge(taskGroup.Update) + } else { + au = taskGroup.Update + } + tg.Update = &structs.UpdateStrategy{ - Stagger: *taskGroup.Update.Stagger, - MaxParallel: *taskGroup.Update.MaxParallel, - HealthCheck: *taskGroup.Update.HealthCheck, - MinHealthyTime: *taskGroup.Update.MinHealthyTime, - HealthyDeadline: *taskGroup.Update.HealthyDeadline, - ProgressDeadline: *taskGroup.Update.ProgressDeadline, - AutoRevert: *taskGroup.Update.AutoRevert, - AutoPromote: *taskGroup.Update.AutoPromote, - Canary: *taskGroup.Update.Canary, + Stagger: *au.Stagger, + MaxParallel: *au.MaxParallel, + HealthCheck: *au.HealthCheck, + MinHealthyTime: *au.MinHealthyTime, + HealthyDeadline: *au.HealthyDeadline, + ProgressDeadline: *au.ProgressDeadline, + Canary: *au.Canary, + } + + if au.AutoRevert != nil { + tg.Update.AutoRevert = *au.AutoRevert + } + + if au.AutoPromote != nil { + tg.Update.AutoPromote = *au.AutoPromote } } @@ -734,6 +749,10 @@ func ApiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup) { } } +func ApiTgToStructsTG(taskGroup *api.TaskGroup, tg *structs.TaskGroup) { + apiTgToStructsTG(taskGroup, tg, nil) +} + // ApiTaskToStructsTask is a copy and type conversion between the API // representation of a task from a struct representation of a task. func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) { diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index aed3894e3063..77329a127bb4 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2040,6 +2040,79 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { } } +func TestJobs_ApiJobToStructsJobUpdate(t *testing.T) { + apiJob := &api.Job{ + Update: &api.UpdateStrategy{ + Stagger: helper.TimeToPtr(1 * time.Second), + MaxParallel: helper.IntToPtr(5), + HealthCheck: helper.StringToPtr(structs.UpdateStrategyHealthCheck_Manual), + MinHealthyTime: helper.TimeToPtr(1 * time.Minute), + HealthyDeadline: helper.TimeToPtr(3 * time.Minute), + ProgressDeadline: helper.TimeToPtr(3 * time.Minute), + AutoRevert: helper.BoolToPtr(false), + AutoPromote: nil, + Canary: helper.IntToPtr(1), + }, + TaskGroups: []*api.TaskGroup{ + { + Update: &api.UpdateStrategy{ + Canary: helper.IntToPtr(2), + AutoRevert: helper.BoolToPtr(true), + }, + }, { + Update: &api.UpdateStrategy{ + Canary: helper.IntToPtr(3), + AutoPromote: helper.BoolToPtr(true), + }, + }, + }, + } + + structsJob := ApiJobToStructJob(apiJob) + + // Update has been moved from job down to the groups + jobUpdate := structs.UpdateStrategy{ + Stagger: 1000000000, + MaxParallel: 5, + HealthCheck: "", + MinHealthyTime: 0, + HealthyDeadline: 0, + ProgressDeadline: 0, + AutoRevert: false, + AutoPromote: false, + Canary: 0, + } + + // But the groups inherit settings from the job update + group1 := structs.UpdateStrategy{ + Stagger: 1000000000, + MaxParallel: 5, + HealthCheck: "manual", + MinHealthyTime: 60000000000, + HealthyDeadline: 180000000000, + ProgressDeadline: 180000000000, + AutoRevert: true, + AutoPromote: false, + Canary: 2, + } + + group2 := structs.UpdateStrategy{ + Stagger: 1000000000, + MaxParallel: 5, + HealthCheck: "manual", + MinHealthyTime: 60000000000, + HealthyDeadline: 180000000000, + ProgressDeadline: 180000000000, + AutoRevert: false, + AutoPromote: true, + Canary: 3, + } + + require.Equal(t, jobUpdate, structsJob.Update) + require.Equal(t, group1, *structsJob.TaskGroups[0].Update) + require.Equal(t, group2, *structsJob.TaskGroups[1].Update) +} + // TestHTTP_JobValidate_SystemMigrate asserts that a system job with a migrate // stanza fails to validate but does not panic (see #5477). func TestHTTP_JobValidate_SystemMigrate(t *testing.T) {