From dff64ef9ea3ecdca593d6d57f7171bf22aafbe3f Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Thu, 4 Jan 2018 15:04:45 -0800 Subject: [PATCH] Fix service.check_restart stanza propagation There was a bug in jobspec parsing, a bug in CheckRestart merging, and a bug in CheckRestart canonicalization. All are now tested. --- CHANGELOG.md | 1 + api/tasks.go | 6 ++-- command/agent/job_endpoint_test.go | 26 ++++++++++++-- jobspec/parse.go | 3 +- jobspec/parse_test.go | 36 +++++++++++++++++++ .../test-fixtures/service-check-restart.hcl | 21 +++++++++++ 6 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 jobspec/test-fixtures/service-check-restart.hcl diff --git a/CHANGELOG.md b/CHANGELOG.md index 2837e14ae76..62f09e41f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ BUG FIXES: * core: Fix search endpoint forwarding for multi-region clusters [[GH-3680](https://github.com/hashicorp/nomad/issues/3680)] * config: Revert minimum CPU limit back to 20 from 100. + * discovery: Fix handling of `service.check_restart` [[GH-3718](https://github.com/hashicorp/nomad/issues/3718)] ## 0.7.1 (December 19, 2017) diff --git a/api/tasks.go b/api/tasks.go index 7dc2950b187..847bb869fa7 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -136,7 +136,7 @@ func (c *CheckRestart) Merge(o *CheckRestart) *CheckRestart { nc.Grace = o.Grace } - if nc.IgnoreWarnings { + if !nc.IgnoreWarnings { nc.IgnoreWarnings = o.IgnoreWarnings } @@ -189,9 +189,9 @@ func (s *Service) Canonicalize(t *Task, tg *TaskGroup, job *Job) { // Canonicallize CheckRestart on Checks and merge Service.CheckRestart // into each check. - for _, c := range s.Checks { + for i, c := range s.Checks { + s.Checks[i].CheckRestart = c.CheckRestart.Merge(s.CheckRestart) c.CheckRestart.Canonicalize() - c.CheckRestart = c.CheckRestart.Merge(s.CheckRestart) } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 019a82ae059..b595e28ab2a 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -1212,6 +1212,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { Name: "serviceA", Tags: []string{"1", "2"}, PortLabel: "foo", + CheckRestart: &api.CheckRestart{ + Limit: 4, + Grace: helper.TimeToPtr(11 * time.Second), + }, Checks: []api.ServiceCheck{ { Id: "hello", @@ -1228,10 +1232,17 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { InitialStatus: "ok", CheckRestart: &api.CheckRestart{ Limit: 3, - Grace: helper.TimeToPtr(10 * time.Second), IgnoreWarnings: true, }, }, + { + Id: "check2id", + Name: "check2", + Type: "tcp", + PortLabel: "foo", + Interval: 4 * time.Second, + Timeout: 2 * time.Second, + }, }, }, }, @@ -1425,10 +1436,21 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { InitialStatus: "ok", CheckRestart: &structs.CheckRestart{ Limit: 3, - Grace: 10 * time.Second, + Grace: 11 * time.Second, IgnoreWarnings: true, }, }, + { + Name: "check2", + Type: "tcp", + PortLabel: "foo", + Interval: 4 * time.Second, + Timeout: 2 * time.Second, + CheckRestart: &structs.CheckRestart{ + Limit: 4, + Grace: 11 * time.Second, + }, + }, }, }, }, diff --git a/jobspec/parse.go b/jobspec/parse.go index d25f38bd0f8..b1214b3790a 100644 --- a/jobspec/parse.go +++ b/jobspec/parse.go @@ -912,6 +912,7 @@ func parseServices(jobName string, taskGroupName string, task *api.Task, service "port", "check", "address_mode", + "check_restart", } if err := helper.CheckHCLKeys(o.Val, valid); err != nil { return multierror.Prefix(err, fmt.Sprintf("service (%d) ->", idx)) @@ -1042,7 +1043,7 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { if ot, ok := co.Val.(*ast.ObjectType); ok { checkRestartList = ot.List } else { - return fmt.Errorf("check_restart '%s': should be an object", check.Name) + return fmt.Errorf("check '%s': should be an object", check.Name) } if cro := checkRestartList.Filter("check_restart"); len(cro.Items) > 0 { diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 2dfc890d42f..4134e9ee451 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -631,6 +631,42 @@ func TestParse(t *testing.T) { }, false, }, + { + "service-check-restart.hcl", + &api.Job{ + ID: helper.StringToPtr("service_check_restart"), + Name: helper.StringToPtr("service_check_restart"), + Type: helper.StringToPtr("service"), + TaskGroups: []*api.TaskGroup{ + { + Name: helper.StringToPtr("group"), + Tasks: []*api.Task{ + { + Name: "task", + Services: []*api.Service{ + { + Name: "http-service", + CheckRestart: &api.CheckRestart{ + Limit: 3, + Grace: helper.TimeToPtr(10 * time.Second), + IgnoreWarnings: true, + }, + Checks: []api.ServiceCheck{ + { + Name: "random-check", + Type: "tcp", + PortLabel: "9001", + }, + }, + }, + }, + }, + }, + }, + }, + }, + false, + }, } for _, tc := range cases { diff --git a/jobspec/test-fixtures/service-check-restart.hcl b/jobspec/test-fixtures/service-check-restart.hcl new file mode 100644 index 00000000000..d34f70003c4 --- /dev/null +++ b/jobspec/test-fixtures/service-check-restart.hcl @@ -0,0 +1,21 @@ +job "service_check_restart" { + type = "service" + group "group" { + task "task" { + service { + name = "http-service" + check_restart { + limit = 3 + grace = "10s" + ignore_warnings = true + } + check { + name = "random-check" + type = "tcp" + port = "9001" + } + } + } + } +} +