Idea #9550
closed[SDKs] Go SDK supports an ARVADOS_KEEP_SERVICES environment variable
Description
ARVADOS_KEEP_SERVICES is an environment variable that lets the client set its own list of Keep services, instead of fetching it from the API server. It is a space-separated list of Keep API endpoint URLs. Those endpoints are expected to be non-disk services: the client should assume it does not know how many replicas the service will store, should send the X-Keep-Desired-Replicas request header, and respect the X-Keep-Replicas-Stored response header.
A client should pay attention to this env var if its api_host/token/insecure settings were also taken from the environment.
Functional requirements:
- When the variable is unset or empty, the Keep client should behave as it currently does.
- When the variable is set, it does not query the API server for the Keep services list. Instead it uses the value of the variable as the services list, following the rules above.
Related issues
Updated by Brett Smith about 8 years ago
- Target version set to 2016-07-20 sprint
Updated by Brett Smith about 8 years ago
- Target version deleted (
2016-07-20 sprint)
Updated by Brett Smith about 8 years ago
- Target version set to Arvados Future Sprints
Updated by Tom Clegg about 8 years ago
- Description updated (diff)
- Category set to SDKs
Updated by Tom Clegg about 8 years ago
- Assigned To set to Tom Clegg
- Target version changed from Arvados Future Sprints to 2016-08-03 sprint
Updated by Radhika Chippada about 8 years ago
Review comments:
My bad. I got redmine email when Lucas changed the state and I mistook it as it is a reminder for me to review :(. I just realized it when I wanted to update the status as part of adding my comments. (In the future I should update the status when I start reviewing, not when I complete it ...). Anyways, since I already compiled all my comments, here I go.
arvadosclient.go
- Can you please update this comment to make the discovery part more explicit:
"Use provided list of keep services, instead of using the ..." to something like "Use provided list of keep services, if any. (arvadosclient does not do a discovery to ...)"?
- Please update this comment
// Create a new ArvadosClient, initialized with standard Arvados environment // variables ARVADOS_API_HOST, ARVADOS_API_TOKEN, and (optionally) // ARVADOS_API_HOST_INSECURE.
discover.go
- Can you please cleanup the DiscoverKeepServers func a little to make it more readable with the new update
+ // Keep services are not provided in environment variable; Get keep services from api server var list svcList - - // Get keep services from api server err := this.Arvados.Call("GET", "keep_services", "", "accessible", nil, &list) if err != nil { return err
discovery_test.go
- Are we using ExampleRefreshServices anywhere?
Misc
- Sorry for my lack of knowledge, but can you please explain why disk typed keep services cannot be listed here? If this is the case, do we need to enforce that only disk typed services are configured?
- Can you please add in all places where the env variable is being read that only non-disk typed keepservices are to be listed in this variable?
- Any documentation updates?
Updated by Lucas Di Pentima about 8 years ago
Some questions on my own:
- Shouldn't be the env var content be validated with a regex before being added to the service roots?
- On DiscoverKeepServers func, why is the uuid prefix hardcoded like that? 0000-bi6l4-xxxx
- Also on DiscoverKeepServers(): Wouldn't be cleaner code if the new conditional block assemble a list to be fed to loadKeepServers(list), so that the call to the API server can be on the "else" part of the conditional, avoiding to call SetServiceRoots directly?
Updated by Tom Clegg about 8 years ago
Radhika Chippada wrote:
"Use provided list of keep services, instead of using the ..." to something like "Use provided list of keep services, if any. (arvadosclient does not do a discovery to ...)"?
Updated
[...]
Updated
- Can you please cleanup the DiscoverKeepServers func a little to make it more readable with the new update
Updated
- Are we using ExampleRefreshServices anywhere?
This is for godoc. I've renamed it to ExampleKeepClient_RefreshServices so it actually works now. See https://golang.org/pkg/testing/#hdr-Examples
- Sorry for my lack of knowledge, but can you please explain why disk typed keep services cannot be listed here? If this is the case, do we need to enforce that only disk typed services are configured?
Any type of service will work, but we treat them like non-disk services because we have no way of knowing what replication level they offer.
- Any documentation updates?
I think we should update the docs when both 9550 and 9551 are merged. Added a task. Any thoughts about where we should mention it?
Updated by Tom Clegg about 8 years ago
Lucas Di Pentima wrote:
- Shouldn't be the env var content be validated with a regex before being added to the service roots?
Good point. Checking now with the URL parser. Also updated to ignore extra spaces, so " https://foo "
doesn't get result in attempting "/abcdef0", "https://foo/abcdef0", and "/abcdef0".
- On DiscoverKeepServers func, why is the uuid prefix hardcoded like that? 0000-bi6l4-xxxx
It should look like a UUID so the rendezvous hash code doesn't get upset. I used 00000 as the fake site prefix (as opposed to zzzzz "for testing only"); bi6l4, the same infix used for real keep_service UUIDs; and a unique part that's unique.
- Also on DiscoverKeepServers(): Wouldn't be cleaner code if the new conditional block assemble a list to be fed to loadKeepServers(list), so that the call to the API server can be on the "else" part of the conditional, avoiding to call SetServiceRoots directly?
I considered that but it would mean adding a bunch of code to parse URL strings into KeepService structs, just so we can format them back to strings and check some conditions that we already know. And SetServiceRoots is a public method...
Updated by Tom Clegg about 8 years ago
- Status changed from In Progress to Resolved
- % Done changed from 33 to 100
Applied in changeset arvados|commit:305cee61ba4a122f7d63a01f4a7b9b98737b8646.