Story #15003

[config] Go package & tool for preprocessing cluster config file

Added by Tom Clegg 7 months ago. Updated 3 months ago.

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

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0
Release relationship:
Auto

Description

This tool preprocesses cluster configs so individual components only need to understand a single config object, despite complications like supporting old config formats.

Initial functionality:
  • Read the cluster config file from disk
  • Apply defaults (perhaps by layering on-disk config after loading yaml file embedded in Go binary)
  • Migrate values from old configs to their new positions (e.g., NodeProfiles → Services)
  • Error out if the old configs cannot be migrated/ignored safely
Possible future functionality (not implemented here):
  • Load secrets from disk ("signing key is file:///etc/key")
  • Load pre-cluster-config-file configs like keepstore.yml
  • Layered configs
  • Includes

Subtasks

Task #15096: Review 15003-preprocess-configResolvedTom Clegg

Task #15262: Review 15003-disable-config-warningsResolvedTom Clegg

Task #15288: Review 15003-new-config-structResolvedTom Clegg

Task #15302: Review 15003-duration-formatResolvedLucas Di Pentima

Task #15314: Add RailsAPI configs to authoritative Go config structResolvedTom Clegg

Task #15315: Ensure CloudVMs.DriverParameters are in config-dumpResolvedTom Clegg

Task #15331: Review 15003-config-default-locationResolvedTom Clegg

Task #15459: Review 15003-real-configs-flagged-unknownIn ProgressTom Clegg


Related issues

Related to Arvados - Bug #10282: [Go programs]Warn or error when unrecognized keys appear in config filesClosed

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

Associated revisions

Revision d235817f
Added by Tom Clegg 5 months ago

Merge branch '15003-preprocess-config'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision fb6909ef
Added by Tom Clegg 5 months ago

Merge branch '15003-disable-config-warnings'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 97d368d1
Added by Tom Clegg 5 months ago

Merge branch '15003-new-config-struct'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision fdadf296
Added by Tom Clegg 5 months ago

Merge branch '15003-duration-format'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 21dfd233
Added by Tom Clegg 4 months ago

Merge branch '15003-config-default-location'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 056d1819
Added by Tom Clegg 3 months ago

Merge branch '15003-real-configs-flagged-unknown'

refs #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision 055512c1
Added by Tom Clegg 3 months ago

Merge branch '15003-ensure-all-sample-keys'

closes #15003

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Clegg 7 months ago

  • Target version set to To Be Groomed

#2 Updated by Tom Clegg 7 months ago

  • Description updated (diff)
  • Story points set to 3.0

#3 Updated by Tom Clegg 7 months ago

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

#4 Updated by Tom Morris 7 months ago

  • Target version changed from To Be Groomed to Arvados Future Sprints

#5 Updated by Tom Morris 6 months ago

  • Assigned To set to Tom Clegg
  • Target version changed from Arvados Future Sprints to 2019-04-24 Sprint

#6 Updated by Tom Clegg 6 months ago

  • Status changed from New to In Progress

#7 Updated by Tom Clegg 6 months ago

15003-preprocess-config @ cb13593decb097f501c0a1d64510a653b3233395 https://ci.curoverse.com/view/Developer/job/developer-run-tests/1209/
  • embeds the default config data in the arvados-server binary
  • adds "arvados-server config-dump" and "arvados-server config-check" commands

#8 Updated by Tom Clegg 6 months ago

  • Target version changed from 2019-04-24 Sprint to 2019-05-08 Sprint

#10 Updated by Lucas Di Pentima 6 months ago

This looks good AFAICT, the only thing I think it would be needed is documentation, both on the Admin site & the sub-commands' help text.

#11 Updated by Tom Clegg 5 months ago

  • Target version changed from 2019-05-08 Sprint to 2019-05-22 Sprint

#12 Updated by Tom Clegg 5 months ago

  • Related to Bug #10282: [Go programs]Warn or error when unrecognized keys appear in config files added

#14 Updated by Tom Clegg 5 months ago

  • Target version changed from 2019-05-22 Sprint to 2019-06-05 Sprint

#15 Updated by Tom Clegg 5 months ago

  • Story points changed from 3.0 to 1.0

#16 Updated by Peter Amstutz 5 months ago

Warn if Services → Workbench2 → ExternalURL is missing or empty.

#17 Updated by Tom Clegg 5 months ago

15003-disable-config-warnings @ 311d552b31fbb97d10cdba7e248279d197751f79 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1254/

Disables warnings until we fill in the rest of the default/template file.

(Jenkins tests were bitten by #14878)

#18 Updated by Lucas Di Pentima 5 months ago

This LGTM, thanks!

#20 Updated by Tom Clegg 5 months ago

15003-new-config-struct @ 60560d144b7abf7ff98eb12ca3a591211df19ce9 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1266/
  • Remove NodeProfiles from config struct/docs/examples (migration allows old configs to continue working)
  • Update dispatch configs to new config structure (mostly, CloudVMs.* and Dispatch.* move to Containers.CloudVMs.*)
  • Add SAMPLE entries to default config file so "unknown key" checks work on sections like RemoteClusters and InstanceTypes
  • Use the "service command" library for the health check aggregator (remove duplicated logging/httpserver stuff)

#21 Updated by Lucas Di Pentima 5 months ago

Just a couple of comments:

  • Code comment at sdk/go/arvados/config.go:L252 got outdated
  • Couldn’t the map from lib/service/cmd.go:L148 be used from the Map() func at sdk/go/arvados/config.go?

With that, it LGTM.

#22 Updated by Tom Clegg 5 months ago

Ah yes, the lib/service map was a subset of Map(). Fixed both, thanks.

#23 Updated by Tom Clegg 5 months ago

15003-duration-format @ 6e73eff3926a2e7345333edd02531e8e6fbe15ef -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1272/

Spell durations like "336h" instead of "336h0m0s" when dumping config.

#24 Updated by Lucas Di Pentima 5 months ago

Interesting string replacing strategy :) LGTM, thanks.

