Bug #15642

[config] Don't erase cluster config values just because their counterparts don't appear in legacy configs

Added by Tom Clegg 10 months ago. Updated 8 months ago.

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

100%

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

Description

The legacy config loaders for arv-git-httpd and keep-web have code like this:

        cluster.ManagementToken = oc.ManagementToken

As a result, if the legacy arv-git-httpd config file exists, but does not mention ManagementToken, the active configuration uses a blank ManagementToken. The correct behavior in such cases is to use the value given in a different legacy config file, or the value given in the new cluster config file, or the default value (which happens to be blank in this specific case).

To fix this, the old config structs should have *string instead of string fields, and the overrides should be conditional on Field != nil, as in the crunch-dispatch-slurm loading code, which doesn't have this bug:

        if oc.PollPeriod != nil {
                cluster.Containers.CloudVMs.PollInterval = *oc.PollPeriod
        }

Subtasks

Task #15810: Review 15642-cluster-config-fixResolvedLucas Di Pentima


Related issues

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

Associated revisions

Revision d0dd092c
Added by Lucas Di Pentima 8 months ago

Merge branch '15642-cluster-config-fix'
Closes #15642

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Tom Clegg 10 months ago

  • Target version set to Arvados Future Sprints

#2 Updated by Tom Clegg 10 months ago

  • Release set to 22

#3 Updated by Tom Clegg 10 months ago

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

#4 Updated by Tom Clegg 8 months ago

  • Target version changed from Arvados Future Sprints to 2019-11-20 Sprint

#5 Updated by Lucas Di Pentima 8 months ago

  • Assigned To set to Lucas Di Pentima

#6 Updated by Lucas Di Pentima 8 months ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima 8 months ago

Updates at 7716328 - branch 15642-cluster-config-fix
Test run: https://ci.curoverse.com/job/developer-run-tests/1642/

Bugs fixed, with tests. Ready for review.

#8 Updated by Eric Biagiotti 8 months ago

Lucas Di Pentima wrote:

Updates at 7716328 - branch 15642-cluster-config-fix
Test run: https://ci.curoverse.com/job/developer-run-tests/1642/

Bugs fixed, with tests. Ready for review.

This LGTM, thanks!

#9 Updated by Lucas Di Pentima 8 months ago

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

Also available in: Atom PDF