Refactor Consul Syncer into new ServiceClient #2467

schmichael · 2017-03-22T00:03:53Z

Fixes #2478 #2474 #1995 #2294

The new client only handles agent and task service advertisement. Server
discovery is mostly unchanged.

The Nomad client agent now handles all Consul operations instead of the
executor handling task related operations. When upgrading from an
earlier version of Nomad existing executors will be told to deregister
from Consul so that the Nomad agent can re-register the task's services
and checks.

Drivers - other than qemu - now support an Exec method for executing
abritrary commands in a task's environment. This is used to implement
script checks.

Interfaces are used extensively to avoid interacting with Consul in
tests that don't assert any Consul related behavior.

schmichael · 2017-03-30T21:24:21Z

command/agent/consul/client.go

+		client:         consulClient,
+		logger:         logger,
+		retryInterval:  defaultSyncInterval, //TODO what should this default to?!
+		syncInterval:   defaultSyncInterval,


schmichael · 2017-03-30T21:24:51Z

command/agent/consul/client.go

+					lastOk = false
+					c.logger.Printf("[WARN] consul: failed to update services in Consul: %v", err)
+				}
+				//TODO Log? and jitter/backoff


exp backoff from 1...30s

schmichael · 2017-03-30T21:29:44Z

command/agent/consul/client.go

+	c.logger.Printf("[DEBUG] consul: registered %d services / %d checks; deregisterd %d services / %d checks", regServiceN, regCheckN, deregServiceN, deregCheckN)
+	return nil
+
+	//TODO Labels and gotos are nasty; move to a function?


add helper functions to get a snapshot of the maps and another function to merge outstanding functions back in

schmichael · 2017-03-30T22:54:48Z

command/agent/consul/script.go

