From a4449c84d7497493f7d8beb2796c92a6caae769b Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 18 Dec 2017 16:18:42 -0800 Subject: [PATCH 1/3] Services should not require a port Fixes #3673 --- command/agent/consul/client.go | 17 +- command/agent/consul/int_test.go | 24 ++- command/agent/consul/unit_test.go | 5 + nomad/mock/mock.go | 2 +- nomad/structs/structs.go | 14 +- nomad/structs/structs_test.go | 146 ++++++++++++++---- .../docs/job-specification/service.html.md | 9 +- 7 files changed, 164 insertions(+), 53 deletions(-) diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go index c22cb8d74..b8db963ae 100644 --- a/command/agent/consul/client.go +++ b/command/agent/consul/client.go @@ -663,7 +663,7 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se ip, port, err := getAddress(addrMode, portLabel, task.Resources.Networks, net) if err != nil { - return nil, fmt.Errorf("unable to get address for check %q: %v", check.Name, err) + return nil, fmt.Errorf("error getting address for check %q: %v", check.Name, err) } checkReg, err := createCheckReg(serviceID, checkID, check, ip, port) @@ -1036,6 +1036,11 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host chkReg.Timeout = check.Timeout.String() chkReg.Interval = check.Interval.String() + // Require an address for http or tcp checks + if port == 0 && check.RequiresPort() { + return nil, fmt.Errorf("%s checks require an address", check.Type) + } + switch check.Type { case structs.ServiceCheckHTTP: proto := check.Protocol @@ -1089,9 +1094,15 @@ func isOldNomadService(id string) bool { return strings.HasPrefix(id, prefix) } -// getAddress returns the ip and port to use for a service or check. An error -// is returned if an ip and port cannot be determined. +// getAddress returns the IP and port to use for a service or check. If no port +// label is specified (an empty value), zero values are returned because no +// address could be resolved. func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet *cstructs.DriverNetwork) (string, int, error) { + // No port label specified, no address can be assembled + if portLabel == "" { + return "", 0, nil + } + switch addrMode { case structs.AddressModeAuto: if driverNet.Advertise() { diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go index ba9975532..ee0fff748 100644 --- a/command/agent/consul/int_test.go +++ b/command/agent/consul/int_test.go @@ -87,8 +87,17 @@ func TestConsul_Integration(t *testing.T) { task.Config = map[string]interface{}{ "run_for": "1h", } + // Choose a port that shouldn't be in use - task.Resources.Networks[0].ReservedPorts = []structs.Port{{Label: "http", Value: 3}} + netResource := &structs.NetworkResource{ + Device: "eth0", + IP: "127.0.0.1", + MBits: 50, + ReservedPorts: []structs.Port{{Label: "http", Value: 3}}, + } + alloc.Resources.Networks[0] = netResource + alloc.TaskResources["web"].Networks[0] = netResource + task.Resources.Networks[0] = netResource task.Services = []*structs.Service{ { Name: "httpd", @@ -96,13 +105,12 @@ func TestConsul_Integration(t *testing.T) { Tags: []string{"nomad", "test", "http"}, Checks: []*structs.ServiceCheck{ { - Name: "httpd-http-check", - Type: "http", - Path: "/", - Protocol: "http", - PortLabel: "http", - Interval: 9000 * time.Hour, - Timeout: 1, // fail as fast as possible + Name: "httpd-http-check", + Type: "http", + Path: "/", + Protocol: "http", + Interval: 9000 * time.Hour, + Timeout: 1, // fail as fast as possible }, { Name: "httpd-script-check", diff --git a/command/agent/consul/unit_test.go b/command/agent/consul/unit_test.go index 88acc4ee1..4d8123b88 100644 --- a/command/agent/consul/unit_test.go +++ b/command/agent/consul/unit_test.go @@ -1566,8 +1566,13 @@ func TestGetAddress(t *testing.T) { { Name: "InvalidMode", Mode: "invalid-mode", + PortLabel: "80", ErrContains: "invalid address mode", }, + { + Name: "EmptyIsOk", + Mode: structs.AddressModeHost, + }, } for _, tc := range cases { diff --git a/nomad/mock/mock.go b/nomad/mock/mock.go index d0c889b09..c4921a644 100644 --- a/nomad/mock/mock.go +++ b/nomad/mock/mock.go @@ -292,7 +292,7 @@ func Alloc() *structs.Allocation { IP: "192.168.0.100", ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}}, MBits: 50, - DynamicPorts: []structs.Port{{Label: "http"}}, + DynamicPorts: []structs.Port{{Label: "http", Value: 9876}}, }, }, }, diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 439e6a858..fa994e194 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -3131,8 +3131,8 @@ func (s *Service) Validate() error { } for _, c := range s.Checks { - if s.PortLabel == "" && c.RequiresPort() { - mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but the service %+q has no port", c.Name, s.Name)) + if s.PortLabel == "" && c.PortLabel == "" && c.RequiresPort() { + mErr.Errors = append(mErr.Errors, fmt.Errorf("check %s invalid: check requires a port but neither check nor service %+q have a port", c.Name, s.Name)) continue } @@ -3577,8 +3577,16 @@ func validateServices(t *Task) error { } } + // Iterate over a sorted list of keys to make error listings stable + keys := make([]string, 0, len(servicePorts)) + for p := range servicePorts { + keys = append(keys, p) + } + sort.Strings(keys) + // Ensure all ports referenced in services exist. - for servicePort, services := range servicePorts { + for _, servicePort := range keys { + services := servicePorts[servicePort] _, ok := portLabels[servicePort] if !ok { names := make([]string, 0, len(services)) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 26b2d2a02..4897e0422 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -1235,55 +1235,93 @@ func TestTask_Validate_Service_Check(t *testing.T) { // TestTask_Validate_Service_Check_AddressMode asserts that checks do not // inherit address mode but do inherit ports. func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { - task := &Task{ - Resources: &Resources{ - Networks: []*NetworkResource{ - { - DynamicPorts: []Port{ - { - Label: "http", - Value: 9999, + getTask := func(s *Service) *Task { + return &Task{ + Resources: &Resources{ + Networks: []*NetworkResource{ + { + DynamicPorts: []Port{ + { + Label: "http", + Value: 9999, + }, }, }, }, }, - }, - Services: []*Service{ - { + Services: []*Service{s}, + } + } + + cases := []struct { + Service *Service + ErrContains string + }{ + { + Service: &Service{ Name: "invalid-driver", PortLabel: "80", AddressMode: "host", }, - { - Name: "http-driver", + ErrContains: `port label "80" referenced`, + }, + { + Service: &Service{ + Name: "http-driver-fail-1", PortLabel: "80", AddressMode: "driver", Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-1", Type: "tcp", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `check "invalid-check-1" cannot use a numeric port`, + }, + { + Service: &Service{ + Name: "http-driver-fail-2", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-2", Type: "tcp", PortLabel: "80", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `check "invalid-check-2" cannot use a numeric port`, + }, + { + Service: &Service{ + Name: "http-driver-fail-3", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should fail Name: "invalid-check-3", Type: "tcp", PortLabel: "missing-port-label", Interval: time.Second, Timeout: time.Second, }, + }, + }, + ErrContains: `port label "missing-port-label" referenced`, + }, + { + Service: &Service{ + Name: "http-driver-passes", + PortLabel: "80", + AddressMode: "driver", + Checks: []*ServiceCheck{ { - // Should pass Name: "valid-script-check", Type: "script", Command: "ok", @@ -1291,7 +1329,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { Timeout: time.Second, }, { - // Should pass Name: "valid-host-check", Type: "tcp", PortLabel: "http", @@ -1299,7 +1336,6 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { Timeout: time.Second, }, { - // Should pass Name: "valid-driver-check", Type: "tcp", AddressMode: "driver", @@ -1309,23 +1345,65 @@ func TestTask_Validate_Service_Check_AddressMode(t *testing.T) { }, }, }, - } - err := validateServices(task) - if err == nil { - t.Fatalf("expected errors but task validated successfully") - } - errs := err.(*multierror.Error).Errors - if expected := 4; len(errs) != expected { - for i, err := range errs { - t.Logf("errs[%d] -> %s", i, err) - } - t.Fatalf("expected %d errors but found %d", expected, len(errs)) + { + Service: &Service{ + Name: "empty-address-3673-passes-1", + Checks: []*ServiceCheck{ + { + Name: "valid-port-label", + Type: "tcp", + PortLabel: "http", + Interval: time.Second, + Timeout: time.Second, + }, + { + Name: "empty-is-ok", + Type: "script", + Command: "ok", + Interval: time.Second, + Timeout: time.Second, + }, + }, + }, + }, + { + Service: &Service{ + Name: "empty-address-3673-passes-2", + }, + }, + { + Service: &Service{ + Name: "empty-address-3673-fails", + Checks: []*ServiceCheck{ + { + Name: "empty-is-not-ok", + Type: "tcp", + Interval: time.Second, + Timeout: time.Second, + }, + }, + }, + ErrContains: `invalid: check requires a port but neither check nor service`, + }, } - assert.Contains(t, errs[0].Error(), `check "invalid-check-1" cannot use a numeric port`) - assert.Contains(t, errs[1].Error(), `check "invalid-check-2" cannot use a numeric port`) - assert.Contains(t, errs[2].Error(), `port label "80" referenced`) - assert.Contains(t, errs[3].Error(), `port label "missing-port-label" referenced`) + for _, tc := range cases { + tc := tc + task := getTask(tc.Service) + t.Run(tc.Service.Name, func(t *testing.T) { + err := validateServices(task) + if err == nil && tc.ErrContains == "" { + // Ok! + return + } + if err == nil { + t.Fatalf("no error returned. expected: %s", tc.ErrContains) + } + if !strings.Contains(err.Error(), tc.ErrContains) { + t.Fatalf("expected %q but found: %v", tc.ErrContains, err) + } + }) + } } func TestTask_Validate_Service_Check_CheckRestart(t *testing.T) { diff --git a/website/source/docs/job-specification/service.html.md b/website/source/docs/job-specification/service.html.md index 9707d8ec4..578f0c1ed 100644 --- a/website/source/docs/job-specification/service.html.md +++ b/website/source/docs/job-specification/service.html.md @@ -96,7 +96,7 @@ does not automatically enable service discovery. interpolated and revalidated. This can cause certain service names to pass validation at submit time but fail at runtime. -- `port` `(string: )` - Specifies the label of the port on which this +- `port` `(string: )` - Specifies the label of the port on which this service is running. Note this is the _label_ of the port and not the port number unless `address_mode = driver`. The port label must match one defined in the [`network`][network] stanza unless you're also using @@ -174,14 +174,15 @@ scripts. add the IP of the service and the port, so this is just the relative URL to the health check endpoint. This is required for http-based health checks. -- `port` `(string: )` - Specifies the label of the port on which the +- `port` `(string: )` - Specifies the label of the port on which the check will be performed. Note this is the _label_ of the port and not the port number unless `address_mode = driver`. The port label must match one defined in the [`network`][network] stanza. If a port value was declared on the `service`, this will inherit from that value if not supplied. If supplied, this value takes precedence over the `service.port` value. This is useful for - services which operate on multiple ports. Checks will use the host IP and - ports by default. In Nomad 0.7.1 or later numeric ports may be used if + services which operate on multiple ports. `http` and `tcp` checks require a + port while `script` checks do not. Checks will use the host IP and ports by + default. In Nomad 0.7.1 or later numeric ports may be used if `address_mode="driver"` is set on the check. - `protocol` `(string: "http")` - Specifies the protocol for the http-based From e12f0d1d2cb96a5110364df1b4c3b36a8c7bb966 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 19 Dec 2017 16:19:40 -0800 Subject: [PATCH 2/3] Fix missing fields in json jobspec docs --- website/source/api/json-jobs.html.md | 42 ++++++++++++++++++- .../docs/job-specification/task.html.md | 3 +- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/website/source/api/json-jobs.html.md b/website/source/api/json-jobs.html.md index 0090a7feb..1fe359af9 100644 --- a/website/source/api/json-jobs.html.md +++ b/website/source/api/json-jobs.html.md @@ -300,13 +300,17 @@ The `Task` object supports the following keys: } ``` +- `KillSignal` - Specifies a configurable kill signal for a task, where the + default is SIGINT. Note that this is only supported for drivers which accept + sending signals (currently `docker`, `exec`, `raw_exec`, and `java` drivers). + - `KillTimeout` - `KillTimeout` is a time duration in nanoseconds. It can be used to configure the time between signaling a task it will be killed and actually killing it. Drivers first sends a task the `SIGINT` signal and then sends `SIGTERM` if the task doesn't die after the `KillTimeout` duration has elapsed. The default `KillTimeout` is 5 seconds. -- `leader` - Specifies whether the task is the leader task of the task group. If +- `Leader` - Specifies whether the task is the leader task of the task group. If set to true, when the leader task completes, all other tasks within the task group will be gracefully shutdown. @@ -346,6 +350,23 @@ The `Task` object supports the following keys: defined in the resources block. This could be a label of either a dynamic or a static port. + - `AddressMode`: Specifies what address (host or driver-specific) this + service should advertise. This setting is supported in Docker since + Nomad 0.6 and rkt since Nomad 0.7. Valid options are: + + - `auto` - Allows the driver to determine whether the host or driver + address should be used. Defaults to `host` and only implemented by + Docker. If you use a Docker network plugin such as weave, Docker will + automatically use its address. + + - `driver` - Use the IP specified by the driver, and the port specified + in a port map. A numeric port may be specified since port maps aren't + required by all network plugins. Useful for advertising SDN and + overlay network addresses. Task will fail if driver network cannot be + determined. Only implemented for Docker and rkt. + + - `host` - Use the host IP and port. + - `Checks`: `Checks` is an array of check objects. A check object defines a health check associated with the service. Nomad supports the `script`, `http` and `tcp` Consul Checks. Script checks are not supported for the @@ -357,6 +378,25 @@ The `Task` object supports the following keys: - `Name`: The name of the health check. + - `AddressMode`: Same as `AddressMode` on `Service`. Unlike services, + checks do not have an `auto` address mode as there's no way for + Nomad to know which is the best address to use for checks. Consul + needs access to the address for any HTTP or TCP checks. Added in + Nomad 0.7.1. Unlike `PortLabel`, this setting is *not* inherited + from the `Service`. + + - `PortLabel`: Specifies the label of the port on which the check will + be performed. Note this is the _label_ of the port and not the port + number unless `AddressMode: "driver"`. The port label must match one + defined in the Network stanza. If a port value was declared on the + `Service`, this will inherit from that value if not supplied. If + supplied, this value takes precedence over the `Service.PortLabel` + value. This is useful for services which operate on multiple ports. + `http` and `tcp` checks require a port while `script` checks do not. + Checks will use the host IP and ports by default. In Nomad 0.7.1 or + later numeric ports may be used if `AddressMode: "driver"` is set on + the check. + - `Header`: Headers for HTTP checks. Should be an object where the values are an array of values. Headers will be written once for each value. diff --git a/website/source/docs/job-specification/task.html.md b/website/source/docs/job-specification/task.html.md index f23406297..f5bb18897 100644 --- a/website/source/docs/job-specification/task.html.md +++ b/website/source/docs/job-specification/task.html.md @@ -56,8 +56,7 @@ job "docs" { - `kill_signal` `(string)` - Specifies a configurable kill signal for a task, where the default is SIGINT. Note that this is only supported for drivers - which accept sending signals (currently Docker, exec, raw_exec, and Java - drivers). + sending signals (currently `docker`, `exec`, `raw_exec`, and `java` drivers). - `leader` `(bool: false)` - Specifies whether the task is the leader task of the task group. If set to true, when the leader task completes, all other From c5fd8b2b202cdad6dbb943a1c19a5d16eaa8fd80 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 19 Dec 2017 16:41:35 -0800 Subject: [PATCH 3/3] Strip mocked dynamic port for fsm test --- nomad/fsm_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/nomad/fsm_test.go b/nomad/fsm_test.go index 8414cb75c..fdaf681b7 100644 --- a/nomad/fsm_test.go +++ b/nomad/fsm_test.go @@ -944,9 +944,14 @@ func TestFSM_UpsertAllocs_StrippedResources(t *testing.T) { fsm := testFSM(t) alloc := mock.Alloc() + + // Need to remove mock dynamic port from alloc as it won't be computed + // in this test + alloc.TaskResources["web"].Networks[0].DynamicPorts[0].Value = 0 + fsm.State().UpsertJobSummary(1, mock.JobSummary(alloc.JobID)) job := alloc.Job - resources := alloc.Resources + origResources := alloc.Resources alloc.Resources = nil req := structs.AllocUpdateRequest{ Job: job, @@ -973,10 +978,10 @@ func TestFSM_UpsertAllocs_StrippedResources(t *testing.T) { alloc.AllocModifyIndex = out.AllocModifyIndex // Resources should be recomputed - resources.DiskMB = alloc.Job.TaskGroups[0].EphemeralDisk.SizeMB - alloc.Resources = resources + origResources.DiskMB = alloc.Job.TaskGroups[0].EphemeralDisk.SizeMB + alloc.Resources = origResources if !reflect.DeepEqual(alloc, out) { - t.Fatalf("bad: %#v %#v", alloc, out) + t.Fatalf("not equal: % #v", pretty.Diff(alloc, out)) } }