From 86e6230de638fcb36c55cedfcd0e555beda44c5f Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 30 Nov 2015 17:09:41 -0800 Subject: [PATCH 1/3] Adds a retry capability to lock monitors in the API client. --- api/lock.go | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/api/lock.go b/api/lock.go index a76685f04c..5ef4ed909b 100644 --- a/api/lock.go +++ b/api/lock.go @@ -2,6 +2,7 @@ package api import ( "fmt" + "strings" "sync" "time" ) @@ -22,9 +23,15 @@ const ( // DefaultLockRetryTime is how long we wait after a failed lock acquisition // before attempting to do the lock again. This is so that once a lock-delay - // is in affect, we do not hot loop retrying the acquisition. + // is in effect, we do not hot loop retrying the acquisition. DefaultLockRetryTime = 5 * time.Second + // DefaultMonitorRetryTime is how long we wait after a failed monitor check + // of a lock (500 response code). This allows the monitor to ride out brief + // periods of unavailability, subject to the MonitorRetries setting in the + // lock options which is by default set to 0, disabling this feature. + DefaultMonitorRetryTime = 2 * time.Second + // LockFlagValue is a magic flag we set to indicate a key // is being used for a lock. It is used to detect a potential // conflict with a semaphore. @@ -62,11 +69,12 @@ type Lock struct { // LockOptions is used to parameterize the Lock behavior. type LockOptions struct { - Key string // Must be set and have write permissions - Value []byte // Optional, value to associate with the lock - Session string // Optional, created if not specified - SessionName string // Optional, defaults to DefaultLockSessionName - SessionTTL string // Optional, defaults to DefaultLockSessionTTL + Key string // Must be set and have write permissions + Value []byte // Optional, value to associate with the lock + Session string // Optional, created if not specified + SessionName string // Optional, defaults to DefaultLockSessionName + SessionTTL string // Optional, defaults to DefaultLockSessionTTL + MonitorRetries int // Optional, defaults to 0 which means no retries } // LockKey returns a handle to a lock struct which can be used @@ -327,8 +335,24 @@ func (l *Lock) monitorLock(session string, stopCh chan struct{}) { kv := l.c.KV() opts := &QueryOptions{RequireConsistent: true} WAIT: + retries := l.opts.MonitorRetries +RETRY: pair, meta, err := kv.Get(l.opts.Key, opts) if err != nil { + // TODO (slackpad) - Make a real error type here instead of using + // a string check. + const serverError = "Unexpected response code: 500" + + // If configured we can try to ride out a brief Consul unavailability + // by doing retries. Note that we have to attempt the retry in a non- + // blocking fashion so that we have a clean place to reset the retry + // counter if service is restored. + if retries > 0 && strings.Contains(err.Error(), serverError) { + time.Sleep(DefaultMonitorRetryTime) + retries-- + opts.WaitIndex = 0 + goto RETRY + } return } if pair != nil && pair.Session == session { From c2a8fee76c49697e0740122b75e53ecfe24a411d Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 30 Nov 2015 20:16:22 -0800 Subject: [PATCH 2/3] Adds custom retry time for lock monitors. --- api/lock.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/api/lock.go b/api/lock.go index 5ef4ed909b..c1f6edf822 100644 --- a/api/lock.go +++ b/api/lock.go @@ -69,12 +69,13 @@ type Lock struct { // LockOptions is used to parameterize the Lock behavior. type LockOptions struct { - Key string // Must be set and have write permissions - Value []byte // Optional, value to associate with the lock - Session string // Optional, created if not specified - SessionName string // Optional, defaults to DefaultLockSessionName - SessionTTL string // Optional, defaults to DefaultLockSessionTTL - MonitorRetries int // Optional, defaults to 0 which means no retries + Key string // Must be set and have write permissions + Value []byte // Optional, value to associate with the lock + Session string // Optional, created if not specified + SessionName string // Optional, defaults to DefaultLockSessionName + SessionTTL string // Optional, defaults to DefaultLockSessionTTL + MonitorRetries int // Optional, defaults to 0 which means no retries + MonitorRetryTime time.Duration // Optional, defaults to DefaultMonitorRetryTime } // LockKey returns a handle to a lock struct which can be used @@ -104,6 +105,9 @@ func (c *Client) LockOpts(opts *LockOptions) (*Lock, error) { return nil, fmt.Errorf("invalid SessionTTL: %v", err) } } + if opts.MonitorRetryTime == 0 { + opts.MonitorRetryTime = DefaultMonitorRetryTime + } l := &Lock{ c: c, opts: opts, @@ -348,7 +352,7 @@ RETRY: // blocking fashion so that we have a clean place to reset the retry // counter if service is restored. if retries > 0 && strings.Contains(err.Error(), serverError) { - time.Sleep(DefaultMonitorRetryTime) + time.Sleep(l.opts.MonitorRetryTime) retries-- opts.WaitIndex = 0 goto RETRY From 380658f6f19b74406b91d50808a0603e65c6a7e4 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Mon, 30 Nov 2015 20:16:40 -0800 Subject: [PATCH 3/3] Adds a unit test for lock monitor retries. --- api/lock_test.go | 119 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/api/lock_test.go b/api/lock_test.go index f4bad9e6b8..7d8cbfec35 100644 --- a/api/lock_test.go +++ b/api/lock_test.go @@ -2,6 +2,10 @@ package api import ( "log" + "net/http" + "net/http/httptest" + "net/http/httputil" + "strings" "sync" "testing" "time" @@ -369,3 +373,118 @@ func TestLock_ReclaimLock(t *testing.T) { t.Fatalf("should not be leader") } } + +func TestLock_MonitorRetry(t *testing.T) { + t.Parallel() + raw, s := makeClient(t) + defer s.Stop() + + // Set up a server that always responds with 500 errors. + failer := func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(500) + } + outage := httptest.NewServer(http.HandlerFunc(failer)) + defer outage.Close() + + // Set up a reverse proxy that will send some requests to the + // 500 server and pass everything else through to the real Consul + // server. + var mutex sync.Mutex + errors := 0 + director := func(req *http.Request) { + mutex.Lock() + defer mutex.Unlock() + + req.URL.Scheme = "http" + if errors > 0 && req.Method == "GET" && strings.Contains(req.URL.Path, "/v1/kv/test/lock") { + req.URL.Host = outage.URL[7:] // Strip off "http://". + errors-- + } else { + req.URL.Host = raw.config.Address + } + } + proxy := httptest.NewServer(&httputil.ReverseProxy{Director: director}) + defer proxy.Close() + + // Make another client that points at the proxy instead of the real + // Consul server. + config := raw.config + config.Address = proxy.URL[7:] // Strip off "http://". + c, err := NewClient(&config) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Set up a lock with retries enabled. + opts := &LockOptions{ + Key: "test/lock", + SessionTTL: "60s", + MonitorRetries: 3, + } + lock, err := c.LockOpts(opts) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Make sure the default got set. + if lock.opts.MonitorRetryTime != DefaultMonitorRetryTime { + t.Fatalf("bad: %d", lock.opts.MonitorRetryTime) + } + + // Now set a custom time for the test. + opts.MonitorRetryTime = 250 * time.Millisecond + lock, err = c.LockOpts(opts) + if err != nil { + t.Fatalf("err: %v", err) + } + if lock.opts.MonitorRetryTime != 250*time.Millisecond { + t.Fatalf("bad: %d", lock.opts.MonitorRetryTime) + } + + // Should get the lock. + leaderCh, err := lock.Lock(nil) + if err != nil { + t.Fatalf("err: %v", err) + } + if leaderCh == nil { + t.Fatalf("not leader") + } + + // Poke the key using the raw client to force the monitor to wake up + // and check the lock again. This time we will return errors for some + // of the responses. + mutex.Lock() + errors = 2 + mutex.Unlock() + pair, _, err := raw.KV().Get("test/lock", &QueryOptions{}) + if err != nil { + t.Fatalf("err: %v", err) + } + if _, err := raw.KV().Put(pair, &WriteOptions{}); err != nil { + t.Fatalf("err: %v", err) + } + time.Sleep(5 * opts.MonitorRetryTime) + + // Should still be the leader. + select { + case <-leaderCh: + t.Fatalf("should be leader") + default: + } + + // Now return an overwhelming number of errors. + mutex.Lock() + errors = 10 + mutex.Unlock() + if _, err := raw.KV().Put(pair, &WriteOptions{}); err != nil { + t.Fatalf("err: %v", err) + } + time.Sleep(5 * opts.MonitorRetryTime) + + // Should lose leadership. + select { + case <-leaderCh: + case <-time.After(time.Second): + t.Fatalf("should not be leader") + } +}