Arvados

same as in https://dev.arvados.org/issues/7491#note-4

keepclient.go:144 func getOrHead()

If the client was unable to get a final answer from one or more servers (either due to network failure or the server returned a HTTP 408, 429, or 500+ error) it should not return BlockNotFound, it should return some other error. Keepproxy probably does not need to be changed, because it already responds 502 Gateway error as the default case for errors other than BlockNotFound.

Comments from Tom on another #7491:

• Return BlockNotFound only if all servers succeeded and replied exactly 404.
• If not returning BlockNotFound, return something like fmt.Errorf("%+v", errs) so the caller can see/report what went wrong.

Unresolved questions (ask Tom):

• What about 401 (Unauthorized) and 403 (Permission denied)?
  • If the same error is returned from every server, that's probably the one that should be returned, but need to define more error types and handle them in keepproxy.
• What is the correct response if the query gets a mix of 401, 403 and/or 404 errors?
  • Could happen if the site has a mix of servers with permissions enabled or disabled.

• In keepclient.go, define a new struct KeepRequestError
  • This should be equivalent to the arvados.errors.KeepRequestError in the Python SDK
  • Should contain an map of keep servers with the last error received resulting from contacting each keep server
• getOrHead() should return BlockNotFound if all servers replied http 404.
• getOrHead() should return KeepRequestError for any other error
• In keepproxy.go, GetBlockHandler.ServeHTTP, add a check for KeepRequestError. This should return a HTTP 422 status code and return a formatted text string of KeepRequestError in the body.

All the details from the description are implemented as suggested. One exception:

"In all cases, return err.Error() in the response body"

Setting the body with error message is interfering with "http.Error(resp, err.Error(), status)" in the defer func (Panic: runtime error: invalid memory address or nil pointer dereference). This is resulting in errors in KeepClient which is expecting error and nil reader etc. So, I have not implemented this. Please clarify requirement around this.

Reviewing 7492-keepproxy-upstream-errors:

• Is there a reason why you are using

  respErr.Error() == "Block not found'

  instead of

  respErr == keepclient.BlockNotFound

  or

  c.Check(err, Equals, keepclient.BlockNotFound)

  ?

• Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in getOrHead():

count404 := len(serversToTry)
...
if resp.StatusCode == 404 {
  count404--
}
...
if count404 == 0 {
}

• Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?

Is there a reason why you are using respErr.Error() "Block not found'

Updated to use respErr keepclient.BlockNotFound and c.Check(err, Equals, keepclient.BlockNotFound). Much better now.

Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in getOrHead() ...

Removed the unnecessary getSortedRoots call and used numServers variable instead. I did not want to make the change to count404-- as you suggested. Doing so results in code such as if count404 == 0 { set status code to 404 }, which can be quite counter-intuitive even if I use a better variable name :)

Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?

Added one more test set to do Get with a bad token and expired token. If you have any other specific tests in mind, please let me know.

Thanks.

Radhika Chippada wrote:

Is there a reason why you are using respErr.Error() "Block not found'

Updated to use respErr keepclient.BlockNotFound and c.Check(err, Equals, keepclient.BlockNotFound). Much better now.

Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in getOrHead() ...

Removed the unnecessary getSortedRoots call and used numServers variable instead. I did not want to make the change to count404-- as you suggested. Doing so results in code such as if count404 == 0 { set status code to 404 }, which can be quite counter-intuitive even if I use a better variable name :)

Thanks.

Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?

Added one more test set to do Get with a bad token and expired token. If you have any other specific tests in mind, please let me know.

Yes, can we have a test where keepproxy generates a Temporary() error? The original goal of this story was for keepproxy to start returning 502 Bad Gateway when one of the keep servers behind the proxy failed in a "temporary" way so as to trigger retry behavior on the external client.

Yes, can we have a test where keepproxy generates a Temporary() error ...

Added another test to keepproxy_test with "connection refused" temporary error. Also, improved test assertions in keepclient_test. Thanks.

Radhika Chippada wrote:

Yes, can we have a test where keepproxy generates a Temporary() error ...

Added another test to keepproxy_test with "connection refused" temporary error. Also, improved test assertions in keepclient_test. Thanks.

Unfortunately, that is not testing what I was looking to be tested. This is testing whether a failed connection to the proxy results in a Temporary error. I'm asking for a test where the client can connect to the proxy, but the proxy can't connect to the keep node.

0bb6625 looks good to me.