Feature #15467

Revisit new config, ordered lists must end in "List" and unordered sets should be turned into maps

Added by Peter Amstutz 4 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Peter Amstutz
Category:
-
Target version:
2019-08-14 Sprint
Start date:
07/26/2019
Due date:
% Done:

100%

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

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

Task #15478: Review 15467-config-fixupsResolvedEric Biagiotti

Task #15504: Fix dev clustersResolvedPeter Amstutz

Task #15510: Review 15467-legacy-configResolvedEric Biagiotti

Task #15515: Support missing KeepServices item, update documentation,ResolvedPeter Amstutz

Task #15518: Review 15467-update-install-docsResolvedEric Biagiotti

Related issues

Has duplicate Arvados - Story #15147: Replace lists of strings in new config with mapsDuplicate

Associated revisions

Revision eae1dba3 (diff)
Added by Peter Amstutz 4 months ago

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

Revision a93774da
Added by Peter Amstutz 4 months ago

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

Revision f9d9a612
Added by Peter Amstutz 4 months ago

Merge branch '15467-legacy-config' refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision a3ac6ea1 (diff)
Added by Peter Amstutz 4 months ago

ec2 SecurityGroupIDs should be a StringSet refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 327e331c (diff)
Added by Peter Amstutz 4 months ago

Fix DisabledAPIs in schema controller, keys are symbols, refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 8829652b (diff)
Added by Peter Amstutz 4 months ago

run-tests ensure that ARVADOS_CONFIG is set, refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 56c87b7c (diff)
Added by Peter Amstutz 4 months ago

Fix discovery doc dockerImageFormats, refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision e6c087eb
Added by Peter Amstutz 4 months ago

Merge branch 'run-tests-fix' refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 75310a63
Added by Peter Amstutz 4 months ago

Merge branch '15467-update-install-docs' refs #15467

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz 4 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 4 months ago

  • Status changed from In Progress to New

#3 Updated by Tom Clegg 4 months ago

  • Description updated (diff)

#4 Updated by Peter Amstutz 4 months ago

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

#5 Updated by Peter Amstutz 4 months ago

  • Assigned To set to Peter Amstutz

#6 Updated by Tom Morris 4 months ago

  • Story points set to 1.0

#7 Updated by Peter Amstutz 4 months ago

  • Status changed from New to In Progress

#9 Updated by Peter Amstutz 4 months 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 4 months 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 4 months 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 4 months 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 4 months ago

  • Target version changed from 2019-07-31 Sprint to 2019-08-14 Sprint

#14 Updated by Peter Amstutz 4 months ago

  • Has duplicate Story #15147: Replace lists of strings in new config with maps added

#15 Updated by Peter Amstutz 4 months 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 4 months ago

Peter Amstutz wrote:

15467-legacy-config @ 6c89457a8f144d03f230656a1f4c43675d066b8c

This LGTM, thanks!

#17 Updated by Eric Biagiotti 4 months 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 4 months 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

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

#19 Updated by Eric Biagiotti 4 months ago

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

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 4 months ago

Eric Biagiotti wrote:

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

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 4 months ago

Peter Amstutz wrote:

Eric Biagiotti wrote:

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

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 4 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF