Feature #14715

[keepproxy] Use cluster config file

Added by Peter Amstutz 8 months ago. Updated 27 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
08/12/2019
Due date:
% Done:

100%

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

Subtasks

Task #15385: Review 14715-keepprox-configResolvedEric Biagiotti


Related issues

Related to Arvados - Story #13648: [Epic] Use one cluster configuration file for all componentsNew

Associated revisions

Revision d4ed3e64
Added by Eric Biagiotti about 1 month ago

Merge branch 'master' into 14715-keepprox-config

refs #14715

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision ef23c3d1
Added by Eric Biagiotti 27 days ago

Merge branch '14715-keepprox-config'

refs #14715

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

History

#1 Updated by Peter Amstutz 8 months ago

  • Related to Story #13648: [Epic] Use one cluster configuration file for all components added

#2 Updated by Tom Morris 7 months ago

  • Target version changed from To Be Groomed to Arvados Future Sprints
  • Story points set to 1.0

#3 Updated by Tom Morris 3 months ago

  • Target version changed from Arvados Future Sprints to 2019-07-03 Sprint

#4 Updated by Eric Biagiotti 3 months ago

  • Assigned To set to Eric Biagiotti

#5 Updated by Eric Biagiotti 3 months ago

  • Target version changed from 2019-07-03 Sprint to 2019-07-17 Sprint

#6 Updated by Eric Biagiotti 2 months ago

  • Target version changed from 2019-07-17 Sprint to 2019-07-31 Sprint

#7 Updated by Eric Biagiotti about 2 months ago

  • Status changed from New to In Progress

#8 Updated by Eric Biagiotti about 2 months ago

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.

#9 Updated by Eric Biagiotti about 2 months ago

  • Target version changed from 2019-07-31 Sprint to 2019-08-14 Sprint

#10 Updated by Eric Biagiotti about 1 month ago

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?

#11 Updated by Tom Clegg about 1 month ago

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:

+The legacy keepproxy config is stored at @/etc/arvados/keepproxy/keepproxy.yml@ and can still be used with the @-config=path/to/legacy/config@ keepproxy command line argument.

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]}/"

#12 Updated by Eric Biagiotti about 1 month ago

  • Target version changed from 2019-08-14 Sprint to 2019-08-28 Sprint

#13 Updated by Eric Biagiotti about 1 month ago

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

#15 Updated by Tom Clegg about 1 month ago

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...?)

#16 Updated by Eric Biagiotti 28 days ago

Updated documentation at de47033cb700ba5655dc6cfde77b888e8a94e87f

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

#17 Updated by Tom Clegg 27 days ago

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.

#18 Updated by Eric Biagiotti 27 days ago

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

#19 Updated by Tom Clegg 27 days ago

LGTM, thanks

#20 Updated by Eric Biagiotti 27 days ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF