From b7e1d12d3f0e22280b9aeb23ca445a86a5ed6b7c Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Mon, 23 Aug 2021 13:08:35 -0300 Subject: [PATCH] 17696: Adds default storage classes config loading to KeepClient, with tests. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- sdk/go/keepclient/keepclient.go | 45 ++++++++---- sdk/go/keepclient/keepclient_test.go | 101 ++++++++++++++++++--------- sdk/go/keepclient/support.go | 6 +- 3 files changed, 106 insertions(+), 46 deletions(-) diff --git a/sdk/go/keepclient/keepclient.go b/sdk/go/keepclient/keepclient.go index 3bc6f4afcd..4baebbd8a1 100644 --- a/sdk/go/keepclient/keepclient.go +++ b/sdk/go/keepclient/keepclient.go @@ -97,17 +97,18 @@ type HTTPClient interface { // KeepClient holds information about Arvados and Keep servers. type KeepClient struct { - Arvados *arvadosclient.ArvadosClient - Want_replicas int - localRoots map[string]string - writableLocalRoots map[string]string - gatewayRoots map[string]string - lock sync.RWMutex - HTTPClient HTTPClient - Retries int - BlockCache *BlockCache - RequestID string - StorageClasses []string + Arvados *arvadosclient.ArvadosClient + Want_replicas int + localRoots map[string]string + writableLocalRoots map[string]string + gatewayRoots map[string]string + lock sync.RWMutex + HTTPClient HTTPClient + Retries int + BlockCache *BlockCache + RequestID string + StorageClasses []string + DefaultStorageClasses []string // Set by cluster's exported config // set to 1 if all writable services are of disk type, otherwise 0 replicasPerService int @@ -119,11 +120,31 @@ type KeepClient struct { disableDiscovery bool } -// MakeKeepClient creates a new KeepClient, calls +func (kc *KeepClient) loadDefaultClasses() error { + scData, err := kc.Arvados.ClusterConfig("StorageClasses") + if err != nil { + return err + } + classes := scData.(map[string]interface{}) + for scName := range classes { + scConf, _ := classes[scName].(map[string]interface{}) + isDefault, ok := scConf["Default"].(bool) + if ok && isDefault { + kc.DefaultStorageClasses = append(kc.DefaultStorageClasses, scName) + } + } + return nil +} + +// MakeKeepClient creates a new KeepClient, loads default storage classes, calls // DiscoverKeepServices(), and returns when the client is ready to // use. func MakeKeepClient(arv *arvadosclient.ArvadosClient) (*KeepClient, error) { kc := New(arv) + err := kc.loadDefaultClasses() + if err != nil { + DebugPrintf("DEBUG: Unable to load the default storage classes cluster config") + } return kc, kc.discoverServices() } diff --git a/sdk/go/keepclient/keepclient_test.go b/sdk/go/keepclient/keepclient_test.go index cddf03bc37..ed2cee6451 100644 --- a/sdk/go/keepclient/keepclient_test.go +++ b/sdk/go/keepclient/keepclient_test.go @@ -24,7 +24,6 @@ import ( "git.arvados.org/arvados.git/sdk/go/arvadosclient" "git.arvados.org/arvados.git/sdk/go/arvadostest" . "gopkg.in/check.v1" - check "gopkg.in/check.v1" ) // Gocheck boilerplate @@ -76,9 +75,23 @@ func (s *ServerRequiredSuite) TestMakeKeepClient(c *C) { } } +func (s *ServerRequiredSuite) TestDefaultStorageClasses(c *C) { + arv, err := arvadosclient.MakeArvadosClient() + c.Assert(err, IsNil) + + cc, err := arv.ClusterConfig("StorageClasses") + c.Assert(err, IsNil) + c.Assert(cc, NotNil) + c.Assert(cc.(map[string]interface{})["default"], NotNil) + + kc, err := MakeKeepClient(arv) + c.Assert(err, IsNil) + c.Assert(kc.DefaultStorageClasses, DeepEquals, []string{"default"}) +} + func (s *ServerRequiredSuite) TestDefaultReplications(c *C) { arv, err := arvadosclient.MakeArvadosClient() - c.Assert(err, Equals, nil) + c.Assert(err, IsNil) kc, err := MakeKeepClient(arv) c.Check(err, IsNil) @@ -133,7 +146,7 @@ func RunFakeKeepServer(st http.Handler) (ks KeepServer) { // bind to 0.0.0.0 or [::] which is not a valid address for Dial() ks.listener, err = net.ListenTCP("tcp", &net.TCPAddr{IP: []byte{127, 0, 0, 1}, Port: 0}) if err != nil { - panic(fmt.Sprintf("Could not listen on any port")) + panic("Could not listen on any port") } ks.url = fmt.Sprintf("http://%s", ks.listener.Addr().String()) go http.Serve(ks.listener, st) @@ -243,25 +256,46 @@ func (s *StandaloneSuite) TestUploadWithStorageClasses(c *C) { func (s *StandaloneSuite) TestPutWithStorageClasses(c *C) { nServers := 5 for _, trial := range []struct { - replicas int - clientClasses []string - putClasses []string // putClasses takes precedence over clientClasses - minRequests int - maxRequests int - success bool + replicas int + defaultClasses []string + clientClasses []string // clientClasses takes precedence over defaultClasses + putClasses []string // putClasses takes precedence over clientClasses + minRequests int + maxRequests int + success bool }{ - {1, []string{"class1"}, nil, 1, 1, true}, - {2, []string{"class1"}, nil, 1, 2, true}, - {3, []string{"class1"}, nil, 2, 3, true}, - {1, []string{"class1", "class2"}, nil, 1, 1, true}, - {3, nil, []string{"class1"}, 2, 3, true}, - {1, nil, []string{"class1", "class2"}, 1, 1, true}, - {1, []string{"class404"}, []string{"class1", "class2"}, 1, 1, true}, - {1, []string{"class1"}, []string{"class404", "class2"}, nServers, nServers, false}, - {nServers*2 + 1, []string{"class1"}, nil, nServers, nServers, false}, - {1, []string{"class404"}, nil, nServers, nServers, false}, - {1, []string{"class1", "class404"}, nil, nServers, nServers, false}, - {1, nil, []string{"class1", "class404"}, nServers, nServers, false}, + {1, []string{"class1"}, nil, nil, 1, 1, true}, + {2, []string{"class1"}, nil, nil, 1, 2, true}, + {3, []string{"class1"}, nil, nil, 2, 3, true}, + {1, []string{"class1", "class2"}, nil, nil, 1, 1, true}, + + // defaultClasses doesn't matter when any of the others is specified. + {1, []string{"class1"}, []string{"class1"}, nil, 1, 1, true}, + {2, []string{"class1"}, []string{"class1"}, nil, 1, 2, true}, + {3, []string{"class1"}, []string{"class1"}, nil, 2, 3, true}, + {1, []string{"class1"}, []string{"class1", "class2"}, nil, 1, 1, true}, + {3, []string{"class1"}, nil, []string{"class1"}, 2, 3, true}, + {1, []string{"class1"}, nil, []string{"class1", "class2"}, 1, 1, true}, + {1, []string{"class1"}, []string{"class404"}, []string{"class1", "class2"}, 1, 1, true}, + {1, []string{"class1"}, []string{"class1"}, []string{"class404", "class2"}, nServers, nServers, false}, + {nServers*2 + 1, []string{}, []string{"class1"}, nil, nServers, nServers, false}, + {1, []string{"class1"}, []string{"class404"}, nil, nServers, nServers, false}, + {1, []string{"class1"}, []string{"class1", "class404"}, nil, nServers, nServers, false}, + {1, []string{"class1"}, nil, []string{"class1", "class404"}, nServers, nServers, false}, + + // Talking to an older cluster (with no default storage classes advertising) + {1, nil, []string{"class1"}, nil, 1, 1, true}, + {2, nil, []string{"class1"}, nil, 1, 2, true}, + {3, nil, []string{"class1"}, nil, 2, 3, true}, + {1, nil, []string{"class1", "class2"}, nil, 1, 1, true}, + {3, nil, nil, []string{"class1"}, 2, 3, true}, + {1, nil, nil, []string{"class1", "class2"}, 1, 1, true}, + {1, nil, []string{"class404"}, []string{"class1", "class2"}, 1, 1, true}, + {1, nil, []string{"class1"}, []string{"class404", "class2"}, nServers, nServers, false}, + {nServers*2 + 1, []string{}, []string{"class1"}, nil, nServers, nServers, false}, + {1, nil, []string{"class404"}, nil, nServers, nServers, false}, + {1, nil, []string{"class1", "class404"}, nil, nServers, nServers, false}, + {1, nil, nil, []string{"class1", "class404"}, nServers, nServers, false}, } { c.Logf("%+v", trial) st := &StubPutHandler{ @@ -278,6 +312,7 @@ func (s *StandaloneSuite) TestPutWithStorageClasses(c *C) { kc, _ := MakeKeepClient(arv) kc.Want_replicas = trial.replicas kc.StorageClasses = trial.clientClasses + kc.DefaultStorageClasses = trial.defaultClasses arv.ApiToken = "abc123" localRoots := make(map[string]string) writableLocalRoots := make(map[string]string) @@ -293,17 +328,17 @@ func (s *StandaloneSuite) TestPutWithStorageClasses(c *C) { StorageClasses: trial.putClasses, }) if trial.success { - c.Check(err, check.IsNil) + c.Check(err, IsNil) } else { - c.Check(err, check.NotNil) + c.Check(err, NotNil) } - c.Check(len(st.handled) >= trial.minRequests, check.Equals, true, check.Commentf("len(st.handled)==%d, trial.minRequests==%d", len(st.handled), trial.minRequests)) - c.Check(len(st.handled) <= trial.maxRequests, check.Equals, true, check.Commentf("len(st.handled)==%d, trial.maxRequests==%d", len(st.handled), trial.maxRequests)) - if !trial.success && trial.replicas == 1 && c.Check(len(st.requests) >= 2, check.Equals, true) { + c.Check(len(st.handled) >= trial.minRequests, Equals, true, Commentf("len(st.handled)==%d, trial.minRequests==%d", len(st.handled), trial.minRequests)) + c.Check(len(st.handled) <= trial.maxRequests, Equals, true, Commentf("len(st.handled)==%d, trial.maxRequests==%d", len(st.handled), trial.maxRequests)) + if !trial.success && trial.replicas == 1 && c.Check(len(st.requests) >= 2, Equals, true) { // Max concurrency should be 1. First request // should have succeeded for class1. Second // request should only ask for class404. - c.Check(st.requests[1].Header.Get("X-Keep-Storage-Classes"), check.Equals, "class404") + c.Check(st.requests[1].Header.Get("X-Keep-Storage-Classes"), Equals, "class404") } } } @@ -390,7 +425,7 @@ func (s *StandaloneSuite) TestPutB(c *C) { expectPath: hash, expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 5), } @@ -434,7 +469,7 @@ func (s *StandaloneSuite) TestPutHR(c *C) { expectPath: hash, expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 5), } @@ -485,7 +520,7 @@ func (s *StandaloneSuite) TestPutWithFail(c *C) { expectPath: hash, expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 4), } @@ -547,7 +582,7 @@ func (s *StandaloneSuite) TestPutWithTooManyFail(c *C) { expectPath: hash, expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 1), } @@ -1157,7 +1192,7 @@ func (s *StandaloneSuite) TestPutBWant2ReplicasWithOnlyOneWritableLocalRoot(c *C expectPath: hash, expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 5), } @@ -1376,7 +1411,7 @@ func (s *StandaloneSuite) TestPutBRetry(c *C) { expectPath: Md5String("foo"), expectAPIToken: "abc123", expectBody: "foo", - expectStorageClass: "", + expectStorageClass: "default", returnStorageClasses: "", handled: make(chan string, 5), }, diff --git a/sdk/go/keepclient/support.go b/sdk/go/keepclient/support.go index 633ec18968..8d299815b2 100644 --- a/sdk/go/keepclient/support.go +++ b/sdk/go/keepclient/support.go @@ -164,7 +164,11 @@ func (kc *KeepClient) BlockWrite(ctx context.Context, req arvados.BlockWriteOpti req.Hash = fmt.Sprintf("%x", m.Sum(nil)) } if req.StorageClasses == nil { - req.StorageClasses = kc.StorageClasses + if len(kc.StorageClasses) > 0 { + req.StorageClasses = kc.StorageClasses + } else { + req.StorageClasses = kc.DefaultStorageClasses + } } if req.Replicas == 0 { req.Replicas = kc.Want_replicas -- 2.30.2