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.
hashicorp · Jan 5, 2018 · dff64ef · dff64ef
1 parent 39e08eb
commit dff64ef
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 6 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"
+            }
+          }
+        }
+    }
+}
+