Arvados

• embeds the default config data in the arvados-server binary
• adds "arvados-server config-dump" and "arvados-server config-check" commands

merged master

15003-preprocess-config @ 689901263ebfdd996da3711236615038e6245db3 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1218/

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.

15003-preprocess-config @ 42b483f3a722ad8a506040160e54d0080a197318 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1235/

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

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)

This LGTM, thanks!

in progress...

15003-new-config-struct @ d6358ef9fc0d8474827830a7ea0a451832e1fbec -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1258/

• 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)

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.

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

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.

Interesting string replacing strategy :) LGTM, thanks.

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

• Instead of stdin, load config from /etc/arvados/config.yml by default, or use -config arg to override.

15003-config-default-location LGTM, thanks!

15003-real-configs-flagged-unknown @ 54d059bfc6d84dbc20613ddd17d812d571e93eda -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1381/

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?

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/

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.

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.

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.

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.

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

This LGTM, thanks!