Feature #13647

[keepstore] Load cluster configuration file

Added by Tom Clegg about 1 year ago. Updated 5 days ago.

Status:
In Progress
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/11/2019
Due date:
% Done:

50%

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

Subtasks

Task #15440: ReviewNewLucas Di Pentima

Task #15462: Review 13647-load-old-configResolvedPeter Amstutz


Related issues

Related to Arvados - Story #13648: [Epic] Use one cluster configuration file for all componentsNew

Associated revisions

Revision 7f2569f2
Added by Tom Clegg 10 days ago

Merge branch '13647-load-old-config'

refs #13647

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Clegg about 1 year ago

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

#2 Updated by Tom Clegg about 1 year ago

  • Tracker changed from Story to Feature

#3 Updated by Tom Morris 7 months ago

  • Target version set to To Be Groomed

#4 Updated by Tom Morris 6 months ago

  • Target version changed from To Be Groomed to Arvados Future Sprints
  • Story points set to 2.0

#5 Updated by Tom Morris about 1 month ago

  • Target version changed from Arvados Future Sprints to 2019-07-03 Sprint

#6 Updated by Tom Morris about 1 month ago

  • Target version changed from 2019-07-03 Sprint to Arvados Future Sprints

#7 Updated by Tom Clegg 19 days ago

  • Target version changed from Arvados Future Sprints to 2019-07-17 Sprint
  • Assigned To set to Tom Clegg

#8 Updated by Tom Clegg 12 days ago

13647-load-old-config @ 0c894574ca46b77209127a4908844c2e0e734cea -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1384/

rearranges some things:
  • new Loader type, with SetupFlags() method used by all commands, so these will all use the overrides from the specified keepstore config
    • "arvados-server config-check -legacy-keepstore-config /path/to/keepstore.yml"
    • "arvados-server controller -legacy-keepstore-config /path/to/keepstore.yml"
  • change "version" command so "arvados-server controller -version" and "arvados-controller -version" work

The minimal example (which we should be able to expand to other components + the rest of the keepstore configs after merging this) loads /etc/arvados/keepstore/keepstore.yml (or the specified location) if it exists, and uses those values to override the cluster config. The "apply keepstore config" func doesn't need to emit warnings about overrides, but it should use ldr.Logger to warn about configs that are no longer supported, or can't be automatically migrated.

I think we can address auto-detection for ambiguous -config args in components that used to have their own -config format (e.g., "keepstore -config foo.yml") like this:
  • first try to load foo.yml as a cluster config file (the way source:lib/service/cmd.go does it)
  • if err == config.ErrNoClustersDefined then
    • log a warning that we're guessing the config is a legacy keepstore config
    • use flagset.Visit() to make a new args slice, changing "-config" to "-legacy-keepstore-config" and leaving everything else the same
    • replace the flagset and loader with new ones, and redo flag parsing & config loading using the munged args

#9 Updated by Peter Amstutz 11 days ago

While you're tinkering with this, could you add an ARVADOS_CONFIG environment variable to control where it looks for config.yml? This would be helpful for testing.

#11 Updated by Peter Amstutz 11 days ago

config/cmd.go checkCommand.RunCommand():

    loader.SkipDeprecated = true
    withoutDepr, err := loader.Load()
    if err != nil {
        return 1
    }
    loader.SkipDeprecated = false
    withDepr, err := loader.Load()
    if err != nil {
        return 1
    }

It isn't totally obvious what this does. Needs some comments. What does "SkipDeprecated" mean? Suppress warnings, don't load them at all, or something else?

service/cmd.go command.RunCommand

    loader := config.NewLoader(stdin, log)
    loader.SetupFlags(flags)

NewLoader() calls ldr.SetupFlags() a NewFlagSet() that seems like a throwaway object? What's the purpose? Should NewLoader() take flags as a parameter?

I think we can address auto-detection for ambiguous -config args in components that used to have their own -config format (e.g., "keepstore -config foo.yml") like this:
  • first try to load foo.yml as a cluster config file (the way source:lib/service/cmd.go does it)
  • if err == config.ErrNoClustersDefined then
    • log a warning that we're guessing the config is a legacy keepstore config
    • use flagset.Visit() to make a new args slice, changing "-config" to "-legacy-keepstore-config" and leaving everything else the same
    • replace the flagset and loader with new ones, and redo flag parsing & config loading using the munged args

Seems reasonable. Could this try-and-try-again behavior be done in a generic way instead of being duplicated across every component? It really just needs to know what the fallback flag is, right? Maybe even an option to NewLoader() and put the behavior in Load() ?

So if I'm following along, what I should be doing for crunch-dispatch-slurm is:

  • Update crunch-dispatch-slurm to use the main config
  • Implement loadOldCrunchDispatchSlurmConfig() and add it in the right place
  • Add -legacy-crunch-dispatch-slurm-config option
  • In the main method, create a Loader and call loader.Load() to get the config object

Anything else?

#12 Updated by Tom Clegg 11 days ago

Peter Amstutz wrote:

config/cmd.go checkCommand.RunCommand():
It isn't totally obvious what this does. Needs some comments. What does "SkipDeprecated" mean? Suppress warnings, don't load them at all, or something else?

