14713: Fix handling default/missing keys, can't load default file

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/lib/config/deprecated.go b/lib/config/deprecated.go
index 614f5dcf97767851f3a6ae90408ab2a4309e79d5..8f3f3d7edd4d8219230cd1f348370190d07a0b07 100644 (file)
--- a/lib/config/deprecated.go
+++ b/lib/config/deprecated.go
@@ -108,19 +108,17 @@ type oldKeepstoreConfig struct {
        Debug *bool
 }
 
-func (ldr *Loader) loadOldConfigHelper(component, path, defaultPath string, target interface{}) error {
+func (ldr *Loader) loadOldConfigHelper(component, path string, target interface{}) error {
        if path == "" {
                return nil
        }
        buf, err := ioutil.ReadFile(path)
-       if os.IsNotExist(err) && path == defaultPath {
-               return nil
-       } else if err != nil {
+       if err != nil {
                return err
-       } else {
-               ldr.Logger.Warnf("you should remove the legacy %v config file (%s) after migrating all config keys to the cluster configuration file (%s)", component, path, ldr.Path)
        }
 
+       ldr.Logger.Warnf("you should remove the legacy %v config file (%s) after migrating all config keys to the cluster configuration file (%s)", component, path, ldr.Path)
+
        err = yaml.Unmarshal(buf, target)
        if err != nil {
                return fmt.Errorf("%s: %s", path, err)
@@ -131,8 +129,10 @@ func (ldr *Loader) loadOldConfigHelper(component, path, defaultPath string, targ
 // update config using values from an old-style keepstore config file.
 func (ldr *Loader) loadOldKeepstoreConfig(cfg *arvados.Config) error {
        var oc oldKeepstoreConfig
-       err := ldr.loadOldConfigHelper("keepstore", ldr.KeepstorePath, defaultKeepstoreConfigPath, &oc)
-       if err != nil {
+       err := ldr.loadOldConfigHelper("keepstore", ldr.KeepstorePath, &oc)
+       if os.IsNotExist(err) && ldr.KeepstorePath == defaultKeepstoreConfigPath {
+               return nil
+       } else if err != nil {
                return err
        }
 
@@ -153,27 +153,27 @@ func (ldr *Loader) loadOldKeepstoreConfig(cfg *arvados.Config) error {
 }
 
 type oldCrunchDispatchSlurmConfig struct {
-       Client arvados.Client
+       Client *arvados.Client
 
-       SbatchArguments []string
-       PollPeriod      arvados.Duration
-       PrioritySpread  int64
+       SbatchArguments *[]string
+       PollPeriod      *arvados.Duration
+       PrioritySpread  *int64
 
        // crunch-run command to invoke. The container UUID will be
        // appended. If nil, []string{"crunch-run"} will be used.
        //
        // Example: []string{"crunch-run", "--cgroup-parent-subsystem=memory"}
-       CrunchRunCommand []string
+       CrunchRunCommand *[]string
 
        // Extra RAM to reserve (in Bytes) for SLURM job, in addition
        // to the amount specified in the container's RuntimeConstraints
-       ReserveExtraRAM int64
+       ReserveExtraRAM *int64
 
        // Minimum time between two attempts to run the same container
-       MinRetryPeriod arvados.Duration
+       MinRetryPeriod *arvados.Duration
 
        // Batch size for container queries
-       BatchSize int64
+       BatchSize *int64
 }
 
 const defaultCrunchDispatchSlurmConfigPath = "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml"
@@ -181,8 +181,10 @@ const defaultCrunchDispatchSlurmConfigPath = "/etc/arvados/crunch-dispatch-slurm
 // update config using values from an crunch-dispatch-slurm config file.
 func (ldr *Loader) loadOldCrunchDispatchSlurmConfig(cfg *arvados.Config) error {
        var oc oldCrunchDispatchSlurmConfig
-       err := ldr.loadOldConfigHelper("crunch-dispatch-slurm", ldr.CrunchDispatchSlurmPath, defaultCrunchDispatchSlurmConfigPath, &oc)
-       if err != nil {
+       err := ldr.loadOldConfigHelper("crunch-dispatch-slurm", ldr.CrunchDispatchSlurmPath, &oc)
+       if os.IsNotExist(err) && ldr.CrunchDispatchSlurmPath == defaultCrunchDispatchSlurmConfigPath {
+               return nil
+       } else if err != nil {
                return err
        }
 
@@ -191,30 +193,45 @@ func (ldr *Loader) loadOldCrunchDispatchSlurmConfig(cfg *arvados.Config) error {
                return err
        }
 
-       u := arvados.URL{}
-       u.Host = oc.Client.APIHost
-       if oc.Client.Scheme != "" {
-               u.Scheme = oc.Client.Scheme
-       } else {
-               u.Scheme = "https"
+       if oc.Client != nil {
+               u := arvados.URL{}
+               u.Host = oc.Client.APIHost
+               if oc.Client.Scheme != "" {
+                       u.Scheme = oc.Client.Scheme
+               } else {
+                       u.Scheme = "https"
+               }
+               cluster.Services.Controller.ExternalURL = u
+               cluster.SystemRootToken = oc.Client.AuthToken
+               cluster.TLS.Insecure = oc.Client.Insecure
        }
-       cluster.Services.Controller.ExternalURL = u
-       cluster.SystemRootToken = oc.Client.AuthToken
-       cluster.TLS.Insecure = oc.Client.Insecure
 
-       cluster.Containers.SLURM.SbatchArgumentsList = oc.SbatchArguments
-       cluster.Containers.CloudVMs.PollInterval = oc.PollPeriod
-       cluster.Containers.SLURM.PrioritySpread = oc.PrioritySpread
-       if len(oc.CrunchRunCommand) >= 1 {
-               cluster.Containers.CrunchRunCommand = oc.CrunchRunCommand[0]
+       if oc.SbatchArguments != nil {
+               cluster.Containers.SLURM.SbatchArgumentsList = *oc.SbatchArguments
        }
-       if len(oc.CrunchRunCommand) >= 2 {
-               cluster.Containers.CrunchRunArgumentsList = oc.CrunchRunCommand[1:]
+       if oc.PollPeriod != nil {
+               cluster.Containers.CloudVMs.PollInterval = *oc.PollPeriod
+       }
+       if oc.PrioritySpread != nil {
+               cluster.Containers.SLURM.PrioritySpread = *oc.PrioritySpread
+       }
+       if oc.CrunchRunCommand != nil {
+               if len(*oc.CrunchRunCommand) >= 1 {
+                       cluster.Containers.CrunchRunCommand = (*oc.CrunchRunCommand)[0]
+               }
+               if len(*oc.CrunchRunCommand) >= 2 {
+                       cluster.Containers.CrunchRunArgumentsList = (*oc.CrunchRunCommand)[1:]
+               }
+       }
+       if oc.ReserveExtraRAM != nil {
+               cluster.Containers.ReserveExtraRAM = arvados.ByteSize(*oc.ReserveExtraRAM)
+       }
+       if oc.MinRetryPeriod != nil {
+               cluster.Containers.MinRetryPeriod = *oc.MinRetryPeriod
+       }
+       if oc.BatchSize != nil {
+               cluster.API.MaxItemsPerResponse = int(*oc.BatchSize)
        }
-       cluster.Containers.ReserveExtraRAM = arvados.ByteSize(oc.ReserveExtraRAM)
-       cluster.Containers.MinRetryPeriod = oc.MinRetryPeriod
-
-       cluster.API.MaxItemsPerResponse = int(oc.BatchSize)
 
        cfg.Clusters[cluster.ClusterID] = *cluster
        return nil

diff --git a/lib/config/export.go b/lib/config/export.go
index a050492b3e422706961c5b0920c51c5f2835c49b..dbbaac127415da357b4628f41ed735f5278580bf 100644 (file)
--- a/lib/config/export.go
+++ b/lib/config/export.go
@@ -101,7 +101,7 @@ var whitelist = map[string]bool{
        "Containers.MaxDispatchAttempts":               false,
        "Containers.MaxRetryAttempts":                  true,
        "Containers.MinRetryPeriod":                    true,
-       "Containers.ReserveExtraRam":                   true,
+       "Containers.ReserveExtraRAM":                   true,
        "Containers.SLURM":                             false,
        "Containers.StaleLockTimeout":                  false,
        "Containers.SupportedDockerImageFormats":       true,

diff --git a/lib/config/load_test.go b/lib/config/load_test.go
index 340eb0a0a7e6900cc090bd170dea373a9100b664..fa04c007515c1cf155bdefff87be4b2daa30eee5 100644 (file)
--- a/lib/config/load_test.go
+++ b/lib/config/load_test.go
@@ -168,7 +168,9 @@ func (s *LoadSuite) TestSampleKeys(c *check.C) {
 }
 
 func (s *LoadSuite) TestMultipleClusters(c *check.C) {
-       cfg, err := testLoader(c, `{"Clusters":{"z1111":{},"z2222":{}}}`, nil).Load()
+       ldr := testLoader(c, `{"Clusters":{"z1111":{},"z2222":{}}}`, nil)
+       ldr.SkipDeprecated = true
+       cfg, err := ldr.Load()
        c.Assert(err, check.IsNil)
        c1, err := cfg.GetCluster("z1111")
        c.Assert(err, check.IsNil)