Story #9550

[SDKs] Go SDK supports an ARVADOS_KEEP_SERVICES environment variable

Added by Brett Smith almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Start date:
07/06/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

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.

Subtasks

Task #9657: Support in old SDKResolvedTom Clegg

Task #9664: Update docsClosedTom Clegg

Task #9642: Review 9550-keep-services-envResolvedTom Clegg


Related issues

Copied to Arvados - Story #9551: [SDKs] Python SDK supports an ARVADOS_KEEP_SERVICES environment variableResolved07/06/2016

Associated revisions

Revision 305cee61
Added by Tom Clegg almost 5 years ago

Merge branch '9550-keep-services-env'

closes #9550

Revision 25236212 (diff)
Added by Ward Vandewege over 4 years ago

Make sure that we can set ARVADOS_KEEP_SERVICES via the
crunch-dispatch-slurm configuration file.

refs #10041 #9550

History

#1 Updated by Brett Smith almost 5 years ago

  • Target version set to 2016-07-20 sprint

#2 Updated by Brett Smith almost 5 years ago

  • Target version deleted (2016-07-20 sprint)

#3 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

#4 Updated by Brett Smith almost 5 years ago

  • Target version set to Arvados Future Sprints

#5 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)
  • Category set to SDKs

#6 Updated by Brett Smith almost 5 years ago

  • Story points set to 1.0

#7 Updated by Tom Clegg almost 5 years ago

  • Assigned To set to Tom Clegg
  • Target version changed from Arvados Future Sprints to 2016-08-03 sprint

#8 Updated by Tom Clegg almost 5 years ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg almost 5 years ago

9550-keep-services-env @ acfc0f5

#10 Updated by Radhika Chippada almost 5 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?

#11 Updated by Lucas Di Pentima almost 5 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?

#12 Updated by Tom Clegg almost 5 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?

#13 Updated by Tom Clegg almost 5 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...

#14 Updated by Lucas Di Pentima almost 5 years ago

Looks good to me.

#15 Updated by Tom Clegg almost 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 33 to 100

Applied in changeset arvados|commit:305cee61ba4a122f7d63a01f4a7b9b98737b8646.

Also available in: Atom PDF