It means don't load them at all.

Added comments to the SkipDeprecated field and the "load twice" code block.

NewLoader() calls ldr.SetupFlags() a NewFlagSet() that seems like a throwaway object? What's the purpose? Should NewLoader() take flags as a parameter?

This is how the config library gets to define the config-related cli flags, defaults, etc. Two lines later, flags.Parse(args) -- now that the loader has registered its CLI flags, parse the arguments we've been given. So if we are called as "arvados-server config-dump -legacy-keepstore-config /foo" then the loader's KeepstorePath changes to "/foo" when we call flags.Parse(args).

Seems reasonable. Could this try-and-try-again behavior be done in a generic way instead of being duplicated across every component? It really just needs to know what the fallback flag is, right? Maybe even an option to NewLoader() and put the behavior in Load() ?

Yes, this should surely be in the config package, and the component would just do this

args, err := loader.MungeLegacyConfigArgs(args, "-legacy-keepstore-config")

It might be annoying to make this work with stuff like "-config -". I propose we just don't support loading old configs that way, i.e., don't try to munge anything if the -config arg isn't a [symlink to a] regular file.

  • Update crunch-dispatch-slurm to use the main config
  • Implement loadOldCrunchDispatchSlurmConfig() and add it in the right place
  • Add -legacy-crunch-dispatch-slurm-config option
  • In the main method, create a Loader and call loader.Load() to get the config object

Sounds right.

#13 Updated by Tom Clegg 11 days ago

13647-load-old-config @ 3310da050bd763055a67b2395f122ab5c01cbdf9 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1389/
  • adds MungeLegacyConfigArgs method to loader

#14 Updated by Peter Amstutz 11 days ago

Tom Clegg wrote:

13647-load-old-config @ 3310da050bd763055a67b2395f122ab5c01cbdf9 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1389/
  • adds MungeLegacyConfigArgs method to loader

This LGTM.

#15 Updated by Peter Amstutz 10 days ago

Tom Clegg wrote:

Peter Amstutz wrote:
NewLoader() calls ldr.SetupFlags() a NewFlagSet() that seems like a throwaway object? What's the purpose? Should NewLoader() take flags as a parameter?

This is how the config library gets to define the config-related cli flags, defaults, etc. Two lines later, flags.Parse(args) -- now that the loader has registered its CLI flags, parse the arguments we've been given. So if we are called as "arvados-server config-dump -legacy-keepstore-config /foo" then the loader's KeepstorePath changes to "/foo" when we call flags.Parse(args).

func NewLoader(stdin io.Reader, logger logrus.FieldLogger) *Loader {
    ldr := &Loader{Stdin: stdin, Logger: logger}
    ldr.SetupFlags(flag.NewFlagSet("", flag.ContinueOnError)) // what is this line for ???
    ldr.Path = "-" 
    return ldr
}
func (c *command) RunCommand(prog string, args []string, stdin io.Reader, stdout, stderr io.Writer) int {
        ...

    loader := config.NewLoader(stdin, log)
    loader.SetupFlags(flags)
    versionFlag := flags.Bool("version", false, "Write version information to stdout and exit 0")

    err = flags.Parse(args)

        ...
}

I still don't understand how the flag.NewFlagSet() that is passed to ldr.SetupFlags() in NewLoader() ever gets used for anything? The RunCommmand() method calls SetupFlags() on a different flag set that is actually used for Parse().

#16 Updated by Tom Clegg 10 days ago

I still don't understand how the flag.NewFlagSet() that is passed to ldr.SetupFlags() in NewLoader() ever gets used for anything? The RunCommmand() method calls SetupFlags() on a different flag set that is actually used for Parse().

Ooooh, sorry. I thought you were asking about the flagset in RunCommand.

The flagset in NewLoader() is indeed a throwaway. The only reason to call SetupFlags() there is to get side effect of setting the default values. I've added a comment to explain that.

The next line (ldr.Path = "-") is a bug left over from testing, though -- I've deleted that, and fixed the tests to do that themselves.

13647-load-old-config @ e723b02bc8e5382575f27b0ce803f775adf6d479 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1390/

#17 Updated by Peter Amstutz 10 days ago

Tom Clegg wrote:

I still don't understand how the flag.NewFlagSet() that is passed to ldr.SetupFlags() in NewLoader() ever gets used for anything? The RunCommmand() method calls SetupFlags() on a different flag set that is actually used for Parse().

Ooooh, sorry. I thought you were asking about the flagset in RunCommand.

The flagset in NewLoader() is indeed a throwaway. The only reason to call SetupFlags() there is to get side effect of setting the default values. I've added a comment to explain that.

The next line (ldr.Path = "-") is a bug left over from testing, though -- I've deleted that, and fixed the tests to do that themselves.

13647-load-old-config @ e723b02bc8e5382575f27b0ce803f775adf6d479 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1390/

Great. This LGTM.

#18 Updated by Tom Morris 5 days ago

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

#19 Updated by Peter Amstutz 5 days ago

  • Status changed from New to In Progress

Also available in: Atom PDF