From a54b4e08f8d3cb24630a7722f1402697eb1651e0 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:14:50 -0700 Subject: [PATCH 1/7] Drive-by comment correction --- command/agent/consul/syncer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/consul/syncer.go b/command/agent/consul/syncer.go index 76bcfcd8d..644715512 100644 --- a/command/agent/consul/syncer.go +++ b/command/agent/consul/syncer.go @@ -158,7 +158,7 @@ func NewSyncer(consulConfig *config.ConsulConfig, shutdownCh chan struct{}, logg cfg := consul.DefaultConfig() - // If a nil config was provided, fall back to the default config + // If a nil consulConfig was provided, fall back to the default config if consulConfig == nil { consulConfig = cconfig.DefaultConfig().ConsulConfig } From 08c88102a7a626d2e791f5806d1ccd31b4a6acf2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:15:07 -0700 Subject: [PATCH 2/7] There is no "docker" check type --- nomad/structs/structs.go | 1 - 1 file changed, 1 deletion(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 68c4c5d2f..fcae08ad0 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1484,7 +1484,6 @@ func (tg *TaskGroup) GoString() string { const ( ServiceCheckHTTP = "http" ServiceCheckTCP = "tcp" - ServiceCheckDocker = "docker" ServiceCheckScript = "script" ) From af8db7ec185fa37bf402005b5c44f9d00d2df42a Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:17:43 -0700 Subject: [PATCH 3/7] Don't export ServiceCheck validate --- nomad/structs/structs.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index fcae08ad0..1ae3abbce 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1509,7 +1509,8 @@ func (sc *ServiceCheck) Copy() *ServiceCheck { return nsc } -func (sc *ServiceCheck) Validate() error { +// validate a Service's ServiceCheck +func (sc *ServiceCheck) validate() error { t := strings.ToLower(sc.Type) if t != ServiceCheckTCP && t != ServiceCheckHTTP && t != ServiceCheckScript { return fmt.Errorf("service check must be either http, tcp or script type") @@ -1622,7 +1623,7 @@ func (s *Service) Validate() error { mErr.Errors = append(mErr.Errors, fmt.Errorf("check %q is not valid since service %q doesn't have port", c.Name, s.Name)) continue } - if err := c.Validate(); err != nil { + if err := c.validate(); err != nil { mErr.Errors = append(mErr.Errors, err) } } From 79c675cf727ef91be19fc60695a00d0765ffbf72 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:19:40 -0700 Subject: [PATCH 4/7] Guard against an interval and timeout being less than 1s --- nomad/structs/structs.go | 46 +++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 1ae3abbce..4907a579a 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1485,6 +1485,15 @@ const ( ServiceCheckHTTP = "http" ServiceCheckTCP = "tcp" ServiceCheckScript = "script" + + // minCheckInterval is the minimum check interval permitted. Consul + // currently has its MinInterval set to 1s. Mirror that here for + // consistency. + minCheckInterval = 1 * time.Second + + // minCheckTimeout is the minimum check timeout permitted for Consul + // script TTL checks. + minCheckTimeout = 1 * time.Second ) // The ServiceCheck data model represents the consul health check that @@ -1511,20 +1520,26 @@ func (sc *ServiceCheck) Copy() *ServiceCheck { // validate a Service's ServiceCheck func (sc *ServiceCheck) validate() error { - t := strings.ToLower(sc.Type) - if t != ServiceCheckTCP && t != ServiceCheckHTTP && t != ServiceCheckScript { - return fmt.Errorf("service check must be either http, tcp or script type") - } - if sc.Type == ServiceCheckHTTP && sc.Path == "" { - return fmt.Errorf("service checks of http type must have a valid http path") + switch strings.ToLower(sc.Type) { + case ServiceCheckTCP: + case ServiceCheckHTTP: + if sc.Path == "" { + return fmt.Errorf("http type must have a valid http path") + } + case ServiceCheckScript: + if sc.Command == "" { + return fmt.Errorf("script type must have a valid script path") + } + + if sc.Timeout <= minCheckTimeout { + return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) + } + default: + return fmt.Errorf(`invalid type (%+q), must be one of "http", "tcp", or "script" type`, sc.Type) } - if sc.Type == ServiceCheckScript && sc.Command == "" { - return fmt.Errorf("service checks of script type must have a valid script path") - } - - if sc.Interval <= 0 { - return fmt.Errorf("service checks must have positive time intervals") + if sc.Interval <= minCheckInterval { + return fmt.Errorf("interval (%v) can not be lower than %v", sc.Interval, minCheckInterval) } return nil } @@ -1620,11 +1635,12 @@ func (s *Service) Validate() error { for _, c := range s.Checks { if s.PortLabel == "" && c.RequiresPort() { - mErr.Errors = append(mErr.Errors, fmt.Errorf("check %q is not valid since service %q doesn't have port", c.Name, s.Name)) + mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but the service %+q has no port", c.Name)) continue } + if err := c.validate(); err != nil { - mErr.Errors = append(mErr.Errors, err) + mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: %v", c.Name, err)) } } return mErr.ErrorOrNil() @@ -1858,7 +1874,7 @@ func validateServices(t *Task) error { knownServices := make(map[string]struct{}) for i, service := range t.Services { if err := service.Validate(); err != nil { - outer := fmt.Errorf("service %d validation failed: %s", i, err) + outer := fmt.Errorf("service[%d] %+q validation failed: %s", i, service.Name, err) mErr.Errors = append(mErr.Errors, outer) } if _, ok := knownServices[service.Name]; ok { From baac19cad6961d2eb47d42d3dbd072959cc9abc2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:22:53 -0700 Subject: [PATCH 5/7] Remove diff check for ServiceID, may it R.I.P. --- nomad/structs/diff_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 135256341..5555a2c33 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -2486,12 +2486,6 @@ func TestTaskDiff(t *testing.T) { Old: "foo", New: "bar", }, - { - Type: DiffTypeNone, - Name: "ServiceID", - Old: "", - New: "", - }, }, }, }, @@ -2757,12 +2751,6 @@ func TestTaskDiff(t *testing.T) { Old: "", New: "", }, - { - Type: DiffTypeNone, - Name: "ServiceID", - Old: "", - New: "", - }, }, Objects: []*ObjectDiff{ { From c3a3fdc2305174b17da1f640b8e8bad3c2c0da44 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:28:27 -0700 Subject: [PATCH 6/7] Upon further review, the Timeout needs to be validate for more than script checks. This value is used for Consul HTTP and TCP checks. --- nomad/structs/structs.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4907a579a..b7e25a3da 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1530,10 +1530,6 @@ func (sc *ServiceCheck) validate() error { if sc.Command == "" { return fmt.Errorf("script type must have a valid script path") } - - if sc.Timeout <= minCheckTimeout { - return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) - } default: return fmt.Errorf(`invalid type (%+q), must be one of "http", "tcp", or "script" type`, sc.Type) } @@ -1541,6 +1537,11 @@ func (sc *ServiceCheck) validate() error { if sc.Interval <= minCheckInterval { return fmt.Errorf("interval (%v) can not be lower than %v", sc.Interval, minCheckInterval) } + + if sc.Timeout <= minCheckTimeout { + return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) + } + return nil } From 4f14d51013dfe7bd62bf2bee181283b4dac4e0f2 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 13 Jun 2016 18:55:15 -0700 Subject: [PATCH 7/7] Fix up validation and allow existing unset timeouts to continue to be unset --- nomad/structs/structs.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index b7e25a3da..cb97d2908 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1482,6 +1482,7 @@ func (tg *TaskGroup) GoString() string { } const ( + // TODO add Consul TTL check ServiceCheckHTTP = "http" ServiceCheckTCP = "tcp" ServiceCheckScript = "script" @@ -1522,26 +1523,32 @@ func (sc *ServiceCheck) Copy() *ServiceCheck { func (sc *ServiceCheck) validate() error { switch strings.ToLower(sc.Type) { case ServiceCheckTCP: + if sc.Timeout > 0 && sc.Timeout <= minCheckTimeout { + return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) + } case ServiceCheckHTTP: if sc.Path == "" { return fmt.Errorf("http type must have a valid http path") } + + if sc.Timeout > 0 && sc.Timeout <= minCheckTimeout { + return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) + } case ServiceCheckScript: if sc.Command == "" { return fmt.Errorf("script type must have a valid script path") } + + // TODO: enforce timeout on the Client side and reenable + // validation. default: return fmt.Errorf(`invalid type (%+q), must be one of "http", "tcp", or "script" type`, sc.Type) } - if sc.Interval <= minCheckInterval { + if sc.Interval > 0 && sc.Interval <= minCheckInterval { return fmt.Errorf("interval (%v) can not be lower than %v", sc.Interval, minCheckInterval) } - if sc.Timeout <= minCheckTimeout { - return fmt.Errorf("timeout %v is lower than required minimum timeout %v", sc.Timeout, minCheckInterval) - } - return nil }