Arvados

A couple thoughts:

API client and keepstore have different socket settings, which probably why they were not combined in the first place.

The pointer to the Arvados client object a public field of the KeepClient struct. If it's okay to pool ArvadosClient objects and update the ApiToken field, then it should be okay to pool the KeepClient objects and update the Arvados field.

Peter Amstutz wrote:

I don't see how we can share the same HTTP client for both API requests and keep blocks when we have settings like this:

Good point. ArvadosClient would need two http.Clients, one for Keep and one for API.

Keeping the list of keep services on ArvadosClient would be an improvement, but it still needs to be updatable (I'm thinking the keep service roots should get their own struct with a lock that ArvadosClient can point to). Although we can do that and have the keep services list stay on the keep client object, and then the keep client object is safely copyable again.

In order for this to be a reasonable experience for callers I think we just have to use the appropriate number of http.Client objects, discovery doc requests, and keep_services/accessible requests, no matter how many times the caller does this:

arv := arvados.NewClientFromEnv()
arv.Token = token
arv.KeepClient().Get(...)

It should be trivial for callers to get efficient resource usage without juggling pools of ArvadosClient and KeepClient objects.

Some related fixes:

9005-conn-leak @ cad24bba2240b47f59bc5719a035e85ff5eb60ef

9005-keep-http-client @ 6fe6390690471cee8ba23984e3560fc4ced8b180

I'd also like to make keep-web remember the list of keep services from one request to the next. In the meantime the above branch should bring back HTTP keepalive without leaking unlimited FDs.

Seems like this should be in sync with either the "disk" or "non-disk" keep client settings. How about declaring some constants in keepclient.go for all the various timeouts just so its a bit more visible, then use those constants here?

    h := &proxyHandler{
        Handler:    rest,
        KeepClient: kc,
        timeout:    timeout,
        transport: &http.Transport{
            Dial: (&net.Dialer{
                Timeout:   20 * time.Second,
                KeepAlive: 10 * time.Second,
            }).Dial,
            TLSClientConfig:     arvadosclient.MakeTLSConfig(kc.Arvados.ApiInsecure),
            TLSHandshakeTimeout: 10 * time.Second,
        },
        ApiTokenCache: &ApiTokenCache{
            tokens:     make(map[string]int64),
            expireTime: 300,
        },
    }

Also, instead of spinning up a goroutine to periodically call CloseIdleConnections() there are settings on http.Transport we could use instead:

https://golang.org/pkg/net/http/

 For control over proxies, TLS configuration, keep-alives, compression, and other settings, create a Transport:

tr := &http.Transport{
    MaxIdleConns:       10,
    IdleConnTimeout:    30 * time.Second,
    DisableCompression: true,
}
client := &http.Client{Transport: tr}
resp, err := client.Get("https://example.com")

9005-keep-http-client @ 75c5b123e0b4cbfebed9b15364a97c2209f94740

SDK & keepproxy now copy http.DefaultTransport and modify the timeouts instead of building from scratch, so we can benefit from the default Go settings (non-zero MaxIdleConns, etc).

9005-share-discovery @ 916cf89b440fd13a9b9c055d817b34d339442ea3

• keepclient.ClearCache() - better name? maybe RefreshServiceDiscovery()?

So the strategy is to keep a global map of API server address to keep service list, spin up a goroutine which keeps that service list up to date, and request the latest service list on a channel whenever we need it.

• It looks like it uses the first API token passed to it as the API token for subsequent keep services requests? That should probably be reflected in the comment to MakeKeepClient().

• Agree that "ClearCache" is not a good name. RefreshServiceDiscovery() is fine, or ResetServiceCache()

• Using RWMutex on KeepClient instead of a regular mutex is a bit less efficient than a regular Mutex when there isn't much contention and/or the critical section is really small: https://groups.google.com/forum/#!topic/prometheus-developers/gi-A4lV3Z-Y

• Nitpick, inconsistent implementation of parsing booleans:

NewClientFromEnv:

strings.ToLower(os.Getenv("ARVADOS_API_HOST_INSECURE")); s == "1" || s == "yes" || s == "true" 

MakeArvadosClient:

     var matchTrue = regexp.MustCompile("^(?i:1|yes|true)$")

Workbench CollectionUploadTest is failing.

• Made a "StringBool" function and used that everywhere I found a copy&pasted matchTrue regexp.
• Renamed ClearCache to RefreshServiceDiscovery
• Punted RWMutex change

passed https://ci.curoverse.com/job/developer-run-tests/313/

Seems like it'll be a problem to remember the first token we see for a given API host and reuse it forever.

Is there any reason we shouldn't make the /arvados/v1/keep_services/accessible API exempt from the token-checking middleware, like we do for the discovery doc?

Tom Clegg wrote:

Seems like it'll be a problem to remember the first token we see for a given API host and reuse it forever.

Is there any reason we shouldn't make the /arvados/v1/keep_services/accessible API exempt from the token-checking middleware, like we do for the discovery doc?

The SDK will need to be backwards compatible with current API servers that require a token. Otherwise I don't have a problem with making the "accessible" route exempt from token checking.

Another option is to use the anonymous user token, which could be published in the discovery doc if it isn't already?

• ignores auth status for keep_services/accessible, so it doesn't matter which token the client uses.
• ideally we'd clear the api token in keepclient/discover.go and omit the Auth header if token is empty... but for now it seems more useful to at least have a chance of working with older API servers.

Probably the sanest way of dealing with the upgrade is to add a note to Upgrading to master, "if you upgrade keep-web, make sure to upgrade API server too" ...?

LGTM @ 5d03f6499055ef109ca2c8d5d59941b25de1fa47