Project

General

Profile

Actions

Story #15003

closed

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

Added by Tom Clegg over 3 years ago. Updated almost 3 years 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 8 (1 open7 closed)

Task #15096: Review 15003-preprocess-configResolvedTom Clegg04/26/2019

Actions
Task #15262: Review 15003-disable-config-warningsResolvedTom Clegg04/26/2019

Actions
Task #15288: Review 15003-new-config-structResolvedTom Clegg04/26/2019

Actions
Task #15302: Review 15003-duration-formatResolvedLucas Di Pentima04/26/2019

Actions
Task #15314: Add RailsAPI configs to authoritative Go config structResolvedTom Clegg04/26/2019

Actions
Task #15315: Ensure CloudVMs.DriverParameters are in config-dumpResolvedTom Clegg04/26/2019

Actions
Task #15331: Review 15003-config-default-locationResolvedTom Clegg04/26/2019

Actions
Task #15459: Review 15003-real-configs-flagged-unknownIn ProgressTom Clegg04/26/2019

Actions

Related issues

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

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

Actions
Actions #1

Updated by Tom Clegg over 3 years ago

  • Target version set to To Be Groomed
Actions #2

Updated by Tom Clegg over 3 years ago

  • Description updated (diff)
  • Story points set to 3.0
Actions #3

Updated by Tom Clegg over 3 years ago

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

Updated by Tom Morris about 3 years ago

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

Updated by Tom Morris about 3 years ago

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

Updated by Tom Clegg about 3 years ago

  • Status changed from New to In Progress
Actions #7

Updated by Tom Clegg about 3 years 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
Actions #8

Updated by Tom Clegg about 3 years ago

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

Updated by Lucas Di Pentima about 3 years 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.

Actions #11

Updated by Tom Clegg about 3 years ago

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

Updated by Tom Clegg about 3 years ago

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

Updated by Tom Clegg about 3 years ago

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

Updated by Tom Clegg about 3 years ago

  • Story points changed from 3.0 to 1.0
Actions #16

Updated by Peter Amstutz about 3 years ago

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

Actions #17

Updated by Tom Clegg about 3 years 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)

Actions #18

Updated by Lucas Di Pentima about 3 years ago

This LGTM, thanks!

Actions #20

Updated by Tom Clegg about 3 years 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)
Actions #21

Updated by Lucas Di Pentima about 3 years 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.

Actions #22

Updated by Tom Clegg about 3 years ago

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

Actions #23

Updated by Tom Clegg about 3 years 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.

Actions #24

Updated by Lucas Di Pentima about 3 years ago

Interesting string replacing strategy :) LGTM, thanks.

Actions #25

Updated by Tom Clegg about 3 years ago

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

Updated by Tom Clegg about 3 years 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
Actions #27

Updated by Ward Vandewege about 3 years ago

  • Release set to 22
Actions #28

Updated by Tom Clegg about 3 years 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.
Actions #29

Updated by Lucas Di Pentima about 3 years ago

15003-config-default-location LGTM, thanks!

Actions #30

Updated by Tom Clegg about 3 years ago

  • Status changed from In Progress to Resolved
Actions #31

Updated by Tom Clegg almost 3 years ago

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

Updated by Lucas Di Pentima almost 3 years 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?

Actions #34

Updated by Tom Clegg almost 3 years 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/

Actions #35

Updated by Lucas Di Pentima almost 3 years 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.

Actions #36

Updated by Tom Clegg almost 3 years 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.

Actions #37

Updated by Lucas Di Pentima almost 3 years 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.

Actions #38

Updated by Tom Clegg almost 3 years 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.

Actions #39

Updated by Tom Clegg almost 3 years 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
Actions #40

Updated by Lucas Di Pentima almost 3 years ago

This LGTM, thanks!

Actions #41

Updated by Tom Clegg almost 3 years ago

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

Also available in: Atom PDF