From 444dfb847a5d6eda3a84ef5f4e508703d0634a91 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Wed, 4 Aug 2021 16:57:35 -0400 Subject: [PATCH] 17967: Add StorageClasses config section. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/config/config.default.yml | 24 ++++++- lib/config/export.go | 4 ++ lib/config/generated_config.go | 24 ++++++- lib/config/load.go | 53 +++++++++++++++ lib/config/load_test.go | 121 +++++++++++++++++++++++++++++++-- sdk/go/arvados/config.go | 10 ++- 6 files changed, 228 insertions(+), 8 deletions(-) diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 66f508b5ad..eb05c22fd7 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -1228,6 +1228,26 @@ Clusters: Price: 0.1 Preemptible: false + StorageClasses: + + # If you use multiple storage classes, specify them here, using + # the storage class name as the key (in place of "SAMPLE" in + # this sample entry). + SAMPLE: + + # Priority determines the order volumes should be searched + # when reading data, in cases where a keepstore server has + # access to multiple volumes with different storage classes. + Priority: 0 + + # Default determines which storage class(es) should be used + # when a user/client writes data or saves a new collection + # without specifying storage classes. + # + # If any StorageClasses are configured, at least one of them + # must have Default: true. + Default: true + Volumes: SAMPLE: # AccessViaHosts specifies which keepstore processes can read @@ -1251,7 +1271,9 @@ Clusters: ReadOnly: false Replication: 1 StorageClasses: - default: true + # If you have configured storage classes (see StorageClasses + # section above), add an entry here for each storage class + # satisfied by this volume. SAMPLE: true Driver: S3 DriverParameters: diff --git a/lib/config/export.go b/lib/config/export.go index bbc5ea6c55..2a3d0e173a 100644 --- a/lib/config/export.go +++ b/lib/config/export.go @@ -205,6 +205,10 @@ var whitelist = map[string]bool{ "Services.*": true, "Services.*.ExternalURL": true, "Services.*.InternalURLs": false, + "StorageClasses": true, + "StorageClasses.*": true, + "StorageClasses.*.Default": true, + "StorageClasses.*.Priority": true, "SystemLogs": false, "SystemRootToken": false, "TLS": false, diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go index ee23084135..6fe0c73c59 100644 --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@ -1234,6 +1234,26 @@ Clusters: Price: 0.1 Preemptible: false + StorageClasses: + + # If you use multiple storage classes, specify them here, using + # the storage class name as the key (in place of "SAMPLE" in + # this sample entry). + SAMPLE: + + # Priority determines the order volumes should be searched + # when reading data, in cases where a keepstore server has + # access to multiple volumes with different storage classes. + Priority: 0 + + # Default determines which storage class(es) should be used + # when a user/client writes data or saves a new collection + # without specifying storage classes. + # + # If any StorageClasses are configured, at least one of them + # must have Default: true. + Default: true + Volumes: SAMPLE: # AccessViaHosts specifies which keepstore processes can read @@ -1257,7 +1277,9 @@ Clusters: ReadOnly: false Replication: 1 StorageClasses: - default: true + # If you have configured storage classes (see StorageClasses + # section above), add an entry here for each storage class + # satisfied by this volume. SAMPLE: true Driver: S3 DriverParameters: diff --git a/lib/config/load.go b/lib/config/load.go index 169b252a0e..248960beb9 100644 --- a/lib/config/load.go +++ b/lib/config/load.go @@ -269,6 +269,7 @@ func (ldr *Loader) Load() (*arvados.Config, error) { ldr.loadOldKeepBalanceConfig, ) } + loadFuncs = append(loadFuncs, ldr.setImplicitStorageClasses) for _, f := range loadFuncs { err = f(&cfg) if err != nil { @@ -296,6 +297,7 @@ func (ldr *Loader) Load() (*arvados.Config, error) { checkKeyConflict(fmt.Sprintf("Clusters.%s.PostgreSQL.Connection", id), cc.PostgreSQL.Connection), ldr.checkEmptyKeepstores(cc), ldr.checkUnlistedKeepstores(cc), + ldr.checkStorageClasses(cc), // TODO: check non-empty Rendezvous on // services other than Keepstore } { @@ -336,6 +338,57 @@ func (ldr *Loader) checkToken(label, token string) error { return nil } +func (ldr *Loader) setImplicitStorageClasses(cfg *arvados.Config) error { +cluster: + for id, cc := range cfg.Clusters { + if len(cc.StorageClasses) > 0 { + continue cluster + } + for _, vol := range cc.Volumes { + if len(vol.StorageClasses) > 0 { + continue cluster + } + } + // No explicit StorageClasses config info at all; fill + // in implicit defaults. + for id, vol := range cc.Volumes { + vol.StorageClasses = map[string]bool{"default": true} + cc.Volumes[id] = vol + } + cc.StorageClasses = map[string]arvados.StorageClassConfig{"default": {Default: true}} + cfg.Clusters[id] = cc + } + return nil +} + +func (ldr *Loader) checkStorageClasses(cc arvados.Cluster) error { + classOnVolume := map[string]bool{} + for volid, vol := range cc.Volumes { + if len(vol.StorageClasses) == 0 { + return fmt.Errorf("%s: volume has no StorageClasses listed", volid) + } + for classid := range vol.StorageClasses { + if _, ok := cc.StorageClasses[classid]; !ok { + return fmt.Errorf("%s: volume refers to storage class %q that is not defined in StorageClasses", volid, classid) + } + classOnVolume[classid] = true + } + } + haveDefault := false + for classid, sc := range cc.StorageClasses { + if !classOnVolume[classid] && len(cc.Volumes) > 0 { + ldr.Logger.Warnf("there are no volumes providing storage class %q", classid) + } + if sc.Default { + haveDefault = true + } + } + if !haveDefault { + return fmt.Errorf("there is no default storage class (at least one entry in StorageClasses must have Default: true)") + } + return nil +} + func checkKeyConflict(label string, m map[string]string) error { saw := map[string]bool{} for k := range m { diff --git a/lib/config/load_test.go b/lib/config/load_test.go index 396faca484..d4896c39cb 100644 --- a/lib/config/load_test.go +++ b/lib/config/load_test.go @@ -29,6 +29,8 @@ func Test(t *testing.T) { var _ = check.Suite(&LoadSuite{}) +var emptyConfigYAML = `Clusters: {"z1111": {}}` + // Return a new Loader that reads cluster config from configdata // (instead of the usual default /etc/arvados/config.yml), and logs to // logdst or (if that's nil) c.Log. @@ -59,7 +61,7 @@ func (s *LoadSuite) TestEmpty(c *check.C) { } func (s *LoadSuite) TestNoConfigs(c *check.C) { - cfg, err := testLoader(c, `Clusters: {"z1111": {}}`, nil).Load() + cfg, err := testLoader(c, emptyConfigYAML, nil).Load() c.Assert(err, check.IsNil) c.Assert(cfg.Clusters, check.HasLen, 1) cc, err := cfg.GetCluster("z1111") @@ -79,7 +81,7 @@ func (s *LoadSuite) TestMungeLegacyConfigArgs(c *check.C) { f, err = ioutil.TempFile("", "") c.Check(err, check.IsNil) defer os.Remove(f.Name()) - io.WriteString(f, "Clusters: {aaaaa: {}}\n") + io.WriteString(f, emptyConfigYAML) newfile := f.Name() for _, trial := range []struct { @@ -562,11 +564,122 @@ func (s *LoadSuite) TestListKeys(c *check.C) { c.Errorf("Should have produced an error") } - var logbuf bytes.Buffer - loader := testLoader(c, string(DefaultYAML), &logbuf) + loader := testLoader(c, string(DefaultYAML), nil) cfg, err := loader.Load() c.Assert(err, check.IsNil) if err := checkListKeys("", cfg); err != nil { c.Error(err) } } + +func (s *LoadSuite) TestImplicitStorageClasses(c *check.C) { + // If StorageClasses and Volumes.*.StorageClasses are all + // empty, there is a default storage class named "default". + ldr := testLoader(c, `{"Clusters":{"z1111":{}}}`, nil) + cfg, err := ldr.Load() + c.Assert(err, check.IsNil) + cc, err := cfg.GetCluster("z1111") + c.Assert(err, check.IsNil) + c.Check(cc.StorageClasses, check.HasLen, 1) + c.Check(cc.StorageClasses["default"].Default, check.Equals, true) + c.Check(cc.StorageClasses["default"].Priority, check.Equals, 0) + + // The implicit "default" storage class is used by all + // volumes. + ldr = testLoader(c, ` +Clusters: + z1111: + Volumes: + z: {}`, nil) + cfg, err = ldr.Load() + c.Assert(err, check.IsNil) + cc, err = cfg.GetCluster("z1111") + c.Assert(err, check.IsNil) + c.Check(cc.StorageClasses, check.HasLen, 1) + c.Check(cc.StorageClasses["default"].Default, check.Equals, true) + c.Check(cc.StorageClasses["default"].Priority, check.Equals, 0) + c.Check(cc.Volumes["z"].StorageClasses["default"], check.Equals, true) + + // The "default" storage class isn't implicit if any classes + // are configured explicitly. + ldr = testLoader(c, ` +Clusters: + z1111: + StorageClasses: + local: + Default: true + Priority: 111 + Volumes: + z: + StorageClasses: + local: true`, nil) + cfg, err = ldr.Load() + c.Assert(err, check.IsNil) + cc, err = cfg.GetCluster("z1111") + c.Assert(err, check.IsNil) + c.Check(cc.StorageClasses, check.HasLen, 1) + c.Check(cc.StorageClasses["local"].Default, check.Equals, true) + c.Check(cc.StorageClasses["local"].Priority, check.Equals, 111) + + // It is an error for a volume to refer to a storage class + // that isn't listed in StorageClasses. + ldr = testLoader(c, ` +Clusters: + z1111: + StorageClasses: + local: + Default: true + Priority: 111 + Volumes: + z: + StorageClasses: + nx: true`, nil) + _, err = ldr.Load() + c.Assert(err, check.ErrorMatches, `z: volume refers to storage class "nx" that is not defined.*`) + + // It is an error for a volume to refer to a storage class + // that isn't listed in StorageClasses ... even if it's + // "default", which would exist implicitly if it weren't + // referenced explicitly by a volume. + ldr = testLoader(c, ` +Clusters: + z1111: + Volumes: + z: + StorageClasses: + default: true`, nil) + _, err = ldr.Load() + c.Assert(err, check.ErrorMatches, `z: volume refers to storage class "default" that is not defined.*`) + + // If the "default" storage class is configured explicitly, it + // is not used implicitly by any volumes, even if it's the + // only storage class. + var logbuf bytes.Buffer + ldr = testLoader(c, ` +Clusters: + z1111: + StorageClasses: + default: + Default: true + Priority: 111 + Volumes: + z: {}`, &logbuf) + _, err = ldr.Load() + c.Assert(err, check.ErrorMatches, `z: volume has no StorageClasses listed`) + + // If StorageClasses are configured explicitly, there must be + // at least one with Default: true. (Calling one "default" is + // not sufficient.) + ldr = testLoader(c, ` +Clusters: + z1111: + StorageClasses: + default: + Priority: 111 + Volumes: + z: + StorageClasses: + default: true`, nil) + _, err = ldr.Load() + c.Assert(err, check.ErrorMatches, `there is no default storage class.*`) +} diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index 9e7eb521ee..cc1de1be42 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -238,8 +238,9 @@ type Cluster struct { PreferDomainForUsername string UserSetupMailText string } - Volumes map[string]Volume - Workbench struct { + StorageClasses map[string]StorageClassConfig + Volumes map[string]Volume + Workbench struct { ActivationContactLink string APIClientConnectTimeout Duration APIClientReceiveTimeout Duration @@ -281,6 +282,11 @@ type Cluster struct { } } +type StorageClassConfig struct { + Default bool + Priority int +} + type Volume struct { AccessViaHosts map[URL]VolumeAccess ReadOnly bool -- 2.30.2