Feature #14717

[websockets] Use cluster config

Added by Peter Amstutz 6 months ago. Updated about 2 hours ago.

Status:
New
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/18/2019
Due date:
% Done:

0%

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

Subtasks

Task #15394: Review 14717-ws-new-configIn ProgressPeter Amstutz


Related issues

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

History

#1 Updated by Peter Amstutz 6 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 6 months ago

  • Status changed from In Progress to New

#3 Updated by Peter Amstutz 6 months ago

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

#4 Updated by Tom Morris 6 months ago

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

#5 Updated by Tom Morris about 1 month ago

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

#6 Updated by Peter Amstutz about 1 month ago

  • Assigned To set to Peter Amstutz

#7 Updated by Peter Amstutz 19 days ago

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

#8 Updated by Peter Amstutz 5 days ago

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

#9 Updated by Peter Amstutz 4 days ago

14717-ws-new-config @ 7388e2f812805496e67930fdd316bb19657cb9fa

based on 14713-cds-new-config branch so that one probably needs to merge first.

#10 Updated by Tom Clegg about 7 hours ago

debug printf

+       fmt.Printf("Clllllllllllient %v %v", *oc.Client, cluster.Services.Controller.ExternalURL)

LoadDefaults() seems like a testing-only thing ("zzzzz"), and is only used by one test -- maybe we could relegate this to the test package and let the test(s) do something like config.NewLoader(bytes.NewBuffer(arvadostest.DefaultConfigFile), nil)?

Not sure about renaming PingTimeout to KeepaliveTimeout given "Keepalive" already has other meanings in both HTTP and TCP. SSH uses "server alive messages" and "client alive messages" to disambiguate. Even "ping timeout" isn't a great description -- we use it for two different things, 1) a ping interval and 2) a timeout for all sends/writes -- so perhaps
  • call it ClientAliveInterval and (potentially / in future) use it for SSH sessions, too? or
  • call it SendTimeout

nit: should WebsocketsPath just be WebsocketPath?

#11 Updated by Peter Amstutz about 3 hours ago

Tom Clegg wrote:

debug printf

[...]

Fixed.

LoadDefaults() seems like a testing-only thing ("zzzzz"), and is only used by one test -- maybe we could relegate this to the test package and let the test(s) do something like config.NewLoader(bytes.NewBuffer(arvadostest.DefaultConfigFile), nil)?

Removed. Added in a new behavior to make loading the main config optional (for backwards compatibility when only the component config is available) which has the side effect of providing a way to ask it for just the defaults.

Not sure about renaming PingTimeout to KeepaliveTimeout given "Keepalive" already has other meanings in both HTTP and TCP. SSH uses "server alive messages" and "client alive messages" to disambiguate. Even "ping timeout" isn't a great description -- we use it for two different things, 1) a ping interval and 2) a timeout for all sends/writes -- so perhaps
  • call it ClientAliveInterval and (potentially / in future) use it for SSH sessions, too? or
  • call it SendTimeout

Fixed.

nit: should WebsocketsPath just be WebsocketPath?

Fixed.

As noted above, revised loading behavior a bit to ensure that either/or/both the main config or component are available.

14717-ws-new-config @ b2df3cba90166bc67dd93600ac1a44ac6f9e81bd

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

#12 Updated by Tom Clegg about 2 hours ago

Is this "/etc/arvados/config.yml is optional if it doesn't exist and a per-component config was loaded" stuff really necessary? Especially since 1.4 is already telling everyone to move to /etc/arvados/config.yml, I'm thinking we can just write "make sure it exists" in the upgrading docs instead. I get the idea of surviving an upgrade even if you haven't read the docs, but I'm not sure it's worth the added special cases -- e.g., on a new system with no configs at all, it looks like c-d-s will error out with "legacy config file doesn't exist" which seems to point in the wrong direction; all code used by legacy components needs to avoid relying on the configured cluster ID because it might be the fake value "zzzzz"; "config-dump" still won't be able to tell you what config c-d-s is using until you create a [empty] cluster config file; etc.

I think we're better off making config loading work the same way regardless of who's asking (although there's already the exception that if you're using custom config file locations you need to use config-dump -legacy-* in order to match your some-component -config results if you're using alternate config file locations).

Also available in: Atom PDF