First big step towards solving issues 2217 and 2465 #3037

magiconair · 2017-05-12T14:44:07Z

not done yet

magiconair · 2017-05-20T20:56:33Z

TestAgent looks better now. Still one test failing. Need to fix the httpTest code.

magiconair · 2017-05-21T08:05:48Z

Tests are fixed and the agent tests now run fully in parallel. Going from 108 sec locally down to 15 ...

magiconair · 2017-05-21T18:46:15Z

TestAgent refactor is done and the tests pass. However, quite regularly one of the tests hangs because the leader election fails. This behavior can be reduced by reducing the parallelism of go test but it is something we should look at.

slackpad

Noted a few small things but overall looks good. I like the approach, lack of callbacks and associated de-nesting, and elimination of several test frameworks :-)

slackpad · 2017-05-22T13:46:11Z

command/agent/agent.go

+		a.wgServers.Add(1)
+		go func() {
+			defer a.wgServers.Done()
+			up.Done()


How does this ensure that Serve() actually got called? It seems like this can still bail out too early since it's before the actual Serve() call.

It does not guarantee it. Until the Go team fixes the bug I think this is the best we can do with the minimal amount of overhead. This guarantees that the go routine that executes Serve gets at least scheduled once before Start returns. My understanding is that it yields again on the next blocking call which should happen within Serve. I don't see how it would yield between up.Done() and Serve until I got it wrong.

Fair enough - I'm not sure how you'd do better than this given the current situation w/o polling an endpoint, which would be truly horrible to do.

slackpad · 2017-05-22T13:47:26Z

command/agent/agent.go

+		srv.Shutdown()
+	}
+	for _, srv := range a.httpServers {
+		a.logger.Println("[INFO] agent: Stopping HTTP endpoint", srv.Addr)


This should be plural "endpoints" too.

No, since it now logs one line per endpoint with the specific address of the endpoint.

slackpad · 2017-05-22T13:49:05Z

command/agent/agent.go

 	err := a.delegate.Shutdown()
+	a.logger.Print("[INFO] agent: delegate down")


This message probably won't make sense to users. We could probably remove this, or move it to debug level.

slackpad · 2017-05-22T13:53:07Z

command/agent/agent_endpoint_test.go

@@ -285,7 +277,10 @@ func TestAgent_Reload(t *testing.T) {
 	}()

 	retry.Run(t, func(r *retry.R) {
-		if got, want := len(cmd.httpServers), 1; got != want {
+		if cmd == nil || cmd.agent == nil {


I don't think cmd can be nil here. Would this fail the race detector on cmd.agent since it looks like the cmd.Run() in a separate goroutine sets it?

Good point. I check.

slackpad · 2017-05-22T14:11:02Z

command/agent/dns_test.go

 		in, _, err := c.Exchange(m, addr.String())
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}

 		// Only 1 is passing, so we should only get 1 answer
+		for _, a := range in.Answer {


This looks like leftover debug code.

I have a look

slackpad · 2017-05-22T14:23:11Z

command/agent/listen.go

+}
+
+func ListenUnix(addr string, perm FilePermissions) (net.Listener, error) {
+	// todo(fs): move this somewhere else


This did get moved to startHTTP().

will remove

slackpad · 2017-05-22T14:32:24Z

command/agent/testagent.go

+)
+
+func init() {
+	rand.Seed(time.Now().UnixNano()) // seed random number generator


We've got https://github.com/hashicorp/consul/tree/master/vendor/github.com/sean-/seed vendored and can use that, though this is probably cheaper and fine for unit tests.

Do we really need a lib for that?

It's shared across projects so there's some justification, though it could be paste-able pretty easily. There's some nuance in doing it correctly (doesn't matter for unit tests), so that probably tips in favor vs. pasting.

slackpad · 2017-05-22T14:33:49Z

command/agent/testagent.go

+// during creation or startup instead of returning errors. It manages a
+// temporary data directory which is removed after shutdown.
+//
+// todo(fs): do we need the temp data dir if we run in dev mode?


We don't, though when I tried to use dev mode here I got some failures (some of the tests use features that don't work in dev mode). We will need to be able to tell the test agent whether or not to use dev mode and default that to true most likely, and opt-out in a few places.

This seems like something for a different PR though.

I can check what the impact is but since we're not using all CPUs on the agent test yet I think there is more potential somewhere else. I got at most half of the cores used. With go test --parallel 24 test execution went from 15 to 11 sec so not such a huge gain.

Yeah, it's probably better coverage-wise to activate non-dev as well. Dev mode bypasses some key places that are worth kicking during our unit tests.

slackpad · 2017-05-22T14:36:22Z

command/agent/testagent.go

+}
+
+// BoolTrue and BoolFalse exist to create a *bool value.
+var BoolTrue = true


Can these be const?

No, since you can't create a reference to a const. Tried that, too :)

Sometimes I miss (a few small parts of) C++ (every once in a while).

slackpad · 2017-05-22T14:38:29Z

command/util_test.go


+func nextConfig() *agent.Config {


Can we kill this stuff?

I'm working on that right now. That and then the code in command/server_test.go

magiconair · 2017-05-24T09:45:20Z

rebased to master to pick up Azure RetryJoin and latest go-sockaddr

slackpad

Noted some small things but otherwise looks good to merge!

slackpad · 2017-05-30T19:18:19Z

command/agent/acl_test.go

 	if err == nil || !strings.Contains(err.Error(), rootDenied) {
 		t.Fatalf("err: %v", err)
 	}

 	// The ACL master token should also not call the server, but should give
-	// us a working agent token.
-	acl, err := agent.resolveToken("towel")
+	// us a working a token.


This should still be "agent".

search and replace. You've got eagle eyes.

slackpad · 2017-05-30T19:23:11Z

command/agent/agent.go

@@ -75,16 +78,20 @@ type clientServer interface {
 // mode, it runs a full Consul server. In client-only mode, it only forwards
 // requests to other Consul servers.
 type Agent struct {
+	// id is an optional log prefix.
+	id string


This might be confusing w/the existing node ID. Maybe we can call this logPrefix so it's more explicit.

I think it makes more sense to just provide a properly configured logger which is used everywhere. Since I've fixed the logger passing we can move this into the TestAgent

slackpad · 2017-05-30T19:30:30Z

command/agent/agent.go

+	}
+
+	// wait for servers to be up
+	// todo(fs): not sure whether this is the right approach.


This looks pretty reasonable - if the server doesn't start then we should hit the timeout case and abort the startup, which I think is the minimum we should do. Failing the agent later if a server error occurs seems good for robustness, but should be done separately in a future PR.

(that'll let the process supervisor try to clean it up)

slackpad · 2017-05-30T19:32:30Z

command/agent/agent.go

+				break
+			}
+			l, err = tls.Listen("tcp", p.Addr, tlscfg)
+		}


We should add a default here so we don't just panic if we mis-configure the addrs in the future.

slackpad · 2017-05-30T19:33:53Z

command/agent/agent.go

+	// Shutdown is called before the Serve go routine was scheduled then
+	// the Serve go routine never returns. This deadlocks the agent
+	// shutdown for some tests since it will wait forever.
+	if strings.Contains("*tls.listener", fmt.Sprintf("%T", l)) {


Why this pattern vs. a type assertion?

Type assertions don't work with non-exported types :/

I'll update the comment to make that clear since you're not the first one to ask.

slackpad · 2017-05-30T19:36:42Z

command/agent/agent.go

+		// srv.Close()
+
+		// graceful shutdown
+		ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)


Do you want to log anything if we timeout here? Right now I don't think we'd know if we were hitting this.

slackpad · 2017-05-30T20:43:58Z

command/agent/config.go

+// connections to consul.
+func (c *Config) IncomingTLSConfig() (*tls.Config, error) {
+	tc := &tlsutil.Config{
+		VerifyIncoming:           c.VerifyIncoming || c.VerifyIncomingHTTPS,


This method is only set up properly for HTTPS listeners (not RPC listeners), so we should rename it to be more specific and update the comment.

slackpad · 2017-05-30T20:52:07Z

command/agent/event_endpoint_test.go

+		time.Sleep(50 * time.Millisecond)
+		p := &UserEvent{Name: "second"}
+		if err := a.UserEvent("dc1", "root", p); err != nil {
+			t.Fatalf("err: %v", err)


This won't work inside a goroutine, right?

which part? the time.Sleep or the t.Fatal ?

The t.Fatal

slackpad · 2017-05-30T20:57:17Z

command/agent/http.go

-			}
-			list = ln
-		} else {
-			list = tcpKeepAliveListener{ln.(*net.TCPListener)}


It looks like this detail got lost in the refactor, was that intentional?

I thought the tcpKeepAliveListener would be used by default in the http package - which it is but only if you use the ListenAndServe methods - which we don't. I'll add it back in. Forgot to do that.

slackpad · 2017-05-30T21:18:46Z

command/exec_test.go

-	waitForLeader(t, a1.httpAddr)
+	t.Parallel()
+	cfg := agent.TestConfig()
+	cfg.DisableRemoteExec = agent.Bool(false)


This could be &BoolFalse.

* use agent logger for consul/serf/raft/dns/agent/... * support optional id for concurrent tests

* don't use retry to try restarting the agent this caused some issues when the startup would fail in a separate go routine * clear out the data directory on every retry since the ports are stored in the raft data files * set a unique id for every agent to allow for tracking of concurrent output

* capture all output to test.log only and print only failures * always print output on travis * test github.com/hashicorp/consul/consul separately until we've found the interactions with the other packages during testing.

We need to init the flagset that cli uses to generate the help outside of the Run method since Run isn't called anymore for printing help.

* refactor DNS server to be ready for multiple bind addresses * drop tcpKeepAliveListener since it is default for the HTTP servers * add startup timeout watcher for HTTP servers identical to DNS server

slackpad · 2017-05-31T00:43:56Z

command/exec_test.go

@@ -31,7 +31,7 @@ func TestExecCommand_implements(t *testing.T) {
 func TestExecCommandRun(t *testing.T) {
 	t.Parallel()
 	cfg := agent.TestConfig()
-	cfg.DisableRemoteExec = agent.Bool(false)
+	cfg.DisableRemoteExec = &agent.BoolFalse


There were a bunch more of these in this file (I didn't tag them all) :-)

I'll revert to the Bool() function since it is more flexible in situations where you need to derive the pointer from another value. The main issue is the use of *bool in the first place IMO.

slackpad · 2017-05-31T00:44:37Z

command/agent/agent.go

+		select {
+		case <-done:
+			// server down within timeout
+		case <-ctx.Done():


I think you can just check the error from ctx.Done() here, this is correct but you don't need the extra channel.

yes, and a good idea as well since we're timing out every time. The server doesn't shut down cleanly and I need to figure out why. I've refactored it to not use the second channel and will investigate the code that hangs the server during shutdown.

slackpad · 2017-05-31T00:45:08Z

command/agent/testagent.go

 		agent.LogWriter = a.LogWriter
+		agent.logger = log.New(logOutput, id, log.LstdFlags)


I don't see the log prefix anymore in test.log.

Hmm they are there now that I'm looking. Perhaps just some of the tests are missing the prefix.

Only tests that use the TestAgent set the prefix.

1yhud3gtn56jf 2017/05/31 09:42:26 [INFO] raft: Node at 127.0.0.1:50636 [Candidate] entering Candidate state in term 2 1yhud3gtn56jf 2017/05/31 09:42:26 [DEBUG] raft: Votes needed: 1 1yhud3gtn56jf 2017/05/31 09:42:26 [DEBUG] raft: Vote granted from 127.0.0.1:50636 in term 2. Tally: 1 1yhud3gtn56jf 2017/05/31 09:42:26 [INFO] raft: Election won. Tally: 1 1yhud3gtn56jf 2017/05/31 09:42:26 [INFO] raft: Node at 127.0.0.1:50636 [Leader] entering Leader state

I'll use the test name as an id as you suggested earlier. That makes more sense even if the logs are a bit misaligned. grep TestName test.log will then show you everything you need to know.

$ grep TestDNS_Recurse_Truncation test.log === RUN TestDNS_Recurse_Truncation TestDNS_Recurse_Truncation - 2017/05/31 09:51:15 [INFO] raft: Initial configuration (index=1): [{Suffrage:Voter ID:127.0.0.1:54916 Address:127.0.0.1:54916}] TestDNS_Recurse_Truncation - 2017/05/31 09:51:15 [INFO] raft: Node at 127.0.0.1:54916 [Follower] entering Follower state (Leader: "") TestDNS_Recurse_Truncation - 2017/05/31 09:51:15 [INFO] serf: EventMemberJoin: Node 77c888a2-7a88-a600-22a6-33064aa14734 127.0.0.1 TestDNS_Recurse_Truncation - 2017/05/31 09:51:15 [INFO] consul: Adding LAN server Node 77c888a2-7a88-a600-22a6-33064aa14734 (Addr: tcp/127.0.0.1:54916) (DC: dc1) TestDNS_Recurse_Truncation - 2017/05/31 09:51:15 [INFO] serf: EventMemberJoin: Node 77c888a2-7a88-a600-22a6-33064aa14734.dc1 127.0.0.1

magiconair · 2017-05-31T08:01:40Z

pushed changes to master

magiconair self-assigned this May 12, 2017

magiconair force-pushed the issue-2217-multiple-bind-addr branch 3 times, most recently from 8c1fca4 to bb7765a Compare May 18, 2017 14:39

slackpad mentioned this pull request May 18, 2017

Add support for multiple address binds to dns and http servers #2646

Closed

sean- mentioned this pull request May 18, 2017

Add plural helpers hashicorp/go-sockaddr#11

Merged

magiconair force-pushed the issue-2217-multiple-bind-addr branch from bb7765a to 2ca6e7f Compare May 20, 2017 20:51

magiconair force-pushed the issue-2217-multiple-bind-addr branch 2 times, most recently from c66eaa5 to 5fb0ce5 Compare May 21, 2017 08:02

magiconair force-pushed the issue-2217-multiple-bind-addr branch from 5fb0ce5 to c0e249d Compare May 21, 2017 15:22

magiconair force-pushed the issue-2217-multiple-bind-addr branch from 5bae6bc to 138d16f Compare May 21, 2017 19:00

magiconair changed the title ~~WIP: work towards solving issue 2217~~ WIP: work towards solving issues 2217 and 2465 May 21, 2017

magiconair requested review from slackpad and kyhavlov May 21, 2017 19:09

magiconair mentioned this pull request May 21, 2017

testutil: Should ensure consul executable is correct version or use embedded code #2465

Open

slackpad reviewed May 22, 2017

View reviewed changes

magiconair force-pushed the issue-2217-multiple-bind-addr branch 3 times, most recently from fd0843e to 2191f6a Compare May 23, 2017 19:06

rhyas mentioned this pull request May 24, 2017

Allow binding multiple client addresses #2217

Closed

magiconair force-pushed the issue-2217-multiple-bind-addr branch from 2e8f77d to f0401ee Compare May 24, 2017 09:44

slackpad mentioned this pull request May 25, 2017

[FEAT] Allow embedding consul #2487

Closed

magiconair mentioned this pull request May 26, 2017

0.8.0 short-time till "leave" leads to EOF #2880

Closed

magiconair force-pushed the issue-2217-multiple-bind-addr branch from 3cbe801 to f673590 Compare May 29, 2017 07:36

slackpad reviewed May 30, 2017

View reviewed changes

magiconair added 20 commits May 31, 2017 00:29

test: add test name to output

5b5566d

test: start sync after agent start

2e14c09

consul: use correct shutdownCh for Router

f15e90b

agent: print more useful shutdown message

1df74e3

dns: do not log socket close during shutdown

3e39f04

test: call t.Fatal() from main go routine

fe77785

agent: fix logging

10540f8

* use agent logger for consul/serf/raft/dns/agent/... * support optional id for concurrent tests

test: use UniqueID prefix for check tests to unify log output

7c5892a

test: add test for starting/stopping lots of agents

56cf015

test: skip tests that use cmd.Run until signal handling is fixed

6148910

test: run mock dns recursors on random ports

eea16e1

test: allow tests to control anti-entropy syncs

f498f24

test: increase sleep time for test

aa92adc

build: update makefile

205cbe5

* capture all output to test.log only and print only failures * always print output on travis * test github.com/hashicorp/consul/consul separately until we've found the interactions with the other packages during testing.

vendor: patch github.com/mitchellh/cli until help output is fixed

c068066

We need to init the flagset that cli uses to generate the help outside of the Run method since Run isn't called anymore for printing help.

test: fail if there is no leader

f4aa2ad

agent: refactor DNS and HTTP server

b6c69eb

* refactor DNS server to be ready for multiple bind addresses * drop tcpKeepAliveListener since it is default for the HTTP servers * add startup timeout watcher for HTTP servers identical to DNS server

agent: start HTTPS only if there is a cert and key

2620092

Address review comments

ec72969

magiconair force-pushed the issue-2217-multiple-bind-addr branch from f673590 to ec72969 Compare May 30, 2017 23:09

magiconair merged commit ec72969 into master May 30, 2017

magiconair deleted the issue-2217-multiple-bind-addr branch May 30, 2017 23:12

magiconair changed the title ~~WIP: work towards solving issues 2217 and 2465~~ First big step towards solving issues 2217 and 2465 May 30, 2017

slackpad reviewed May 31, 2017

View reviewed changes

automaticgiant mentioned this pull request Sep 24, 2019

==> Starting Consul agent... ==> Error starting agent: agent: timeout starting DNS servers #3802

Closed

		err := a.delegate.Shutdown()
		a.logger.Print("[INFO] agent: delegate down")

		agent.LogWriter = a.LogWriter
		agent.logger = log.New(logOutput, id, log.LstdFlags)

First big step towards solving issues 2217 and 2465 #3037

First big step towards solving issues 2217 and 2465 #3037

Conversation

magiconair commented May 12, 2017

magiconair commented May 20, 2017

magiconair commented May 21, 2017

magiconair commented May 21, 2017

slackpad left a 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

magiconair commented May 24, 2017

slackpad left a 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

magiconair commented May 31, 2017