Retry all servers on RPC call failure #1735

schmichael · 2016-09-22T20:28:13Z

rpcproxy is refactored into serverlist which prioritizes good servers
over servers in a remote DC or who have had a failure.

Registration, heartbeating, and alloc status updating will retry faster
when new servers are discovered.

Consul discovery will be retried more quickly when no servers are
available (eg on startup or an outage).

rpcproxy is refactored into serverlist which prioritizes good servers over servers in a remote DC or who have had a failure. Registration, heartbeating, and alloc status updating will retry faster when new servers are discovered. Consul discovery will be retried more quickly when no servers are available (eg on startup or an outage).

Remove use of periodic consul handlers in the client and just use goroutines. Consul Discovery is now triggered with a chan instead of using a timer and deadline to trigger. Once discovery is complete a chan is ticked so all goroutines waiting for servers will run. Should speed up bootstraping and recovery while decreasing spinning on timers.

Test asserted endpoint appendend servers, but the new/desired behavior is for the endpoint to set/overwrite servers.

dadgar · 2016-09-26T19:51:50Z

client/client.go

@@ -44,6 +43,10 @@ const (
 	// datacenters looking for the Nomad server service.
 	datacenterQueryLimit = 9

+	// consulReaperIntv is the interval at which the consul reaper will


consul -> Consul

Fixed everywhere (I think)

dadgar · 2016-09-26T19:52:46Z

client/client.go

@@ -154,14 +156,20 @@ type Client struct {
 	vaultClient vaultclient.VaultClient
 }

+var noServers = errors.New("no servers")


Comment and put in a var () block

noServers -> noServersErr

dadgar · 2016-09-26T19:54:53Z

client/client.go

+	heartbeatLock sync.Mutex
+
+	// doDisco triggers consul discovery; see triggerDiscovery
+	doDisco chan struct{}


doDisco -�> triggerDiscoveryCh

Fixed! (Much clearer!)

dadgar · 2016-09-26T19:55:42Z

client/client.go

+
+	// discovered will be ticked whenever consul discovery completes
+	// succesfully
+	discovered chan struct{}


discovered -> serversDiscoveredCh

dadgar · 2016-09-26T19:56:46Z

client/client.go

-		c.rpcProxy.AddPrimaryServer(serverAddr)
+	if len(c.configCopy.Servers) > 0 {
+		if err := c.SetServers(c.configCopy.Servers); err != nil {
+			logger.Printf("[WARN] None of the configured servers are valid: %v", err)


Style for logging is fmt.Sprintf("[%s] %s: %s", level, subSystem, msg). So please add [WARN] client:

dadgar · 2016-09-26T21:05:33Z

client/client.go

+func (c *Client) consulDiscovery() {
+	for {
+		select {
+		case c.doDisco <- struct{}{}:


Change to be case <-c.doDisco:

dadgar · 2016-09-26T21:06:09Z

client/client.go

+	}
+}
+
+func (c *Client) doConsulDisco() error {


Comment plus change name to be consulDiscoveryImpl

dadgar · 2016-09-26T21:55:01Z

client/client.go

+	// Notify waiting rpc calls. Wait briefly in case initial rpc
+	// just failed but the calling goroutine isn't selecting on
+	// discovered yet.
+	const dur = 50 * time.Millisecond


There is always the case that they will be on that border. We can remove this

Removed.

I think this was a clumsy workaround to get around startup races early on that AFAICT (both looking at the code again and testing it manually) is no longer necessary.

dadgar · 2016-09-26T21:56:24Z

client/client.go

-					d := consul.NewExecutorDomain(allocID, taskName)
-					domains = append(domains, d)
-				}
+func (c *Client) consulReaper() {


Comment on method

I would prefer you initialize the ticker here

Fixed.

Good call; there was no need for the ticker to be on the struct at all!

dadgar · 2016-09-26T21:56:42Z

client/client.go

-		c.consulSyncer.AddPeriodicHandler("Nomad Client Services Sync Handler", consulServicesReaperFn)
+}
+
+func (c *Client) doConsulReap() error {


Comments and rename to consulReapImpl

Also fix log line formatting

It's better to just let goroutines fallback to their longer retry intervals then try to be clever here.

dadgar · 2016-09-27T18:00:30Z

client/client.go

@@ -892,9 +948,6 @@ func (c *Client) registerNode() error {
 	}
 	var resp structs.NodeUpdateResponse
 	if err := c.RPC("Node.Register", &req, &resp); err != nil {
-		if time.Since(c.start) > registerErrGrace {


Why did you remove this?

It merely wrapped an error with a tiny bit more context in certain circumstances.

Instead I just added that tiny bit more context to the log line that handles this error, so there's no need to wrap it: https://github.com/hashicorp/nomad/pull/1735/files/ec81fc9baf776efb30bd752f64688ad562d915c6#diff-1cd41a2611da49ce63630d5a51bd82c5L876

It also kind of seemed like dead code since the error wrapping only affected logging. It seemed like maybe once upon a time it affected retry logic, but it doesn't anymore on master or this PR.

If you look at the comment on the registerErrGrace, it does have a point. It is to avoid error logs showing up in the -dev case. I would add it back

dadgar · 2016-09-27T23:28:54Z

CHANGELOG.md

@@ -4,6 +4,7 @@ IMPROVEMENTS:
  * core: Introduce node SecretID which can be used to minimize the available
    surface area of RPCs to malicious Nomad Clients [GH-1597] 
  * cli: `nomad alloc-status` shows allocation creation time [GH-1623]
+  * client: Retry all servers on RPC call failure [GH-1735]


Failed RPCs are retried on all servers

dadgar · 2016-09-27T23:29:20Z

CHANGELOG.md

@@ -17,6 +18,8 @@ BUG FIXES:
  * client/fingerprint: Fix inconsistent CPU MHz fingerprinting [GH-1366]
  * discovery: Fix old services not getting removed from consul on update
    [GH-1668]
+  * discovery: Fix client flapping when server was in a different datacenter


was -> is

+/- different datacenter as the client

github-actions · 2023-04-16T02:13:41Z

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 b-bootstrap-flapping branch from c958d3c to ce76aef Compare September 23, 2016 00:29

schmichael force-pushed the b-bootstrap-flapping branch from ce76aef to 64ac9b9 Compare September 23, 2016 18:48

schmichael changed the title ~~Refactor rpcproxy into a simpler serverlist~~ Retry all servers on RPC call failure Sep 23, 2016

schmichael added 2 commits September 23, 2016 17:02

Fix agent server set test

6b9be92

Test asserted endpoint appendend servers, but the new/desired behavior is for the endpoint to set/overwrite servers.

dadgar reviewed Sep 26, 2016

View reviewed changes

schmichael added 8 commits September 26, 2016 15:06

consul -> Consul

01ff307

noServers -> noServersErr

3bac143

doDisco -> triggerDiscoveryCh; discovered -> serversDiscoveredCh

a35fb33

Also fix log line formatting

Return csv of servers from Stats, not just count

a4a61e8

Flip disco chan; clarify method names/comments

93fbc12

Drop clumsy timeout on discovery notifications

d6b1496

It's better to just let goroutines fallback to their longer retry intervals then try to be clever here.

No need to put reaper ticker on the struct

ac90d1d

Fix lies found in comments by fact checkers

ec81fc9

dadgar reviewed Sep 27, 2016

View reviewed changes

dadgar approved these changes Sep 27, 2016

View reviewed changes

schmichael added 2 commits September 27, 2016 16:04

Remove unused const

ac29e9c

Add #1735 related fixes/improvements to changelog

64aa187

dadgar reviewed Sep 27, 2016

View reviewed changes

Cleanup changelog entries

91936f4

schmichael merged commit 95af837 into master Sep 27, 2016

schmichael deleted the b-bootstrap-flapping branch September 27, 2016 23:33

schmichael mentioned this pull request Sep 27, 2016

clients flap after upgrading to 0.4.1 from 0.3.2 #1641

Closed

schmichael mentioned this pull request Jan 19, 2018

Client RPCs are sticky to server #3738

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry all servers on RPC call failure #1735

Retry all servers on RPC call failure #1735

schmichael commented Sep 22, 2016 •

edited

Loading

dadgar Sep 26, 2016

schmichael Sep 26, 2016

dadgar Sep 26, 2016

dadgar Sep 26, 2016

schmichael Sep 26, 2016

dadgar Sep 26, 2016

schmichael Sep 26, 2016

dadgar Sep 26, 2016

schmichael Sep 26, 2016

dadgar Sep 26, 2016

schmichael Sep 26, 2016

dadgar Sep 26, 2016

schmichael Sep 27, 2016

dadgar Sep 26, 2016

schmichael Sep 27, 2016

dadgar Sep 26, 2016

schmichael Sep 27, 2016

dadgar Sep 26, 2016

schmichael Sep 27, 2016

dadgar Sep 26, 2016

schmichael Sep 27, 2016

dadgar Sep 27, 2016

schmichael Sep 27, 2016

dadgar Sep 27, 2016

dadgar Sep 27, 2016

dadgar Sep 27, 2016

github-actions bot commented Apr 16, 2023

Retry all servers on RPC call failure #1735

Retry all servers on RPC call failure #1735

Conversation

schmichael commented Sep 22, 2016 • 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

github-actions bot commented Apr 16, 2023

schmichael commented Sep 22, 2016 •

edited

Loading