#25 Updated by Tom Clegg 5 months ago

  • Target version changed from 2019-06-05 Sprint to 2019-06-19 Sprint

#26 Updated by Tom Clegg 4 months ago

On dev clusters, Services entries seem to be incorrectly flagged as deprecated/unknown:

deprecated or unknown config entry: Clusters.4xphq.Services.Controller.InternalURLs.http://localhost:9004
deprecated or unknown config entry: Clusters.4xphq.Services.DispatchCloud.InternalURLs.http://4xphq.arvadosapi.com:9006
deprecated or unknown config entry: Clusters.4xphq.Services.RailsAPI.InternalURLs.http://localhost:8000

#27 Updated by Ward Vandewege 4 months ago

  • Release set to 22

#28 Updated by Tom Clegg 4 months ago

15003-config-default-location @ 49ca4b9e9d96d35f704216bda401cc20cd972ca8 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1302/
  • Instead of stdin, load config from /etc/arvados/config.yml by default, or use -config arg to override.

#29 Updated by Lucas Di Pentima 4 months ago

15003-config-default-location LGTM, thanks!

#30 Updated by Tom Clegg 4 months ago

  • Status changed from In Progress to Resolved

#31 Updated by Tom Clegg 3 months ago

  • Target version changed from 2019-06-19 Sprint to 2019-07-17 Sprint
  • Status changed from Resolved to In Progress

#33 Updated by Lucas Di Pentima 3 months ago

I believe there're at least a couple more places on the defaul config yaml file that the SAMPLE trick is needed. I tried adding to the TestCheckNoDeprecatedKeys's test config the following:

  Workbench:
    UserProfileFormField:
      testProperty:
        Type: text
    ApplicationMimetypesWithViewIcon:
      ruby: {}

... then ran lib/config tests and got:

FAIL: cmd_test.go:38: CommandSuite.TestCheckNoDeprecatedKeys

cmd_test.go:60:
    c.Check(code, check.Equals, 0)
... obtained int = 1
... expected int = 0

cmd_test.go:62:
    c.Check(stderr.String(), check.Equals, "")
... obtained string = "" +
...     "deprecated or unknown config entry: Clusters.z1234.Workbench.ApplicationMimetypesWithViewIcon.ruby\n" +
...     "deprecated or unknown config entry: Clusters.z1234.Workbench.UserProfileFormField\n" 
... expected string = "" 

OOPS: 18 passed, 1 FAILED

This will surely bite us in the future, do you think there's an automatic way of validating new config items?

#34 Updated by Tom Clegg 3 months ago

I've added SAMPLE entries for those two.

As for automating this, perhaps we could load the defaults file as if it were a real config, then use reflection to detect any map[string]anything that doesn't have a SAMPLE key?

15003-real-configs-flagged-unknown @ 93cfe7c262708fb09eda5aad1839c832816d4591 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1386/

#35 Updated by Lucas Di Pentima 3 months ago

Tom Clegg wrote:

[...]

As for automating this, perhaps we could load the defaults file as if it were a real config, then use reflection to detect any map[string]anything that doesn't have a SAMPLE key?

Yes, I think it would be very useful. Other possibility is to not print any warning on those empty map[string]anything because it's assumed that's a 'list' config item? That would make unnecessary to add SAMPLE: {} entries on the default config that are somewhat confusing IMO.

#36 Updated by Tom Clegg 3 months ago

Lucas Di Pentima wrote:

Yes, I think it would be very useful. Other possibility is to not print any warning on those empty map[string]anything because it's assumed that's a 'list' config item? That would make unnecessary to add SAMPLE: {} entries on the default config that are somewhat confusing IMO.

We'd still want to check the fields inside map[string]RemoteCluster, so we'd need samples for things like that. A map[string]struct{} can always be treated as if it had {SAMPLE: {}}, whether or not an example is given in the defaults file, so we could potentially drop those. However, I don't think it's especially easy for the checker to figure out when it's looking at a map[string]struct{} entry -- the extra-key check happens before loading into the config struct.

#37 Updated by Lucas Di Pentima 3 months ago

Ok, so if the SAMPLE key check is simple enough to do in this story, It'll be enough to avoid future issues like this.

#38 Updated by Tom Clegg 3 months ago

I think the easiest way to prevent these from going undetected is a test that loads the defaults file as if it were a real config, and traverses the resulting Config object to confirm that every map[string]anything has a SAMPLE entry.

#39 Updated by Tom Clegg 3 months ago

15003-ensure-all-sample-keys @ cfb6b08b94d423a768862481b629cfb21fcc70a2 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1388/

This test also revealed that ManagedProperties was a map[string]interface{} with no type checks for its entries, so I fixed that.

FAIL: load_test.go:131: LoadSuite.TestDefaultConfigHasAllSAMPLEKeys

load_test.go:134:
    s.checkSAMPLEKeys(c, "", cfg)
load_test.go:118:
    c.Errorf("%s is a map with string keys (type %T) but config.default.yml has no SAMPLE key", path, x)
... Error: .Clusters.xxxxx.Collections.ManagedProperties.SAMPLE is a map with string keys (type map[string]interface {}) but config.default.yml has no SAMPLE key

#40 Updated by Lucas Di Pentima 3 months ago

This LGTM, thanks!

#41 Updated by Tom Clegg 3 months ago

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

Also available in: Atom PDF