From 38f52694a09be8444283b14725c6d8f9bf80fc70 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Fri, 10 Sep 2021 08:40:52 -0400 Subject: [PATCH 1/3] nodes/ endpoint supports timeout --- http/service.go | 37 +++++++++++-------------------------- http/service_test.go | 11 ++++++++--- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/http/service.go b/http/service.go index ef2f80e4..05545157 100644 --- a/http/service.go +++ b/http/service.go @@ -143,7 +143,7 @@ const ( numAuthFail = "authFail" // Default timeout for cluster communications. - defaulTimeout = 30 * time.Second + defaultTimeout = 30 * time.Second // PermAll means all actions permitted. PermAll = "all" @@ -677,7 +677,7 @@ func (s *Service) handleNodes(w http.ResponseWriter, r *http.Request) { return } - t, err := timeout(r, time.Duration(1)) + timeout, err := timeoutParam(r, defaultTimeout) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -712,7 +712,7 @@ func (s *Service) handleNodes(w http.ResponseWriter, r *http.Request) { return } - nodesResp, err := s.checkNodes(filteredNodes, t) + nodesResp, err := s.checkNodes(filteredNodes, timeout) if err != nil { http.Error(w, fmt.Sprintf("check nodes: %s", err.Error()), http.StatusInternalServerError) @@ -773,7 +773,7 @@ func (s *Service) handleExecute(w http.ResponseWriter, r *http.Request) { resp := NewResponse() - timeout, isTx, timings, redirect, err := reqParams(r, defaulTimeout) + timeout, isTx, timings, redirect, err := reqParams(r, defaultTimeout) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return @@ -855,7 +855,7 @@ func (s *Service) handleQuery(w http.ResponseWriter, r *http.Request) { resp := NewResponse() - timeout, isTx, timings, redirect, err := reqParams(r, defaulTimeout) + timeout, isTx, timings, redirect, err := reqParams(r, defaultTimeout) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return @@ -1009,7 +1009,7 @@ func (s *Service) LeaderAPIAddr() string { return "" } - apiAddr, err := s.cluster.GetNodeAPIAddr(nodeAddr, defaulTimeout) + apiAddr, err := s.cluster.GetNodeAPIAddr(nodeAddr, defaultTimeout) if err != nil { return "" @@ -1044,7 +1044,7 @@ func (s *Service) checkNodes(nodes []*store.Server, timeout time.Duration) (map[ defer mu.Unlock() start := time.Now() - apiAddr, err := s.cluster.GetNodeAPIAddr(raftAddr, defaulTimeout) + apiAddr, err := s.cluster.GetNodeAPIAddr(raftAddr, timeout) if err != nil { resp[id].error = err.Error() return @@ -1197,19 +1197,19 @@ func isRedirect(req *http.Request) (bool, error) { return queryParam(req, "redirect") } -// timeoutParam returns the value, if any, set for timeout. If not set, -// it returns the value passed in as a default. +// timeoutParam returns the value, if any, set for timeout. If not set, it +// returns the value passed in as a default. func timeoutParam(req *http.Request, def time.Duration) (time.Duration, error) { q := req.URL.Query() timeout := strings.TrimSpace(q.Get("timeout")) if timeout == "" { return def, nil } - d, err := time.ParseDuration(timeout) + t, err := time.ParseDuration(timeout) if err != nil { return 0, err } - return d, nil + return t, nil } // isTx returns whether the HTTP request is requesting a transaction. @@ -1254,21 +1254,6 @@ func isTimings(req *http.Request) (bool, error) { return queryParam(req, "timings") } -// timeout returns the timeout included in the query, or the given default -func timeout(req *http.Request, d time.Duration) (time.Duration, error) { - q := req.URL.Query() - tStr := q.Get("timeout") - if tStr == "" { - return d, nil - } - - t, err := time.ParseDuration(tStr) - if err != nil { - return d, nil - } - return t, nil -} - // level returns the requested consistency level for a query func level(req *http.Request) (command.QueryRequest_Level, error) { q := req.URL.Query() diff --git a/http/service_test.go b/http/service_test.go index 64bf5699..f8e925ec 100644 --- a/http/service_test.go +++ b/http/service_test.go @@ -854,6 +854,7 @@ func Test_timeoutQueryParam(t *testing.T) { tests := []struct { u string dur string + err bool }{ { u: "http://localhost:4001/nodes?timeout=5s", @@ -873,15 +874,19 @@ func Test_timeoutQueryParam(t *testing.T) { }, { u: "http://localhost:4001/nodes?timeout=zdfjkh", - dur: defStr, + err: true, }, } for _, tt := range tests { req.URL = mustURLParse(tt.u) - timeout, err := timeout(&req, def) + timeout, err := timeoutParam(&req, def) if err != nil { - t.Fatalf("failed to get timeout: %s", err) + if tt.err { + // Error is expected, all is OK. + continue + } + t.Fatalf("failed to get timeout as expected: %s", err) } if timeout != mustParseDuration(tt.dur) { t.Fatalf("got wrong timeout, expected %s, got %s", mustParseDuration(tt.dur), timeout) From 74cd0b13786c266849e63ae7ea89c1a17d4b858c Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Fri, 10 Sep 2021 08:51:13 -0400 Subject: [PATCH 2/3] CHANGELOG and docs --- CHANGELOG.md | 1 + DOC/DIAGNOSTICS.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c52a4081..3ef85bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - [PR #888](https://github.com/rqlite/rqlite/pull/888): Expose stats about BoltDB on the `status/` endpoint. - [PR #889](https://github.com/rqlite/rqlite/pull/889): Add OS-level information to `status/` output. - [PR #892](https://github.com/rqlite/rqlite/pull/892): More Snapshot metrics. +- [PR #894](https://github.com/rqlite/rqlite/pull/894): Support controlling timeout on nodes/ endpoint. ## 6.4.3 (September 8th 2021) ### Implementation changes and bug fixes diff --git a/DOC/DIAGNOSTICS.md b/DOC/DIAGNOSTICS.md index e2d3f36b..ce78e197 100644 --- a/DOC/DIAGNOSTICS.md +++ b/DOC/DIAGNOSTICS.md @@ -37,6 +37,7 @@ runtime: ```bash curl localhost:4001/nodes?pretty curl localhost:4001/nodes?nonvoters&pretty # Also check non-voting nodes. +curl localhost:4001/nodes?timeout=5s # Give up if all nodes don't respond within 5 seconds. Default is 30 seconds. ``` You can also request the same nodes information via the CLI: From 6b7318d18811052399d1d31e323eb8b06d7c1748 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Fri, 10 Sep 2021 09:10:03 -0400 Subject: [PATCH 3/3] Set missing connection timeouts --- cluster/client.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/cluster/client.go b/cluster/client.go index 403f58d4..0b93ab97 100644 --- a/cluster/client.go +++ b/cluster/client.go @@ -106,6 +106,10 @@ func (c *Client) GetNodeAPIAddr(nodeAddr string, timeout time.Duration) (string, } // Read length of response. + if err := conn.SetDeadline(time.Now().Add(timeout)); err != nil { + handleConnError(conn) + return "", err + } _, err = io.ReadFull(conn, b) if err != nil { return "", err @@ -175,12 +179,11 @@ func (c *Client) Execute(er *command.ExecuteRequest, nodeAddr string, timeout ti return nil, err } + // Read length of response. if err := conn.SetDeadline(time.Now().Add(timeout)); err != nil { handleConnError(conn) return nil, err } - - // Read length of response. _, err = io.ReadFull(conn, b) if err != nil { return nil, err @@ -189,6 +192,10 @@ func (c *Client) Execute(er *command.ExecuteRequest, nodeAddr string, timeout ti // Read in the actual response. p = make([]byte, sz) + if err := conn.SetDeadline(time.Now().Add(timeout)); err != nil { + handleConnError(conn) + return nil, err + } _, err = io.ReadFull(conn, p) if err != nil { return nil, err