+			}
+
+			if err != nil {
+				//FIXME Backoff? Retry faster?


remove this

schmichael · 2017-03-30T22:58:50Z

client/task_runner.go

+
+		//FIXME is there a better place to do this? used to be in executor
+		// Prepare services
+		interpolateServices(r.getTaskEnv(), r.task)


dadgar · 2017-04-03T22:13:31Z

command/agent/consul/client.go

+// ScriptExecutor is the interface the ServiceClient uses to execute script
+// checks inside a container.
+type ScriptExecutor interface {
+	Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error)


This should be defined on the DriverHandle interface

Or a new interface that is defined there that wraps the DriverHandle. I don't think that is the best approach because for plugin versions of drivers there will not be a good way to do that.

dadgar · 2017-04-03T22:19:03Z

client/driver/java.go

@@ -390,6 +388,10 @@ func (h *javaHandle) Update(task *structs.Task) error {
 	return nil
 }

+func (h *javaHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error) {
+	return execChroot(ctx, h.taskDir, cmd, args)


Java driver won't always be chroot'ed. Only on linux

Handled in #2554

dadgar · 2017-04-03T22:19:32Z

client/driver/raw_exec.go

@@ -250,6 +245,10 @@ func (h *rawExecHandle) Update(task *structs.Task) error {
 	return nil
 }

+func (h *rawExecHandle) Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error) {
+	return execChroot(ctx, "", cmd, args)


Not really an appropriate name...

Handled in #2554

dadgar · 2017-04-03T22:24:04Z

client/driver/utils.go

+// execChroot executes cmd with args inside chroot if set and returns the
+// output, exit code, and error. If chroot is an empty string the command is
+// executed on the host.
+func execChroot(ctx context.Context, chroot, name string, args []string) ([]byte, int, error) {


I don't really like this. I feel like the executor should be used to execute commands and it should launch them into the same cgroup/chroot/namespaces etc that it already has context for.

Added #2554 to handle this in a followup PR.

dadgar · 2017-04-06T21:29:56Z

client/task_runner.go

@@ -1361,6 +1426,9 @@ func (r *TaskRunner) handleUpdate(update *structs.Allocation) error {
 // given limit. It returns whether the task was destroyed and the error
 // associated with the last kill attempt.
 func (r *TaskRunner) handleDestroy() (destroyed bool, err error) {
+	// Remove from Consul


The current behavior is to deregister services as soon as they exit and re-register once that have have started. This will leave the service registered even as the task may be restarting with an arbitrarily long delay

dadgar · 2017-04-06T21:41:42Z

nomad/server.go

+	defaultConsulDiscoveryInterval time.Duration = 9 * time.Second
+
+	// defaultConsulDiscoveryIntervalRetry is how often to poll Consul for
+	// new servers if there is no leader and the last Consul query failed.


I think this is the opposite of the behavior we want. This makes it so under error condition we speed up instead of backing off?

dadgar · 2017-04-06T21:44:53Z

command/agent/consul/client.go

+	"github.com/hashicorp/nomad/nomad/structs"
+)
+
+var mark = struct{}{}


dadgar · 2017-04-06T21:47:54Z

command/agent/consul/client.go

+// ScriptExecutor is the interface the ServiceClient uses to execute script
+// checks inside a container.
+type ScriptExecutor interface {
+	Exec(ctx context.Context, cmd string, args []string) ([]byte, int, error)


Or a new interface that is defined there that wraps the DriverHandle. I don't think that is the best approach because for plugin versions of drivers there will not be a good way to do that.

dadgar · 2017-04-06T21:49:07Z

command/agent/consul/client.go

+type ServiceClient struct {
+	client        AgentAPI
+	logger        *log.Logger
+	retryInterval time.Duration


dadgar · 2017-04-06T22:45:44Z

command/agent/consul/script.go

+type scriptHandle struct {
+	// cancel the script
+	cancel func()
+	done   chan struct{}


dadgar · 2017-04-06T22:47:38Z

command/agent/consul/script.go

+			case context.DeadlineExceeded:
+				// Log deadline exceeded every time, but flip last check to false
+				s.lastCheckOk = false
+				s.logger.Printf("[WARN] consul.checks: check %q timed out (%s)", s.check.Name, s.check.Timeout)


It would be nice to have the allocation ID and task name.

dadgar · 2017-04-06T22:51:15Z

command/agent/consul/script.go

+			}
+			if err != nil {
+				state = api.HealthCritical
+				output = []byte(err.Error())


string -> []byte -> string. Instead:

var outputMsg string if err != nil { outputMsg = err.Error() } else { outputMsg = string(output) }

dadgar · 2017-04-06T22:57:21Z

command/agent/consul/script_test.go

+
+func (b *blockingScriptExec) Exec(ctx context.Context, _ string, _ []string) ([]byte, int, error) {
+	b.running <- mark
+	cmd := exec.CommandContext(ctx, "/bin/sleep", "9000")


Won't work on Windows. Check out https://github.com/hashicorp/nomad/blob/master/client/driver/driver_test.go#L38
https://github.com/hashicorp/nomad/blob/master/helper/testtask/testtask.go

dadgar · 2017-04-06T23:00:36Z

command/agent/consul/script_test.go

+	"github.com/hashicorp/consul/api"
+	"github.com/hashicorp/nomad/nomad/structs"
+)
+


Where are the normal case tests:

Getting correct exit code

Getting output

Getting exec errors

dadgar · 2017-04-17T17:16:21Z

client/task_runner.go

 	var mErr multierror.Error
 	r.handleLock.Lock()
 	if r.handle != nil {
+		// Need to check driver abilities for updating services


Stale comment?

dadgar · 2017-04-17T17:20:43Z

client/task_runner.go

@@ -1061,8 +1072,12 @@ func (r *TaskRunner) run() {
 	}
 }

-// cleanup calls Driver.Cleanup when a task is stopping. Errors are logged.
+// cleanup removes Consul entries and calls Driver.Cleanup when a task is


This doesn't get called through restarts which means the check is registered during the restart sleep. (the wait channel firing in the run loop)

Can you add a test for this

Great catch! Fixed and tested: 9625e19

Ensured test failed without the RemoveTask call when restarting.

dadgar · 2017-04-17T17:22:26Z

command/agent/agent.go

 		}
 		if !conf.TLSConfig.EnableHTTP {
-			consulServices[consul.GenerateServiceKey(httpServ)] = httpServ
+			consulServices = append(consulServices, httpServ)


Bump! Just want to make sure this doesn't slip through

dadgar · 2017-04-17T20:53:06Z

command/agent/consul/client.go

+				c.logger.Printf("[WARN] consul.sync: failed to update services in Consul: %v", err)
+			}
+			failures++
+			if !retryTimer.Stop() {


Can you comment here what you are doing with the timer. I know it is the recommended practice but still not super common

Comment added. I hate the timer API.

dadgar · 2017-04-17T20:58:26Z

command/agent/consul/client.go

+}
+
+// commit operations and returns false if shutdown signalled before committing.
+func (c *ServiceClient) commit(ops *operations) bool {


Not really using the return value anywhere in that meaningful of a way. Remove?

dadgar · 2017-04-17T21:21:12Z

command/agent/consul/client.go

+		}
+
+		// Service exists and wasn't updated, don't add it later
+		delete(newIDs, existingID)


What if the port changes?

Totally missed the fact that port changes are excluded from service and check IDs, so great catch. Added a huge test to exercise port changes and did lots of fixing: 970979d

dadgar · 2017-04-17T21:27:14Z

command/agent/consul/mock.go

+import (
+	"log"
+
+	"github.com/hashicorp/consul/api"


we have been following the habit of _testing.go to put importable stubs/mocks/etc

dadgar · 2017-04-17T21:32:11Z

command/agent/consul/script.go

+				return
+			case context.DeadlineExceeded:
+				// Log deadline exceeded every time, but flip last check to false
+				s.lastCheckOk = false


Won't the code == 0 in this case?

dadgar · 2017-04-17T21:36:24Z

command/agent/consul/script.go

+			cancel()
+
+			state := api.HealthCritical
+			switch code {


https://www.consul.io/docs/agent/checks.html#check-scripts

Think you want a default case

state is initialized to the default case (critical/failing)

dadgar · 2017-04-17T21:40:02Z

command/agent/consul/script_test.go

+		os.Exit(m.Run())
+	}
+}
+


Can you test:

exit code 0

exit code 1

exit code 2

Capturing output

Unknown command

Added except "Unknown command" which depends on the Exec implementation to return an error (which is tested): 9c14e8b

Exec implementations are tested directly for example: https://github.com/hashicorp/nomad/pull/2467/files#diff-1eb2911dd4ed9b612194897284c68251R304

schmichael · 2017-04-19T04:32:47Z

Ready for another review. Note that #2218 isn't supported for non-agent checks. Will do that in a follow up PR.

dadgar · 2017-04-19T17:09:38Z

command/agent/agent.go

@@ -680,3 +703,59 @@ func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error {
 	go a.consulService.Run()
 	return nil
 }
+
+// consulSupportsTLSSkipVerify returns true if Consul supports TLSSkipVerify.
+func consulSupportsTLSSkipVerify(self map[string]map[string]interface{}) bool {


Use: https://github.com/hashicorp/go-version

dadgar · 2017-04-19T17:12:40Z

command/agent/consul/script_test.go

+		}
+	}
+
+	// Test exit codes with errors


Use go 1.8 table driven test style

dadgar · 2017-04-19T17:14:01Z

command/agent/consul/unit_test.go

@@ -242,6 +260,188 @@ func TestConsul_ChangeTags(t *testing.T) {
 	}
 }

+// TestConsul_ChangePorts asserts that changing the ports on a service updates
+// it in Consul. Since ports are part of the service ID this is a slightly


ports are not part of the service ID

dadgar · 2017-04-19T17:21:19Z

Also fix the diff tests: https://travis-ci.org/hashicorp/nomad/builds/223416717#L5545

dadgar · 2017-04-19T17:24:05Z

nomad/structs/structs.go

@@ -2074,6 +2074,7 @@ type ServiceCheck struct {
 	Interval      time.Duration // Interval of the check
 	Timeout       time.Duration // Timeout of the response from the check before consul fails the check
 	InitialStatus string        // Initial status of the check
+	TLSSkipVerify bool          // Skip TLS verification when Protocol=https


Add to API package, job spec, tests, diff.

https://github.com/hashicorp/nomad/blob/master/api/tasks.go#L81

https://github.com/hashicorp/nomad/blob/master/jobspec/parse.go#L941

https://github.com/hashicorp/nomad/blob/master/command/agent/job_endpoint.go#L496

Also document on both HCL and JSON job spec

I was going to implement it for non-agent checks in a followup PR (and doc it then), but that seems silly. Fixing.

Fixes #2478 #2474 #1995 #2294 The new client only handles agent and task service advertisement. Server discovery is mostly unchanged. The Nomad client agent now handles all Consul operations instead of the executor handling task related operations. When upgrading from an earlier version of Nomad existing executors will be told to deregister from Consul so that the Nomad agent can re-register the task's services and checks. Drivers - other than qemu - now support an Exec method for executing abritrary commands in a task's environment. This is used to implement script checks. Interfaces are used extensively to avoid interacting with Consul in tests that don't assert any Consul related behavior.

...and still protect against leaking agent entries in Consul on shutdown.

Previous implementation assumed all struct fields were included in service and check IDs. Service IDs never include port labels and check IDs *optionally* include port labels, so lots of things had to change. Added a really big test to exercise this.

Support for TLSSkipVerify in other checks coming soon!

Fixes #2218

dadgar · 2017-04-19T19:50:46Z

nomad/structs/diff_test.go

@@ -1900,20 +1900,22 @@ func TestTaskGroupDiff(t *testing.T) {

 func TestTaskDiff(t *testing.T) {
 	cases := []struct {
+		Name       string


Bless your soul

github-actions · 2023-04-02T02:10:45Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

schmichael force-pushed the f-consul-refactor branch 3 times, most recently from 307426f to 6bfab4b Compare March 29, 2017 23:38

schmichael closed this Mar 29, 2017

schmichael reopened this Mar 29, 2017

schmichael changed the title ~~WIP Consul Refactor~~ Refactor Consul Syncer into new ServiceClient Mar 29, 2017

schmichael force-pushed the f-consul-refactor branch 2 times, most recently from 4196022 to 99e7126 Compare March 30, 2017 18:35

schmichael commented Mar 30, 2017

View reviewed changes

command/agent/consul/script.go Outdated

}

if err != nil {

//FIXME Backoff? Retry faster?

Copy link

Member Author

schmichael Mar 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

schmichael commented Mar 30, 2017

View reviewed changes

This was referenced Mar 31, 2017

Nomad does not register service checks of type script #2180

Closed

Orphaned Health Check #1524

Closed

dadgar requested changes Apr 5, 2017

View reviewed changes

dadgar reviewed Apr 6, 2017

View reviewed changes

schmichael mentioned this pull request Apr 12, 2017

exec/java DriverHandle.Exec methods should be run by executor #2554

Closed

schmichael force-pushed the f-consul-refactor branch from d95713b to 1b453be Compare April 13, 2017 23:44

schmichael mentioned this pull request Apr 14, 2017

Support consul's tls_skip_verify field in service checks #2218

Closed

schmichael force-pushed the f-consul-refactor branch 2 times, most recently from f0f1691 to d295329 Compare April 14, 2017 20:53

dadgar reviewed Apr 17, 2017

View reviewed changes

schmichael force-pushed the f-consul-refactor branch 2 times, most recently from 2713281 to 9866a15 Compare April 19, 2017 04:31

dadgar reviewed Apr 19, 2017

View reviewed changes

schmichael added 16 commits April 19, 2017 12:42

Remove commits return value

7de3ada

...and still protect against leaking agent entries in Consul on shutdown.

Follow _testing.go convention for testing tools

6a56b0e

Test script check exit codes

465cc51

Always fail script checks when deadline exceeded

d3f3af8

Metricsify new Consul client

de3d783

Explain weird timer logic

f0bec63

Unregister from Consul when waiting for restart

8e0c97e

Only register HTTPS agent check when Consul>=0.7.2

3468383

Support for TLSSkipVerify in other checks coming soon!

Forgot an important word

5948daf

Use spiffy new Go 1.8 subtest feature

6d9e61b

Use go-version instead of manual version parsing

64057d4

Fix diff test + bonus upgrade to subtests

86bc2fb

Add TLSSkipVerify support to api and parser

7c67166

Skip checks with TLSSkipVerify if it's unsupported

4cf34ed

Fixes #2218

Document tls_skip_verify

a5dcf6b

dadgar reviewed Apr 19, 2017

View reviewed changes

schmichael force-pushed the f-consul-refactor branch from 152ed01 to a5dcf6b Compare April 19, 2017 19:51

schmichael added 3 commits April 19, 2017 13:05

Thanks go vet!

b9ea276

Fix Windows build.

fb3b30b

Fix consul test build on Windows

58430bf

schmichael merged commit 53eb407 into master Apr 19, 2017

schmichael deleted the f-consul-refactor branch April 19, 2017 23:55

This was referenced Apr 20, 2017

Using a task name that is a prefix of another task's name can cause consul service flapping #2474

Closed

Every Nomad executor queries all consul services and health checks every 5 seconds. #1995

Closed

Execute exec/java script checks in containers #2585

Merged

schmichael mentioned this pull request May 1, 2017

Service Registration and Health Checks flapping when task name starts with another task name #2294

Closed

schmichael mentioned this pull request Jun 9, 2017

Fix Nomad server health check #2701

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2023

Refactor Consul Syncer into new ServiceClient #2467

Refactor Consul Syncer into new ServiceClient #2467

Conversation

schmichael commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 2, 2023

schmichael commented Mar 22, 2017 •

edited

Loading