Arvados

keepproxy.yml example config dump

    {
      "Client": {
        "Scheme": "",                
        "APIHost": "zzzzz.arvadosapi.com:443",    
        "AuthToken": "",            
        "Insecure": false            
      },        
      "Listen": ":25107",  
      "DisableGet": false,
      "DisablePut": false,
      "DefaultReplicas": 0,
      "Timeout": "15s",
      "PIDFile": "",    
      "Debug": false,    
      "ManagementToken": "" 
    }

We are dropping support for PIDFile. Regarding DisableGet/DisablePut, these will not be implemented in the cluster config and when loading old config files, keepproxy will error out if either are set to True. The remaining config attributes are all available elsewhere in the cluster config, so no keepproxy section is needed.

Latest at 1166aeb6033725709ded753a0c00f69320a9a873
Tests: https://ci.curoverse.com/view/Developer/job/developer-run-tests/1473/

I noticed that some components (keep-web, ws) have doc.go files. Keepproxy doesn't have one, should I make one?

Install docs¶

In config-migration.html this suggests the legacy config won't be used unless you add a -config=... argument, which isn't true:

In config-migration.html the table uses the notation "Collections:DefaultReplication" -- but I think everywhere else we spell this sort of thing "Collections.DefaultReplication".

Pre-existing bug: keepproxy comes with a systemd unit file so systems that have systemd should skip all the runit stuff. Should update along the lines of dispatch-cloud/keep-balance.

Unclosed span tag:

<pre><code>~$ <span class="userinput">exec keepproxy

In this example there's one "zzzzz" and one "uuid_prefix" -- should probably just choose one. I think most places we use "uuid_prefix", with the userinput span so it stands out.

+  zzzzz:
+    Services:
+      <span class="userinput">Keepproxy:
+        ExternalURL: https://keep.uuid_prefix.your.domain
+        InternalURLs:
+         "http://localhost:25107": {}

Config¶

Should surely be KeepproxyPath:

+       if legacyConfigArg != "-legacy-keepproxy-config" {
+               ldr.WebsocketPath = "" 
+       }

Keepproxy¶

run() should return errors instead of calling log.Fatal()

I don't think we can use AssertPathExists=/etc/arvados/config.yml yet -- that file might not exist at all until this version instructs the operator to create it.

TestLegacyConfig looks like it's testing the config migration. Could it move to lib/config?

In arvbox this looks like it should say https:

+        ExternalURL: "http://$localip:${services[keepproxy-ssl]}/"

Latest at 82ef8e6c9b95804159e199d7dd9128da82366a50
Tests: https://ci.curoverse.com/view/Developer/job/developer-run-tests/1484/

Individual commits with fixes detailed inline below.

Tom Clegg wrote:

Install docs¶

[...]

Documentation fixes in 82ef8e6c9b95804159e199d7dd9128da82366a50
Also removed usage.go to have the same CLI help as other migrated services.

Config¶

Should surely be KeepproxyPath:

+ if legacyConfigArg != "-legacy-keepproxy-config" {
+ ldr.WebsocketPath = ""
+ }

Keepproxy¶

run() should return errors instead of calling log.Fatal()

I don't think we can use AssertPathExists=/etc/arvados/config.yml yet -- that file might not exist at all until this version instructs the operator to create it.

The above keepproxy and config issues are fixed at 729c7c95d0d9c8d0357143717d5f2bb8cdd743de

TestLegacyConfig looks like it's testing the config migration. Could it move to lib/config?

Moved in 5ae0a422e60788a4039d17d2d8dfb60f250329c7

In arvbox this looks like it should say https:

+ ExternalURL: "http://$localip:${services[keepproxy-ssl]}/"

Fixed by merging master into my branch at d4ed3e6460469f2766e1f1676c538d6c86e000b6

Rerun of fuse tests, which appear to have hung. https://ci.curoverse.com/job/developer-run-tests-services-fuse/1506/console

LGTM, except I think the "legacy keepproxy config" explanation is misleading:

If the -legacy-keepproxy-config command line argument is provided, this will take precedence over /etc/arvados/keepproxy/keepproxy.yml and the cluster config.

"Takes precedence" implies both files are loaded, which is true for legacy+cluster configs, but not for default/specified legacy config paths.

If migrating to the centralized config, ...

This sounds like migrating to centralized config is optional. I think "if" could change to "after".

How about:

The legacy keepproxy config (loaded from /etc/arvados/keepproxy/keepproxy.yml or a different location specified via -legacy-keepproxy-config command line argument) takes precedence over the centralized config. After you migrate everything from the legacy config to the centralized config, you should delete /etc/arvados/keepproxy/keepproxy.yml and stop using the -legacy-keepproxy-config argument.

(As an aside, it seems like the migration doc is missing the key point that "arvados-server config-check" and "arvados-server config-dump" are the recommended/easy way to migrate legacy configs other than RailsAPI/Workbench1. Not sure which ticket that should belong to, but it seems like the "old/new configs" table here wouldn't have been necessary if we had already done it, so perhaps we should do it soon...?)

Updated documentation at de47033cb700ba5655dc6cfde77b888e8a94e87f

Added a ticket for config migration/upgrade doc improvements at https://dev.arvados.org/issues/15572

Looks good, thanks. One last thing -- maybe the warning about "if you use DisableGet etc., keepproxy will fail to start" belongs in the "before you upgrade..." page rather than the "migrating your config after upgrading" page.

Tom Clegg wrote:

Looks good, thanks. One last thing -- maybe the warning about "if you use DisableGet etc., keepproxy will fail to start" belongs in the "before you upgrade..." page rather than the "migrating your config after upgrading" page.

Good idea. Fixed in cdc569ed1c4777d3293327ee10b8f1c8bec06c6a

LGTM, thanks