From: radhika Date: Wed, 28 Oct 2015 16:18:33 +0000 (-0400) Subject: Merge branch 'master' into 7492-keepproxy-upstream-errors X-Git-Tag: 1.1.0~1282^2~1 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/3445ed729dbccd21878373e01477be79e74a8453?hp=d954b40925d8315ede49e9a05c05c0ba24a74ba9 Merge branch 'master' into 7492-keepproxy-upstream-errors --- diff --git a/sdk/go/keepclient/keepclient.go b/sdk/go/keepclient/keepclient.go index 67c304deaf..2f809b3256 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") @@ -145,7 +171,12 @@ func (kc *KeepClient) getOrHead(method string, locator string) (io.ReadCloser, i var errs []string tries_remaining := 1 + kc.Retries + serversToTry := kc.getSortedRoots(locator) + + numServers := len(serversToTry) + count404 := 0 + var retryList []string for tries_remaining > 0 { @@ -181,6 +212,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 +234,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 == numServers { + 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 c03ba90736..df4638619f 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" ) @@ -556,7 +557,10 @@ func (s *StandaloneSuite) TestGetFail(c *C) { kc.Retries = 0 r, n, url2, err := kc.Get(hash) - c.Check(err, Equals, BlockNotFound) + errNotFound, _ := err.(*ErrNotFound) + c.Check(errNotFound, NotNil) + c.Check(strings.Contains(errNotFound.Error(), "HTTP 500"), Equals, true) + c.Check(errNotFound.Temporary(), Equals, true) c.Check(n, Equals, int64(0)) c.Check(url2, Equals, "") c.Check(r, Equals, nil) @@ -601,7 +605,10 @@ 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(errNotFound.Error(), "connection refused"), Equals, true) + c.Check(errNotFound.Temporary(), 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..d3dbeaf89e 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 == keepclient.BlockNotFound { + 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..997163eca4 100644 --- a/services/keepproxy/keepproxy_test.go +++ b/services/keepproxy/keepproxy_test.go @@ -30,6 +30,12 @@ var _ = Suite(&ServerRequiredSuite{}) // Tests that require the Keep server running type ServerRequiredSuite struct{} +// Gocheck boilerplate +var _ = Suite(&NoKeepServerSuite{}) + +// Test with no keepserver to simulate errors +type NoKeepServerSuite struct{} + // Wait (up to 1 second) for keepproxy to listen on a port. This // avoids a race condition where we hit a "connection refused" error // because we start testing the proxy too soon. @@ -65,6 +71,18 @@ func (s *ServerRequiredSuite) TearDownSuite(c *C) { arvadostest.StopAPI() } +func (s *NoKeepServerSuite) SetUpSuite(c *C) { + arvadostest.StartAPI() +} + +func (s *NoKeepServerSuite) SetUpTest(c *C) { + arvadostest.ResetEnv() +} + +func (s *NoKeepServerSuite) TearDownSuite(c *C) { + arvadostest.StopAPI() +} + func setupProxyService() { client := &http.Client{Transport: &http.Transport{ @@ -251,7 +269,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 +285,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 +315,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 +331,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") } @@ -473,3 +503,84 @@ func (s *ServerRequiredSuite) TestGetIndex(c *C) { _, err = kc.GetIndex("proxy", "xyz") c.Assert((err != nil), Equals, true) } + +func (s *ServerRequiredSuite) TestPutAskGetInvalidToken(c *C) { + kc := runProxy(c, []string{"keepproxy"}, 28852, false) + waitForListener() + defer closeListener() + + // Put a test block + hash, rep, err := kc.PutB([]byte("foo")) + c.Check(err, Equals, nil) + c.Check(rep, Equals, 2) + + for _, token := range []string{ + "nosuchtoken", + "2ym314ysp27sk7h943q6vtc378srb06se3pq6ghurylyf3pdmx", // expired + } { + // Change token to given bad token + kc.Arvados.ApiToken = token + + // Ask should result in error + _, _, err = kc.Ask(hash) + c.Check(err, NotNil) + errNotFound, _ := err.(keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, false) + c.Assert(strings.Contains(err.Error(), "HTTP 403"), Equals, true) + + // Get should result in error + _, _, _, err = kc.Get(hash) + c.Check(err, NotNil) + errNotFound, _ = err.(keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, false) + c.Assert(strings.Contains(err.Error(), "HTTP 403 \"Missing or invalid Authorization header\""), Equals, true) + } +} + +func (s *ServerRequiredSuite) TestAskGetKeepProxyConnectionError(c *C) { + arv, err := arvadosclient.MakeArvadosClient() + c.Assert(err, Equals, nil) + + // keepclient with no such keep server + kc := keepclient.New(&arv) + locals := map[string]string{ + "proxy": "http://localhost:12345", + } + kc.SetServiceRoots(locals, nil, nil) + + // Ask should result in temporary connection refused error + hash := fmt.Sprintf("%x", md5.Sum([]byte("foo"))) + _, _, err = kc.Ask(hash) + c.Check(err, NotNil) + errNotFound, _ := err.(*keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, true) + c.Assert(strings.Contains(err.Error(), "connection refused"), Equals, true) + + // Get should result in temporary connection refused error + _, _, _, err = kc.Get(hash) + c.Check(err, NotNil) + errNotFound, _ = err.(*keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, true) + c.Assert(strings.Contains(err.Error(), "connection refused"), Equals, true) +} + +func (s *NoKeepServerSuite) TestAskGetNoKeepServerError(c *C) { + kc := runProxy(c, []string{"keepproxy"}, 29999, false) + waitForListener() + defer closeListener() + + // Ask should result in temporary connection refused error + hash := fmt.Sprintf("%x", md5.Sum([]byte("foo"))) + _, _, err := kc.Ask(hash) + c.Check(err, NotNil) + errNotFound, _ := err.(*keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, true) + c.Assert(strings.Contains(err.Error(), "HTTP 502"), Equals, true) + + // Get should result in temporary connection refused error + _, _, _, err = kc.Get(hash) + c.Check(err, NotNil) + errNotFound, _ = err.(*keepclient.ErrNotFound) + c.Check(errNotFound.Temporary(), Equals, true) + c.Assert(strings.Contains(err.Error(), "HTTP 502"), Equals, true) +} diff --git a/tools/keep-rsync/keep-rsync_test.go b/tools/keep-rsync/keep-rsync_test.go index 1cef216af5..143e155227 100644 --- a/tools/keep-rsync/keep-rsync_test.go +++ b/tools/keep-rsync/keep-rsync_test.go @@ -355,7 +355,7 @@ func (s *ServerRequiredSuite) TestErrorDuringRsync_ErrorGettingBlockFromSrc(c *C blobSigningKey = "thisisfakeblobsigningkey" err := performKeepRsync(kcSrc, kcDst, blobSigningKey, "") - c.Check(strings.HasSuffix(err.Error(), "Block not found"), Equals, true) + c.Check(strings.Contains(err.Error(), "HTTP 403 \"Forbidden\""), Equals, true) } // Test rsync with error during Put to src.