From: radhika Date: Tue, 20 Oct 2015 14:59:41 +0000 (-0400) Subject: 7492: better error reporting of upstream errors in keepproxy. X-Git-Tag: 1.1.0~1282^2~13 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/b534fd13b3446067ddf6f8bd3c5b19c3463bc655 7492: better error reporting of upstream errors in keepproxy. --- diff --git a/sdk/go/keepclient/keepclient.go b/sdk/go/keepclient/keepclient.go index 67c304deaf..a018eb40f7 100644 --- a/sdk/go/keepclient/keepclient.go +++ b/sdk/go/keepclient/keepclient.go @@ -22,7 +22,33 @@ import ( // A Keep "block" is 64MB. const BLOCKSIZE = 64 * 1024 * 1024 -var BlockNotFound = errors.New("Block not found") +// Error interface with an error and boolean indicating whether the error is temporary +type Error interface { + error + Temporary() bool +} + +// multipleResponseError is of type Error +type multipleResponseError struct { + error + isTemp bool +} + +func (e *multipleResponseError) Temporary() bool { + return e.isTemp +} + +// BlockNotFound is a multipleResponseError where isTemp is false +var BlockNotFound = &ErrNotFound{multipleResponseError{ + error: errors.New("Block not found"), + isTemp: false, +}} + +// ErrNotFound is a multipleResponseError where isTemp can be true or false +type ErrNotFound struct { + multipleResponseError +} + var InsufficientReplicasError = errors.New("Could not write sufficient replicas") var OversizeBlockError = errors.New("Exceeded maximum block size (" + strconv.Itoa(BLOCKSIZE) + ")") var MissingArvadosApiHost = errors.New("Missing required environment variable ARVADOS_API_HOST") @@ -143,6 +169,7 @@ func (kc *KeepClient) PutR(r io.Reader) (locator string, replicas int, err error func (kc *KeepClient) getOrHead(method string, locator string) (io.ReadCloser, int64, string, error) { var errs []string + var count404 int tries_remaining := 1 + kc.Retries serversToTry := kc.getSortedRoots(locator) @@ -181,6 +208,8 @@ func (kc *KeepClient) getOrHead(method string, locator string) (io.ReadCloser, i // server side failure, transient // error, can try again. retryList = append(retryList, host) + } else if resp.StatusCode == 404 { + count404++ } } else { // Success. @@ -201,7 +230,16 @@ func (kc *KeepClient) getOrHead(method string, locator string) (io.ReadCloser, i } log.Printf("DEBUG: %s %s failed: %v", method, locator, errs) - return nil, 0, "", BlockNotFound + var err error + if count404 == len(kc.getSortedRoots(locator)) { + err = BlockNotFound + } else { + err = &ErrNotFound{multipleResponseError{ + error: fmt.Errorf("%s %s failed: %v", method, locator, errs), + isTemp: len(serversToTry) > 0, + }} + } + return nil, 0, "", err } // Get() retrieves a block, given a locator. Returns a reader, the diff --git a/sdk/go/keepclient/keepclient_test.go b/sdk/go/keepclient/keepclient_test.go index aaba0695f0..3714a544fc 100644 --- a/sdk/go/keepclient/keepclient_test.go +++ b/sdk/go/keepclient/keepclient_test.go @@ -14,6 +14,7 @@ import ( "net" "net/http" "os" + "strings" "testing" ) @@ -554,7 +555,9 @@ func (s *StandaloneSuite) TestGetFail(c *C) { kc.SetServiceRoots(map[string]string{"x": ks.url}, nil, nil) r, n, url2, err := kc.Get(hash) - c.Check(err, Equals, BlockNotFound) + errNotFound, _ := err.(ErrNotFound) + c.Check(errNotFound, NotNil) + c.Check(strings.Contains(err.Error(), "use of closed network connection"), Equals, true) c.Check(n, Equals, int64(0)) c.Check(url2, Equals, "") c.Check(r, Equals, nil) @@ -599,7 +602,9 @@ func (s *StandaloneSuite) TestGetNetError(c *C) { kc.SetServiceRoots(map[string]string{"x": "http://localhost:62222"}, nil, nil) r, n, url2, err := kc.Get(hash) - c.Check(err, Equals, BlockNotFound) + errNotFound, _ := err.(ErrNotFound) + c.Check(errNotFound, NotNil) + c.Check(strings.Contains(err.Error(), "connection refused"), Equals, true) c.Check(n, Equals, int64(0)) c.Check(url2, Equals, "") c.Check(r, Equals, nil) diff --git a/services/keepproxy/keepproxy.go b/services/keepproxy/keepproxy.go index 7900096caf..3cf1e28ba8 100644 --- a/services/keepproxy/keepproxy.go +++ b/services/keepproxy/keepproxy.go @@ -362,7 +362,7 @@ func (this GetBlockHandler) ServeHTTP(resp http.ResponseWriter, req *http.Reques log.Println("Warning:", GetRemoteAddress(req), req.Method, proxiedURI, "Content-Length not provided") } - switch err { + switch respErr := err.(type) { case nil: status = http.StatusOK resp.Header().Set("Content-Length", fmt.Sprint(expectLength)) @@ -375,10 +375,16 @@ func (this GetBlockHandler) ServeHTTP(resp http.ResponseWriter, req *http.Reques err = ContentLengthMismatch } } - case keepclient.BlockNotFound: - status = http.StatusNotFound + case keepclient.Error: + if respErr.Error() == "Block not found" { + status = http.StatusNotFound + } else if respErr.Temporary() { + status = http.StatusBadGateway + } else { + status = 422 + } default: - status = http.StatusBadGateway + status = http.StatusInternalServerError } } diff --git a/services/keepproxy/keepproxy_test.go b/services/keepproxy/keepproxy_test.go index 7643e4b0fa..17caefbb21 100644 --- a/services/keepproxy/keepproxy_test.go +++ b/services/keepproxy/keepproxy_test.go @@ -172,14 +172,18 @@ func (s *ServerRequiredSuite) TestPutAskGet(c *C) { { _, _, err := kc.Ask(hash) - c.Check(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Check(strings.Contains(err.Error(), "Block not found"), Equals, true) log.Print("Finished Ask (expected BlockNotFound)") } { reader, _, _, err := kc.Get(hash) c.Check(reader, Equals, nil) - c.Check(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Check(strings.Contains(err.Error(), "Block not found"), Equals, true) log.Print("Finished Get (expected BlockNotFound)") } @@ -251,7 +255,9 @@ func (s *ServerRequiredSuite) TestPutAskGetForbidden(c *C) { { _, _, err := kc.Ask(hash) - c.Check(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 403"), Equals, true) log.Print("Ask 1") } @@ -265,14 +271,18 @@ func (s *ServerRequiredSuite) TestPutAskGetForbidden(c *C) { { blocklen, _, err := kc.Ask(hash) - c.Assert(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 403"), Equals, true) c.Check(blocklen, Equals, int64(0)) log.Print("Ask 2") } { _, blocklen, _, err := kc.Get(hash) - c.Assert(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 403"), Equals, true) c.Check(blocklen, Equals, int64(0)) log.Print("Get") } @@ -291,7 +301,9 @@ func (s *ServerRequiredSuite) TestGetDisabled(c *C) { { _, _, err := kc.Ask(hash) - c.Check(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 400"), Equals, true) log.Print("Ask 1") } @@ -305,14 +317,18 @@ func (s *ServerRequiredSuite) TestGetDisabled(c *C) { { blocklen, _, err := kc.Ask(hash) - c.Assert(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 400"), Equals, true) c.Check(blocklen, Equals, int64(0)) log.Print("Ask 2") } { _, blocklen, _, err := kc.Get(hash) - c.Assert(err, Equals, keepclient.BlockNotFound) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound, NotNil) + c.Assert(strings.Contains(err.Error(), "HTTP 400"), Equals, true) c.Check(blocklen, Equals, int64(0)) log.Print("Get") }