Feature #15467
Revisit new config, ordered lists must end in "List" and unordered sets should be turned into maps
100%
Description
In Cluster configuration we say "Arrays and lists are not to be used" (and recently added "unless order is truly significant.")
But we already added some new config keys that broke this rule, including:- DriverParameters.SecurityGroupIDs
- DisabledAPIs
- NewUserNotificationRecipients
- NewInactiveUserNotificationRecipients
These should be changed from lists to maps. For example, instead of
SecurityGroupIDs: - a - b - c
we should have
SecurityGroupIDs: a: {} b: {} c: {}
We can maintain compatibility with old configs the way we did for InstanceTypes: if the JSON value starts with '[', unmarshal into a slice of strings and build an appropriate map. See (*InstanceTypeMap)UnmarshalJSON() in source:sdk/go/arvados/config.go.
We also need a test in lib/config that- prohibits list/array values on keys that don't have the suffix "List".
- prohibits the suffix "List" on keys whose values aren't lists/arrays.
Subtasks
Related issues
Associated revisions
Merge branch '15467-config-fixups'
Also includes 14713-cds-new-config and 14717-ws-new-config
Updates run_test_server.py and run-tests.sh to use new config.yml
refs #14713
refs #14717
refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
Merge branch '15467-legacy-config' refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
ec2 SecurityGroupIDs should be a StringSet refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
Fix DisabledAPIs in schema controller, keys are symbols, refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
run-tests ensure that ARVADOS_CONFIG is set, refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
Fix discovery doc dockerImageFormats, refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
Merge branch 'run-tests-fix' refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
Merge branch '15467-update-install-docs' refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
History
#1
Updated by Peter Amstutz over 1 year ago
- Status changed from New to In Progress
#2
Updated by Peter Amstutz over 1 year ago
- Status changed from In Progress to New
#3
Updated by Tom Clegg over 1 year ago
- Description updated (diff)
#4
Updated by Peter Amstutz over 1 year ago
- Target version changed from 2019-07-17 Sprint to 2019-07-31 Sprint
#5
Updated by Peter Amstutz over 1 year ago
- Assigned To set to Peter Amstutz
#6
Updated by Tom Morris over 1 year ago
- Story points set to 1.0
#7
Updated by Peter Amstutz over 1 year ago
- Status changed from New to In Progress
#8
Updated by Peter Amstutz over 1 year ago
#9
Updated by Peter Amstutz over 1 year ago
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
#10
Updated by Eric Biagiotti over 1 year ago
Peter Amstutz wrote:
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
In lib/config/load_test.go, TestListKeys:
- I think this test needs cases where YAML is loaded and the errors are actually raised. Seems like testing a test, but this is really default config validation code that doesn't have coverage.
- ln 402: It's difficult to follow all the reflection here. Maybe a comment or some well-named variables would help.
#11
Updated by Peter Amstutz over 1 year ago
Eric Biagiotti wrote:
Peter Amstutz wrote:
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
In lib/config/load_test.go, TestListKeys:
- I think this test needs cases where YAML is loaded and the errors are actually raised. Seems like testing a test, but this is really default config validation code that doesn't have coverage.
Ok, I added a couple of synthetic cases.
- ln 402: It's difficult to follow all the reflection here. Maybe a comment or some well-named variables would help.
Cleaned up a bit.
15467-config-fixups @ 1bc9d9cb0dac110676f9fd60b2fc128d54b1abf5
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1437/
#12
Updated by Eric Biagiotti over 1 year ago
Peter Amstutz wrote:
Ok, I added a couple of synthetic cases.
It's missing the case where it ends in "List" but isn't a list, but other than that it LGTM if the tests pass. Thanks!
#13
Updated by Peter Amstutz over 1 year ago
- Target version changed from 2019-07-31 Sprint to 2019-08-14 Sprint
#14
Updated by Peter Amstutz over 1 year ago
- Has duplicate Story #15147: Replace lists of strings in new config with maps added
#15
Updated by Peter Amstutz over 1 year ago
15467-legacy-config @ 6c89457a8f144d03f230656a1f4c43675d066b8c
Workbench and API server use -skip-legacy to suppress loading legacy config files of Go components (that they didn't load before).
Go components specify which legacy config file to use as argument to MungeLegacyConfigArgs, disable loading other config files.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1442/
#16
Updated by Eric Biagiotti over 1 year ago
Peter Amstutz wrote:
15467-legacy-config @ 6c89457a8f144d03f230656a1f4c43675d066b8c
This LGTM, thanks!
#17
Updated by Eric Biagiotti over 1 year ago
Reviewing 0ea1d6435771e00c7b84b50dc0cff1a13a1d7b67
- In install-dispatch.html, under Containers.ReserveExtraRAM, the introductory sentence says the value will be in bytes, but the example shows an arvados.ByteSize value. Maybe describe the units like we do for PollInterval?
- The "00000-bi6l4-000000000000000" used in the Containers.SLURM.KeepServices documentation is a bit confusing. I could imagine a user seeing that and thinking "Does my KeepServices have a different UUID, am I supposed to replace this?".
#18
Updated by Peter Amstutz over 1 year ago
Eric Biagiotti wrote:
Reviewing 0ea1d6435771e00c7b84b50dc0cff1a13a1d7b67
- In install-dispatch.html, under Containers.ReserveExtraRAM, the introductory sentence says the value will be in bytes, but the example shows an arvados.ByteSize value. Maybe describe the units like we do for PollInterval?
Added documentation about suffixes.
- The "00000-bi6l4-000000000000000" used in the Containers.SLURM.KeepServices documentation is a bit confusing. I could imagine a user seeing that and thinking "Does my KeepServices have a different UUID, am I supposed to replace this?".
Changed over to SbatchEnvironmentVariables as discussed on chat.
15467-update-install-docs @ a5e9c92d5d9e3a5575f9a103723d1814f5a51b66
#19
Updated by Eric Biagiotti over 1 year ago
Looks like lib/dispatchcloud tests failed.
Also, the code snippet under Containers.SLURM.SbatchEnvironmentVariables
needs to be updated from "http://127.0.0.1:25107": {}
to "ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2"
or whatever example makes sense.
#20
Updated by Peter Amstutz over 1 year ago
Eric Biagiotti wrote:
Looks like lib/dispatchcloud tests failed.
I suspect that is just a flaky test, but I submitted a new run:
https://ci.curoverse.com/job/developer-run-tests/1446/
Also, the code snippet under
Containers.SLURM.SbatchEnvironmentVariables
needs to be updated from"http://127.0.0.1:25107": {}
to"ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2"
or whatever example makes sense.
Fixed
15467-update-install-docs @ 5dbee91f63a39f19b1b8a63f0fda0f7115e7356e
#21
Updated by Eric Biagiotti over 1 year ago
Peter Amstutz wrote:
Eric Biagiotti wrote:
Looks like lib/dispatchcloud tests failed.
I suspect that is just a flaky test, but I submitted a new run:
https://ci.curoverse.com/job/developer-run-tests/1446/
Also, the code snippet under
Containers.SLURM.SbatchEnvironmentVariables
needs to be updated from"http://127.0.0.1:25107": {}
to"ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2"
or whatever example makes sense.Fixed
15467-update-install-docs @ 5dbee91f63a39f19b1b8a63f0fda0f7115e7356e
This LGTM, thanks!
#22
Updated by Peter Amstutz over 1 year ago
- Status changed from In Progress to Resolved
#23
Updated by Peter Amstutz about 1 year ago
- Release set to 22
Arvbox config doesn't need to set SupportedDockerImageFormats
Since it is already the default, refs #15467
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>