consul: make failures_before_critical and success_before_passing work…

… with group services This PR fixes some job submission plumbing to make sure the Consul Check parameters - failure_before_critical - success_before_passing work with group-level services. They already work with task-level services.
hashicorp · Jun 15, 2021 · b4a631c · b4a631c
1 parent ab9b589
commit b4a631c
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 85 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@ IMPROVEMENTS:
 * cli: Added `-monitor` flag to `deployment status` command and automatically monitor deployments from `job run` command. [[GH-10661](https://github.com/hashicorp/nomad/pull/10661)]
 
 BUG FIXES:
+* consul: Fixed a bug where consul check parameters missing in group services [[GH-10764](https://github.com/hashicorp/nomad/pull/10764)]
 * consul/connect: Fixed an overly restrictive connect constraint [[GH-10754](https://github.com/hashicorp/nomad/pull/10754)]
 * quotas (Enterprise): Fixed a bug where quotas were evaluated before constraints, resulting in quota capacity being used up by filtered nodes. [[GH-10753](https://github.com/hashicorp/nomad/issues/10753)]
 

diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go
@@ -1044,67 +1044,7 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup,
 		}
 	}
 
-	if l := len(apiTask.Services); l != 0 {
-		structsTask.Services = make([]*structs.Service, l)
-		for i, service := range apiTask.Services {
-			structsTask.Services[i] = &structs.Service{
-				Name:              service.Name,
-				PortLabel:         service.PortLabel,
-				Tags:              service.Tags,
-				CanaryTags:        service.CanaryTags,
-				EnableTagOverride: service.EnableTagOverride,
-				AddressMode:       service.AddressMode,
-				Meta:              helper.CopyMapStringString(service.Meta),
-				CanaryMeta:        helper.CopyMapStringString(service.CanaryMeta),
-				OnUpdate:          service.OnUpdate,
-			}
-
-			if l := len(service.Checks); l != 0 {
-				structsTask.Services[i].Checks = make([]*structs.ServiceCheck, l)
-				for j, check := range service.Checks {
-					onUpdate := service.OnUpdate // Inherit from service as default
-					if check.OnUpdate != "" {
-						onUpdate = check.OnUpdate
-					}
-					structsTask.Services[i].Checks[j] = &structs.ServiceCheck{
-						Name:                   check.Name,
-						Type:                   check.Type,
-						Command:                check.Command,
-						Args:                   check.Args,
-						Path:                   check.Path,
-						Protocol:               check.Protocol,
-						PortLabel:              check.PortLabel,
-						AddressMode:            check.AddressMode,
-						Interval:               check.Interval,
-						Timeout:                check.Timeout,
-						InitialStatus:          check.InitialStatus,
-						TLSSkipVerify:          check.TLSSkipVerify,
-						Header:                 check.Header,
-						Method:                 check.Method,
-						Body:                   check.Body,
-						GRPCService:            check.GRPCService,
-						GRPCUseTLS:             check.GRPCUseTLS,
-						SuccessBeforePassing:   check.SuccessBeforePassing,
-						FailuresBeforeCritical: check.FailuresBeforeCritical,
-						OnUpdate:               onUpdate,
-					}
-					if check.CheckRestart != nil {
-						structsTask.Services[i].Checks[j].CheckRestart = &structs.CheckRestart{
-							Limit:          check.CheckRestart.Limit,
-							Grace:          *check.CheckRestart.Grace,
-							IgnoreWarnings: check.CheckRestart.IgnoreWarnings,
-						}
-					}
-				}
-			}
-
-			// Task services can't have a connect block. We still convert it so that
-			// we can later return a validation error.
-			if service.Connect != nil {
-				structsTask.Services[i].Connect = ApiConsulConnectToStructs(service.Connect)
-			}
-		}
-	}
+	structsTask.Services = ApiServicesToStructs(apiTask.Services)
 
 	structsTask.Resources = ApiResourcesToStructs(apiTask.Resources)
 
@@ -1272,7 +1212,6 @@ func ApiPortToStructs(in api.Port) structs.Port {
 	}
 }
 
-//TODO(schmichael) refactor and reuse in service parsing above
 func ApiServicesToStructs(in []*api.Service) []*structs.Service {
 	if len(in) == 0 {
 		return nil
@@ -1301,26 +1240,28 @@ func ApiServicesToStructs(in []*api.Service) []*structs.Service {
 					onUpdate = check.OnUpdate
 				}
 				out[i].Checks[j] = &structs.ServiceCheck{
-					Name:          check.Name,
-					Type:          check.Type,
-					Command:       check.Command,
-					Args:          check.Args,
-					Path:          check.Path,
-					Protocol:      check.Protocol,
-					PortLabel:     check.PortLabel,
-					Expose:        check.Expose,
-					AddressMode:   check.AddressMode,
-					Interval:      check.Interval,
-					Timeout:       check.Timeout,
-					InitialStatus: check.InitialStatus,
-					TLSSkipVerify: check.TLSSkipVerify,
-					Header:        check.Header,
-					Method:        check.Method,
-					Body:          check.Body,
-					GRPCService:   check.GRPCService,
-					GRPCUseTLS:    check.GRPCUseTLS,
-					TaskName:      check.TaskName,
-					OnUpdate:      onUpdate,
+					Name:                   check.Name,
+					Type:                   check.Type,
+					Command:                check.Command,
+					Args:                   check.Args,
+					Path:                   check.Path,
+					Protocol:               check.Protocol,
+					PortLabel:              check.PortLabel,
+					Expose:                 check.Expose,
+					AddressMode:            check.AddressMode,
+					Interval:               check.Interval,
+					Timeout:                check.Timeout,
+					InitialStatus:          check.InitialStatus,
+					TLSSkipVerify:          check.TLSSkipVerify,
+					Header:                 check.Header,
+					Method:                 check.Method,
+					Body:                   check.Body,
+					GRPCService:            check.GRPCService,
+					GRPCUseTLS:             check.GRPCUseTLS,
+					TaskName:               check.TaskName,
+					SuccessBeforePassing:   check.SuccessBeforePassing,
+					FailuresBeforeCritical: check.FailuresBeforeCritical,
+					OnUpdate:               onUpdate,
 				}
 				if check.CheckRestart != nil {
 					out[i].Checks[j].CheckRestart = &structs.CheckRestart{

diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go
@@ -2029,7 +2029,9 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 									Limit:          3,
 									IgnoreWarnings: true,
 								},
-								TaskName: "task1",
+								TaskName:               "task1",
+								SuccessBeforePassing:   2,
+								FailuresBeforeCritical: 3,
 							},
 						},
 						Connect: &api.ConsulConnect{
@@ -2408,8 +2410,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 									Limit:          3,
 									IgnoreWarnings: true,
 								},
-								TaskName: "task1",
-								OnUpdate: "require_healthy",
+								TaskName:               "task1",
+								OnUpdate:               "require_healthy",
+								SuccessBeforePassing:   2,
+								FailuresBeforeCritical: 3,
 							},
 						},
 						Connect: &structs.ConsulConnect{