From 01e4baab04398fe4296afd4bd2316fc4332e1b22 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 10 Dec 2021 14:07:24 -0600 Subject: [PATCH 01/58] determine permission for user access to index --- auth/auth.go | 114 +++++++++++++++++++ auth/auth_test.go | 231 +++++++++++++++++++++++++++++++++++++++ ctl/server.go | 2 +- install/featurebase.conf | 3 +- server/config.go | 10 ++ 5 files changed, 358 insertions(+), 2 deletions(-) create mode 100644 auth/auth_test.go diff --git a/auth/auth.go b/auth/auth.go index 4e617c998..344055337 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -14,6 +14,15 @@ package auth +import ( + "fmt" + "io/ioutil" + "log" + "path/filepath" + + "gopkg.in/yaml.v2" +) + type Auth struct { // Enable AuthZ/AuthN for featurebase server Enable bool `toml:"enable"` @@ -35,4 +44,109 @@ type Auth struct { // Scope URL ScopeURL string `toml:"scope-url"` + + // Permissions file for groups + PermissionsFile string `toml:"permissions"` +} + +type GroupPermissions struct { + Permissions []Permissions `yaml:"group_permissions"` +} + +type Permissions struct { + GroupId string `yaml:"groupId"` + Index string `yaml:"index"` + Permission string `yaml:"permission"` +} + +func ReadPermissionsFile(filePath string) (yamlData []byte) { + filePathAbs, _ := filepath.Abs(filePath) + yamlData, err := ioutil.ReadFile(filePathAbs) + if err != nil { + panic(err) + } + return yamlData +} + +func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { + err := yaml.Unmarshal([]byte(data), &p) + if err != nil { + log.Fatalf("Error %s", err) + } +} + +func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) { + // read yaml permissions file + yamlData := ReadPermissionsFile(Auth.PermissionsFile) + + // get group permissions + var p GroupPermissions + p.CreatePermissionsStruct(yamlData) + + // check permissions for all groups and index, and return most permissive + return p.ResolvePermissions(groups, index) +} + +func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) { + + // get union of groups the user is part of obtained from identity provider and groups in permissions file + var groupMatch []Permissions + for _, group := range groups { + for i := range p.Permissions { + if group["id"] == p.Permissions[i].GroupId { + groupMatch = append(groupMatch, p.Permissions[i]) + } + } + } + + if len(groupMatch) == 0 { + return "", fmt.Errorf("User is NOT allowed access to FeatureBase") + } + + // check that user's groups have access to the index user want to access + var indexMatch []Permissions + indexCheck := map[string]bool{} + for _, g := range groupMatch { + for _, idx := range index { + if idx == g.Index { + indexMatch = append(indexMatch, g) + indexCheck[idx] = true + } + } + } + + // check that user has access to every index + indexCount := 0 + for _, value := range indexCheck { + if value { + indexCount += 1 + } + } + + if indexCount != len(index) { + return "", fmt.Errorf("User is not allowed access to index: %s", index) + } + + // check permissions for index user has access to + allPermissions := map[string]bool{ + "admin": false, + "write": false, + "read": false, + } + + for _, g := range indexMatch { + if !allPermissions[g.Permission] { + allPermissions[g.Permission] = true + } + } + + if allPermissions["admin"] { + return "admin", error(nil) + } else if allPermissions["write"] { + return "write", error(nil) + } else if allPermissions["read"] { + return "read", error(nil) + } else { + return "", fmt.Errorf("No permissions found") + } } diff --git a/auth/auth_test.go b/auth/auth_test.go new file mode 100644 index 000000000..ddd081e80 --- /dev/null +++ b/auth/auth_test.go @@ -0,0 +1,231 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package auth_test + +import ( + "fmt" + "reflect" + "strings" + "testing" + + "github.com/molecula/featurebase/v2/auth" +) + +func createStruct(inputs [][]string) (permissions auth.GroupPermissions) { + var sliceStruct []auth.Permissions + for _, i := range inputs { + groupId := i[0] + index := i[1] + permission := i[2] + p := auth.Permissions{groupId, index, permission} + sliceStruct = append(sliceStruct, p) + } + permissions = auth.GroupPermissions{Permissions: sliceStruct} + return permissions +} + +func TestAuth_CreatePermissionsStruct(t *testing.T) { + var singleInput = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read"`) + + var emptyInput = []byte(`group_permissions: + - group: + groupId: "" + index: "" + permission: ""`) + + var multiInput = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + var slice1 [][]string + var slice2 [][]string + var slice3 [][]string + var subslice1 []string + var subslice2 []string + var subslice3 []string + subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read") + subslice2 = append(subslice2, "", "", "") + subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin") + slice1 = append(slice1, subslice1) + slice2 = append(slice2, subslice2) + slice3 = append(slice3, subslice1, subslice3) + singleStruct := createStruct(slice1) + emptyStruct := createStruct(slice2) + multiStruct := createStruct(slice3) + + tests := []struct { + input []byte + output auth.GroupPermissions + }{ + {singleInput, singleStruct}, + {emptyInput, emptyStruct}, + {multiInput, multiStruct}, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + var p auth.GroupPermissions + p.CreatePermissionsStruct(test.input) + + if !reflect.DeepEqual(p, test.output) { + t.Fatalf("Expected output %s, but got %s", test.output, p) + } + }, + ) + } +} + +func createGroupMaps(groups []string) []map[string]string { + + var group1 []map[string]string + for _, i := range groups { + map1 := map[string]string{} + map1["id"] = i + group1 = append(group1, map1) + } + return group1 +} + +func TestAuth_ResolvePermissions(t *testing.T) { + // initializes different example of permissions file in yaml + var permissions1 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read"`) + + var permissions2 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "write"`) + + var permissions3 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + var permissions4 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + // initializes groups that are returned from identity provider + groupsList1 := []string{} + groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"} + groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"} + + tests := []struct { + permissions []byte + groups []map[string]string + index []string + userAccess string + err string + }{ + { + permissions1, + createGroupMaps(groupsList1), + []string{"test"}, + "", + "User is NOT allowed access to FeatureBase", + }, + { + permissions1, + createGroupMaps(groupsList2), + []string{"test1"}, + "", + "User is not allowed access to index", + }, + { + permissions1, + createGroupMaps(groupsList2), + []string{"test"}, + "read", + "", + }, + { + permissions2, + createGroupMaps(groupsList3), + []string{"test"}, + "write", + "", + }, + { + permissions3, + createGroupMaps(groupsList3), + []string{"test"}, + "admin", + "", + }, + { + permissions2, + createGroupMaps(groupsList3), + []string{"test"}, + "write", + "", + }, + { + permissions4, + createGroupMaps(groupsList2), + []string{"test"}, + "", + "No permissions found", + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + var p auth.GroupPermissions + p.CreatePermissionsStruct(test.permissions) + + p1, err := p.ResolvePermissions(test.groups, test.index) + + if p1 != test.userAccess { + t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1) + } + + if err != nil { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error()) + } + } + + }) + } +} diff --git a/ctl/server.go b/ctl/server.go index c5d43a937..06f7d1bb2 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -130,5 +130,5 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.") flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") flags.StringVar(&srv.Config.Auth.ScopeURL, "auth.scope-url", srv.Config.Auth.ScopeURL, "Identity Provider's Scope URL.") - + flags.StringVar(&srv.Config.Auth.PermissionsFile, "auth.permissions", srv.Config.Auth.PermissionsFile, "Permissions' file with group authorization.") } diff --git a/install/featurebase.conf b/install/featurebase.conf index 540a410f4..5b1e73345 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -380,4 +380,5 @@ log-path = "/var/log/molecula/featurebase.log" # authorize-url = "" # token-url = "" # group-endpoint-url = "" -# scope-url = "" \ No newline at end of file +# scope-url = "" +# permissions = "" \ No newline at end of file diff --git a/server/config.go b/server/config.go index 0c1989c7a..6f74d4f25 100644 --- a/server/config.go +++ b/server/config.go @@ -620,6 +620,7 @@ func (c *Config) ValidateAuth() ([]error, error) { "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, "ScopeURL": c.Auth.ScopeURL, + "PermissionsFile": c.Auth.PermissionsFile, } errors := make([]error, 0) @@ -636,6 +637,15 @@ func (c *Config) ValidateAuth() ([]error, error) { continue } } + + if strings.Contains(name, "File") { + yamlData := auth.ReadPermissionsFile(value) + var p auth.GroupPermissions + p.CreatePermissionsStruct(yamlData) + if len(p.Permissions) == 0 { + errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value)) + } + } } if len(errors) > 0 { return errors, fmt.Errorf("there were errors validating config") From 9e2cf81127a32e918453e920582ee02afa4984bc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 10:35:56 -0600 Subject: [PATCH 02/58] added unit tests --- server/config.go | 35 +++++++++++++++++-------- server/config_internal_test.go | 47 +++++++++++++++++++++++++++++----- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/server/config.go b/server/config.go index 52f798abf..1f3752577 100644 --- a/server/config.go +++ b/server/config.go @@ -7,6 +7,7 @@ import ( "log" "net" "net/url" + "path/filepath" "runtime" "strconv" "strings" @@ -613,37 +614,51 @@ func (c *Config) ValidateAuth() ([]error, error) { errors := make([]error, 0) for name, value := range authConfig { if value == "" { - errors = append(errors, fmt.Errorf("empty string for auth config %s", name)) + errors = append(errors, fmt.Errorf("Empty string for auth config %s", name)) continue } if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { - errors = append(errors, fmt.Errorf("invalid URL for auth config %s: %s", name, err)) + errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err)) continue } } if strings.Contains(name, "File") { - yamlData := auth.ReadPermissionsFile(value) - var p auth.GroupPermissions - p.CreatePermissionsStruct(yamlData) - if len(p.Permissions) == 0 { - errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value)) + fileExt := filepath.Ext(value) + if (fileExt != ".yaml") && (fileExt != ".yml") { + errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value)) + continue } } } + if len(errors) > 0 { - return errors, fmt.Errorf("there were errors validating config") + return errors, fmt.Errorf("There were errors validating config") } return errors, nil } +func (c *Config) ValidatePermissions() (err error) { + + yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile) + var p auth.GroupPermissions + p.CreatePermissionsStruct(yamlData) + if len(p.Permissions) == 0 { + return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile) + } + return nil +} + func (c *Config) MustValidateAuth() { if errors, err := c.ValidateAuth(); err != nil { - for _, e := range errors { - log.Println(e) + for _, e1 := range errors { + log.Println(e1) + } + if e2 := c.ValidatePermissions(); e2 != nil { + log.Println(e2) } log.Fatal(err) } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 7c762b23e..8003a1f2f 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -279,12 +279,15 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } func TestConfig_validateAuth(t *testing.T) { - errorMesgEmpty := "empty string" - errorMesgURL := "invalid URL" + errorMesgEmpty := "Empty string" + errorMesgURL := "Invalid URL" + errorMesgPermissions := "Invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" - notValidURL := "not-a-url" + validFilename := "permissions.yaml" + invalidFilename := "permissions.txt" + invalidURL := "not-a-url" emptyString := "" enable := true disable := false @@ -303,6 +306,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -312,6 +316,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -322,6 +327,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -331,6 +337,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -341,6 +348,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -350,6 +358,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -359,6 +368,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -368,6 +378,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -376,6 +387,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -385,6 +397,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -392,6 +405,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -401,6 +415,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -412,10 +427,11 @@ func TestConfig_validateAuth(t *testing.T) { Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, - AuthorizeURL: notValidURL, + AuthorizeURL: invalidURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, }, }, { @@ -429,9 +445,26 @@ func TestConfig_validateAuth(t *testing.T) { ClientId: validClientID, ClientSecret: validClientSecret, AuthorizeURL: validTestURL, - TokenURL: notValidURL, - GroupEndpointURL: notValidURL, + TokenURL: invalidURL, + GroupEndpointURL: invalidURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, + }, + }, + { + // Auth enabled, permissions file is set to invalid string + []string{ + errorMesgPermissions, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, + PermissionsFile: invalidFilename, }, }, { @@ -445,6 +478,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, }, }, { @@ -458,6 +492,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, } From 32228bb1344a0cc55aa33fdbb1d7ea74cdd259d0 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 10:48:40 -0600 Subject: [PATCH 03/58] updated go.mod --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index add6082e3..b66980688 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d // indirect golang.org/x/sync v0.0.0-20210220032951-036812b2e83c google.golang.org/grpc v1.28.0 - gopkg.in/yaml.v2 v2.3.0 // indirect + gopkg.in/yaml.v2 v2.3.0 modernc.org/mathutil v1.0.0 modernc.org/strutil v1.0.0 sigs.k8s.io/yaml v1.2.0 // indirect From 3c8a7384baae950bffc54ea8c52241ea279c96aa Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 13:05:35 -0600 Subject: [PATCH 04/58] added reviewer's suggestions --- auth/auth.go | 35 ++++++++++----------- auth/auth_test.go | 56 +++++++++++++--------------------- server/config.go | 10 +++--- server/config_internal_test.go | 6 ++-- 4 files changed, 46 insertions(+), 61 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 0d19f9461..7376e2551 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -56,9 +56,9 @@ func ReadPermissionsFile(filePath string) (yamlData []byte) { } func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { - err := yaml.Unmarshal([]byte(data), &p) + err := yaml.Unmarshal([]byte(data), p) if err != nil { - log.Fatalf("Error %s", err) + log.Fatalf("error %s", err) } } @@ -87,7 +87,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } if len(groupMatch) == 0 { - return "", fmt.Errorf("User is NOT allowed access to FeatureBase") + return "", fmt.Errorf("user is NOT allowed access to FeatureBase") } // check that user's groups have access to the index user want to access @@ -102,16 +102,15 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } } - // check that user has access to every index - indexCount := 0 - for _, value := range indexCheck { - if value { - indexCount += 1 + // check which index user does NOT have access to, and return in error mesg + if len(indexCheck) != len(index) { + var indexNotFound []string + for _, idx := range index { + if !indexCheck[idx] { + indexNotFound = append(indexNotFound, idx) + } } - } - - if indexCount != len(index) { - return "", fmt.Errorf("User is not allowed access to index: %s", index) + return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound) } // check permissions for index user has access to @@ -122,18 +121,16 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } for _, g := range indexMatch { - if !allPermissions[g.Permission] { - allPermissions[g.Permission] = true - } + allPermissions[g.Permission] = true } if allPermissions["admin"] { - return "admin", error(nil) + return "admin", nil } else if allPermissions["write"] { - return "write", error(nil) + return "write", nil } else if allPermissions["read"] { - return "read", error(nil) + return "read", nil } else { - return "", fmt.Errorf("No permissions found") + return "", fmt.Errorf("no permissions found") } } diff --git a/auth/auth_test.go b/auth/auth_test.go index ddd081e80..e27463c59 100644 --- a/auth/auth_test.go +++ b/auth/auth_test.go @@ -22,19 +22,6 @@ import ( "github.com/molecula/featurebase/v2/auth" ) -func createStruct(inputs [][]string) (permissions auth.GroupPermissions) { - var sliceStruct []auth.Permissions - for _, i := range inputs { - groupId := i[0] - index := i[1] - permission := i[2] - p := auth.Permissions{groupId, index, permission} - sliceStruct = append(sliceStruct, p) - } - permissions = auth.GroupPermissions{Permissions: sliceStruct} - return permissions -} - func TestAuth_CreatePermissionsStruct(t *testing.T) { var singleInput = []byte(`group_permissions: - group: @@ -58,21 +45,22 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { index: "test" permission: "admin"`) - var slice1 [][]string - var slice2 [][]string - var slice3 [][]string - var subslice1 []string - var subslice2 []string - var subslice3 []string - subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read") - subslice2 = append(subslice2, "", "", "") - subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin") - slice1 = append(slice1, subslice1) - slice2 = append(slice2, subslice2) - slice3 = append(slice3, subslice1, subslice3) - singleStruct := createStruct(slice1) - emptyStruct := createStruct(slice2) - multiStruct := createStruct(slice3) + singleStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, + }, + } + emptyStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"", "", ""}, + }, + } + multiStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, + {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, + }, + } tests := []struct { input []byte @@ -89,7 +77,7 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { p.CreatePermissionsStruct(test.input) if !reflect.DeepEqual(p, test.output) { - t.Fatalf("Expected output %s, but got %s", test.output, p) + t.Fatalf("expected output %s, but got %s", test.output, p) } }, ) @@ -162,14 +150,14 @@ func TestAuth_ResolvePermissions(t *testing.T) { createGroupMaps(groupsList1), []string{"test"}, "", - "User is NOT allowed access to FeatureBase", + "user is NOT allowed access to FeatureBase", }, { permissions1, createGroupMaps(groupsList2), []string{"test1"}, "", - "User is not allowed access to index", + "user is not allowed access to index", }, { permissions1, @@ -204,7 +192,7 @@ func TestAuth_ResolvePermissions(t *testing.T) { createGroupMaps(groupsList2), []string{"test"}, "", - "No permissions found", + "no permissions found", }, } @@ -217,12 +205,12 @@ func TestAuth_ResolvePermissions(t *testing.T) { p1, err := p.ResolvePermissions(test.groups, test.index) if p1 != test.userAccess { - t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1) + t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1) } if err != nil { if !strings.Contains(err.Error(), test.err) { - t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error()) + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) } } diff --git a/server/config.go b/server/config.go index 1f3752577..595697067 100644 --- a/server/config.go +++ b/server/config.go @@ -614,14 +614,14 @@ func (c *Config) ValidateAuth() ([]error, error) { errors := make([]error, 0) for name, value := range authConfig { if value == "" { - errors = append(errors, fmt.Errorf("Empty string for auth config %s", name)) + errors = append(errors, fmt.Errorf("empty string for auth config %s", name)) continue } if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { - errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err)) + errors = append(errors, fmt.Errorf("invalid URL for auth config %s: %s", name, err)) continue } } @@ -629,14 +629,14 @@ func (c *Config) ValidateAuth() ([]error, error) { if strings.Contains(name, "File") { fileExt := filepath.Ext(value) if (fileExt != ".yaml") && (fileExt != ".yml") { - errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value)) + errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value)) continue } } } if len(errors) > 0 { - return errors, fmt.Errorf("There were errors validating config") + return errors, fmt.Errorf("there were errors validating config") } return errors, nil } @@ -647,7 +647,7 @@ func (c *Config) ValidatePermissions() (err error) { var p auth.GroupPermissions p.CreatePermissionsStruct(yamlData) if len(p.Permissions) == 0 { - return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile) + return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) } return nil } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 8003a1f2f..7f387172e 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -279,9 +279,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } func TestConfig_validateAuth(t *testing.T) { - errorMesgEmpty := "Empty string" - errorMesgURL := "Invalid URL" - errorMesgPermissions := "Invalid file extension" + errorMesgEmpty := "empty string" + errorMesgURL := "invalid URL" + errorMesgPermissions := "invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" From c25ab78b03a915475749a1ada0697a819aa42c21 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Mon, 13 Dec 2021 13:10:16 -0600 Subject: [PATCH 05/58] use higherlevel iterator in order to account for ops log --- cmd/roaring-migrate/main.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/cmd/roaring-migrate/main.go b/cmd/roaring-migrate/main.go index 3ccc909a6..4ca91bec0 100644 --- a/cmd/roaring-migrate/main.go +++ b/cmd/roaring-migrate/main.go @@ -273,21 +273,19 @@ func Migrate(dataDir, backupPath string) error { }) //raw is now sorted by shard - // need index/field/shard - // make rbf file in backup - rowSize := uint64(0) //? - clear := false - log := false cache := &rbfFile{ temp: filepath.Join(backupPath, "_SCRATCH"), } + bm := roaring.NewSliceBitmap() for _, filename := range raw { index, field, view, shard := Extract(filename) + content, err := ioutil.ReadFile(dataDir + filename) if err != nil { return err } - itr, err := roaring.NewRoaringIterator(content) + err = bm.UnmarshalBinary(content) + if err != nil { return err } @@ -300,10 +298,13 @@ func Migrate(dataDir, backupPath string) error { return err } key := string(txkey.Prefix(index, field, view, shard)) - _, _, err = tx.ImportRoaringBits(key, itr, clear, log, rowSize) - if err != nil { - tx.Rollback() - return err + itr, ok := bm.Containers.Iterator(0) + if ok { + for itr.Next() { + k, v := itr.Value() + tx.PutContainer(key, k, v) + + } } err = tx.Commit() if err != nil { From 7141662c037cfb1cec75a20d4164de1fbfffce32 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Wed, 15 Dec 2021 07:16:30 -0700 Subject: [PATCH 06/58] Defer unlock & rollback during rbf.DB.Begin() --- rbf/db.go | 19 ++++++------------- rbf/tx.go | 10 +++++++--- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index cc4b65700..99c4f0e2f 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -444,15 +444,10 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { } db.mu.Lock() - // note: We cannot defer db.mu.Unlock() here because - // we call tx.Rollback() before if db.readMetaPage - // returns an error, and thus we will deadlock against - // ourselves when the Rollback tries to acquire the db.mu. - // This is why db.mu.Unlock() is done manually below. + defer db.mu.Unlock() if !db.opened { cleanup() - db.mu.Unlock() return nil, ErrClosed } @@ -470,6 +465,11 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { DeleteEmptyContainer: true, } + defer func() { + if err != nil { + tx.rollback(true) + } + }() if writable { tx.dirtyPages = make(map[uint32][]byte) @@ -480,10 +480,6 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { // This page is only written at the end of a dirty transaction. page, err := db.readMetaPage() if err != nil { - // we will deadlock in tx.Rollback() - // on db.mu.Lock unless we manually db.mu.Unlock first. - db.mu.Unlock() - tx.Rollback() return nil, err } copy(tx.meta[:], page) @@ -499,13 +495,10 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { // this avoids recomputing the cache if there are no write txs for a while. if db.rootRecords == nil { if db.rootRecords, err = tx.RootRecords(); err != nil { - db.mu.Unlock() - tx.Rollback() return nil, err } } - db.mu.Unlock() return tx, nil } diff --git a/rbf/tx.go b/rbf/tx.go index 37c76a351..38fed48f6 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -126,7 +126,9 @@ func (tx *Tx) Commit() error { return tx.db.removeTx(tx) } -func (tx *Tx) Rollback() { +func (tx *Tx) Rollback() { tx.rollback(false) } + +func (tx *Tx) rollback(hasDBLock bool) { tx.mu.Lock() defer tx.mu.Unlock() @@ -141,8 +143,10 @@ func (tx *Tx) Rollback() { } // Disconnect transaction from DB. - tx.db.mu.Lock() - defer tx.db.mu.Unlock() + if !hasDBLock { + tx.db.mu.Lock() + defer tx.db.mu.Unlock() + } vprint.PanicOn(tx.db.removeTx(tx)) } From 48913caafdbb3a8ae37ed356eceb29831732be26 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Wed, 15 Dec 2021 13:35:30 -0600 Subject: [PATCH 07/58] renamed package to authz, inmplemented reviewer's feedback --- auth/auth.go => authz/authz.go | 61 ++++++------ auth/auth_test.go => authz/authz_test.go | 112 +++++++++++------------ server/config.go | 20 +++- server/config_internal_test.go | 26 +++--- server/server.go | 14 +++ 5 files changed, 128 insertions(+), 105 deletions(-) rename auth/auth.go => authz/authz.go (62%) rename auth/auth_test.go => authz/authz_test.go (61%) diff --git a/auth/auth.go b/authz/authz.go similarity index 62% rename from auth/auth.go rename to authz/authz.go index 7376e2551..f48943c04 100644 --- a/auth/auth.go +++ b/authz/authz.go @@ -1,11 +1,23 @@ -// Copyright 2021 Molecula Corp. All rights reserved. -package auth +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package authz import ( "fmt" + "io" "io/ioutil" - "log" - "path/filepath" "gopkg.in/yaml.v2" ) @@ -46,48 +58,39 @@ type Permissions struct { Permission string `yaml:"permission"` } -func ReadPermissionsFile(filePath string) (yamlData []byte) { - filePathAbs, _ := filepath.Abs(filePath) - yamlData, err := ioutil.ReadFile(filePathAbs) +type Group struct { + ID string `json:"id"` + Name string `json:"displayName"` +} + +func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) { + permsData, err := ioutil.ReadAll(permsFile) if err != nil { - panic(err) + return fmt.Errorf("reading permissions failed with error: %s", err) } - return yamlData -} -func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { - err := yaml.Unmarshal([]byte(data), p) + err = yaml.Unmarshal(permsData, p) if err != nil { - log.Fatalf("error %s", err) + return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } + + return nil } -func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) { - // read yaml permissions file - yamlData := ReadPermissionsFile(Auth.PermissionsFile) - - // get group permissions - var p GroupPermissions - p.CreatePermissionsStruct(yamlData) - - // check permissions for all groups and index, and return most permissive - return p.ResolvePermissions(groups, index) -} - -func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) { +func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) { // get union of groups the user is part of obtained from identity provider and groups in permissions file var groupMatch []Permissions for _, group := range groups { for i := range p.Permissions { - if group["id"] == p.Permissions[i].GroupId { + if group.ID == p.Permissions[i].GroupId { groupMatch = append(groupMatch, p.Permissions[i]) } } } if len(groupMatch) == 0 { - return "", fmt.Errorf("user is NOT allowed access to FeatureBase") + return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups) } // check that user's groups have access to the index user want to access @@ -110,7 +113,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index indexNotFound = append(indexNotFound, idx) } } - return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound) + return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound) } // check permissions for index user has access to diff --git a/auth/auth_test.go b/authz/authz_test.go similarity index 61% rename from auth/auth_test.go rename to authz/authz_test.go index e27463c59..eaec5df13 100644 --- a/auth/auth_test.go +++ b/authz/authz_test.go @@ -11,7 +11,7 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. -package auth_test +package authz_test import ( "fmt" @@ -19,23 +19,23 @@ import ( "strings" "testing" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" ) -func TestAuth_CreatePermissionsStruct(t *testing.T) { - var singleInput = []byte(`group_permissions: +func TestAuth_ReadPermissionsFile(t *testing.T) { + var singleInput = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" - permission: "read"`) + permission: "read"` - var emptyInput = []byte(`group_permissions: + var emptyInput = `group_permissions: - group: groupId: "" index: "" - permission: ""`) + permission: ""` - var multiInput = []byte(`group_permissions: + var multiInput = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" @@ -43,28 +43,28 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` - singleStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + singleStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, }, } - emptyStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + emptyStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"", "", ""}, }, } - multiStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + multiStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, }, } tests := []struct { - input []byte - output auth.GroupPermissions + input string + output authz.GroupPermissions }{ {singleInput, singleStruct}, {emptyInput, emptyStruct}, @@ -73,8 +73,11 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - var p auth.GroupPermissions - p.CreatePermissionsStruct(test.input) + permFile := strings.NewReader(test.input) + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permFile); err != nil { + t.Fatalf("readPermissionsFile error: %s", err) + } if !reflect.DeepEqual(p, test.output) { t.Fatalf("expected output %s, but got %s", test.output, p) @@ -84,26 +87,15 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { } } -func createGroupMaps(groups []string) []map[string]string { - - var group1 []map[string]string - for _, i := range groups { - map1 := map[string]string{} - map1["id"] = i - group1 = append(group1, map1) - } - return group1 -} - func TestAuth_ResolvePermissions(t *testing.T) { // initializes different example of permissions file in yaml - var permissions1 = []byte(`group_permissions: + var permissions1 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" - permission: "read"`) + permission: "read"` - var permissions2 = []byte(`group_permissions: + var permissions2 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" @@ -111,9 +103,9 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "write"`) + permission: "write"` - var permissions3 = []byte(`group_permissions: + var permissions3 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" @@ -121,9 +113,9 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` - var permissions4 = []byte(`group_permissions: + var permissions4 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" @@ -131,65 +123,68 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` // initializes groups that are returned from identity provider - groupsList1 := []string{} - groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"} - groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"} + groupsList1 := []authz.Group{} + groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}} + groupsList3 := []authz.Group{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "name"}, + {"dca35310-ecda-4f23-86cd-876aee559900", "name"}, + } tests := []struct { - permissions []byte - groups []map[string]string - index []string - userAccess string - err string + yamlData string + groups []authz.Group + index []string + userAccess string + err string }{ { permissions1, - createGroupMaps(groupsList1), + groupsList1, []string{"test"}, "", - "user is NOT allowed access to FeatureBase", + "NOT allowed access to FeatureBase", }, { permissions1, - createGroupMaps(groupsList2), + groupsList2, []string{"test1"}, "", - "user is not allowed access to index", + "NOT allowed access to index", }, { permissions1, - createGroupMaps(groupsList2), + groupsList2, []string{"test"}, "read", "", }, { permissions2, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "write", "", }, { permissions3, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "admin", "", }, { permissions2, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "write", "", }, { permissions4, - createGroupMaps(groupsList2), + groupsList2, []string{"test"}, "", "no permissions found", @@ -199,10 +194,11 @@ func TestAuth_ResolvePermissions(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - var p auth.GroupPermissions - p.CreatePermissionsStruct(test.permissions) + permFile := strings.NewReader(test.yamlData) + var p authz.GroupPermissions + p.ReadPermissionsFile(permFile) - p1, err := p.ResolvePermissions(test.groups, test.index) + p1, err := p.GetPermissions(test.groups, test.index) if p1 != test.userAccess { t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1) diff --git a/server/config.go b/server/config.go index 595697067..1f3b62b4a 100644 --- a/server/config.go +++ b/server/config.go @@ -7,13 +7,14 @@ import ( "log" "net" "net/url" + "os" "path/filepath" "runtime" "strconv" "strings" "time" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" petcd "github.com/molecula/featurebase/v2/etcd" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/storage" @@ -232,7 +233,7 @@ type Config struct { SchemaDetailsOn bool `toml:"schema-details-on"` // Enable AuthZ/AuthN - Auth auth.Auth `toml:"auth"` + Auth authz.Auth `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -642,13 +643,22 @@ func (c *Config) ValidateAuth() ([]error, error) { } func (c *Config) ValidatePermissions() (err error) { + permsFile, err := os.Open(c.Auth.PermissionsFile) + if err != nil { + return err + } + + var p *authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return err + } - yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile) - var p auth.GroupPermissions - p.CreatePermissionsStruct(yamlData) if len(p.Permissions) == 0 { return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) } + + defer permsFile.Close() + return nil } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 7f387172e..4d6dcc71c 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -9,7 +9,7 @@ import ( "strings" "testing" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" ) type addrs struct{ bind, advertise string } @@ -294,7 +294,7 @@ func TestConfig_validateAuth(t *testing.T) { tests := []struct { expErrs []string - input auth.Auth + input authz.Auth }{ { @@ -308,7 +308,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: emptyString, ClientSecret: emptyString, @@ -329,7 +329,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: emptyString, @@ -350,7 +350,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: emptyString, ClientSecret: validClientSecret, @@ -370,7 +370,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -389,7 +389,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -407,7 +407,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -423,7 +423,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgURL, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -440,7 +440,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgURL, errorMesgURL, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -456,7 +456,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgPermissions, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -470,7 +470,7 @@ func TestConfig_validateAuth(t *testing.T) { { // Auth enabled, all configs are set properly []string{}, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -484,7 +484,7 @@ func TestConfig_validateAuth(t *testing.T) { { // Auth disabled, all configs are set to empty string []string{}, - auth.Auth{ + authz.Auth{ Enable: disable, ClientId: emptyString, ClientSecret: emptyString, diff --git a/server/server.go b/server/server.go index a6d0049ae..b876f3eb6 100644 --- a/server/server.go +++ b/server/server.go @@ -29,6 +29,7 @@ import ( "golang.org/x/sync/errgroup" pilosa "github.com/molecula/featurebase/v2" + "github.com/molecula/featurebase/v2/authz" "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/encoding/proto" petcd "github.com/molecula/featurebase/v2/etcd" @@ -224,6 +225,19 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable { m.Config.MustValidateAuth() + + // Read permissions file + permsFile, err := os.Open(m.Config.Auth.PermissionsFile) + if err != nil { + return err + } + + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return err + } + + defer permsFile.Close() } // Initialize server. From 484cbbcd0943bb5bc5d453d9248188904a90a5f2 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Wed, 15 Dec 2021 14:19:12 -0600 Subject: [PATCH 08/58] fixed func name --- authz/authz_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/authz_test.go b/authz/authz_test.go index eaec5df13..b1fbc4d6a 100644 --- a/authz/authz_test.go +++ b/authz/authz_test.go @@ -87,7 +87,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { } } -func TestAuth_ResolvePermissions(t *testing.T) { +func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml var permissions1 = `group_permissions: - group: From c59ce837b3b15d2baa15c5d3a367475fbaf83a1a Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 15 Dec 2021 16:09:45 -0600 Subject: [PATCH 09/58] ensure field bit depth is set when restoring shard we have to manually set the cache value here bc it wont get set until the node is restarted otherwise --- api.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/api.go b/api.go index d39a61f3d..70a27da94 100644 --- a/api.go +++ b/api.go @@ -2768,6 +2768,14 @@ func (api *API) RestoreShard(ctx context.Context, indexName string, shard uint64 if err != nil { return err } + bd, err := view.bitDepth([]uint64{shard}) + if err != nil { + return err + } + err = fld.cacheBitDepth(bd) + if err != nil { + return err + } } return nil From 2ca29e6018621b203462296f72be3675e7405ffb Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 11:45:35 -0600 Subject: [PATCH 10/58] addressed reviewer's comments and added more tests --- authz/authz.go | 77 ++++------- authz/authz_test.go | 157 +++++++++------------- server/config.go | 118 +++++++++++----- server/config_internal_test.go | 238 +++++++++++++-------------------- server/server.go | 7 +- 5 files changed, 268 insertions(+), 329 deletions(-) diff --git a/authz/authz.go b/authz/authz.go index f48943c04..42c3bba2f 100644 --- a/authz/authz.go +++ b/authz/authz.go @@ -49,27 +49,23 @@ type Auth struct { } type GroupPermissions struct { - Permissions []Permissions `yaml:"group_permissions"` -} - -type Permissions struct { - GroupId string `yaml:"groupId"` - Index string `yaml:"index"` - Permission string `yaml:"permission"` + Permissions map[string]map[string]string } type Group struct { - ID string `json:"id"` - Name string `json:"displayName"` + UserID string + GroupID string `json:"id"` + GroupName string `json:"displayName"` } func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) { permsData, err := ioutil.ReadAll(permsFile) + if err != nil { return fmt.Errorf("reading permissions failed with error: %s", err) } - err = yaml.Unmarshal(permsData, p) + err = yaml.UnmarshalStrict(permsData, &p.Permissions) if err != nil { return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } @@ -77,54 +73,33 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return nil } -func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) { +func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { - // get union of groups the user is part of obtained from identity provider and groups in permissions file - var groupMatch []Permissions - for _, group := range groups { - for i := range p.Permissions { - if group.ID == p.Permissions[i].GroupId { - groupMatch = append(groupMatch, p.Permissions[i]) - } - } - } - - if len(groupMatch) == 0 { - return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups) - } - - // check that user's groups have access to the index user want to access - var indexMatch []Permissions - indexCheck := map[string]bool{} - for _, g := range groupMatch { - for _, idx := range index { - if idx == g.Index { - indexMatch = append(indexMatch, g) - indexCheck[idx] = true - } - } - } - - // check which index user does NOT have access to, and return in error mesg - if len(indexCheck) != len(index) { - var indexNotFound []string - for _, idx := range index { - if !indexCheck[idx] { - indexNotFound = append(indexNotFound, idx) - } - } - return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound) - } - - // check permissions for index user has access to allPermissions := map[string]bool{ "admin": false, "write": false, "read": false, } - for _, g := range indexMatch { - allPermissions[g.Permission] = true + if len(groups) == 0 { + return "", fmt.Errorf("user is not part of any groups in identity provider") + } + + var groupsDenied []string + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + if perm, ok := p.Permissions[group.GroupID][index]; ok { + allPermissions[perm] = true + } else { + return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index) + } + } else { + groupsDenied = append(groupsDenied, group.GroupID) + } + } + + if len(groupsDenied) == len(groups) { + return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied) } if allPermissions["admin"] { diff --git a/authz/authz_test.go b/authz/authz_test.go index b1fbc4d6a..fcec43185 100644 --- a/authz/authz_test.go +++ b/authz/authz_test.go @@ -23,64 +23,43 @@ import ( ) func TestAuth_ReadPermissionsFile(t *testing.T) { - var singleInput = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read"` + singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read"` - var emptyInput = `group_permissions: - - group: - groupId: "" - index: "" - permission: ""` + multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +"dca35310-ecda-4f23-86cd-876aee559900": + "test": "admin"` - var multiInput = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` - - singleStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, - }, + singleStruct := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, } - emptyStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"", "", ""}, - }, - } - multiStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, - {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, - }, + + multiStruct := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + "dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"}, } tests := []struct { input string - output authz.GroupPermissions + output map[string]map[string]string }{ {singleInput, singleStruct}, - {emptyInput, emptyStruct}, {multiInput, multiStruct}, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { permFile := strings.NewReader(test.input) + var p authz.GroupPermissions - if err := p.ReadPermissionsFile(permFile); err != nil { + err := p.ReadPermissionsFile(permFile) + if err != nil { t.Fatalf("readPermissionsFile error: %s", err) } - if !reflect.DeepEqual(p, test.output) { - t.Fatalf("expected output %s, but got %s", test.output, p) + if !reflect.DeepEqual(p.Permissions, test.output) { + t.Fatalf("expected output %s, but got %s", test.output, p.Permissions) } }, ) @@ -89,103 +68,84 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml - var permissions1 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read"` + permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read"` - var permissions2 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "write"` + permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": "write"` - var permissions3 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` + permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "write" + "test2": "read" +"dca35310-ecda-4f23-86cd-876aee559900": + "test": "admin"` - var permissions4 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` + permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": ""` // initializes groups that are returned from identity provider + groupName := "name" + userId := "user-id" groupsList1 := []authz.Group{} - groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}} + groupsList2 := []authz.Group{{userId, "fake-group", groupName}} groupsList3 := []authz.Group{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "name"}, - {"dca35310-ecda-4f23-86cd-876aee559900", "name"}, + {userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName}, + {userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName}, } tests := []struct { yamlData string groups []authz.Group - index []string + index string userAccess string err string }{ { permissions1, groupsList1, - []string{"test"}, + "test", + "", + "user is not part of any groups in identity provider", + }, + { + permissions1, + groupsList3, + "test1", + "", + "NOT allowed access to index", + }, + { + permissions2, + groupsList2, + "test", "", "NOT allowed access to FeatureBase", }, { permissions1, - groupsList2, - []string{"test1"}, - "", - "NOT allowed access to index", - }, - { - permissions1, - groupsList2, - []string{"test"}, + groupsList3, + "test", "read", "", }, { permissions2, groupsList3, - []string{"test"}, + "test", "write", "", }, { permissions3, groupsList3, - []string{"test"}, + "test", "admin", "", }, - { - permissions2, - groupsList3, - []string{"test"}, - "write", - "", - }, { permissions4, - groupsList2, - []string{"test"}, + groupsList3, + "test", "", "no permissions found", }, @@ -195,8 +155,11 @@ func TestAuth_GetPermissions(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { permFile := strings.NewReader(test.yamlData) + var p authz.GroupPermissions - p.ReadPermissionsFile(permFile) + if err := p.ReadPermissionsFile(permFile); err != nil { + t.Errorf("Error: %s", err) + } p1, err := p.GetPermissions(test.groups, test.index) diff --git a/server/config.go b/server/config.go index 1f3b62b4a..866ffb3ff 100644 --- a/server/config.go +++ b/server/config.go @@ -4,6 +4,7 @@ package server import ( "context" "fmt" + "io" "log" "net" "net/url" @@ -598,9 +599,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin return addrs[0].String(), nil } -func (c *Config) ValidateAuth() ([]error, error) { +func (c *Config) ValidateAuth() (errors []error) { if !c.Auth.Enable { - return []error{}, nil + return errors } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, @@ -609,10 +610,8 @@ func (c *Config) ValidateAuth() ([]error, error) { "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, "ScopeURL": c.Auth.ScopeURL, - "PermissionsFile": c.Auth.PermissionsFile, } - errors := make([]error, 0) for name, value := range authConfig { if value == "" { errors = append(errors, fmt.Errorf("empty string for auth config %s", name)) @@ -626,50 +625,99 @@ func (c *Config) ValidateAuth() ([]error, error) { continue } } + } - if strings.Contains(name, "File") { - fileExt := filepath.Ext(value) - if (fileExt != ".yaml") && (fileExt != ".yml") { - errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value)) + if len(errors) > 0 { + return errors + } + return nil +} + +func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { + + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return append(errors, err) + } + + if len(p.Permissions) == 0 { + return append(errors, fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile)) + } + + for groupId, indexPerm := range p.Permissions { + if groupId == "" { + errors = append(errors, fmt.Errorf("empty string for group id in permissions file %s", c.Auth.PermissionsFile)) + continue + } + + for index, perm := range indexPerm { + if index == "" { + errors = append(errors, fmt.Errorf("empty string for index for group id %s in permissions file %s ", groupId, c.Auth.PermissionsFile)) + continue + } + + if perm == "" { + errors = append(errors, fmt.Errorf("empty string for permission for group id %s and index %s in permissions file %s", groupId, index, c.Auth.PermissionsFile)) + continue + } + + if !((perm == "admin") || (perm == "write") || (perm == "read")) { + errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile)) continue } } } - if len(errors) > 0 { - return errors, fmt.Errorf("there were errors validating config") + return errors } - return errors, nil -} - -func (c *Config) ValidatePermissions() (err error) { - permsFile, err := os.Open(c.Auth.PermissionsFile) - if err != nil { - return err - } - - var p *authz.GroupPermissions - if err := p.ReadPermissionsFile(permsFile); err != nil { - return err - } - - if len(p.Permissions) == 0 { - return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) - } - - defer permsFile.Close() return nil } +func (c *Config) ValidatePermissionsFile() (err error) { + + if c.Auth.PermissionsFile == "" { + return fmt.Errorf("empty string for auth config permissions file") + } + + fileExt := filepath.Ext(c.Auth.PermissionsFile) + if (fileExt != ".yaml") && (fileExt != ".yml") { + return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile) + } + return nil +} + func (c *Config) MustValidateAuth() { - if errors, err := c.ValidateAuth(); err != nil { - for _, e1 := range errors { - log.Println(e1) + + errorsAuth := c.ValidateAuth() + if len(errorsAuth) > 0 { + for _, e := range errorsAuth { + log.Println(e) } - if e2 := c.ValidatePermissions(); e2 != nil { - log.Println(e2) + } + + var errorsPerm []error + errorsPermFile := c.ValidatePermissionsFile() + if errorsPermFile == nil { + permsFile, err := os.Open(c.Auth.PermissionsFile) + if err != nil { + log.Println(err) } - log.Fatal(err) + + defer permsFile.Close() + + errorsPerm = c.ValidatePermissions(permsFile) + if len(errorsPerm) > 0 { + for _, e := range errorsPerm { + log.Println(e) + } + } + + } else { + log.Println(errorsPermFile) + } + + if len(errorsAuth) > 0 || len(errorsPerm) > 0 || errorsPermFile != nil { + log.Fatal(fmt.Errorf("there were errors validating authN/authZ config and/or permissions")) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 4d6dcc71c..48ba081db 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -281,12 +281,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty := "empty string" errorMesgURL := "invalid URL" - errorMesgPermissions := "invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" - validFilename := "permissions.yaml" - invalidFilename := "permissions.txt" invalidURL := "not-a-url" emptyString := "" enable := true @@ -306,7 +303,6 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, - errorMesgEmpty, }, authz.Auth{ Enable: enable, @@ -316,106 +312,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: emptyString, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: emptyString, - ClientSecret: validClientSecret, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, }, }, { @@ -431,40 +327,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, - PermissionsFile: validFilename, - }, - }, - { - // Auth enabled, some strings are set to invalid URL - []string{ - errorMesgURL, - errorMesgURL, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: invalidURL, - GroupEndpointURL: invalidURL, - ScopeURL: validTestURL, - PermissionsFile: validFilename, - }, - }, - { - // Auth enabled, permissions file is set to invalid string - []string{ - errorMesgPermissions, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: validTestURL, - ScopeURL: validTestURL, - PermissionsFile: invalidFilename, }, }, { @@ -478,7 +340,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, - PermissionsFile: validFilename, }, }, { @@ -492,7 +353,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, - PermissionsFile: emptyString, }, }, } @@ -502,9 +362,9 @@ func TestConfig_validateAuth(t *testing.T) { c := NewConfig() c.Auth = test.input - errors, err := c.ValidateAuth() + errors := c.ValidateAuth() if len(test.expErrs) > 0 { - if err == nil { + if errors == nil { t.Fatal("expected errors, but none were found") } } @@ -522,3 +382,97 @@ func TestConfig_validateAuth(t *testing.T) { }) } } + +func TestConfig_validatePermissions(t *testing.T) { + permissions0 := `` + + permissions1 := `"": + "test": "read"` + + permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": + "": "write"` + + permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": ""` + + permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": "readwrite"` + + tests := []struct { + err string + input string + }{ + { + "no group permissions found in permissions file", + permissions0, + }, + { + "empty string for group id", + permissions1, + }, + { + "empty string for index", + permissions2, + }, + { + "empty string for permission", + permissions3, + }, + { + "not a valid permission", + permissions4, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + c := NewConfig() + c.Auth.PermissionsFile = "test.yaml" + + permFile := strings.NewReader(test.input) + errors := c.ValidatePermissions(permFile) + + if errors == nil { + t.Fatal("expected errors, but none were found") + } + + for _, err := range errors { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) + + } + } + }) + } +} + +func TestConfig_validatePermissionsFilename(t *testing.T) { + + tests := []struct { + err string + input string + }{ + { + "empty string for auth config permissions file", + "", + }, + { + "invalid file extension for auth config permissions file", + "permissions.txt", + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + c := NewConfig() + c.Auth.PermissionsFile = test.input + + if err := c.ValidatePermissionsFile(); err != nil { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) + } + } + }) + } +} diff --git a/server/server.go b/server/server.go index b876f3eb6..c2418630b 100644 --- a/server/server.go +++ b/server/server.go @@ -226,18 +226,17 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable { m.Config.MustValidateAuth() - // Read permissions file permsFile, err := os.Open(m.Config.Auth.PermissionsFile) if err != nil { return err } + defer permsFile.Close() + var p authz.GroupPermissions - if err := p.ReadPermissionsFile(permsFile); err != nil { + if err = p.ReadPermissionsFile(permsFile); err != nil { return err } - - defer permsFile.Close() } // Initialize server. From 3ed4487ae269be42307ad19fa217a751b38177d4 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 12:31:13 -0600 Subject: [PATCH 11/58] scratch space for FB-992: create benchmark for checkpointing Note also the commented-out debug printf in checkpoint, there as a reference. This is interesting because it turns out that MOST of checkpoint writes is not actually writing new pages in most cases. The actual "pages in WAL : pages in map" ratio is typically around 30:1 apparently. This would likely be different in cases where we were updating existing data, though. This is scratch space to prep for an actual work. The final results will likely be different. --- rbf/db.go | 1 + rbf/db_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/rbf/db.go b/rbf/db.go index 99c4f0e2f..65794c9eb 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -200,6 +200,7 @@ func (db *DB) checkpoint() error { return nil } + // fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size) for i := 0; i < db.walPageN; i++ { page, err := db.readWALPageAt(i) if err != nil { diff --git a/rbf/db_test.go b/rbf/db_test.go index 56170d6eb..614def465 100644 --- a/rbf/db_test.go +++ b/rbf/db_test.go @@ -13,6 +13,7 @@ import ( _ "net/http/pprof" + "github.com/felixge/fgprof" "github.com/molecula/featurebase/v2/rbf" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "golang.org/x/sync/errgroup" @@ -336,6 +337,91 @@ func TestDB_MultiTx(t *testing.T) { } } +// benchmarkOneCheckpoint +func benchmarkOneCheckpoint(b *testing.B) { + cfg := rbfcfg.NewDefaultConfig() + // extremely low to force checkpointing + cfg.MinWALCheckpointSize = rbf.PageSize * 16 + cfg.MaxWALCheckpointSize = rbf.PageSize * 64 + var _ rbfcfg.Config + db := MustOpenDB(b, cfg) + defer MustCloseDB(b, db) + + // Run multiple readers in separate goroutines. + ctx, cancel := context.WithCancel(context.Background()) + g, ctx := errgroup.WithContext(ctx) + for i := 0; i < 4; i++ { + g.Go(func() error { + for { + if ctx.Err() != nil { + return nil // cancelled, return no error + } else if err := func() error { + tx, err := db.Begin(false) + if err != nil { + return err + } + defer tx.Rollback() + + // time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond)))) + + for i := 0; i < rand.Intn(1000); i++ { + v := rand.Intn(1 << 20) + if _, err := tx.Contains("x", uint64(v)); err != nil { + return err + } + } + return nil + }(); err != nil { + return err + } + + // time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond)))) + } + }) + } + + // Continuously set/clear bits while readers are executing. + for i := 0; i < 1000; i++ { + func() { + tx, err := db.Begin(true) + if err != nil { + b.Fatal(err) + } + defer tx.Rollback() + + for j := 0; j < rand.Intn(100); j++ { + v := rand.Intn(1 << 20) + if _, err := tx.Add("x", uint64(v)); err != nil { + b.Fatal(err) + } + + } + + if err := tx.Commit(); err != nil { + b.Fatal(err) + } + }() + } + + // Stop readers & wait. + cancel() + if err := g.Wait(); err != nil { + b.Fatal(err) + } +} + +func BenchmarkDbCheckpoint(b *testing.B) { + out, err := os.Create("cp.out") + if err != nil { + b.Fatalf("creating log file: %v", err) + } + done := fgprof.Start(out, fgprof.FormatPprof) + for i := 0; i < b.N; i++ { + benchmarkOneCheckpoint(b) + } + done() +} + // better diagnosis of deadlocks/hung situations versus just really slow "Quick" tests. func TestMain(m *testing.M) { l, err := net.Listen("tcp", ":0") From 5c889c72bdab24f960d69470adc99301f558fa96 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 13:04:45 -0600 Subject: [PATCH 12/58] make test hit the lock harder Discovered test was running slightly strange and spending an unreasonable amount of time on rand.Intn(), possibly because we weren't caching the value used as the loop condition. Tweaked that, also made the pool a bit different. Now it takes ~50 seconds for benchtime 100x, and produces a profile with a TON of time spent waiting on sleeps (expected) and the condition variable for waiting on checkpoints (the thing we want to measure, really). --- rbf/db_test.go | 54 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/rbf/db_test.go b/rbf/db_test.go index 614def465..5c8504b1b 100644 --- a/rbf/db_test.go +++ b/rbf/db_test.go @@ -337,8 +337,11 @@ func TestDB_MultiTx(t *testing.T) { } } +// premake pool of random values +const randPool = (1 << 18) + // benchmarkOneCheckpoint -func benchmarkOneCheckpoint(b *testing.B) { +func benchmarkOneCheckpoint(b *testing.B, randInts []int) { cfg := rbfcfg.NewDefaultConfig() // extremely low to force checkpointing cfg.MinWALCheckpointSize = rbf.PageSize * 16 @@ -350,7 +353,8 @@ func benchmarkOneCheckpoint(b *testing.B) { // Run multiple readers in separate goroutines. ctx, cancel := context.WithCancel(context.Background()) g, ctx := errgroup.WithContext(ctx) - for i := 0; i < 4; i++ { + for i := 0; i < 8; i++ { + i := i g.Go(func() error { for { if ctx.Err() != nil { @@ -362,10 +366,11 @@ func benchmarkOneCheckpoint(b *testing.B) { } defer tx.Rollback() - // time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond)))) + time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond)))) - for i := 0; i < rand.Intn(1000); i++ { - v := rand.Intn(1 << 20) + times := rand.Intn(1000) + 1 + for j := 0; j < times; j++ { + v := randInts[((i<<10)+j)%(randPool-1)] if _, err := tx.Contains("x", uint64(v)); err != nil { return err } @@ -374,13 +379,13 @@ func benchmarkOneCheckpoint(b *testing.B) { }(); err != nil { return err } - // time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond)))) } }) } // Continuously set/clear bits while readers are executing. + next := 0 for i := 0; i < 1000; i++ { func() { tx, err := db.Begin(true) @@ -389,14 +394,22 @@ func benchmarkOneCheckpoint(b *testing.B) { } defer tx.Rollback() - for j := 0; j < rand.Intn(100); j++ { - v := rand.Intn(1 << 20) - if _, err := tx.Add("x", uint64(v)); err != nil { - b.Fatal(err) + times := rand.Intn(100) + for j := 0; j < times; j++ { + v := randInts[next] + next = (next + 1) % (randPool - 1) + if j&7 == 0 { + // some removes but they're less frequent + if _, err := tx.Remove("x", uint64(v)); err != nil { + b.Fatal(err) + } + } else { + if _, err := tx.Add("x", uint64(v)); err != nil { + b.Fatal(err) + } } } - if err := tx.Commit(); err != nil { b.Fatal(err) } @@ -416,9 +429,24 @@ func BenchmarkDbCheckpoint(b *testing.B) { b.Fatalf("creating log file: %v", err) } done := fgprof.Start(out, fgprof.FormatPprof) - for i := 0; i < b.N; i++ { - benchmarkOneCheckpoint(b) + b.StopTimer() + // premake these because otherwise it's >5% of CPU in the reads + randInts := make([]int, randPool) + for i := range randInts { + v1, v2 := rand.Intn(1<<24), rand.Intn(1<<24) + // minimum gives us a skewed distribution which makes lower values more + // likely than higher values, so we get a mix of container types + if v1 < v2 { + randInts[i] = v1 + } else { + randInts[i] = v2 + } } + b.StartTimer() + for i := 0; i < b.N; i++ { + benchmarkOneCheckpoint(b, randInts) + } + b.StopTimer() done() } From a631e25dc517e5473ea70863bae8633e0232ce00 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 13:14:42 -0600 Subject: [PATCH 13/58] refactor: removeTx responsible for getting/releasing its own lock We change nothing substantive here, except that there's a window between when a write transaction updates the root pages and when it removes itself from the db tx list and possibly causes a checkpoint where it's not holding the db lock. The issue here is that we want to be able to *keep* the lock but still return, so no one else can start transactions, but the specific Rollback or Commit that removed the last outstanding transaction doesn't block forever. This will, later, allow us to exercise finer-grained control over when we allow transactions. This is a separate commit so we can run the test suite against it, and verify that this part in particular didn't break anything. --- rbf/db.go | 7 ++++++- rbf/tx.go | 14 +++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 65794c9eb..7f27ca418 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -503,8 +503,13 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { return tx, nil } -// removeTx removes an active transaction from the database. +// removeTx removes an active transaction from the database. it obtains +// the db lock, and currently drops it, but will later possibly be leaving +// it retained by an asynchronous op that wants to happen before we start +// running new tx. func (db *DB) removeTx(tx *Tx) error { + db.mu.Lock() + defer db.mu.Unlock() // Release writer lock if tx is writable. if tx.writable { tx.db.rwmu.Unlock() diff --git a/rbf/tx.go b/rbf/tx.go index 38fed48f6..2cf7420db 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -109,20 +109,24 @@ func (tx *Tx) Commit() error { // future plan: after checkpoint is moved to background // or not every removeTx, then we can move the // tx.db.rootRecords = tx.rootRecords into removeTx(). - + // + // ... or maybe not: let's do that part here, and then removeTx + // may or may not start a checkpoint, possibly asynchronously. + // // avoid race detector firing on a write race here - // vs the read of rootRecords at db.Begin() + // vs the read of rootRecords at db.Begin(), then release + // the lock, because we need removeTx to grab the lock to + // work, but if it wants to checkpoint, it wants to be able to return + // to us here and still be holding the lock. tx.db.mu.Lock() - defer tx.db.mu.Unlock() tx.db.rootRecords = tx.rootRecords tx.db.pageMap = tx.pageMap tx.db.walPageN = tx.walPageN + tx.db.mu.Unlock() return tx.db.removeTx(tx) } // Disconnect transaction from DB. - tx.db.mu.Lock() - defer tx.db.mu.Unlock() return tx.db.removeTx(tx) } From b8f59d922c72e1e1b8d4b1ee36bbd89e6e0d9096 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 14:37:59 -0600 Subject: [PATCH 14/58] checkpoint rework/refactoring: logger, async-ish checkpoint Trying to make the checkpoint be asynchronous-at-all, and also allowing it to log. --- rbf/cfg/cfg.go | 6 ++ rbf/db.go | 162 ++++++++++++++++++++++++++++++++++++++---------- rbf/rbf_test.go | 8 +++ 3 files changed, 144 insertions(+), 32 deletions(-) diff --git a/rbf/cfg/cfg.go b/rbf/cfg/cfg.go index cc2cf7a8a..671c6fe43 100644 --- a/rbf/cfg/cfg.go +++ b/rbf/cfg/cfg.go @@ -2,6 +2,7 @@ package cfg import ( + "github.com/molecula/featurebase/v2/logger" "github.com/spf13/pflag" ) @@ -35,6 +36,11 @@ type Config struct { // CursorCacheSize is the number of copies of Cursor{} to keep in our // readyCursorCh arena to avoid GC pressure. CursorCacheSize int64 `toml:"cursor-cache-size"` + + // Logger specifies a logger for asynchronous errors, such as + // background checkpoints. It cannot be set from toml. The default is + // to use stderr. + Logger logger.Logger `toml:"-"` } func NewDefaultConfig() *Config { diff --git a/rbf/db.go b/rbf/db.go index 7f27ca418..830e31f5a 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -11,6 +11,7 @@ import ( "syscall" "github.com/benbjohnson/immutable" + "github.com/molecula/featurebase/v2/logger" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/syswrap" ) @@ -38,6 +39,7 @@ type DB struct { pageMap *PageMap // pgno-to-WALID mapping txs map[*Tx]struct{} // active transactions opened bool // true if open + logger logger.Logger // for diagnostics from async things wal []byte // wal mmap walFile *os.File // wal file descriptor @@ -62,6 +64,11 @@ func NewDB(path string, cfg *rbfcfg.Config) *DB { txs: make(map[*Tx]struct{}), pageMap: NewPageMap(), Path: path, + logger: cfg.Logger, + } + if db.logger == nil { + // default to writing to stdout if not told otherwise + db.logger = logger.NewStandardLogger(os.Stderr) } db.haltCond = sync.NewCond(&db.mu) @@ -134,7 +141,7 @@ func (db *DB) Open() (err error) { if err := db.openWAL(); err != nil { return fmt.Errorf("wal open: %w", err) } else if err := db.checkpoint(); err != nil { - return fmt.Errorf("checkpoint: %w", err) + return fmt.Errorf("startup checkpoint: %w", err) } return nil @@ -158,10 +165,12 @@ func (db *DB) openWAL() (err error) { // Determine the number of whole pages in the WAL. var pageN int + var fileSize int64 if fi, err := db.walFile.Stat(); err != nil { return fmt.Errorf("wal stat: %w", err) } else { - pageN = int(fi.Size() / PageSize) + fileSize = fi.Size() + pageN = int(fileSize / PageSize) } // Read backwards through the WAL to find the last valid meta page. @@ -169,14 +178,45 @@ func (db *DB) openWAL() (err error) { if page, err := db.readWALPageAt(pageN - 1); err != nil { return err } else if IsMetaPage(page) { + // We now face a challenge. Probably this is a meta page. + // But consider a sequence of pages written which gets + // interrupted right before the meta page is written. + // If the last page is a bitmap page, it could LOOK LIKE a meta + // page. So we have to check the page before it. If that page + // is a bitmap header, then actually this is a bitmap page, right? + // If that page doesn't exist, of course, we're fine, except + // for the philosophical question of why we wrote a meta page + // when no pages had changed. + if pageN > 1 { + if page, err = db.readWALPageAt(pageN - 2); err != nil { + return err + } + if IsBitmapHeader(page) { + // But wait! + // What if this *is* a meta page, and the page before it is + // actually a *bitmap page* that looks like a bitmap header? And + // so on. + // + // Rather than try to resolve this, in this insanely unlikely + // situation, we read from the beginning which allows us to + // always know what we're seeing, because every bitmap page + // comes *after* a bitmap header page, and thus, we know when + // we might be seeing one. + pageN, err = db.methodicalWALPageN(pageN) + if err != nil { + return err + } + } + } break } } - - // Truncate WAL to the last valid meta page. - if err := db.walFile.Truncate(int64(pageN * PageSize)); err != nil { - return fmt.Errorf("wal truncate: %w", err) - } else if _, err := db.walFile.Seek(int64(pageN*PageSize), io.SeekStart); err != nil { + if fileSize != int64(pageN*PageSize) { + if err := db.walFile.Truncate(int64(pageN * PageSize)); err != nil { + return fmt.Errorf("wal truncate: %w", err) + } + } + if _, err := db.walFile.Seek(int64(pageN*PageSize), io.SeekStart); err != nil { return fmt.Errorf("wal seek: %w", err) } db.walPageN = pageN @@ -184,6 +224,27 @@ func (db *DB) openWAL() (err error) { return nil } +// methodicalWALPageN tries to determine the last meta page in a very reliable +// but slow way. This handles the theoretical but hard to imagine creating +// edge case where we have a bitmap page which happens to look like a meta +// page, and the write got interrupted before the meta page got written. +func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) { + for i := 0; i < pageN; i++ { + var page []byte + if page, err = db.readWALPageAt(i); err != nil { + return -1, err + } + switch { + case IsMetaPage(page): + lastMeta = i + case IsBitmapHeader(page): + // skip the bitmap page, which we can't usefully evaluate + i++ + } + } + return lastMeta, nil +} + // checkpoint moves all WAL pages to the main DB file. // Must be called by a write transaction while under db.mu lock. func (db *DB) checkpoint() error { @@ -199,28 +260,53 @@ func (db *DB) checkpoint() error { if db.walPageN == 0 { return nil } + // We might have either a *PageMap or just the file. If we have the file, + // building the PageMap is fairly expensive because it's fancy and immutable. + // If we have the PageMap *or* some other map, that's two different things + // to iterate. If we have the PageMap, building a map from it is relatively + // cheap, so we'll do it that way. + pages := make(map[uint32]int) + if db.pageMap.size == 0 { + // you'd think we're done, but actually this PROBABLY means that + // this is initial startup, and we haven't read the file yet. We scan + // the file for pages, because it turns out most of them probably + // got overwritten. + for i := 0; i < db.walPageN; i++ { + page, err := db.readWALPageAt(i) + if err != nil { + return err + } + + // Determine page number. Meta pages are always on zero & bitmap + // headers specify the page number of the next page in the WAL. + // All other pages have their page number in the page data. + var pgno uint32 + if IsBitmapHeader(page) { + pgno = readPageNo(page) + if page, err = db.readWALPageAt(i + 1); err != nil { + return err + } + i++ // bitmaps in WAL are two pages + } else if !IsMetaPage(page) { + pgno = readPageNo(page) + } + // record where in the file we have this page + pages[pgno] = i + } + } else { + itr := db.pageMap.Iterator() + itr.First() + for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() { + pages[k] = int(v) + } + } // fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size) - for i := 0; i < db.walPageN; i++ { - page, err := db.readWALPageAt(i) + for pgno, walID := range pages { + page, err := db.readWALPageAt(walID) if err != nil { return err } - - // Determine page number. Meta pages are always on zero & bitmap - // headers specify the page number of the next page in the WAL. - // All other pages have their page number in the page data. - var pgno uint32 - if IsBitmapHeader(page) { - pgno = readPageNo(page) - if page, err = db.readWALPageAt(i + 1); err != nil { - return err - } - i++ // bitmaps in WAL are two pages - } else if !IsMetaPage(page) { - pgno = readPageNo(page) - } - // Write data to the data file. if err := db.writeDBPage(pgno, page); err != nil { return err @@ -509,23 +595,35 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { // running new tx. func (db *DB) removeTx(tx *Tx) error { db.mu.Lock() - defer db.mu.Unlock() - // Release writer lock if tx is writable. + // release the write lock. we have to do this for now. some day we won't, + // and will want to hold it, but right now we can't be sure we can get it. if tx.writable { tx.db.rwmu.Unlock() } - + // remove ourselves from the list of transactions the db is keeping. delete(tx.db.txs, tx) // Disassociate from db. tx.db = nil - // Write pages from WAL to DB. - // TODO(bbj): Move this to an async goroutine. + // Write pages from WAL to DB. As of this instant, we are the ONLY + // transaction, which means that no transaction has an older version + // of the PageMap than we do, and if we're a write, Commit() already + // updated the page map to our page map. So, if we *can* checkpoint, + // the checkpoint gets spawned asynchronously. We could block writes, + // except doing so will deadlock in a weird way. if len(db.txs) == 0 && db.walSize() > db.cfg.MinWALCheckpointSize { - if err := db.checkpoint(); err != nil { - return fmt.Errorf("checkpoint: %w", err) - } + // We are doing this function with the db lock held, which means + // we're *not* releasing the db lock, even though we're returning. + // This is a weird special case, and probably a bad idea. + go func() { + defer db.mu.Unlock() + if err := db.checkpoint(); err != nil { + db.logger.Errorf("async checkpoint: %w", err) + } + }() + } else { + defer db.mu.Unlock() } return nil diff --git a/rbf/rbf_test.go b/rbf/rbf_test.go index 48f16335a..be3461e37 100644 --- a/rbf/rbf_test.go +++ b/rbf/rbf_test.go @@ -11,6 +11,7 @@ import ( "sort" "testing" + "github.com/molecula/featurebase/v2/logger" "github.com/molecula/featurebase/v2/rbf" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/testhook" @@ -65,6 +66,13 @@ func NewDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB { // MustOpenDB returns a db opened on a temporary file. On error, fail test. func MustOpenDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB { tb.Helper() + if len(cfg) == 0 || cfg[0] == nil { + newconf := rbfcfg.NewDefaultConfig() + newconf.Logger = logger.NewLogfLogger(tb) + cfg = []*rbfcfg.Config{newconf} + } else if cfg[0].Logger == nil { + cfg[0].Logger = logger.NewLogfLogger(tb) + } db := NewDB(tb, cfg...) if err := db.Open(); err != nil { tb.Fatal(err) From 806669fa0f5c738a5e1d70ac749c67453540bf1c Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 15:27:16 -0600 Subject: [PATCH 15/58] make db able to fail out if it can't checkpoint, fix silly wrong-units error PageMap uses "WALID", which is a WAL page ID relative to the "base" ID of the WAL, rather than the wal page count you'd get just reading the file. So everything it reports has a fixed offset at any given time. I think this may be left over from a point where there were partial checkpoints. Anyway, the net outcome is that each new transaction was getting different page IDs, but the actual WAL pages did not always reflect that. Each checkpoint increases the offset. This might imply that we can start having problems after 4 billion pages written even if most of them were redundant? Anyway, with that fixed, this seems to work. I think. --- rbf/db.go | 76 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 26 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 830e31f5a..40c98e22a 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -49,6 +49,8 @@ type DB struct { rwmu sync.Mutex // mutex for restricting single writer haltCond *sync.Cond // condition for resuming txs after checkpoint + isDead error // this database died in an unrecoverable way, error out opens + // Path represents the path to the database file. Path string } @@ -247,7 +249,7 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) { // checkpoint moves all WAL pages to the main DB file. // Must be called by a write transaction while under db.mu lock. -func (db *DB) checkpoint() error { +func (db *DB) checkpoint() (err error) { if !db.opened { return nil } else if len(db.txs) > 0 { @@ -260,21 +262,31 @@ func (db *DB) checkpoint() error { if db.walPageN == 0 { return nil } + // wake up things waiting on haltCond when we're done, even if we fail. + // Otherwise, we deadlock with them all stuck waiting on that forever. + defer func() { + if err != nil && db.isDead == nil { + db.isDead = err + } + db.haltCond.Broadcast() + }() + var page []byte // We might have either a *PageMap or just the file. If we have the file, // building the PageMap is fairly expensive because it's fancy and immutable. // If we have the PageMap *or* some other map, that's two different things // to iterate. If we have the PageMap, building a map from it is relatively // cheap, so we'll do it that way. pages := make(map[uint32]int) + if db.pageMap.size == 0 { // you'd think we're done, but actually this PROBABLY means that // this is initial startup, and we haven't read the file yet. We scan // the file for pages, because it turns out most of them probably // got overwritten. for i := 0; i < db.walPageN; i++ { - page, err := db.readWALPageAt(i) + page, err = db.readWALPageAt(i) if err != nil { - return err + return fmt.Errorf("reading WAL page %d: %w", i, err) } // Determine page number. Meta pages are always on zero & bitmap @@ -283,8 +295,12 @@ func (db *DB) checkpoint() error { var pgno uint32 if IsBitmapHeader(page) { pgno = readPageNo(page) - if page, err = db.readWALPageAt(i + 1); err != nil { - return err + if i+1 < db.walPageN { + if page, err = db.readWALPageAt(i + 1); err != nil { + return err + } + } else { + return fmt.Errorf("last page of WAL file (%d) is bitmap header", i) } i++ // bitmaps in WAL are two pages } else if !IsMetaPage(page) { @@ -294,41 +310,37 @@ func (db *DB) checkpoint() error { pages[pgno] = i } } else { + walBase := db.baseWALID() itr := db.pageMap.Iterator() itr.First() for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() { - pages[k] = int(v) + pages[k] = int(v - walBase - 1) } } // fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size) for pgno, walID := range pages { - page, err := db.readWALPageAt(walID) + page, err = db.readWALPageAt(walID) if err != nil { - return err + return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err) } // Write data to the data file. - if err := db.writeDBPage(pgno, page); err != nil { - return err + if err = db.writeDBPage(pgno, page); err != nil { + return fmt.Errorf("writing page %d: %v", pgno, err) } } - // Ensure database file is synced and then truncate the WAL file. - if err := db.fsync(db.file); err != nil { + if err = db.fsync(db.file); err != nil { return fmt.Errorf("db file sync: %w", err) - } else if err := db.walFile.Truncate(0); err != nil { + } else if err = db.walFile.Truncate(0); err != nil { return fmt.Errorf("truncate wal file: %w", err) - } else if err := db.fsync(db.walFile); err != nil { + } else if err = db.fsync(db.walFile); err != nil { return fmt.Errorf("wal file sync: %w", err) - } else if _, err := db.walFile.Seek(0, io.SeekStart); err != nil { + } else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil { return fmt.Errorf("seek wal file: %w", err) } db.walPageN = 0 db.pageMap = NewPageMap() - - // Notify halted transactions that the WAL has been checkpointed. - db.haltCond.Broadcast() - return nil } @@ -537,9 +549,21 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { cleanup() return nil, ErrClosed } + if db.isDead != nil { + err := db.isDead + cleanup() + db.mu.Unlock() + return nil, err + } // Wait for WAL size to be below threshold. for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize { + if db.isDead != nil { + err := db.isDead + cleanup() + db.mu.Unlock() + return nil, err + } db.haltCond.Wait() } @@ -616,14 +640,14 @@ func (db *DB) removeTx(tx *Tx) error { // We are doing this function with the db lock held, which means // we're *not* releasing the db lock, even though we're returning. // This is a weird special case, and probably a bad idea. - go func() { - defer db.mu.Unlock() - if err := db.checkpoint(); err != nil { - db.logger.Errorf("async checkpoint: %w", err) - } - }() - } else { + // go func() { defer db.mu.Unlock() + if err := db.checkpoint(); err != nil { + db.logger.Errorf("async checkpoint: %v", err) + } + // }() + } else { + db.mu.Unlock() } return nil From 6d68e719338bb825ec1ff537b35b9538ec1581b4 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 19 Nov 2021 15:32:58 -0600 Subject: [PATCH 16/58] make the checkpoint async --- rbf/db.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 40c98e22a..9d4769ff3 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -640,12 +640,12 @@ func (db *DB) removeTx(tx *Tx) error { // We are doing this function with the db lock held, which means // we're *not* releasing the db lock, even though we're returning. // This is a weird special case, and probably a bad idea. - // go func() { - defer db.mu.Unlock() - if err := db.checkpoint(); err != nil { - db.logger.Errorf("async checkpoint: %v", err) - } - // }() + go func() { + defer db.mu.Unlock() + if err := db.checkpoint(); err != nil { + db.logger.Errorf("async checkpoint: %v", err) + } + }() } else { db.mu.Unlock() } From c3c02eabb0587a37a58fcf7e99d3b3016a6b8885 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 22 Nov 2021 14:02:53 -0600 Subject: [PATCH 17/58] almost but not quite support async checkpoint This gets us to being able to run reads during a checkpoint, but now we have to wait for new reads to end before we can release the write lock, etc. This is actually slightly slower, but if we could get ONE more step, we could allow new writes during that phase, to a different WAL, if we had a different WAL to write to. --- rbf/db.go | 157 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 126 insertions(+), 31 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 9d4769ff3..3db8889ca 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -28,6 +28,17 @@ var cursorSyncPool = &sync.Pool{ }, } +// txWaiter is a representation of "i need to wait for txs to complete". +// it is created with a function, and will run that function, with the db +// lock held, at some point after every Tx that was open when it was created +// has closed. WARNING: A txWaiter may hold db.rwmu. +type txWaiter struct { + mu sync.Mutex + cond *sync.Cond + waitingOn map[*Tx]struct{} + callback func() +} + // DB options like MaxSize, FsyncEnabled, DoAllocZero // can be set before calling DB.Open(). type DB struct { @@ -49,6 +60,8 @@ type DB struct { rwmu sync.Mutex // mutex for restricting single writer haltCond *sync.Cond // condition for resuming txs after checkpoint + txWaiters []*txWaiter // things waiting for Txs to close + isDead error // this database died in an unrecoverable way, error out opens // Path represents the path to the database file. @@ -142,8 +155,12 @@ func (db *DB) Open() (err error) { // Open write-ahead log & checkpoint to the end since no transactions are open. if err := db.openWAL(); err != nil { return fmt.Errorf("wal open: %w", err) - } else if err := db.checkpoint(); err != nil { - return fmt.Errorf("startup checkpoint: %w", err) + } else { + // checkpoint wants to hold the rwmu lock. + db.rwmu.Lock() + if err := db.checkpoint(); err != nil { + return fmt.Errorf("startup checkpoint: %w", err) + } } return nil @@ -247,9 +264,18 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) { return lastMeta, nil } -// checkpoint moves all WAL pages to the main DB file. -// Must be called by a write transaction while under db.mu lock. +// checkpoint moves all WAL pages to the main DB file. Must be called +// while holding both db.mu and db.rwmu. Should release db.rwmu, but not +// db.mu. func (db *DB) checkpoint() (err error) { + // if we don't spin off a possible async waiter, we should release the + // write lock, if we do, that will release it. + releaseLock := true + defer func() { + if releaseLock { + db.rwmu.Unlock() + } + }() if !db.opened { return nil } else if len(db.txs) > 0 { @@ -332,15 +358,26 @@ func (db *DB) checkpoint() (err error) { // Ensure database file is synced and then truncate the WAL file. if err = db.fsync(db.file); err != nil { return fmt.Errorf("db file sync: %w", err) - } else if err = db.walFile.Truncate(0); err != nil { - return fmt.Errorf("truncate wal file: %w", err) - } else if err = db.fsync(db.walFile); err != nil { - return fmt.Errorf("wal file sync: %w", err) - } else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil { - return fmt.Errorf("seek wal file: %w", err) } - db.walPageN = 0 - db.pageMap = NewPageMap() + // now we've updated the file. There are existing transactions that are still + // using the WAL, though. So we wait for them to terminate before we unlock + // the rwmu and update the metadata about the WAL. + releaseLock = false + // fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n") + db.afterCurrentTx(func() { + // fmt.Printf("truncating WAL\n") + defer db.rwmu.Unlock() + if err = db.walFile.Truncate(0); err != nil { + db.logger.Errorf("truncate wal file: %w", err) + } else if err = db.fsync(db.walFile); err != nil { + db.logger.Errorf("wal file sync: %w", err) + } else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil { + db.logger.Errorf("seek wal file: %w", err) + } + db.walPageN = 0 + db.pageMap = NewPageMap() + // fmt.Printf("checkpoint actually done\n") + }) return nil } @@ -613,43 +650,101 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { return tx, nil } +// afterCurrentTx produces runs the provided callback, with the db lock +// held, after all current Tx terminate. It should be called with the db +// lock held. +func (db *DB) afterCurrentTx(callback func()) { + if len(db.txs) == 0 { + callback() + return + } + txw := &txWaiter{} + txw.cond = sync.NewCond(&txw.mu) + txw.callback = callback + txw.waitingOn = make(map[*Tx]struct{}, len(db.txs)) + for k := range db.txs { + txw.waitingOn[k] = struct{}{} + } + db.txWaiters = append(db.txWaiters, txw) + txw.mu.Lock() + go func() { + for len(txw.waitingOn) > 0 { + // fmt.Printf("afterCurrentTx: %d left\n", len(txw.waitingOn)) + txw.cond.Wait() + } + // fmt.Printf("afterCurrentTx: locking db\n") + db.mu.Lock() + defer db.mu.Unlock() + // remove us from the db's list + for i, v := range db.txWaiters { + if v == txw { + // remove us from the list + copy(db.txWaiters[i:], db.txWaiters[i+1:]) + db.txWaiters = db.txWaiters[:len(db.txWaiters)-1] + break + } + } + // fmt.Printf("afterCurrentTx: running callback\n") + txw.callback() + }() + return +} + // removeTx removes an active transaction from the database. it obtains // the db lock, and currently drops it, but will later possibly be leaving // it retained by an asynchronous op that wants to happen before we start // running new tx. func (db *DB) removeTx(tx *Tx) error { db.mu.Lock() - // release the write lock. we have to do this for now. some day we won't, - // and will want to hold it, but right now we can't be sure we can get it. + defer db.mu.Unlock() + // We might want to trigger a checkpoint. Only for writable + // transactions, and only when either there's nothing else open or we + // really need to. + checkpoint := false if tx.writable { - tx.db.rwmu.Unlock() + walSize := db.walSize() + if walSize > db.cfg.MinWALCheckpointSize { + // Might be a good time for a checkpoint. We'll do a checkpoint + // if we're the only transaction, or if we have to. + if len(db.txs) == 1 || walSize > db.cfg.MaxWALCheckpointSize { + checkpoint = true + } + } + // During checkpointing, we'll be preventing writes, but allowing reads. + if !checkpoint { + tx.db.rwmu.Unlock() + } } // remove ourselves from the list of transactions the db is keeping. delete(tx.db.txs, tx) + for _, txw := range tx.db.txWaiters { + delete(txw.waitingOn, tx) + // let it know we're done. we've still got db.mu.lock, so it won't + // happen just yet, but it'll be able to continue. + if len(txw.waitingOn) == 0 { + txw.cond.Broadcast() + } + } // Disassociate from db. tx.db = nil - // Write pages from WAL to DB. As of this instant, we are the ONLY - // transaction, which means that no transaction has an older version - // of the PageMap than we do, and if we're a write, Commit() already - // updated the page map to our page map. So, if we *can* checkpoint, - // the checkpoint gets spawned asynchronously. We could block writes, - // except doing so will deadlock in a weird way. - if len(db.txs) == 0 && db.walSize() > db.cfg.MinWALCheckpointSize { - // We are doing this function with the db lock held, which means - // we're *not* releasing the db lock, even though we're returning. - // This is a weird special case, and probably a bad idea. - go func() { - defer db.mu.Unlock() + if checkpoint { + // We need to run a checkpoint. This can be semi-asynchronous. + // It needs to wait until every existing transaction has finished, + // because every existing transaction could want to look up pages + // which are in the database before our operations, but which should + // now be in the WAL. We want them to use the WAL instead. + // fmt.Printf("possibly-async checkpoint...\n") + db.afterCurrentTx(func() { + // We still hold db.rwmu here. checkpoint unlocks it when it's + // ready. + // fmt.Printf("checkpoint starting\n") if err := db.checkpoint(); err != nil { db.logger.Errorf("async checkpoint: %v", err) } - }() - } else { - db.mu.Unlock() + }) } - return nil } From 4279e2cb2d8010639de375021a99e40a64538e9f Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Thu, 16 Dec 2021 09:17:13 -0700 Subject: [PATCH 18/58] rebase fixes --- rbf/cursor_test.go | 4 ++-- rbf/db.go | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/rbf/cursor_test.go b/rbf/cursor_test.go index 4f7464bd1..940798c04 100644 --- a/rbf/cursor_test.go +++ b/rbf/cursor_test.go @@ -973,8 +973,8 @@ func TestCursor_SplitBranchCells(t *testing.T) { } // c, _ := tx.Cursor("x") //added just for dot code coverage - c.Dump("ignore for coverage") - + c.Dump("test.dump") + os.Remove("test.dump") } func TestCursor_RemoveCells(t *testing.T) { diff --git a/rbf/db.go b/rbf/db.go index 3db8889ca..21596d144 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -695,8 +695,6 @@ func (db *DB) afterCurrentTx(callback func()) { // it retained by an asynchronous op that wants to happen before we start // running new tx. func (db *DB) removeTx(tx *Tx) error { - db.mu.Lock() - defer db.mu.Unlock() // We might want to trigger a checkpoint. Only for writable // transactions, and only when either there's nothing else open or we // really need to. From 29f5f6d7c2dc84aebfae9d4a67bf9f7e65bcb3c2 Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 15 Dec 2021 12:09:40 -0600 Subject: [PATCH 19/58] copy things rows after getting them and before their finishers during writes When a qcx is a write, every Tx under it closes immediately, thus invalidating all returned data. Thus, if you do a Not() inside a Store(), you're doing a difference on an existence row and some other row call... and both of those rows were run, individually, as separate transactions that got invalidated the moment they were fetched. Oops. --- executor.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index 226759b55..8b0409eb7 100644 --- a/executor.go +++ b/executor.go @@ -4493,7 +4493,11 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string, return nil, err } defer finisher(&err0) - return frag.row(tx, rowID) + row, err := frag.row(tx, rowID) + if qcx.write && err == nil { + row = row.Clone() + } + return row, err } // If no quantum exists then return an empty bitmap. @@ -4532,15 +4536,21 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string, if len(rows) == 0 { return &Row{}, nil } else if len(rows) == 1 { + if qcx.write { + return rows[0].Clone(), nil + } return rows[0], nil } row := rows[0].Union(rows[1:]...) + if qcx.write { + row = row.Clone() + } return row, nil } // executeRowBSIGroupShard executes a range(bsiGroup) call for a local shard. -func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shard uint64) (_ *Row, err0 error) { +func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shard uint64) (cloneable *Row, err0 error) { span, _ := tracing.StartSpanFromContext(ctx, "Executor.executeRowBSIGroupShard") defer span.Finish() @@ -4572,6 +4582,11 @@ func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index return nil, err } defer finisher(&err0) + defer func() { + if qcx.write && cloneable != nil { + cloneable = cloneable.Clone() + } + }() // EQ null _exists - frag.NotNull() // NEQ null frag.NotNull() @@ -4822,6 +4837,9 @@ func (e *executor) executeNotShard(ctx context.Context, qcx *Qcx, index string, if existenceRow, err = existenceFrag.row(tx, 0); err != nil { return nil, err } + if qcx.write { + existenceRow = existenceRow.Clone() + } } // the finishers returned by a write tx, which we might be in if there's // a higher-level write in this call OR ANY OTHER CALL, are safe to From 5764d98f6d0198b2b61b32f5e336de5d59b110d2 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 16 Dec 2021 11:06:17 -0600 Subject: [PATCH 20/58] test fixes and order of operations on changing db.PageMap We need to update db.PageMap after we write the db, but before we truncate the WAL, so new transactions don't pick up the old PageMap and then get a truncated WAL. Also, checkpoint should not abort if there's txs -- that's okay now. --- rbf/db.go | 30 +++++++++++++++++------------- rbf/db_test.go | 6 ++++-- rbf/tx.go | 5 ++--- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 21596d144..cfa5ede43 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -278,8 +278,6 @@ func (db *DB) checkpoint() (err error) { }() if !db.opened { return nil - } else if len(db.txs) > 0 { - return nil // skip if transactions open } // Check if there are any WAL pages, if not do nothing as @@ -364,6 +362,8 @@ func (db *DB) checkpoint() (err error) { // the rwmu and update the metadata about the WAL. releaseLock = false // fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n") + db.walPageN = 0 + db.pageMap = NewPageMap() db.afterCurrentTx(func() { // fmt.Printf("truncating WAL\n") defer db.rwmu.Unlock() @@ -374,8 +374,6 @@ func (db *DB) checkpoint() (err error) { } else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil { db.logger.Errorf("seek wal file: %w", err) } - db.walPageN = 0 - db.pageMap = NewPageMap() // fmt.Printf("checkpoint actually done\n") }) return nil @@ -589,19 +587,22 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { if db.isDead != nil { err := db.isDead cleanup() - db.mu.Unlock() return nil, err } - // Wait for WAL size to be below threshold. - for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize { - if db.isDead != nil { - err := db.isDead - cleanup() - db.mu.Unlock() - return nil, err + // Wait for WAL size to be below threshold, if we're going to write. + // Reads don't care. + if writable { + for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize { + if db.isDead != nil { + err := db.isDead + cleanup() + return nil, err + } + // This implicitly releases db.mu.Lock and comes back with it + // held again. + db.haltCond.Wait() } - db.haltCond.Wait() } tx := &Tx{ @@ -775,6 +776,9 @@ func (db *DB) baseWALID() int64 { // readWALPageByID reads a WAL page by WAL ID. func (db *DB) readWALPageByID(id int64) ([]byte, error) { + if id == db.baseWALID() { + fmt.Printf("id %d oops\n", id) + } return db.readWALPageAt(int(id - db.baseWALID() - 1)) } diff --git a/rbf/db_test.go b/rbf/db_test.go index 5c8504b1b..c0eb3b3c7 100644 --- a/rbf/db_test.go +++ b/rbf/db_test.go @@ -291,7 +291,8 @@ func TestDB_MultiTx(t *testing.T) { time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond) - for i := 0; i < rand.Intn(1000); i++ { + n := rand.Intn(500) + 500 + for i := 0; i < n; i++ { v := rand.Intn(1 << 20) if _, err := tx.Contains("x", uint64(v)); err != nil { return err @@ -316,7 +317,8 @@ func TestDB_MultiTx(t *testing.T) { } defer tx.Rollback() - for j := 0; j < rand.Intn(100); j++ { + n := rand.Intn(90) + 10 + for j := 0; j < n; j++ { v := rand.Intn(1 << 20) if _, err := tx.Add("x", uint64(v)); err != nil { t.Fatal(err) diff --git a/rbf/tx.go b/rbf/tx.go index 2cf7420db..bfb8fc00c 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -102,6 +102,8 @@ func (tx *Tx) Commit() error { // If any pages have been written, ensure we write a new meta page with // the commit flag to mark the end of the transaction. + tx.db.mu.Lock() + defer tx.db.mu.Unlock() if tx.dirty() { if err := tx.flush(); err != nil { return err @@ -118,12 +120,9 @@ func (tx *Tx) Commit() error { // the lock, because we need removeTx to grab the lock to // work, but if it wants to checkpoint, it wants to be able to return // to us here and still be holding the lock. - tx.db.mu.Lock() tx.db.rootRecords = tx.rootRecords tx.db.pageMap = tx.pageMap tx.db.walPageN = tx.walPageN - tx.db.mu.Unlock() - return tx.db.removeTx(tx) } // Disconnect transaction from DB. From 994cc03e88717642f3b06614eef6d7afa859329b Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 16 Dec 2021 14:01:38 -0600 Subject: [PATCH 21/58] fix locking and list management for afterCurrentTx Two issues: First, there was a race condition because we were never using the mutex for anything but the condvar broadcast, second, there was no reason for the afterCurrentTx to need to maintain the list since we already know where in the list we are when we are waking it up. afterCurrentTx still wants to run with the db lock held, because the degenerate case (no outstanding Tx) means that it will be running with it held already. That's for another commit. --- rbf/db.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index cfa5ede43..0662f59d6 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -676,15 +676,6 @@ func (db *DB) afterCurrentTx(callback func()) { // fmt.Printf("afterCurrentTx: locking db\n") db.mu.Lock() defer db.mu.Unlock() - // remove us from the db's list - for i, v := range db.txWaiters { - if v == txw { - // remove us from the list - copy(db.txWaiters[i:], db.txWaiters[i+1:]) - db.txWaiters = db.txWaiters[:len(db.txWaiters)-1] - break - } - } // fmt.Printf("afterCurrentTx: running callback\n") txw.callback() }() @@ -716,12 +707,23 @@ func (db *DB) removeTx(tx *Tx) error { } // remove ourselves from the list of transactions the db is keeping. delete(tx.db.txs, tx) - for _, txw := range tx.db.txWaiters { + for i := 0; i < len(tx.db.txWaiters); i++ { + txw := tx.db.txWaiters[i] + // in practice this probably never matters, but theoretically the + // goroutine that's waiting on the condition variable may + // not have performed its first test on len(txw.waitingOn) yet. + txw.mu.Lock() delete(txw.waitingOn, tx) + txw.mu.Unlock() // let it know we're done. we've still got db.mu.lock, so it won't // happen just yet, but it'll be able to continue. if len(txw.waitingOn) == 0 { + // remove us from the db's list + copy(db.txWaiters[i:], db.txWaiters[i+1:]) + db.txWaiters = db.txWaiters[:len(db.txWaiters)-1] txw.cond.Broadcast() + // decrement i so we don't skip an entry we just copied in to [i] + i-- } } From 47e098c3b1a84c4e0108b10ba050ddf37381448e Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 16 Dec 2021 15:54:06 -0600 Subject: [PATCH 22/58] simplify txWaiter We don't need a condition variable for a thing with a single waiter which waits only once, and a data structure which only one side ever modifies. That's a closable channel. --- rbf/db.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 0662f59d6..4129ac1fe 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -33,8 +33,7 @@ var cursorSyncPool = &sync.Pool{ // lock held, at some point after every Tx that was open when it was created // has closed. WARNING: A txWaiter may hold db.rwmu. type txWaiter struct { - mu sync.Mutex - cond *sync.Cond + ready chan struct{} waitingOn map[*Tx]struct{} callback func() } @@ -660,19 +659,15 @@ func (db *DB) afterCurrentTx(callback func()) { return } txw := &txWaiter{} - txw.cond = sync.NewCond(&txw.mu) + txw.ready = make(chan struct{}) txw.callback = callback txw.waitingOn = make(map[*Tx]struct{}, len(db.txs)) for k := range db.txs { txw.waitingOn[k] = struct{}{} } db.txWaiters = append(db.txWaiters, txw) - txw.mu.Lock() go func() { - for len(txw.waitingOn) > 0 { - // fmt.Printf("afterCurrentTx: %d left\n", len(txw.waitingOn)) - txw.cond.Wait() - } + <-txw.ready // fmt.Printf("afterCurrentTx: locking db\n") db.mu.Lock() defer db.mu.Unlock() @@ -712,16 +707,14 @@ func (db *DB) removeTx(tx *Tx) error { // in practice this probably never matters, but theoretically the // goroutine that's waiting on the condition variable may // not have performed its first test on len(txw.waitingOn) yet. - txw.mu.Lock() delete(txw.waitingOn, tx) - txw.mu.Unlock() // let it know we're done. we've still got db.mu.lock, so it won't // happen just yet, but it'll be able to continue. if len(txw.waitingOn) == 0 { // remove us from the db's list copy(db.txWaiters[i:], db.txWaiters[i+1:]) db.txWaiters = db.txWaiters[:len(db.txWaiters)-1] - txw.cond.Broadcast() + close(txw.ready) // decrement i so we don't skip an entry we just copied in to [i] i-- } From 57ca5591a264740daddb7e819f745f71c9163951 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Fri, 17 Dec 2021 10:59:09 -0700 Subject: [PATCH 23/58] Unlock rbf.DB during WAL copy & fsync() --- rbf/db.go | 149 +++++++++++++++++++++++++++++------------------------- 1 file changed, 79 insertions(+), 70 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index 4129ac1fe..e5a81690e 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -51,9 +51,10 @@ type DB struct { opened bool // true if open logger logger.Logger // for diagnostics from async things - wal []byte // wal mmap - walFile *os.File // wal file descriptor - walPageN int // wal page count + wal []byte // wal mmap + walFile *os.File // wal file descriptor + walPageN int // wal page count + baseWALID int64 // WAL ID of first page mu sync.RWMutex // general mutex rwmu sync.Mutex // mutex for restricting single writer @@ -238,6 +239,7 @@ func (db *DB) openWAL() (err error) { return fmt.Errorf("wal seek: %w", err) } db.walPageN = pageN + db.baseWALID = readMetaWALID(db.data) return nil } @@ -293,79 +295,92 @@ func (db *DB) checkpoint() (err error) { } db.haltCond.Broadcast() }() - var page []byte - // We might have either a *PageMap or just the file. If we have the file, - // building the PageMap is fairly expensive because it's fancy and immutable. - // If we have the PageMap *or* some other map, that's two different things - // to iterate. If we have the PageMap, building a map from it is relatively - // cheap, so we'll do it that way. - pages := make(map[uint32]int) - if db.pageMap.size == 0 { - // you'd think we're done, but actually this PROBABLY means that - // this is initial startup, and we haven't read the file yet. We scan - // the file for pages, because it turns out most of them probably - // got overwritten. - for i := 0; i < db.walPageN; i++ { - page, err = db.readWALPageAt(i) - if err != nil { - return fmt.Errorf("reading WAL page %d: %w", i, err) - } + // Copy the pages from the WAL back to the database outside of the lock. + if err := func() error { + db.mu.Unlock() // This is intentionally reversed so run w/o lock + defer db.mu.Lock() - // Determine page number. Meta pages are always on zero & bitmap - // headers specify the page number of the next page in the WAL. - // All other pages have their page number in the page data. - var pgno uint32 - if IsBitmapHeader(page) { - pgno = readPageNo(page) - if i+1 < db.walPageN { - if page, err = db.readWALPageAt(i + 1); err != nil { - return err - } - } else { - return fmt.Errorf("last page of WAL file (%d) is bitmap header", i) + var page []byte + // We might have either a *PageMap or just the file. If we have the file, + // building the PageMap is fairly expensive because it's fancy and immutable. + // If we have the PageMap *or* some other map, that's two different things + // to iterate. If we have the PageMap, building a map from it is relatively + // cheap, so we'll do it that way. + pages := make(map[uint32]int) + + if db.pageMap.size == 0 { + // you'd think we're done, but actually this PROBABLY means that + // this is initial startup, and we haven't read the file yet. We scan + // the file for pages, because it turns out most of them probably + // got overwritten. + for i := 0; i < db.walPageN; i++ { + page, err = db.readWALPageAt(i) + if err != nil { + return fmt.Errorf("reading WAL page %d: %w", i, err) } - i++ // bitmaps in WAL are two pages - } else if !IsMetaPage(page) { - pgno = readPageNo(page) + + // Determine page number. Meta pages are always on zero & bitmap + // headers specify the page number of the next page in the WAL. + // All other pages have their page number in the page data. + var pgno uint32 + if IsBitmapHeader(page) { + pgno = readPageNo(page) + if i+1 < db.walPageN { + if page, err = db.readWALPageAt(i + 1); err != nil { + return err + } + } else { + return fmt.Errorf("last page of WAL file (%d) is bitmap header", i) + } + i++ // bitmaps in WAL are two pages + } else if !IsMetaPage(page) { + pgno = readPageNo(page) + } + // record where in the file we have this page + pages[pgno] = i + } + } else { + itr := db.pageMap.Iterator() + itr.First() + for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() { + pages[k] = int(v - db.baseWALID - 1) } - // record where in the file we have this page - pages[pgno] = i } - } else { - walBase := db.baseWALID() - itr := db.pageMap.Iterator() - itr.First() - for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() { - pages[k] = int(v - walBase - 1) + + // fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size) + for pgno, walID := range pages { + page, err = db.readWALPageAt(walID) + if err != nil { + return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err) + } + + // Write data to the data file. + if err = db.writeDBPage(pgno, page); err != nil { + return fmt.Errorf("writing page %d: %v", pgno, err) + } } + + // Ensure database file is synced and then truncate the WAL file. + if err = db.fsync(db.file); err != nil { + return fmt.Errorf("db file sync: %w", err) + } + + return nil + }(); err != nil { + return err } - // fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size) - for pgno, walID := range pages { - page, err = db.readWALPageAt(walID) - if err != nil { - return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err) - } - // Write data to the data file. - if err = db.writeDBPage(pgno, page); err != nil { - return fmt.Errorf("writing page %d: %v", pgno, err) - } - } - // Ensure database file is synced and then truncate the WAL file. - if err = db.fsync(db.file); err != nil { - return fmt.Errorf("db file sync: %w", err) - } // now we've updated the file. There are existing transactions that are still // using the WAL, though. So we wait for them to terminate before we unlock // the rwmu and update the metadata about the WAL. releaseLock = false - // fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n") db.walPageN = 0 db.pageMap = NewPageMap() + db.afterCurrentTx(func() { - // fmt.Printf("truncating WAL\n") defer db.rwmu.Unlock() + if err = db.walFile.Truncate(0); err != nil { db.logger.Errorf("truncate wal file: %w", err) } else if err = db.fsync(db.walFile); err != nil { @@ -373,8 +388,10 @@ func (db *DB) checkpoint() (err error) { } else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil { db.logger.Errorf("seek wal file: %w", err) } - // fmt.Printf("checkpoint actually done\n") + + db.baseWALID = readMetaWALID(db.data) }) + return nil } @@ -764,17 +781,9 @@ func (db *DB) readDBPage(pgno uint32) ([]byte, error) { return db.data[offset : offset+PageSize], nil } -// baseWALID returns the WAL ID stored in the database file meta page. -func (db *DB) baseWALID() int64 { - return readMetaWALID(db.data) -} - // readWALPageByID reads a WAL page by WAL ID. func (db *DB) readWALPageByID(id int64) ([]byte, error) { - if id == db.baseWALID() { - fmt.Printf("id %d oops\n", id) - } - return db.readWALPageAt(int(id - db.baseWALID() - 1)) + return db.readWALPageAt(int(id - db.baseWALID - 1)) } // readWALPageAt reads the i-th page in the WAL file. From 1fd872b126356c07cac635888590067bf7f0682f Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 17 Dec 2021 12:55:32 -0600 Subject: [PATCH 24/58] less write locks in fragment.importRoaring/row --- ctl/server.go | 2 +- fragment.go | 75 ++++++++++++++++++++++++------------------------ server.go | 1 - server/config.go | 6 ++-- server/server.go | 2 +- 5 files changed, 42 insertions(+), 44 deletions(-) diff --git a/ctl/server.go b/ctl/server.go index 83edb5456..c5da42847 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -85,7 +85,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.BoolVar(&srv.Config.Storage.FsyncEnabled, "storage.fsync", true, "enable fsync fully safe flush-to-disk") // RowcacheOn - flags.BoolVar((&srv.Config.RowcacheOn), "rowcache-on", srv.Config.RowcacheOn, "turn on the rowcache for all backends (may speed some queries)") + flags.BoolVar((&srv.Config.RowcacheOn), "rowcache-on", srv.Config.RowcacheOn, "Do not use, permanently disabled. Flag exists for backwards compatibility and will be removed.") // RBF specific flags. See pilosa/rbf/cfg/cfg.go for definitions. srv.Config.RBFConfig.DefineFlags(flags) diff --git a/fragment.go b/fragment.go index 83514edad..9b77f90f7 100644 --- a/fragment.go +++ b/fragment.go @@ -593,8 +593,8 @@ func (f *fragment) mutexCheck(tx Tx, details bool, limit int) (map[uint64][]uint // row returns a row by ID. func (f *fragment) row(tx Tx, rowID uint64) (*Row, error) { - f.mu.Lock() - defer f.mu.Unlock() + f.mu.RLock() + defer f.mu.RUnlock() return f.unprotectedRow(tx, rowID) } @@ -937,9 +937,12 @@ func (f *fragment) unprotectedClearRow(tx Tx, rowID uint64) (changed bool, err e return changed, nil } -// unprotectedClearBlock clears all rows for a given block. +// clearBlock clears all rows for a given block. // This updates both the on-disk storage and the in-cache bitmap. -func (f *fragment) unprotectedClearBlock(tx Tx, block int) (changed bool, err error) { +func (f *fragment) clearBlock(tx Tx, block int) (changed bool, err error) { + f.mu.Lock() + defer f.mu.Unlock() + firstRow := uint64(block * HashBlockSize) var wp *io.Writer if f.storage != nil { @@ -2708,20 +2711,24 @@ func (f *fragment) importValue(tx Tx, columnIDs []uint64, values []int64, bitDep func (f *fragment) importRoaring(ctx context.Context, tx Tx, data []byte, clear bool) error { span, ctx := tracing.StartSpanFromContext(ctx, "fragment.importRoaring") defer span.Finish() - span, ctx = tracing.StartSpanFromContext(ctx, "importRoaring.AcquireFragmentLock") - f.mu.Lock() - defer f.mu.Unlock() - span.Finish() - return f.unprotectedImportRoaring(ctx, tx, data, clear) + rowSet, updateCache, err := f.doImportRoaring(ctx, tx, data, clear) + if err != nil { + return errors.Wrap(err, "doImportRoaring") + } + if updateCache { + return f.updateCachePostImport(ctx, rowSet) + } + return nil } -func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []byte, clear bool) error { +func (f *fragment) doImportRoaring(ctx context.Context, tx Tx, data []byte, clear bool) (map[uint64]int, bool, error) { + f.mu.RLock() + defer f.mu.RUnlock() rowSize := uint64(1 << shardVsContainerExponent) span, ctx := tracing.StartSpanFromContext(ctx, "importRoaring.ImportRoaringBits") + defer span.Finish() - useRowCache := storage.RowCacheEnabled() - var changed int var rowSet map[uint64]int var wp *io.Writer if f.storage != nil { @@ -2734,37 +2741,37 @@ func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []b return err } - changed, rowSet, err = tx.ImportRoaringBits(f.index(), f.field(), f.view(), f.shard, rit, clear, true, rowSize) + _, rowSet, err = tx.ImportRoaringBits(f.index(), f.field(), f.view(), f.shard, rit, clear, true, rowSize) return err }) - span.Finish() if err != nil { - return err + return nil, false, err } updateCache := f.CacheType != CacheTypeNone + return rowSet, updateCache, err +} + +func (f *fragment) updateCachePostImport(ctx context.Context, rowSet map[uint64]int) error { + f.mu.Lock() + defer f.mu.Unlock() anyChanged := false for rowID, changes := range rowSet { if changes == 0 { continue } - if useRowCache && f.rowCache != nil { - f.rowCache.Add(rowID, nil) - } - if updateCache { - anyChanged = true - if changes < 0 { - absChanges := uint64(-1 * changes) - if absChanges <= f.cache.Get(rowID) { - f.cache.BulkAdd(rowID, f.cache.Get(rowID)-absChanges) - } else { - f.cache.BulkAdd(rowID, 0) - } + anyChanged = true + if changes < 0 { + absChanges := uint64(-1 * changes) + if absChanges <= f.cache.Get(rowID) { + f.cache.BulkAdd(rowID, f.cache.Get(rowID)-absChanges) } else { - f.cache.BulkAdd(rowID, f.cache.Get(rowID)+uint64(changes)) + f.cache.BulkAdd(rowID, 0) } + } else { + f.cache.BulkAdd(rowID, f.cache.Get(rowID)+uint64(changes)) } } // we only set this if we need to update the cache @@ -2772,26 +2779,18 @@ func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []b f.cache.Invalidate() } - span, _ = tracing.StartSpanFromContext(ctx, "importRoaring.incrementOpN") - - f.incrementOpN(changed) - - span.Finish() return nil } // importRoaringOverwrite overwrites the specified block with the provided data. func (f *fragment) importRoaringOverwrite(ctx context.Context, tx Tx, data []byte, block int) error { - f.mu.Lock() - defer f.mu.Unlock() - // Clear the existing data from fragment block. - if _, err := f.unprotectedClearBlock(tx, block); err != nil { + if _, err := f.clearBlock(tx, block); err != nil { return errors.Wrapf(err, "clearing block: %d", block) } // Union the new block data with the fragment data. - return f.unprotectedImportRoaring(ctx, tx, data, false) + return f.importRoaring(ctx, tx, data, false) } // incrementOpN increase the operation count by one. diff --git a/server.go b/server.go index 8858ab122..0c74a2fa5 100644 --- a/server.go +++ b/server.go @@ -476,7 +476,6 @@ func NewServer(opts ...ServerOption) (*Server, error) { } s.holder = NewHolder(path, s.holderConfig) s.holder.Stats.SetLogger(s.logger) - s.holder.Logger.Infof("RowCacheOn: %v", s.holderConfig.RowcacheOn) cwd, err := os.Getwd() if err != nil { return nil, err diff --git a/server/config.go b/server/config.go index c215d1596..09d82bfdb 100644 --- a/server/config.go +++ b/server/config.go @@ -200,9 +200,9 @@ type Config struct { // "rbf". Storage *storage.Config `toml:"storage"` - // RowcacheOn, if true, turns on the row cache for all storage backends. - // The default is now off because it makes rbf queries faster and uses - // much less memory. + // RowcacheOn permanently disabled. No longer useful w/ RBF. Left + // for backward compatibility but will be removed in a future + // version. RowcacheOn bool `toml:"rowcache-on"` // RBFConfig defines all externally configurable RBF flags. diff --git a/server/server.go b/server/server.go index a6d0049ae..b373d9e35 100644 --- a/server/server.go +++ b/server/server.go @@ -482,7 +482,7 @@ func (m *Command) SetupServer() error { pilosa.OptServerClusterName(m.Config.Cluster.Name), pilosa.OptServerSerializer(proto.Serializer{}), pilosa.OptServerStorageConfig(m.Config.Storage), - pilosa.OptServerRowcacheOn(m.Config.RowcacheOn), + pilosa.OptServerRowcacheOn(false), pilosa.OptServerRBFConfig(m.Config.RBFConfig), pilosa.OptServerMaxQueryMemory(m.Config.MaxQueryMemory), pilosa.OptServerQueryHistoryLength(m.Config.QueryHistoryLength), From 07998622667818811715d993c1f220820a751435 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Fri, 17 Dec 2021 14:50:08 -0600 Subject: [PATCH 25/58] move some locks, nbd --- rbf/db.go | 4 +++- rbf/tx.go | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index e5a81690e..f6b9ce5bc 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -380,6 +380,9 @@ func (db *DB) checkpoint() (err error) { db.afterCurrentTx(func() { defer db.rwmu.Unlock() + db.baseWALID = readMetaWALID(db.data) + db.mu.Unlock() + defer db.mu.Lock() if err = db.walFile.Truncate(0); err != nil { db.logger.Errorf("truncate wal file: %w", err) @@ -389,7 +392,6 @@ func (db *DB) checkpoint() (err error) { db.logger.Errorf("seek wal file: %w", err) } - db.baseWALID = readMetaWALID(db.data) }) return nil diff --git a/rbf/tx.go b/rbf/tx.go index bfb8fc00c..bceabd8f1 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -102,8 +102,6 @@ func (tx *Tx) Commit() error { // If any pages have been written, ensure we write a new meta page with // the commit flag to mark the end of the transaction. - tx.db.mu.Lock() - defer tx.db.mu.Unlock() if tx.dirty() { if err := tx.flush(); err != nil { return err @@ -120,11 +118,15 @@ func (tx *Tx) Commit() error { // the lock, because we need removeTx to grab the lock to // work, but if it wants to checkpoint, it wants to be able to return // to us here and still be holding the lock. + tx.db.mu.Lock() tx.db.rootRecords = tx.rootRecords tx.db.pageMap = tx.pageMap tx.db.walPageN = tx.walPageN + tx.db.mu.Unlock() } + tx.db.mu.Lock() + defer tx.db.mu.Unlock() // Disconnect transaction from DB. return tx.db.removeTx(tx) } From f05f1d0de27bae969d31d4364b7bedd85e007593 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 15:51:30 -0600 Subject: [PATCH 26/58] added more authz functionality --- authz/{authz.go => authorization.go} | 31 ++++++- .../{authz_test.go => authorization_test.go} | 93 ++++++++++++++++++- server/server.go | 31 +++++++ 3 files changed, 151 insertions(+), 4 deletions(-) rename authz/{authz.go => authorization.go} (75%) rename authz/{authz_test.go => authorization_test.go} (68%) diff --git a/authz/authz.go b/authz/authorization.go similarity index 75% rename from authz/authz.go rename to authz/authorization.go index 42c3bba2f..7f311899d 100644 --- a/authz/authz.go +++ b/authz/authorization.go @@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss if perm, ok := p.Permissions[group.GroupID][index]; ok { allPermissions[perm] = true } else { - return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index) + return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index) } } else { groupsDenied = append(groupsDenied, group.GroupID) @@ -99,7 +99,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss } if len(groupsDenied) == len(groups) { - return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied) + return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied) } if allPermissions["admin"] { @@ -112,3 +112,30 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss return "", fmt.Errorf("no permissions found") } } + +func (p *GroupPermissions) IsAdmin(groups []Group) bool { + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + for _, permission := range p.Permissions[group.GroupID] { + if permission == "admin" { + return true + } + } + } + } + return false +} + +func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) { + + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + for index, permission := range p.Permissions[group.GroupID] { + if permission == desiredPermission { + indexList = append(indexList, index) + } + } + } + } + return indexList +} diff --git a/authz/authz_test.go b/authz/authorization_test.go similarity index 68% rename from authz/authz_test.go rename to authz/authorization_test.go index fcec43185..84db21ab5 100644 --- a/authz/authz_test.go +++ b/authz/authorization_test.go @@ -23,6 +23,7 @@ import ( ) func TestAuth_ReadPermissionsFile(t *testing.T) { + singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": "test": "read"` @@ -67,6 +68,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { } func TestAuth_GetPermissions(t *testing.T) { + // initializes different example of permissions file in yaml permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": "test": "read"` @@ -112,14 +114,14 @@ func TestAuth_GetPermissions(t *testing.T) { groupsList3, "test1", "", - "NOT allowed access to index", + "does not have permission to index", }, { permissions2, groupsList2, "test", "", - "NOT allowed access to FeatureBase", + "does not have permission to FeatureBase", }, { permissions1, @@ -176,3 +178,90 @@ func TestAuth_GetPermissions(t *testing.T) { }) } } + +func TestAuth_IsAdmin(t *testing.T) { + + group := []authz.Group{ + {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + } + + groupPermissions1 := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"}, + } + + groupPermissions2 := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + } + + tests := []struct { + groups []authz.Group + groupPermissions map[string]map[string]string + output bool + }{ + { + group, groupPermissions1, true, + }, + { + group, groupPermissions2, false, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + p := authz.GroupPermissions{test.groupPermissions} + resp := p.IsAdmin(test.groups) + if resp != test.output { + t.Errorf("expected %t, but got %t", test.output, resp) + } + }) + } +} + +func TestAuth_GetAuthorizedIndexList(t *testing.T) { + + group := []authz.Group{ + {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + } + + p := authz.GroupPermissions{map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": { + "test1": "admin", + "test2": "read", + "test3": "read", + }, + }} + + tests := []struct { + groups []authz.Group + permission string + output []string + }{ + { + group, + "read", + []string{"test2", "test3"}, + }, + { + group, + "admin", + []string{"test1"}, + }, + { + group, + "write", + nil, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + indexList := p.GetAuthorizedIndexList(test.groups, test.permission) + + if !reflect.DeepEqual(indexList, test.output) { + t.Errorf("expected %s, but got %s", test.output, indexList) + } + }) + } + +} diff --git a/server/server.go b/server/server.go index c2418630b..17ea28933 100644 --- a/server/server.go +++ b/server/server.go @@ -237,6 +237,37 @@ func (m *Command) Start() (err error) { if err = p.ReadPermissionsFile(permsFile); err != nil { return err } + + groups := []authz.Group{ + { + UserID: "user-id", + GroupID: "dca35310-ecda-4f23-86cd-876aee55906b", + GroupName: "group-name", + }, + // { + // UserID: "user-id", + // GroupID: "dca35310-ecda-4f23-86cd-876aee559900", + // GroupName: "group-name", + // }, + } + + index := "test" + + perm, err := p.GetPermissions(groups, index) + fmt.Printf("\nuser has %s access to index %s\n", perm, index) + if err != nil { + fmt.Printf("\np: %s, err: %s\n", perm, err.Error()) + } + + adminAccess := p.IsAdmin(groups) + fmt.Printf("\nAdminAccess: %t\n", adminAccess) + + accessList := []string{"read", "write", "admin"} + for _, a := range accessList { + indexList := p.GetAuthorizedIndexList(groups, a) + fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList) + } + } // Initialize server. From a606bd030a09ac65cbc8cedba80533650f710260 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 16:46:07 -0600 Subject: [PATCH 27/58] addressed reviewer's feedback --- authz/authorization.go | 8 ++++++-- authz/authorization_test.go | 8 +++++--- server/config.go | 16 ++++------------ server/server.go | 31 ------------------------------- 4 files changed, 15 insertions(+), 48 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index 7f311899d..f89a52ea6 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -70,7 +70,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } - return nil + return } func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { @@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss if perm, ok := p.Permissions[group.GroupID][index]; ok { allPermissions[perm] = true } else { - return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index) + return "", fmt.Errorf("user %s does not have permission to index %s", group.UserID, index) } } else { groupsDenied = append(groupsDenied, group.GroupID) @@ -133,6 +133,10 @@ func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermiss for index, permission := range p.Permissions[group.GroupID] { if permission == desiredPermission { indexList = append(indexList, index) + } else if permission == "admin" { + indexList = append(indexList, index) + } else if permission == "write" && desiredPermission == "read" { + indexList = append(indexList, index) } } } diff --git a/authz/authorization_test.go b/authz/authorization_test.go index 84db21ab5..cd985f973 100644 --- a/authz/authorization_test.go +++ b/authz/authorization_test.go @@ -16,6 +16,7 @@ package authz_test import ( "fmt" "reflect" + "sort" "strings" "testing" @@ -227,7 +228,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { "dca35310-ecda-4f23-86cd-876aee55906b": { "test1": "admin", "test2": "read", - "test3": "read", + "test3": "write", }, }} @@ -239,7 +240,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { { group, "read", - []string{"test2", "test3"}, + []string{"test1", "test2", "test3"}, }, { group, @@ -249,7 +250,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { { group, "write", - nil, + []string{"test1", "test3"}, }, } @@ -257,6 +258,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { indexList := p.GetAuthorizedIndexList(test.groups, test.permission) + sort.Strings(indexList) if !reflect.DeepEqual(indexList, test.output) { t.Errorf("expected %s, but got %s", test.output, indexList) diff --git a/server/config.go b/server/config.go index 866ffb3ff..5f4f0573c 100644 --- a/server/config.go +++ b/server/config.go @@ -601,7 +601,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin func (c *Config) ValidateAuth() (errors []error) { if !c.Auth.Enable { - return errors + return } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, @@ -626,11 +626,7 @@ func (c *Config) ValidateAuth() (errors []error) { } } } - - if len(errors) > 0 { - return errors - } - return nil + return errors } func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { @@ -667,11 +663,7 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { } } } - if len(errors) > 0 { - return errors - } - - return nil + return errors } func (c *Config) ValidatePermissionsFile() (err error) { @@ -684,7 +676,7 @@ func (c *Config) ValidatePermissionsFile() (err error) { if (fileExt != ".yaml") && (fileExt != ".yml") { return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile) } - return nil + return } func (c *Config) MustValidateAuth() { diff --git a/server/server.go b/server/server.go index 17ea28933..c2418630b 100644 --- a/server/server.go +++ b/server/server.go @@ -237,37 +237,6 @@ func (m *Command) Start() (err error) { if err = p.ReadPermissionsFile(permsFile); err != nil { return err } - - groups := []authz.Group{ - { - UserID: "user-id", - GroupID: "dca35310-ecda-4f23-86cd-876aee55906b", - GroupName: "group-name", - }, - // { - // UserID: "user-id", - // GroupID: "dca35310-ecda-4f23-86cd-876aee559900", - // GroupName: "group-name", - // }, - } - - index := "test" - - perm, err := p.GetPermissions(groups, index) - fmt.Printf("\nuser has %s access to index %s\n", perm, index) - if err != nil { - fmt.Printf("\np: %s, err: %s\n", perm, err.Error()) - } - - adminAccess := p.IsAdmin(groups) - fmt.Printf("\nAdminAccess: %t\n", adminAccess) - - accessList := []string{"read", "write", "admin"} - for _, a := range accessList { - indexList := p.GetAuthorizedIndexList(groups, a) - fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList) - } - } // Initialize server. From 41f6156bda7e7e21e37e2ca65f1675c03560e688 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 17:57:33 -0600 Subject: [PATCH 28/58] don't use write Tx even when we're using the expensive logic for write Tx --- txfactory.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/txfactory.go b/txfactory.go index 62f527653..12fa12981 100644 --- a/txfactory.go +++ b/txfactory.go @@ -242,10 +242,15 @@ func (qcx *Qcx) GetTx(o Txo) (tx Tx, finisher func(perr *error), err error) { } // qcx.write reflects the top executor determination - // if a write will be done at the end, so we upgrade - // the "local" read Tx to be writes, so that they - // don't deadlock against themselves. - o.Write = o.Write || qcx.write + // if a write will be happen at some point, in which case, to avoid + // locking problems with multi-shard things, we (probably incorrectly) + // treat every Tx as its own individual separate Tx. + // + // But we still want to open non-write transactions individually, we + // just can't recycle them (because write operations will come in and + // we want them to work and commit right away so we're not holding a write + // lock for long). + writeLogic := o.Write || qcx.write // In general, we make ALL write transactions local, and never reuse them // below. Previously this was to help lmdb. @@ -273,7 +278,7 @@ func (qcx *Qcx) GetTx(o Txo) (tx Tx, finisher func(perr *error), err error) { return *qcx.RequiredForAtomicWriteTx, NoopFinisher, nil } - if !o.Write && qcx.Grp != nil { + if !writeLogic && qcx.Grp != nil { // read, with a group in place. finisher = func(perr *error) {} // finisher is a returned value From 9945575bf111f3193ed02212dd4ddc109999a0c0 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 21:21:03 -0600 Subject: [PATCH 29/58] create a new worker every so often if progress isn't happening this is very approximate and may be a mess and may be unbounded, but in practice i think it should be okay. if it's not we'll have an adventure. --- executor.go | 48 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/executor.go b/executor.go index 8b0409eb7..899ac0e19 100644 --- a/executor.go +++ b/executor.go @@ -51,6 +51,9 @@ type executor struct { Node *topology.Node Cluster *cluster + // how many jobs the work queue has seen + workCounter uint64 + // Client used for remote requests. client InternalQueryClient @@ -128,15 +131,47 @@ func newExecutor(opts ...executorOption) *executor { e.work = make(chan job, e.workerPoolSize) _ = testhook.Opened(NewAuditor(), e, nil) for i := 0; i < e.workerPoolSize; i++ { - e.workersWG.Add(1) - go func() { - defer e.workersWG.Done() - worker(e.work) - }() + e.addWorker() } + go func() { + // background task: every so often, check to see whether we have + // work in the queue but none has been taken for a while. if so, we + // need more workers. + prev := atomic.LoadUint64(&e.workCounter) + periodic := time.NewTicker(50 * time.Millisecond) + defer periodic.Stop() + running := true + for running { + <-periodic.C + func() { + e.workMu.Lock() + defer e.workMu.Unlock() + if e.shutdown { + running = false + return + } + if len(e.work) == 0 { + return + } + next := atomic.LoadUint64(&e.workCounter) + if next == prev { + e.addWorker() + prev = next + } + }() + } + }() return e } +func (e *executor) addWorker() { + e.workersWG.Add(1) + go func() { + defer e.workersWG.Done() + e.worker(e.work) + }() +} + func (e *executor) Close() error { e.workMu.Lock() defer e.workMu.Unlock() @@ -5935,8 +5970,9 @@ type job struct { resultChan chan mapResponse } -func worker(work chan job) { +func (e *executor) worker(work chan job) { for j := range work { + atomic.AddUint64(&e.workCounter, 1) // Skip out early if the context is done, but still send // an ack so mapperLocal can be sure we aren't about to // work on something it sent us. From 8f217ab099f4a6a3952b15ba190c6311133c6f39 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 22:05:41 -0600 Subject: [PATCH 30/58] scale down worker pool when it's large if we have more than twice our starting worker pool, and have had no tasks when checking the queue for multiple rounds, send a job telling the system to retire a worker. eventually we'll get down to about 2x the starting pool size if we stay idle. --- executor.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/executor.go b/executor.go index 899ac0e19..be4def11a 100644 --- a/executor.go +++ b/executor.go @@ -64,6 +64,7 @@ type executor struct { workMu sync.RWMutex workersWG sync.WaitGroup workerPoolSize int + currentWorkers int work chan job // Maximum per-request memory usage (Extract() only) @@ -141,6 +142,7 @@ func newExecutor(opts ...executorOption) *executor { periodic := time.NewTicker(50 * time.Millisecond) defer periodic.Stop() running := true + idle := 0 for running { <-periodic.C func() { @@ -151,6 +153,17 @@ func newExecutor(opts ...executorOption) *executor { return } if len(e.work) == 0 { + idle++ + if idle > 10 && e.currentWorkers > (e.workerPoolSize*2) { + select { + case e.work <- job{idleHands: true}: + // we closed an excess worker + default: + // somehow between our test above and now the work + // queue FILLED UP and we stoically accept this + } + } + idle = 0 return } next := atomic.LoadUint64(&e.workCounter) @@ -166,9 +179,11 @@ func newExecutor(opts ...executorOption) *executor { func (e *executor) addWorker() { e.workersWG.Add(1) + e.currentWorkers++ go func() { defer e.workersWG.Done() e.worker(e.work) + e.currentWorkers-- }() } @@ -5968,11 +5983,15 @@ type job struct { ctx context.Context memoryAvailable *int64 // shared, atomic value resultChan chan mapResponse + idleHands bool } func (e *executor) worker(work chan job) { for j := range work { atomic.AddUint64(&e.workCounter, 1) + if j.idleHands { + return + } // Skip out early if the context is done, but still send // an ack so mapperLocal can be sure we aren't about to // work on something it sent us. From 9a2a8f964c99f177697661e9c98dad59eb968ff5 Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 22:22:27 -0600 Subject: [PATCH 31/58] fix silly typo in worker pool downscaling --- executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor.go b/executor.go index be4def11a..69bdbf15d 100644 --- a/executor.go +++ b/executor.go @@ -162,8 +162,8 @@ func newExecutor(opts ...executorOption) *executor { // somehow between our test above and now the work // queue FILLED UP and we stoically accept this } + idle = 0 } - idle = 0 return } next := atomic.LoadUint64(&e.workCounter) From 1439c316d348ac43d084e88d414e5899ecdfe9dd Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 22:25:01 -0600 Subject: [PATCH 32/58] read-only lock for check of shutdown --- executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index 69bdbf15d..45d5169c5 100644 --- a/executor.go +++ b/executor.go @@ -146,8 +146,8 @@ func newExecutor(opts ...executorOption) *executor { for running { <-periodic.C func() { - e.workMu.Lock() - defer e.workMu.Unlock() + e.workMu.RLock() + defer e.workMu.RUnlock() if e.shutdown { running = false return From 8974014d5783dc61c6a850c03f00119194e10f1a Mon Sep 17 00:00:00 2001 From: Seebs Date: Fri, 17 Dec 2021 22:48:40 -0600 Subject: [PATCH 33/58] too tired to be writing code --- executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor.go b/executor.go index 45d5169c5..fb5ef68ce 100644 --- a/executor.go +++ b/executor.go @@ -169,8 +169,8 @@ func newExecutor(opts ...executorOption) *executor { next := atomic.LoadUint64(&e.workCounter) if next == prev { e.addWorker() - prev = next } + prev = next }() } }() From 7826c06eee6014cc1ec5b9079f94ed86c53e1dee Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Sat, 18 Dec 2021 08:58:19 -0600 Subject: [PATCH 34/58] use atomics for currentWorker to avoid race --- executor.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/executor.go b/executor.go index fb5ef68ce..5e03e1a72 100644 --- a/executor.go +++ b/executor.go @@ -64,7 +64,7 @@ type executor struct { workMu sync.RWMutex workersWG sync.WaitGroup workerPoolSize int - currentWorkers int + currentWorkers int64 work chan job // Maximum per-request memory usage (Extract() only) @@ -154,7 +154,7 @@ func newExecutor(opts ...executorOption) *executor { } if len(e.work) == 0 { idle++ - if idle > 10 && e.currentWorkers > (e.workerPoolSize*2) { + if idle > 10 && atomic.LoadInt64(&e.currentWorkers) > int64(e.workerPoolSize*2) { select { case e.work <- job{idleHands: true}: // we closed an excess worker @@ -179,11 +179,11 @@ func newExecutor(opts ...executorOption) *executor { func (e *executor) addWorker() { e.workersWG.Add(1) - e.currentWorkers++ + atomic.AddInt64(&e.currentWorkers, 1) go func() { defer e.workersWG.Done() e.worker(e.work) - e.currentWorkers-- + atomic.AddInt64(&e.currentWorkers, -1) }() } From c14bd08213477c3fc6bc355b24774c39be0bd4cc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Sun, 19 Dec 2021 11:37:41 -0600 Subject: [PATCH 35/58] updated admin to be at the cluster level --- authz/authorization.go | 33 ++++--- authz/authorization_test.go | 153 +++++++++++++++++++++------------ server/config.go | 9 +- server/config_internal_test.go | 32 +++++-- 4 files changed, 149 insertions(+), 78 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index f89a52ea6..77bd67ade 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -49,7 +49,8 @@ type Auth struct { } type GroupPermissions struct { - Permissions map[string]map[string]string + Permissions map[string]map[string]string `yaml:"user-groups"` + Admin string `yaml:"admin"` } type Group struct { @@ -65,7 +66,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return fmt.Errorf("reading permissions failed with error: %s", err) } - err = yaml.UnmarshalStrict(permsData, &p.Permissions) + err = yaml.UnmarshalStrict(permsData, &p) if err != nil { return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } @@ -75,8 +76,11 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { + if admin := p.IsAdmin(groups); admin { + return "admin", nil + } + allPermissions := map[string]bool{ - "admin": false, "write": false, "read": false, } @@ -102,9 +106,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied) } - if allPermissions["admin"] { - return "admin", nil - } else if allPermissions["write"] { + if allPermissions["write"] { return "write", nil } else if allPermissions["read"] { return "read", nil @@ -115,26 +117,29 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss func (p *GroupPermissions) IsAdmin(groups []Group) bool { for _, group := range groups { - if _, ok := p.Permissions[group.GroupID]; ok { - for _, permission := range p.Permissions[group.GroupID] { - if permission == "admin" { - return true - } - } + if p.Admin == group.GroupID { + return true } } return false } func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) { + // if user is admin, find all indexes in permissions file and return them + if admin := p.IsAdmin(groups); admin { + for groupId := range p.Permissions { + for index := range p.Permissions[groupId] { + indexList = append(indexList, index) + } + } + return indexList + } for _, group := range groups { if _, ok := p.Permissions[group.GroupID]; ok { for index, permission := range p.Permissions[group.GroupID] { if permission == desiredPermission { indexList = append(indexList, index) - } else if permission == "admin" { - indexList = append(indexList, index) } else if permission == "write" && desiredPermission == "read" { indexList = append(indexList, index) } diff --git a/authz/authorization_test.go b/authz/authorization_test.go index cd985f973..dab33dbe1 100644 --- a/authz/authorization_test.go +++ b/authz/authorization_test.go @@ -25,29 +25,39 @@ import ( func TestAuth_ReadPermissionsFile(t *testing.T) { - singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read"` + singleInput := `user-groups: + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read" -"dca35310-ecda-4f23-86cd-876aee559900": - "test": "admin"` + multiInput := `user-groups: + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" + "test2": "write" + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - singleStruct := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + singlePermission := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + }, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } - multiStruct := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, - "dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"}, + multiPermission := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read", "test2": "write"}, + "dca35310-ecda-4f23-86cd-876aee559900": {"test": "write"}}, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } tests := []struct { input string - output map[string]map[string]string + output authz.GroupPermissions }{ - {singleInput, singleStruct}, - {multiInput, multiStruct}, + {singleInput, singlePermission}, + {multiInput, multiPermission}, } for i, test := range tests { @@ -60,8 +70,8 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { t.Fatalf("readPermissionsFile error: %s", err) } - if !reflect.DeepEqual(p.Permissions, test.output) { - t.Fatalf("expected output %s, but got %s", test.output, p.Permissions) + if !reflect.DeepEqual(p, test.output) { + t.Fatalf("expected output %s, but got %s", test.output, p) } }, ) @@ -71,20 +81,28 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml - permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read"` + permissions1 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": "write"` + permissions2 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "write" - "test2": "read" -"dca35310-ecda-4f23-86cd-876aee559900": - "test": "admin"` + permissions3 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "write" + "test2": "read" + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": ""` + permissions4 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` // initializes groups that are returned from identity provider groupName := "name" @@ -95,6 +113,7 @@ func TestAuth_GetPermissions(t *testing.T) { {userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName}, {userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName}, } + groupsList4 := []authz.Group{{userId, "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", groupName}} tests := []struct { yamlData string @@ -140,7 +159,7 @@ func TestAuth_GetPermissions(t *testing.T) { }, { permissions3, - groupsList3, + groupsList4, "test", "admin", "", @@ -182,34 +201,37 @@ func TestAuth_GetPermissions(t *testing.T) { func TestAuth_IsAdmin(t *testing.T) { - group := []authz.Group{ - {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + group1 := []authz.Group{ + {"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"}, } - groupPermissions1 := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"}, + group2 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, } - groupPermissions2 := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + groupPermissions := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "write"}, + }, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } tests := []struct { groups []authz.Group - groupPermissions map[string]map[string]string + groupPermissions authz.GroupPermissions output bool }{ { - group, groupPermissions1, true, + group1, groupPermissions, true, }, { - group, groupPermissions2, false, + group2, groupPermissions, false, }, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - p := authz.GroupPermissions{test.groupPermissions} + p := test.groupPermissions resp := p.IsAdmin(test.groups) if resp != test.output { t.Errorf("expected %t, but got %t", test.output, resp) @@ -220,17 +242,30 @@ func TestAuth_IsAdmin(t *testing.T) { func TestAuth_GetAuthorizedIndexList(t *testing.T) { - group := []authz.Group{ - {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + group1 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, } - p := authz.GroupPermissions{map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": { - "test1": "admin", - "test2": "read", - "test3": "write", + group2 := []authz.Group{ + {"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"}, + } + + group3 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee559900", "group-name"}, + } + + p := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": { + "test1": "read", + "test2": "write", + }, + "dca35310-ecda-4f23-86cd-876aee559900": { + "test3": "read", + }, }, - }} + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", + } tests := []struct { groups []authz.Group @@ -238,19 +273,29 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { output []string }{ { - group, + group1, + "read", + []string{"test1", "test2"}, + }, + { + group1, + "write", + []string{"test2"}, + }, + { + group3, + "write", + nil, + }, + { + group2, "read", []string{"test1", "test2", "test3"}, }, { - group, - "admin", - []string{"test1"}, - }, - { - group, + group2, "write", - []string{"test1", "test3"}, + []string{"test1", "test2", "test3"}, }, } diff --git a/server/config.go b/server/config.go index 5f4f0573c..807e19b7f 100644 --- a/server/config.go +++ b/server/config.go @@ -657,12 +657,17 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { continue } - if !((perm == "admin") || (perm == "write") || (perm == "read")) { - errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile)) + if !((perm == "write") || (perm == "read")) { + errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s; expected permissions are read or write", perm, groupId, index, c.Auth.PermissionsFile)) continue } } } + + if p.Admin == "" { + errors = append(errors, fmt.Errorf("empty string for admin in permissions file: %s", c.Auth.PermissionsFile)) + } + return errors } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 48ba081db..11148dfad 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -386,17 +386,29 @@ func TestConfig_validateAuth(t *testing.T) { func TestConfig_validatePermissions(t *testing.T) { permissions0 := `` - permissions1 := `"": - "test": "read"` + permissions1 := `user-groups: + "": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": - "": "write"` + permissions2 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": ""` + permissions3 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": "readwrite"` + permissions4 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "readwrite" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` + + permissions5 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "read"` tests := []struct { err string @@ -422,6 +434,10 @@ func TestConfig_validatePermissions(t *testing.T) { "not a valid permission", permissions4, }, + { + "empty string for admin in permissions file", + permissions5, + }, } for i, test := range tests { From 977a699a98c141a65de5f407d17f03d793cddfcf Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 20 Dec 2021 10:58:41 -0600 Subject: [PATCH 36/58] don't panic on invalid page type debugging tools shouldn't panic when they encounter bugs. insert "you had one job" meme. --- ctl/rbf_pages.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/ctl/rbf_pages.go b/ctl/rbf_pages.go index 2e831892a..ca3484f31 100644 --- a/ctl/rbf_pages.go +++ b/ctl/rbf_pages.go @@ -69,9 +69,9 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { // Print one line for each page. for pgno, info := range infos { + fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) switch info := info.(type) { case *rbf.MetaPageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "meta") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", "") @@ -79,7 +79,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "pageN=%d,walid=%d,rootrec=%d,freelist=%d\n", info.PageN, info.WALID, info.RootRecordPageNo, info.FreelistPageNo) case *rbf.RootRecordPageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "rootrec") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", "") @@ -87,7 +86,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "next=%d\n", info.Next) case *rbf.LeafPageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "leaf") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree)) @@ -95,7 +93,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "flags=x%x,celln=%d\n", info.Flags, info.CellN) case *rbf.BranchPageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "branch") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree)) @@ -103,7 +100,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "flags=x%x,celln=%d\n", info.Flags, info.CellN) case *rbf.BitmapPageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "bitmap") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree)) @@ -111,7 +107,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "-\n") case *rbf.FreePageInfo: - fmt.Fprintf(cmd.Stdout, "%-8d ", pgno) fmt.Fprintf(cmd.Stdout, "%-10s ", "free") if cmd.WithTree { fmt.Fprintf(cmd.Stdout, "%-30q ", "") @@ -119,7 +114,7 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error { fmt.Fprintf(cmd.Stdout, "-\n") default: - panic(fmt.Sprintf("unexpected page info type %T", info)) + fmt.Fprintf(cmd.Stdout, "unknown [%T]\n", info) } } From 5f8a2819187aee3afb6b46b534d6699549792801 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 20 Dec 2021 09:38:16 -0700 Subject: [PATCH 37/58] Fix RBF multi-level branch delete This commit fixes a bug in RBF where deleting all the elements in a bitmap that has a depth greater than 2 will cause the root bitmap to be a branch page with a cell count of zero. This breaks an assertion in `readBranchCell()` which causes a panic post-commit. A new assertion has been added to prevent a branch page from being written with a zero count in the future. --- rbf/cursor.go | 22 ++++++++++++++++++++++ rbf/tx_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/rbf/cursor.go b/rbf/cursor.go index 68c165d94..eda6bec1d 100644 --- a/rbf/cursor.go +++ b/rbf/cursor.go @@ -774,6 +774,25 @@ func (c *Cursor) deleteBranchCell(stackIndex int, key uint64) (err error) { cells[len(cells)-1] = branchCell{} cells = cells[:len(cells)-1] + // Branches are not allowed to have zero element so we must remove the page + // or, in the case of the root page, convert to a leaf page. + if len(cells) == 0 { + // If this is the root page, convert to leaf page. + if stackIndex == 0 { + var buf [PageSize]byte + writePageNo(buf[:], elem.pgno) + writeFlags(buf[:], PageTypeLeaf) + writeCellN(buf[:], len(cells)) + return c.tx.writePage(buf[:]) + } + + // If this is a non-root page, free and remove from parent. + if err := c.tx.freePgno(elem.pgno); err != nil { + return err + } + return c.deleteBranchCell(stackIndex-1, oldPageKey) + } + // If the root only has one node, replace it with its child. if stackIndex == 0 && len(cells) == 1 { target, _, err := c.tx.readPage(cells[0].ChildPgno) @@ -802,6 +821,9 @@ func (c *Cursor) deleteBranchCell(stackIndex int, key uint64) (err error) { writeBranchCell(buf[:], j, offset, cell) offset += align8(branchCellSize) } + + assert(readCellN(buf[:]) > 0) // must have at least one cell + if err := c.tx.writePage(buf[:]); err != nil { return err } diff --git a/rbf/tx_test.go b/rbf/tx_test.go index 1004437a3..f05db6626 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -433,6 +433,52 @@ func TestTx_DeallocateToFreeList(t *testing.T) { } } +func TestTx_Remove(t *testing.T) { + t.Parallel() + + db := MustOpenDB(t) + defer MustCloseDB(t, db) + + tx := MustBegin(t, db, true) + defer tx.Rollback() + + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + + // Insert large array values. + var values []uint64 + for i := 0; i < 1000; i++ { + for j := 0; j < rbf.ArrayMaxSize; j++ { + v := uint64((i << 16) + j) + values = append(values, v) + + if _, err := tx.Add("x", v); err != nil { + t.Fatalf("Add(%d) err=%q", v, err) + } + } + } + + if err := tx.Commit(); err != nil { + t.Fatal(err) + } + + tx = MustBegin(t, db, true) + defer tx.Rollback() + + // Remove all array values. + for _, i := range rand.Perm(len(values)) { + v := values[i] + if _, err := tx.Remove("x", v); err != nil { + t.Fatalf("Remove(%d) err=%q", v, err) + } + } + + if err := tx.Commit(); err != nil { + t.Fatal(err) + } +} + func TestTx_AddRemove_Quick(t *testing.T) { if testing.Short() { t.Skip("-short enabled, skipping") From 6481b4eabe12e3c50ca7c8e735483349484223cb Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 20 Dec 2021 13:24:34 -0700 Subject: [PATCH 38/58] Add rbf check for empty branch pages --- rbf/db.go | 8 +++++ rbf/rbf_test.go | 9 +++++- rbf/tx.go | 26 ++++++++++++++-- rbf/tx_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 3 deletions(-) diff --git a/rbf/db.go b/rbf/db.go index f6b9ce5bc..65dd4fbea 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -265,6 +265,14 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) { return lastMeta, nil } +// Checkpoint performs a manual checkpoint. This is not necessary except for tests. +func (db *DB) Checkpoint() error { + db.mu.Lock() + defer db.mu.Unlock() + db.rwmu.Lock() + return db.checkpoint() +} + // checkpoint moves all WAL pages to the main DB file. Must be called // while holding both db.mu and db.rwmu. Should release db.rwmu, but not // db.mu. diff --git a/rbf/rbf_test.go b/rbf/rbf_test.go index be3461e37..4071a1a95 100644 --- a/rbf/rbf_test.go +++ b/rbf/rbf_test.go @@ -86,7 +86,14 @@ func MustCloseDB(tb testing.TB, db *rbf.DB) { tb.Helper() if err := db.Check(); err != nil && err != rbf.ErrClosed { tb.Fatal(err) - } else if n := db.TxN(); n != 0 { + } + MustCloseDBNoCheck(tb, db) +} + +// MustCloseDBNoCheck closes db. On error, fail test. +func MustCloseDBNoCheck(tb testing.TB, db *rbf.DB) { + tb.Helper() + if n := db.TxN(); n != 0 { tb.Fatalf("db still has %d active transactions; must closed before closing db", n) } else if err := db.Close(); err != nil && err != rbf.ErrClosed { tb.Fatal(err) diff --git a/rbf/tx.go b/rbf/tx.go index bceabd8f1..5c6c7f7c6 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -741,6 +741,27 @@ func (tx *Tx) Check() error { return nil } +func (tx *Tx) checkPage(pgno, parent, typ uint32) error { + switch typ { + case PageTypeBranch: + return tx.checkBranchPage(pgno, parent, typ) + default: + return nil + } +} + +func (tx *Tx) checkBranchPage(pgno, parent, typ uint32) error { + page, _, err := tx.readPage(pgno) + if err != nil { + return err + } + + if readCellN(page) == 0 { + return fmt.Errorf("branch page %d is empty", pgno) + } + return nil +} + // checkPageAllocations ensures that all pages are either in-use or on the freelist. func (tx *Tx) checkPageAllocations() error { freePageSet, err := tx.freePageSet() @@ -830,7 +851,7 @@ func (tx *Tx) inusePageSet() (map[uint32]struct{}, error) { // Traverse freelist and mark pages as in-use. if err := tx.walkTree(readMetaFreelistPageNo(tx.meta[:]), 0, func(pgno, parent, typ uint32) error { m[pgno] = struct{}{} - return nil + return tx.checkPage(pgno, parent, typ) }); err != nil { return m, err } @@ -846,7 +867,8 @@ func (tx *Tx) inusePageSet() (map[uint32]struct{}, error) { if err := tx.walkTree(pgno.(uint32), 0, func(pgno, parent, typ uint32) error { m[pgno] = struct{}{} - return nil + + return tx.checkPage(pgno, parent, typ) }); err != nil { return m, err } diff --git a/rbf/tx_test.go b/rbf/tx_test.go index 1004437a3..345031417 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -2,8 +2,11 @@ package rbf_test import ( + "encoding/binary" "fmt" "math/rand" + "os" + "strings" "sync" "testing" "time" @@ -770,3 +773,83 @@ func TestTx_DeleteBitmapsWithPrefix(t *testing.T) { checkInfos() } + +func TestTx_Check(t *testing.T) { + t.Run("EmptyBranchPage", func(t *testing.T) { + t.Parallel() + + db := MustOpenDB(t) + defer MustCloseDBNoCheck(t, db) + tx := MustBegin(t, db, true) + defer tx.Rollback() + + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + + // Insert enough array containers to split page. + for i := 0; i < 1000; i++ { + if _, err := tx.Add("x", uint64(i<<16)); err != nil { + t.Fatalf("Add(%d) err=%q", i<<16, err) + } + } + + // Read page types for all pages. + infos, err := tx.PageInfos() + if err != nil { + t.Fatal(err) + } + + // Commit & checkpoint to flush to the data file. + if err := tx.Commit(); err != nil { + t.Fatal(err) + } else if err := db.Checkpoint(); err != nil { + t.Fatal(err) + } + + // Corrupt first branch page found by zeroing out the cell count. + var pgno uint32 + for _, info := range infos { + if info, ok := info.(*rbf.BranchPageInfo); ok { + pgno = info.Pgno + page := mustReadPage(t, db.DataPath(), pgno) + binary.BigEndian.PutUint16(page[8:10], 0) // zero cell count + mustWritePage(t, db.DataPath(), pgno, page) + break + } + } + + // Verify that check now returns an error. + if err := db.Check(); err == nil || !strings.Contains(err.Error(), fmt.Sprintf("branch page %d is empty", pgno)) { + t.Fatalf("unexpected error: %#v", err) + } + }) +} + +func mustReadPage(tb testing.TB, path string, pgno uint32) []byte { + tb.Helper() + f, err := os.Open(path) + if err != nil { + tb.Fatal(err) + } + defer f.Close() + + buf := make([]byte, rbf.PageSize) + if _, err := f.ReadAt(buf, int64(pgno)*rbf.PageSize); err != nil { + tb.Fatal(err) + } + return buf +} + +func mustWritePage(tb testing.TB, path string, pgno uint32, buf []byte) { + tb.Helper() + f, err := os.OpenFile(path, os.O_WRONLY, 0666) + if err != nil { + tb.Fatal(err) + } + defer f.Close() + + if _, err := f.WriteAt(buf, int64(pgno)*rbf.PageSize); err != nil { + tb.Fatal(err) + } +} From ddb5020aa66afa04b9080aa169f4a9fea59be194 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 20 Dec 2021 12:22:24 -0600 Subject: [PATCH 39/58] slightly better lock protection around bitDepth in view There's a number of deeper issues here (the fragment is conjuring up a Tx, for instance) but this helps. Also use field.view() to get the view rather than accessing viewMap directly without a lock. Also change field.cacheBitDepth to ratchet upwards -- if we have multiple shards and some shards have lower depths than others, we should use the highest as the cached value, not the most recent. --- api.go | 4 ++-- field.go | 7 +++++-- fragment.go | 2 ++ view.go | 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/api.go b/api.go index 70a27da94..47558678d 100644 --- a/api.go +++ b/api.go @@ -2753,8 +2753,8 @@ func (api *API) RestoreShard(ctx context.Context, indexName string, shard uint64 for _, flv := range flvs { fld := idx.field(flv.Field) - view, ok := fld.viewMap[flv.View] - if !ok { + view := fld.view(flv.View) + if view == nil { view, err = fld.createViewIfNotExists(flv.View) if err != nil { return err diff --git a/field.go b/field.go index 2aa865a1f..d325c477e 100644 --- a/field.go +++ b/field.go @@ -705,8 +705,11 @@ func (f *Field) cacheBitDepth(bd uint64) error { f.mu.Lock() defer f.mu.Unlock() - f.options.BitDepth = bd - if bsig != nil { + if f.options.BitDepth < bd { + f.options.BitDepth = bd + } + + if bsig != nil && bsig.BitDepth < bd { bsig.BitDepth = bd } diff --git a/fragment.go b/fragment.go index 9b77f90f7..8dfa6749c 100644 --- a/fragment.go +++ b/fragment.go @@ -218,6 +218,8 @@ func newFragment(holder *Holder, spec fragSpec, shard uint64, flags byte) *fragm func (f *fragment) cachePath() string { return f.path() + cacheExt } func (f *fragment) bitDepth() (uint64, error) { + f.mu.RLock() + defer f.mu.RUnlock() tx, err := f.holder.BeginTx(false, f.idx, f.shard) if err != nil { return 0, errors.Wrapf(err, "beginning new tx(false, %s, %d)", f.index(), f.shard) diff --git a/view.go b/view.go index f3bd27b3a..5a8e23ae1 100644 --- a/view.go +++ b/view.go @@ -619,7 +619,9 @@ func (v *view) bitDepth(shards []uint64) (uint64, error) { var maxBitDepth uint64 for _, shard := range shards { + v.mu.RLock() frag, ok := v.fragments[shard] + v.mu.RUnlock() if !ok || frag == nil { continue } From 15612b8b925bc88b3a9019e89a17e21d4695c2b6 Mon Sep 17 00:00:00 2001 From: "garrison.davis@molecula.com" Date: Tue, 21 Dec 2021 13:58:10 -0700 Subject: [PATCH 40/58] Run integration on merge to default branch Additionally, the go version is using the GOVERSION build variable instead. --- .gitlab/.gitlab-ci.yml | 52 +++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 0b7cc21e9..a27c1012e 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -11,7 +11,7 @@ include: paths: - .go/pkg/mod/ variables: - GOVERSION: "1.16.9" + GOVERSION: "1.16.10" stages: - lint @@ -19,19 +19,13 @@ stages: - build - integration - #before_script: - #- echo "before_script" - #- git version - #- go env -w GOPRIVATE=github.com/molecula - #- mkdir -p .go - #- go version - #- go env -w GO111MODULE=on - golangci-lint: image: golangci/golangci-lint:v1.39.0 stage: lint extends: .go-cache allow_failure: false + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - echo "Checking for issues in new code" - golangci-lint run -v @@ -41,6 +35,8 @@ build lattice: image: node:14 variables: CI: "false" + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - cd lattice - yarn install @@ -59,6 +55,8 @@ run jest tests: image: node:14 variables: CI: "true" + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - echo "Testing lattice..." - cd lattice @@ -70,8 +68,10 @@ run jest tests: run go tests: stage: test - image: golang:1.16.10 + image: golang:$GOVERSION extends: .go-cache + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - echo "Running featurebase unit tests..." - PKG_LIST=$(go list ./... | grep -v internal/clustertests | paste -s -d, -) @@ -84,6 +84,8 @@ run go tests future: stage: test image: golang:1.17.3 extends: .go-cache + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - echo "Running featurebase unit tests..." - PKG_LIST=$(go list ./... | grep -v internal/clustertests | paste -s -d, -) @@ -95,7 +97,9 @@ run go tests future: run go tests with output: stage: test - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - echo "Running featurebase unit tests to capture JSON output..." - go test -json > test-report.out @@ -108,6 +112,8 @@ upload to sonarcloud: image: sonarsource/sonar-scanner-cli:4.6 variables: SONAR_TOKEN: $SONAR_TOKEN + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - sonar-scanner -Dsonar.projectKey=molecula_featurebase -Dsonar.organization=molecula -Dsonar.sources=. -Dsonar.host.url=https://sonarcloud.io -Dsonar.go.coverage.reportPaths=coverage.out -Dsonar.go.tests.reportPaths=test-report.out -Dsonar.javascript.lcov.reportPaths=lattice/coverage/lcov.info needs: @@ -117,7 +123,9 @@ upload to sonarcloud: build for linux amd64: stage: build - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - rm -r lattice - tar -xvf lattice.tar.gz @@ -130,7 +138,9 @@ build for linux amd64: build for linux arm64: stage: build - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - rm -r lattice - tar -xvf lattice.tar.gz @@ -143,7 +153,9 @@ build for linux arm64: build for darwin amd64: stage: build - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - rm -r lattice - tar -xvf lattice.tar.gz @@ -156,7 +168,9 @@ build for darwin amd64: build for darwin arm64: stage: build - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' script: - rm -r lattice - tar -xvf lattice.tar.gz @@ -169,7 +183,9 @@ build for darwin arm64: package for linux amd64: stage: build - image: golang:1.16.10 + image: golang:$GOVERSION + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' variables: GOOS: "linux" GOARCH: "amd64" @@ -190,6 +206,8 @@ build container fb: - "build for linux amd64" tags: - shell + rules: + - if: '$CI_PIPELINE_SOURCE == "push"' before_script: - echo "${DOCKER_DEPLOY_TOKEN}" | docker login -u ${DOCKER_DEPLOY_USER} --password-stdin ${CI_REGISTRY} script: @@ -205,6 +223,8 @@ deploy node for linux amd64: variables: PROFILE: "default" AWS_SSH_PRIVATE_KEY: $AWS_SSH_PRIVATE_KEY + rules: + - if: '$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH' before_script: - aws configure set aws_access_key_id $AWS_ACCESS_KEY_ID - aws configure set aws_secret_access_key $AWS_SECRET_ACCESS_KEY From 8486efaa79034398d8a27b38ed568d1145bae23f Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 13 Dec 2021 12:27:44 -0600 Subject: [PATCH 41/58] add exponential retry logic to internal http client, use in backup --- cmd/backup.go | 11 ++++---- ctl/backup.go | 9 +++++-- ctl/common.go | 4 +-- http/client.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++---- 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/cmd/backup.go b/cmd/backup.go index 5761b6d57..133f0dbce 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -23,11 +23,12 @@ Backs up a FeatureBase server to a local, tar-formatted snapshot file. } flags := ccmd.Flags() - flags.StringVarP(&cmd.OutputDir, "output", "o", "", "output dir to write to") - flags.BoolVar(&cmd.NoSync, "no-sync", false, "disable file sync") - flags.IntVar(&cmd.Concurrency, "concurrency", cmd.Concurrency, "number of concurrent backup goroutines") - flags.StringVar(&cmd.Host, "host", "localhost:10101", "host:port of FeatureBase.") - flags.StringVar(&cmd.Index, "index", "", "index to backup, default backs up all indexes. ") + flags.StringVarP(&cmd.OutputDir, "output", "o", "", "Output directory to write to.") + flags.BoolVar(&cmd.NoSync, "no-sync", false, "Disable file sync") + flags.IntVar(&cmd.Concurrency, "concurrency", cmd.Concurrency, "Number of concurrent backup goroutines.") + flags.StringVar(&cmd.Host, "host", "localhost:10101", "The address (host:port) of FeatureBase (HTTP).") + flags.StringVar(&cmd.Index, "index", "", "Index to backup, default backs up all indexes. ") + flags.DurationVar(&cmd.RetryPeriod, "retry-period", cmd.RetryPeriod, "Length of time after HTTP request failure to continue retrying request.") ctl.SetTLSConfig(flags, "", &cmd.TLS.CertificatePath, &cmd.TLS.CertificateKeyPath, &cmd.TLS.CACertPath, &cmd.TLS.SkipVerify, &cmd.TLS.EnableClientVerification) return ccmd } diff --git a/ctl/backup.go b/ctl/backup.go index 0e8a257f1..8ba57c278 100644 --- a/ctl/backup.go +++ b/ctl/backup.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "os" "path/filepath" + "time" pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/http" @@ -37,6 +38,9 @@ type BackupCommand struct { // nolint: maligned // Number of concurrent backup goroutines running at a time. Concurrency int + // Amount of time after first failed request to continue retrying. + RetryPeriod time.Duration `json:"retry-period"` + // Reusable client. client pilosa.InternalClient @@ -51,6 +55,7 @@ func NewBackupCommand(stdin io.Reader, stdout, stderr io.Writer) *BackupCommand return &BackupCommand{ CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), Concurrency: 1, + RetryPeriod: time.Minute, } } @@ -70,7 +75,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) { } // Create a client to the server. - client, err := commandClient(cmd) + client, err := commandClient(cmd, http.WithClientRetryPeriod(cmd.RetryPeriod)) if err != nil { return fmt.Errorf("creating client: %w", err) } @@ -262,7 +267,7 @@ func (cmd *BackupCommand) backupShardNode(ctx context.Context, indexName string, logger := cmd.Logger() logger.Printf("backing up shard: index=%q id=%d", indexName, shard) - client := http.NewInternalClientFromURI(&node.URI, http.GetHTTPClient(cmd.tlsConfig)) + client := http.NewInternalClientFromURI(&node.URI, http.GetHTTPClient(cmd.tlsConfig), http.WithClientRetryPeriod(cmd.RetryPeriod)) rc, err := client.ShardReader(ctx, indexName, shard) if err != nil { return fmt.Errorf("fetching shard reader: %w", err) diff --git a/ctl/common.go b/ctl/common.go index c23b42f4c..7ba2df2f7 100644 --- a/ctl/common.go +++ b/ctl/common.go @@ -26,13 +26,13 @@ func SetTLSConfig(flags *pflag.FlagSet, prefix string, certificatePath *string, } // commandClient returns a pilosa.InternalHTTPClient for the command -func commandClient(cmd CommandWithTLSSupport) (*http.InternalClient, error) { +func commandClient(cmd CommandWithTLSSupport, opts ...http.InternalClientOption) (*http.InternalClient, error) { tls := cmd.TLSConfiguration() tlsConfig, err := server.GetTLSConfig(&tls, cmd.Logger()) if err != nil { return nil, errors.Wrap(err, "getting tls config") } - client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig)) + client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig), opts...) if err != nil { return nil, errors.Wrap(err, "getting internal client") } diff --git a/http/client.go b/http/client.go index 70c0639ec..8590a97fa 100644 --- a/http/client.go +++ b/http/client.go @@ -11,6 +11,7 @@ import ( "math/rand" "net/http" "net/url" + "os" "path" "sort" "strconv" @@ -20,6 +21,7 @@ import ( pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/encoding/proto" "github.com/molecula/featurebase/v2/ingest" + "github.com/molecula/featurebase/v2/logger" pnet "github.com/molecula/featurebase/v2/net" "github.com/molecula/featurebase/v2/topology" "github.com/molecula/featurebase/v2/tracing" @@ -31,6 +33,10 @@ type InternalClient struct { defaultURI *pnet.URI serializer pilosa.Serializer + log logger.Logger + + retryPeriod time.Duration + // The client to use for HTTP communication. httpClient *http.Client // the local node's API, used for operations that we can short-circuit that way @@ -40,7 +46,7 @@ type InternalClient struct { // NewInternalClient returns a new instance of InternalClient to connect to host. // If api is non-nil, the client uses it for some same-host operations instead // of going through http. -func NewInternalClient(host string, remoteClient *http.Client) (*InternalClient, error) { +func NewInternalClient(host string, remoteClient *http.Client, opts ...InternalClientOption) (*InternalClient, error) { if host == "" { return nil, pilosa.ErrHostRequired } @@ -50,16 +56,38 @@ func NewInternalClient(host string, remoteClient *http.Client) (*InternalClient, return nil, errors.Wrap(err, "getting URI") } - client := NewInternalClientFromURI(uri, remoteClient) + client := NewInternalClientFromURI(uri, remoteClient, opts...) return client, nil } -func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client) *InternalClient { - return &InternalClient{ +type InternalClientOption func(c *InternalClient) + +// WithClientRetryPeriod is the max amount of total time the client will +// retry failed requests using exponential backoff. +func WithClientRetryPeriod(period time.Duration) InternalClientOption { + return func(c *InternalClient) { + c.retryPeriod = period + } +} + +func WithClientLogger(log logger.Logger) InternalClientOption { + return func(c *InternalClient) { + c.log = log + } +} + +func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client, opts ...InternalClientOption) *InternalClient { + ic := &InternalClient{ defaultURI: defaultURI, serializer: proto.Serializer{}, httpClient: remoteClient, + log: logger.NewStandardLogger(os.Stderr), } + + for _, opt := range opts { + opt(ic) + } + return ic } // MaxShardByIndex returns the number of shards on a server by index. @@ -1717,6 +1745,36 @@ func giveRawResponse(b bool) executeRequestOption { } } +func (c *InternalClient) doWithRetry(req *http.Request) (*http.Response, error) { + sleepDuration := time.Second + resp, err := c.httpClient.Do(req) + // start timer after first request, so if retryPeriod > 0 we + // pretty much always do at least one retry + start := time.Now() + for ; ; resp, err = c.httpClient.Do(req) { + if err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300 { + if time.Since(start) > c.retryPeriod { + break + } + if err != nil { + c.log.Printf("retrying request due to error: '%v'", err) + } else { + if bod, readErr := ioutil.ReadAll(resp.Body); readErr != nil { + c.log.Printf("retrying request due to status: %d, error reading body: '%v', body: '%s'", resp.StatusCode, readErr, bod) + } else { + c.log.Printf("retrying request due to status: %d, body: '%s'", resp.StatusCode, bod) + } + } + + time.Sleep(sleepDuration) + sleepDuration *= 2 + } else { + break + } + } + return resp, err +} + // executeRequest executes the given request and checks the Response. For // responses with non-2XX status, the body is read and closed, and an error is // returned. If the error is nil, the caller must ensure that the response body @@ -1729,7 +1787,7 @@ func (c *InternalClient) executeRequest(req *http.Request, opts ...executeReques tracing.GlobalTracer.InjectHTTPHeaders(req) req.Close = false - resp, err := c.httpClient.Do(req) + resp, err := c.doWithRetry(req) if err != nil { if resp != nil { resp.Body.Close() From cdf4bc4c88272ca61dd9e51aee19e9298b5ab11d Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 13 Dec 2021 16:43:49 -0600 Subject: [PATCH 42/58] add clustertests testing backup's retry --- .circleci/config.yml | 2 +- Makefile | 8 +- http/client.go | 32 +++--- internal/clustertests/cluster_test.go | 132 ++++++++++++++++------- internal/clustertests/pause_node_test.go | 7 +- 5 files changed, 118 insertions(+), 63 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index c3e1a43e4..f18532e49 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -151,7 +151,7 @@ jobs: - checkout-plus - skip-if-root-unchanged - setup_remote_docker - - run: make clustertests-build + - run: make clustertests release: executor: name: golang diff --git a/Makefile b/Makefile index a24d8c40a..e33c92b56 100644 --- a/Makefile +++ b/Makefile @@ -149,12 +149,10 @@ DOCKER_COMPOSE=internal/clustertests/docker-compose.yml clustertests: vendor docker-compose -f $(DOCKER_COMPOSE) down docker-compose -f $(DOCKER_COMPOSE) build - docker-compose -f $(DOCKER_COMPOSE) up --exit-code-from=client1 + docker-compose -f $(DOCKER_COMPOSE) up -d pilosa1 pilosa2 pilosa3 + docker-compose -f $(DOCKER_COMPOSE) run client1 + docker-compose -f $(DOCKER_COMPOSE) down -# Like clustertests, but rebuilds all images. -clustertests-build: vendor - docker-compose -f $(DOCKER_COMPOSE) down -v - docker-compose -f $(DOCKER_COMPOSE) up --exit-code-from=client1 --build # Install Pilosa install: diff --git a/http/client.go b/http/client.go index 8590a97fa..bb08478d6 100644 --- a/http/client.go +++ b/http/client.go @@ -1751,26 +1751,22 @@ func (c *InternalClient) doWithRetry(req *http.Request) (*http.Response, error) // start timer after first request, so if retryPeriod > 0 we // pretty much always do at least one retry start := time.Now() - for ; ; resp, err = c.httpClient.Do(req) { - if err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300 { - if time.Since(start) > c.retryPeriod { - break - } - if err != nil { - c.log.Printf("retrying request due to error: '%v'", err) - } else { - if bod, readErr := ioutil.ReadAll(resp.Body); readErr != nil { - c.log.Printf("retrying request due to status: %d, error reading body: '%v', body: '%s'", resp.StatusCode, readErr, bod) - } else { - c.log.Printf("retrying request due to status: %d, body: '%s'", resp.StatusCode, bod) - } - } - - time.Sleep(sleepDuration) - sleepDuration *= 2 - } else { + for ; err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300; resp, err = c.httpClient.Do(req) { + if time.Since(start) > c.retryPeriod { break } + if err != nil { + c.log.Printf("retrying request due to error: '%v'", err) + } else { + if bod, readErr := ioutil.ReadAll(resp.Body); readErr != nil { + c.log.Printf("retrying request due to status: %d, error reading body: '%v', body: '%s'", resp.StatusCode, readErr, bod) + } else { + c.log.Printf("retrying request due to status: %d, body: '%s'", resp.StatusCode, bod) + } + } + + time.Sleep(sleepDuration) + sleepDuration *= 2 } return resp, err } diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index 2fca2f1ab..00967ac07 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -3,6 +3,8 @@ package clustertest import ( "context" + "fmt" + "io/ioutil" "os" "os/exec" "testing" @@ -30,56 +32,53 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("getting client: %v", err) } - t.Run("long pause", func(t *testing.T) { - err := cli1.CreateIndex(context.Background(), "testidx", pilosa.IndexOptions{}) - if err != nil { - t.Fatalf("creating index: %v", err) - } - err = cli1.CreateFieldWithOptions(context.Background(), "testidx", "testf", pilosa.FieldOptions{CacheType: pilosa.CacheTypeRanked, CacheSize: 100}) - if err != nil { - t.Fatalf("creating field: %v", err) - } + if err := cli1.CreateIndex(context.Background(), "testidx", pilosa.IndexOptions{}); err != nil { + t.Fatalf("creating index: %v", err) + } + if err := cli1.CreateFieldWithOptions(context.Background(), "testidx", "testf", pilosa.FieldOptions{CacheType: pilosa.CacheTypeRanked, CacheSize: 100}); err != nil { + t.Fatalf("creating field: %v", err) + } - req := &pilosa.ImportRequest{ - Index: "testidx", - Field: "testf", - } - req.ColumnIDs = make([]uint64, 10) - req.RowIDs = make([]uint64, 10) + req := &pilosa.ImportRequest{ + Index: "testidx", + Field: "testf", + } + req.ColumnIDs = make([]uint64, 10) + req.RowIDs = make([]uint64, 10) - for i := 0; i < 1000; i++ { - req.RowIDs[i%10] = 0 - req.ColumnIDs[i%10] = uint64((i/10)*pilosa.ShardWidth + i%10) - req.Shard = uint64(i / 10) - if i%10 == 9 { - err = cli1.Import(context.Background(), nil, req, &pilosa.ImportOptions{}) - if err != nil { - t.Fatalf("importing: %v", err) - } - } - } - - // Check query results from each node. - for i, cli := range []*picli.InternalClient{cli1, cli2, cli3} { - r, err := cli.Query(context.Background(), "testidx", &pilosa.QueryRequest{Index: "testidx", Query: "Count(Row(testf=0))"}) + for i := 0; i < 1000; i++ { + req.RowIDs[i%10] = 0 + req.ColumnIDs[i%10] = uint64((i/10)*pilosa.ShardWidth + i%10) + req.Shard = uint64(i / 10) + if i%10 == 9 { + err = cli1.Import(context.Background(), nil, req, &pilosa.ImportOptions{}) if err != nil { - t.Fatalf("count querying pilosa%d: %v", i, err) - } - if r.Results[0].(uint64) != 1000 { - t.Fatalf("count on pilosa%d after import is %d", i, r.Results[0].(uint64)) + t.Fatalf("importing: %v", err) } } + } + + // Check query results from each node. + for i, cli := range []*picli.InternalClient{cli1, cli2, cli3} { + r, err := cli.Query(context.Background(), "testidx", &pilosa.QueryRequest{Index: "testidx", Query: "Count(Row(testf=0))"}) + if err != nil { + t.Fatalf("count querying pilosa%d: %v", i, err) + } + if r.Results[0].(uint64) != 1000 { + t.Fatalf("count on pilosa%d after import is %d", i, r.Results[0].(uint64)) + } + } + t.Run("long pause", func(t *testing.T) { pcmd := exec.Command("/pumba", "pause", "clustertests_pilosa3_1", "--duration", "10s") pcmd.Stdout = os.Stdout pcmd.Stderr = os.Stderr t.Log("pausing pilosa3 for 10s") - err = pcmd.Start() - if err != nil { + + if err := pcmd.Start(); err != nil { t.Fatalf("starting pumba command: %v", err) } - err = pcmd.Wait() - if err != nil { + if err := pcmd.Wait(); err != nil { t.Fatalf("waiting on pumba pause cmd: %v", err) } @@ -98,6 +97,63 @@ func TestClusterStuff(t *testing.T) { } } }) + + t.Run("backup", func(t *testing.T) { + // do backup with node 1 down, but restart it after a few seconds + if err := sendCmd("docker", "stop", "clustertests_pilosa1_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + var backupCmd *exec.Cmd + tmpdir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatalf("getting tmp dir: %v", err) + } + if backupCmd, err = startCmd( + "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest")); err != nil { + t.Fatalf("sending backup command: %v", err) + } + time.Sleep(time.Second * 5) + if err = sendCmd("docker", "start", "clustertests_pilosa1_1"); err != nil { + t.Fatalf("sending start command: %v", err) + } + + if err = backupCmd.Wait(); err != nil { + t.Fatalf("waiting on backup to finish: %v", err) + } + + // now do backup with all nodes down and too short a timeout + // so it fails. Has be to be all 3 because the cluster has + // replicas=3 and the backup command will retry on replicas. + if err = sendCmd("docker", "stop", "clustertests_pilosa2_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + if backupCmd, err = startCmd( + "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=0.5s"); err != nil { + t.Fatalf("sending second backup command: %v", err) + } + time.Sleep(time.Millisecond * 5) // want the backup to get started, then fail + if err = sendCmd("docker", "stop", "clustertests_pilosa1_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + if err = sendCmd("docker", "stop", "clustertests_pilosa3_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + + time.Sleep(time.Second * 5) + if err = sendCmd("docker", "start", "clustertests_pilosa1_1"); err != nil { + t.Fatalf("sending start command: %v", err) + } + if err = sendCmd("docker", "start", "clustertests_pilosa2_1"); err != nil { + t.Fatalf("sending start command: %v", err) + } + if err = sendCmd("docker", "start", "clustertests_pilosa3_1"); err != nil { + t.Fatalf("sending start command: %v", err) + } + if err = backupCmd.Wait(); err == nil { + t.Fatal("backup command should have errored but didn't") + } + + }) } func waitForStatus(t *testing.T, stator func(context.Context) (string, error), status string, n int, sleep time.Duration) { diff --git a/internal/clustertests/pause_node_test.go b/internal/clustertests/pause_node_test.go index 1cfa81417..256078a90 100644 --- a/internal/clustertests/pause_node_test.go +++ b/internal/clustertests/pause_node_test.go @@ -23,11 +23,16 @@ import ( "github.com/pkg/errors" ) -func sendCmd(cmd string, args ...string) error { +func startCmd(cmd string, args ...string) (*exec.Cmd, error) { pcmd := exec.Command(cmd, args...) pcmd.Stdout = os.Stdout pcmd.Stderr = os.Stderr err := pcmd.Start() + return pcmd, err +} + +func sendCmd(cmd string, args ...string) error { + pcmd, err := startCmd(cmd, args...) if err != nil { return errors.Wrap(err, "starting cmd") } From 3105a24542a6334608ee7bf82a59ecf66ab99a83 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 20 Dec 2021 17:32:52 -0600 Subject: [PATCH 43/58] rewind Body on retry this is really not ideal, and there are libraries for this kind of thing, but I'd have to figure out how to make the libraries work with everywhere we're already creating stdlib http clients. --- http/client.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/http/client.go b/http/client.go index bb08478d6..0e2b1b428 100644 --- a/http/client.go +++ b/http/client.go @@ -1745,13 +1745,33 @@ func giveRawResponse(b bool) executeRequestOption { } } +type nopCloser struct { + *bytes.Reader +} + +func (n nopCloser) Close() error { + return nil +} + func (c *InternalClient) doWithRetry(req *http.Request) (*http.Response, error) { sleepDuration := time.Second + newBody := nopCloser{} + if req.Body != nil { + bod, err := ioutil.ReadAll(req.Body) + if err != nil { + return nil, errors.Wrap(err, "reading body") + } + newBody.Reader = bytes.NewReader(bod) + req.Body = newBody + } resp, err := c.httpClient.Do(req) // start timer after first request, so if retryPeriod > 0 we // pretty much always do at least one retry start := time.Now() for ; err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300; resp, err = c.httpClient.Do(req) { + if newBody.Reader != nil { + newBody.Seek(0, io.SeekStart) + } if time.Since(start) > c.retryPeriod { break } From f676fbfc5151c68478c46c9f18da97ede2ac2967 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 20 Dec 2021 17:42:22 -0600 Subject: [PATCH 44/58] add retryability to restore command --- cmd/restore.go | 1 + ctl/restore.go | 21 ++++++++++++++++----- go.mod | 1 + go.sum | 5 +++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/cmd/restore.go b/cmd/restore.go index e9af62d24..71fb5ec7d 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -25,6 +25,7 @@ The Restore command will take a backup archive and restore it to a new, clean cl flags.StringVarP(&cmd.Path, "source", "s", "", "backup file; specify '-' to restore from stdin tar stream") flags.StringVar(&cmd.Host, "host", "localhost:10101", "host:port of FeatureBase.") flags.IntVar(&cmd.Concurrency, "concurrency", 1, "number of concurrent uploads") + flags.DurationVar(&cmd.RetryPeriod, "retry-period", cmd.RetryPeriod, "Length of time after HTTP request failure to continue retrying request.") ctl.SetTLSConfig( flags, "", &cmd.TLS.CertificatePath, diff --git a/ctl/restore.go b/ctl/restore.go index 373581d5d..0804c3fa2 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -13,8 +13,12 @@ import ( "path/filepath" "strconv" "strings" + "time" + + "github.com/hashicorp/go-retryablehttp" pilosa "github.com/molecula/featurebase/v2" + fb_http "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/server" "github.com/molecula/featurebase/v2/topology" "golang.org/x/sync/errgroup" @@ -29,6 +33,10 @@ type RestoreCommand struct { // Filepath to the backup file. Path string + + // Amount of time after first failed request to continue retrying. + RetryPeriod time.Duration `json:"retry-period"` + // Reusable client. client pilosa.InternalClient @@ -62,7 +70,7 @@ func (cmd *RestoreCommand) Run(ctx context.Context) (err error) { return fmt.Errorf("parsing tls config: %w", err) } // Create a client to the server. - client, err := commandClient(cmd) + client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) if err != nil { return fmt.Errorf("creating client: %w", err) } @@ -119,7 +127,8 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. if len(existingSchema) == 0 { cmd.Logger().Printf("Load Schema") url := primary.URI.Path("/schema") - var client http.Client + client := retryablehttp.NewClient() + client.RetryWaitMax = cmd.RetryPeriod _, err = client.Post(url, "application/json", f) } else { schema := &pilosa.Schema{} @@ -174,7 +183,8 @@ func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology logger.Printf("Load idalloc") url := primary.URI.Path("/internal/idalloc/restore") - var client http.Client + client := retryablehttp.NewClient() + client.RetryWaitMax = cmd.RetryPeriod _, err = client.Post(url, "application/octet-stream", f) return err } @@ -244,14 +254,15 @@ func (cmd *RestoreCommand) restoreShard(ctx context.Context, filename string) er defer f.Close() url := node.URI.Path(fmt.Sprintf("/internal/restore/%v/%v", indexName, shard)) - req, err := http.NewRequest("POST", url, f) + req, err := retryablehttp.NewRequest("POST", url, f) if err != nil { return err } req = req.WithContext(ctx) req.Header.Set("Content-Type", "application/octet-stream") - var client http.Client + client := retryablehttp.NewClient() + client.RetryWaitMax = cmd.RetryPeriod resp, err := client.Do(req) if err != nil { return err diff --git a/go.mod b/go.mod index b66980688..58b7e60c2 100644 --- a/go.mod +++ b/go.mod @@ -25,6 +25,7 @@ require ( github.com/gopherjs/gopherjs v0.0.0-20200217142428-fce0ec30dd00 // indirect github.com/gorilla/handlers v1.3.0 github.com/gorilla/mux v1.7.0 + github.com/hashicorp/go-retryablehttp v0.7.0 github.com/improbable-eng/grpc-web v0.13.0 github.com/lib/pq v1.8.0 github.com/molecula/apophenia v0.0.0-20190827192002-68b7a14a478b diff --git a/go.sum b/go.sum index 4224f965d..a60699bb5 100644 --- a/go.sum +++ b/go.sum @@ -186,10 +186,15 @@ github.com/grpc-ecosystem/grpc-gateway v1.9.5/go.mod h1:vNeuVxBJEsws4ogUvrchl83t github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= +github.com/hashicorp/go-cleanhttp v0.5.1 h1:dH3aiDG9Jvb5r5+bYHsikaOUIpcM0xvgMXVoDkXMzJM= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= +github.com/hashicorp/go-hclog v0.9.2 h1:CG6TE5H9/JXsFWJCfoIVpKFIkFe6ysEuHirp4DxCsHI= +github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= +github.com/hashicorp/go-retryablehttp v0.7.0 h1:eu1EI/mbirUgP5C8hVsTNaGZreBDlYiwC1FZWkvQPQ4= +github.com/hashicorp/go-retryablehttp v0.7.0/go.mod h1:vAew36LZh98gCBJNLH42IQ1ER/9wtLZZ8meHqQvEYWY= github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa6eBIzfwKfwNnHU= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= From d3b9193c8d2cf29b135910b0990490d42c86b95f Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 20 Dec 2021 21:48:45 -0600 Subject: [PATCH 45/58] try to fix data race with http lib WARNING: DATA RACE Write at 0x00c008121e80 by goroutine 235: bytes.(*Reader).WriteTo() /usr/local/go/src/bytes/reader.go:139 +0x45 github.com/molecula/featurebase/v2/http.nopCloser.WriteTo() :1 +0x5d io.copyBuffer() /usr/local/go/src/io/io.go:391 +0x482 io.Copy() /usr/local/go/src/io/io.go:368 +0x78 net/http.(*transferWriter).doBodyCopy() /usr/local/go/src/net/http/transfer.go:400 +0x2f net/http.(*transferWriter).writeBody() /usr/local/go/src/net/http/transfer.go:364 +0xc9a net/http.(*Request).write() /usr/local/go/src/net/http/request.go:682 +0x887 net/http.(*persistConn).writeLoop() /usr/local/go/src/net/http/transport.go:2343 +0x349 Previous write at 0x00c008121e80 by goroutine 192: bytes.(*Reader).Seek() /usr/local/go/src/bytes/reader.go:118 +0x824 github.com/molecula/featurebase/v2/http.(*InternalClient).doWithRetry() /go/src/github.com/molecula/featurebase/http/client.go:1773 +0x86d github.com/molecula/featurebase/v2/http.(*InternalClient).executeRequest() /go/src/github.com/molecula/featurebase/http/client.go:1806 +0x15b github.com/molecula/featurebase/v2/http.(*InternalClient).CreateIndex() /go/src/github.com/molecula/featurebase/http/client.go:433 +0xbf8 github.com/molecula/featurebase/v2/server_test.TestMain_Set_Quick.func1() /go/src/github.com/molecula/featurebase/server/server_test.go:64 +0x624 testing.tRunner() /usr/local/go/src/testing/testing.go:1123 +0x202 Goroutine 235 (running) created at: net/http.(*Transport).dialConn() /usr/local/go/src/net/http/transport.go:1709 +0xc30 net/http.(*Transport).dialConnFor() /usr/local/go/src/net/http/transport.go:1421 +0x151 Goroutine 192 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:1168 +0x5bb github.com/molecula/featurebase/v2/server_test.TestMain_Set_Quick() /go/src/github.com/molecula/featurebase/server/server_test.go:45 +0x116 testing.tRunner() /usr/local/go/src/testing/testing.go:1123 +0x202 --- http/client.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/http/client.go b/http/client.go index 0e2b1b428..c32dd86ae 100644 --- a/http/client.go +++ b/http/client.go @@ -1755,22 +1755,22 @@ func (n nopCloser) Close() error { func (c *InternalClient) doWithRetry(req *http.Request) (*http.Response, error) { sleepDuration := time.Second - newBody := nopCloser{} + var bod []byte + var err error if req.Body != nil { - bod, err := ioutil.ReadAll(req.Body) + bod, err = ioutil.ReadAll(req.Body) if err != nil { return nil, errors.Wrap(err, "reading body") } - newBody.Reader = bytes.NewReader(bod) - req.Body = newBody + req.Body = nopCloser{bytes.NewReader(bod)} } resp, err := c.httpClient.Do(req) // start timer after first request, so if retryPeriod > 0 we // pretty much always do at least one retry start := time.Now() for ; err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300; resp, err = c.httpClient.Do(req) { - if newBody.Reader != nil { - newBody.Seek(0, io.SeekStart) + if req.Body != nil { + req.Body = nopCloser{bytes.NewReader(bod)} // can't seek due to races with http lib internals } if time.Since(start) > c.retryPeriod { break From 2bce39644554b3a7eee8ec6af23511ebd55f64d2 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 20 Dec 2021 22:16:06 -0600 Subject: [PATCH 46/58] add retry restore test and custom retry policy --- ctl/restore.go | 10 ++++++++ internal/clustertests/cluster_test.go | 35 +++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/ctl/restore.go b/ctl/restore.go index 0804c3fa2..cf99619cb 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -49,6 +49,7 @@ type RestoreCommand struct { func NewRestoreCommand(stdin io.Reader, stdout, stderr io.Writer) *RestoreCommand { return &RestoreCommand{ CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), + RetryPeriod: time.Second * 30, Concurrency: 1, } } @@ -168,6 +169,13 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. return err } +func RetryWith400(ctx context.Context, resp *http.Response, err error) (bool, error) { + if resp != nil && resp.StatusCode > 400 { // we have some dumb status codes + return true, nil + } + return retryablehttp.DefaultRetryPolicy(ctx, resp, err) +} + func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology.Node) error { logger := cmd.Logger() @@ -185,6 +193,7 @@ func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology client := retryablehttp.NewClient() client.RetryWaitMax = cmd.RetryPeriod + client.CheckRetry = RetryWith400 _, err = client.Post(url, "application/octet-stream", f) return err } @@ -263,6 +272,7 @@ func (cmd *RestoreCommand) restoreShard(ctx context.Context, filename string) er client := retryablehttp.NewClient() client.RetryWaitMax = cmd.RetryPeriod + client.CheckRetry = RetryWith400 resp, err := client.Do(req) if err != nil { return err diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index 00967ac07..af89abc2a 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -4,7 +4,7 @@ package clustertest import ( "context" "fmt" - "io/ioutil" + "net/http" "os" "os/exec" "testing" @@ -104,10 +104,7 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("sending stop command: %v", err) } var backupCmd *exec.Cmd - tmpdir, err := ioutil.TempDir("", "") - if err != nil { - t.Fatalf("getting tmp dir: %v", err) - } + tmpdir := t.TempDir() if backupCmd, err = startCmd( "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest")); err != nil { t.Fatalf("sending backup command: %v", err) @@ -121,6 +118,34 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("waiting on backup to finish: %v", err) } + fmt.Println("STARTING RESTORE") + + client := http.Client{} + if req, err := http.NewRequest(http.MethodDelete, "http://pilosa1:10101/index/testidx", nil); err != nil { + t.Fatalf("getting req: %v", err) + } else if resp, err := client.Do(req); err != nil { + t.Fatalf("doing request: %v", err) + } else if resp.StatusCode >= 400 { + t.Fatalf("bad response: %v", resp) + } + + var restoreCmd *exec.Cmd + if restoreCmd, err = startCmd("featurebase", "restore", "-s", tmpdir+"/backuptest", "--host", "pilosa1:10101"); err != nil { + t.Fatalf("starting restore: %v", err) + } + time.Sleep(time.Millisecond * 50) + if err = sendCmd("docker", "stop", "clustertests_pilosa2_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + + time.Sleep(time.Second * 10) + if err = sendCmd("docker", "start", "clustertests_pilosa2_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } + if err := restoreCmd.Wait(); err != nil { + t.Fatalf("restore failed: %v", err) + } + // now do backup with all nodes down and too short a timeout // so it fails. Has be to be all 3 because the cluster has // replicas=3 and the backup command will retry on replicas. From cde3f6b5ea5744135410f300e92acc895912f756 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 21 Dec 2021 15:34:13 -0600 Subject: [PATCH 47/58] add profiling to backup/restore --- cmd/backup.go | 1 + cmd/restore.go | 1 + ctl/backup.go | 18 +++++++++++++--- ctl/restore.go | 11 +++++++++- ctl/util.go | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ http/handler.go | 2 +- 6 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 ctl/util.go diff --git a/cmd/backup.go b/cmd/backup.go index 133f0dbce..28712123d 100644 --- a/cmd/backup.go +++ b/cmd/backup.go @@ -29,6 +29,7 @@ Backs up a FeatureBase server to a local, tar-formatted snapshot file. flags.StringVar(&cmd.Host, "host", "localhost:10101", "The address (host:port) of FeatureBase (HTTP).") flags.StringVar(&cmd.Index, "index", "", "Index to backup, default backs up all indexes. ") flags.DurationVar(&cmd.RetryPeriod, "retry-period", cmd.RetryPeriod, "Length of time after HTTP request failure to continue retrying request.") + flags.StringVar(&cmd.Pprof, "pprof", cmd.Pprof, "host:port to listen for profiling requests at /debug/pprof and /debug/fgprof.") ctl.SetTLSConfig(flags, "", &cmd.TLS.CertificatePath, &cmd.TLS.CertificateKeyPath, &cmd.TLS.CACertPath, &cmd.TLS.SkipVerify, &cmd.TLS.EnableClientVerification) return ccmd } diff --git a/cmd/restore.go b/cmd/restore.go index 71fb5ec7d..bc9271d0e 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -26,6 +26,7 @@ The Restore command will take a backup archive and restore it to a new, clean cl flags.StringVar(&cmd.Host, "host", "localhost:10101", "host:port of FeatureBase.") flags.IntVar(&cmd.Concurrency, "concurrency", 1, "number of concurrent uploads") flags.DurationVar(&cmd.RetryPeriod, "retry-period", cmd.RetryPeriod, "Length of time after HTTP request failure to continue retrying request.") + flags.StringVar(&cmd.Pprof, "pprof", cmd.Pprof, "host:port to listen for profiling requests at /debug/pprof and /debug/fgprof.") ctl.SetTLSConfig( flags, "", &cmd.TLS.CertificatePath, diff --git a/ctl/backup.go b/ctl/backup.go index 8ba57c278..302041dfe 100644 --- a/ctl/backup.go +++ b/ctl/backup.go @@ -13,9 +13,10 @@ import ( "time" pilosa "github.com/molecula/featurebase/v2" - "github.com/molecula/featurebase/v2/http" + fb_http "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/server" "github.com/molecula/featurebase/v2/topology" + "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -41,6 +42,9 @@ type BackupCommand struct { // nolint: maligned // Amount of time after first failed request to continue retrying. RetryPeriod time.Duration `json:"retry-period"` + // Host:port on which to listen for pprof. + Pprof string `json:"pprof"` + // Reusable client. client pilosa.InternalClient @@ -56,11 +60,19 @@ func NewBackupCommand(stdin io.Reader, stdout, stderr io.Writer) *BackupCommand CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), Concurrency: 1, RetryPeriod: time.Minute, + Pprof: "localhost:43809", } } // Run executes the main program execution. func (cmd *BackupCommand) Run(ctx context.Context) (err error) { + logger := cmd.Logger() + close, err := startProfilingServer(cmd.Pprof, logger) + if err != nil { + return errors.Wrap(err, "starting profiling server") + } + defer close() + // Validate arguments. if cmd.OutputDir == "" { return fmt.Errorf("-o flag required") @@ -75,7 +87,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) { } // Create a client to the server. - client, err := commandClient(cmd, http.WithClientRetryPeriod(cmd.RetryPeriod)) + client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) if err != nil { return fmt.Errorf("creating client: %w", err) } @@ -267,7 +279,7 @@ func (cmd *BackupCommand) backupShardNode(ctx context.Context, indexName string, logger := cmd.Logger() logger.Printf("backing up shard: index=%q id=%d", indexName, shard) - client := http.NewInternalClientFromURI(&node.URI, http.GetHTTPClient(cmd.tlsConfig), http.WithClientRetryPeriod(cmd.RetryPeriod)) + client := fb_http.NewInternalClientFromURI(&node.URI, fb_http.GetHTTPClient(cmd.tlsConfig), fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) rc, err := client.ShardReader(ctx, indexName, shard) if err != nil { return fmt.Errorf("fetching shard reader: %w", err) diff --git a/ctl/restore.go b/ctl/restore.go index cf99619cb..589eb8138 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -5,7 +5,6 @@ import ( "context" "crypto/tls" "encoding/json" - "errors" "fmt" "io" "net/http" @@ -21,6 +20,7 @@ import ( fb_http "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/server" "github.com/molecula/featurebase/v2/topology" + "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -37,6 +37,9 @@ type RestoreCommand struct { // Amount of time after first failed request to continue retrying. RetryPeriod time.Duration `json:"retry-period"` + // Host:port on which to listen for pprof. + Pprof string `json:"pprof"` + // Reusable client. client pilosa.InternalClient @@ -51,12 +54,18 @@ func NewRestoreCommand(stdin io.Reader, stdout, stderr io.Writer) *RestoreComman CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), RetryPeriod: time.Second * 30, Concurrency: 1, + Pprof: "localhost:43809", } } // Run executes the restore. func (cmd *RestoreCommand) Run(ctx context.Context) (err error) { logger := cmd.Logger() + close, err := startProfilingServer(cmd.Pprof, logger) + if err != nil { + return errors.Wrap(err, "starting profiling server") + } + defer close() // Validate arguments. if cmd.Path == "" { diff --git a/ctl/util.go b/ctl/util.go new file mode 100644 index 000000000..60ac082c9 --- /dev/null +++ b/ctl/util.go @@ -0,0 +1,56 @@ +package ctl + +import ( + "context" + "net" + "net/http" + "net/http/pprof" + "runtime" + "time" + + "github.com/felixge/fgprof" + "github.com/molecula/featurebase/v2/logger" + "github.com/pkg/errors" +) + +// startProfilingServer starts a server which handles /debug/pprof and +// /debug/fgprof for use in utilities we might want to profile but +// wouldn't otherwise be running an http server. Caller should call +// the returned close function before exiting to release resources. +func startProfilingServer(addr string, logger logger.Logger) (close func() error, err error) { + if addr == "" { + return func() error { return nil }, nil + } + + sm := http.NewServeMux() + sm.Handle("/debug/fgprof", fgprof.Handler()) + sm.HandleFunc("/debug/pprof/", pprof.Index) + sm.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) + sm.HandleFunc("/debug/pprof/profile", pprof.Profile) + sm.HandleFunc("/debug/pprof/symbol", pprof.Symbol) + sm.HandleFunc("/debug/pprof/trace", pprof.Trace) + s := &http.Server{ + Addr: addr, + Handler: sm, + } + runtime.SetBlockProfileRate(10000000) // 1 sample per 10 ms + runtime.SetMutexProfileFraction(100) // 1% sampling + ln, err := net.Listen("tcp", addr) + if err != nil { + return nil, err + } + go func() { + logger.Printf("Listening for /debug/pprof/ and /debug/fgprof on '%s'", addr) + logger.Printf("%v", s.Serve(ln)) + }() + + return func() error { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + defer cancel() + err := s.Shutdown(ctx) + if err != nil { + return errors.Wrap(err, "shutting down profiling server") + } + return s.Close() + }, nil +} diff --git a/http/handler.go b/http/handler.go index 9e626babf..958f29662 100644 --- a/http/handler.go +++ b/http/handler.go @@ -3348,7 +3348,7 @@ func (h *Handler) handlePostRestore(w http.ResponseWriter, r *http.Request) { //validate shard for this node err = h.api.RestoreShard(ctx, indexName, shard, r.Body) if err != nil { - http.Error(w, fmt.Sprintf("failed to restore shared %v %v err:%v", indexName, shard, err), http.StatusBadRequest) + http.Error(w, fmt.Sprintf("failed to restore shard %v %v err:%v", indexName, shard, err), http.StatusBadRequest) return } From 640ba45129f1df002a2df378fda4ecda50c6aa06 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Wed, 22 Dec 2021 10:48:10 -0600 Subject: [PATCH 48/58] use retryableHTTP in client, fix memory usage of restore instead of awkwardly reading an entire file into a buffer, we use retryablehttp's reader func to open the file fresh if we need to retry, so a small fixed-size buffer can be used internally for copying the contents onto the network. --- client.go | 14 +++++-- cmd/slurp/slurp.go | 12 ++++-- ctl/restore.go | 22 ++++------ http/client.go | 101 ++++++++++++++++++++++----------------------- 4 files changed, 76 insertions(+), 73 deletions(-) diff --git a/client.go b/client.go index fe91eb124..75741fcd3 100644 --- a/client.go +++ b/client.go @@ -80,8 +80,14 @@ type InternalClient interface { GetNodeUsage(ctx context.Context, uri *pnet.URI) (map[string]NodeUsage, error) GetPastQueries(ctx context.Context, uri *pnet.URI) ([]PastQueryStatus, error) - ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, rddbdata io.Reader) error - ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, rddbdata io.Reader) error + // ImportFieldKeys and ImportIndexKeys are mainly used when + // restoring a backup. They take a readerFunc which returns a + // reader rather than taking an io.Reader directly to allow for + // efficient retries (rather than reading the entire request body + // into a buffer and reusing it). Reader returned from the func + // must be properly closed by the implementation. + ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, readerFunc func() (io.Reader, error)) error + ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, readerFunc func() (io.Reader, error)) error // SetInternalAPI tells the client the API it should use for internal/loopback ops // where applicable. @@ -277,11 +283,11 @@ func (n nopInternalClient) GetNodeUsage(ctx context.Context, uri *pnet.URI) (map func (n nopInternalClient) GetPastQueries(ctx context.Context, uri *pnet.URI) ([]PastQueryStatus, error) { return nil, nil } -func (c nopInternalClient) ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, rddbdata io.Reader) error { +func (c nopInternalClient) ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, readerFunc func() (io.Reader, error)) error { return nil } -func (c nopInternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, rddbdata io.Reader) error { +func (c nopInternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, readerFunc func() (io.Reader, error)) error { return nil } diff --git a/cmd/slurp/slurp.go b/cmd/slurp/slurp.go index 446db53b1..800a5a773 100644 --- a/cmd/slurp/slurp.go +++ b/cmd/slurp/slurp.go @@ -92,8 +92,10 @@ func (r *stateMachine) NewHeader(h *tar.Header, tr *tar.Reader) error { byteData, err := ioutil.ReadAll(tr) vprint.PanicOn(err) - br := bytes.NewReader(byteData) - err = r.client.ImportFieldKeys(context.Background(), uri, index, fieldName, false, br) + readerFunc := func() (io.Reader, error) { + return bytes.NewReader(byteData), nil + } + err = r.client.ImportFieldKeys(context.Background(), uri, index, fieldName, false, readerFunc) if err != nil { return err } @@ -106,9 +108,11 @@ func (r *stateMachine) NewHeader(h *tar.Header, tr *tar.Reader) error { } byteData, err := ioutil.ReadAll(tr) vprint.PanicOn(err) + readerFunc := func() (io.Reader, error) { + return bytes.NewReader(byteData), nil + } - br := bytes.NewReader(byteData) - err = r.client.ImportIndexKeys(context.Background(), uri, index, int(partition), false, br) + err = r.client.ImportIndexKeys(context.Background(), uri, index, int(partition), false, readerFunc) if err != nil { return err } diff --git a/ctl/restore.go b/ctl/restore.go index 589eb8138..d0c0b7ec7 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -178,7 +178,7 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. return err } -func RetryWith400(ctx context.Context, resp *http.Response, err error) (bool, error) { +func retryWith400(ctx context.Context, resp *http.Response, err error) (bool, error) { if resp != nil && resp.StatusCode > 400 { // we have some dumb status codes return true, nil } @@ -202,7 +202,7 @@ func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology client := retryablehttp.NewClient() client.RetryWaitMax = cmd.RetryPeriod - client.CheckRetry = RetryWith400 + client.CheckRetry = retryWith400 _, err = client.Post(url, "application/octet-stream", f) return err } @@ -281,7 +281,7 @@ func (cmd *RestoreCommand) restoreShard(ctx context.Context, filename string) er client := retryablehttp.NewClient() client.RetryWaitMax = cmd.RetryPeriod - client.CheckRetry = RetryWith400 + client.CheckRetry = retryWith400 resp, err := client.Do(req) if err != nil { return err @@ -349,13 +349,11 @@ func (cmd *RestoreCommand) restoreIndexTranslationFile(ctx context.Context, file for _, node := range nodes { if err := func() error { - f, err := os.Open(filename) - if err != nil { - return err + readerFunc := func() (io.Reader, error) { + return os.Open(filename) // gets used as an HTTP request body and closed by http library } - defer f.Close() - return cmd.client.ImportIndexKeys(ctx, &node.URI, indexName, partitionID, false, f) + return cmd.client.ImportIndexKeys(ctx, &node.URI, indexName, partitionID, false, readerFunc) }(); err != nil { return err } @@ -410,13 +408,11 @@ func (cmd *RestoreCommand) restoreFieldTranslationFile(ctx context.Context, node for _, node := range nodes { if err := func() error { - f, err := os.Open(filename) - if err != nil { - return err + readerFunc := func() (io.Reader, error) { + return os.Open(filename) } - defer f.Close() - return cmd.client.ImportFieldKeys(ctx, &node.URI, indexName, fieldName, false, f) + return cmd.client.ImportFieldKeys(ctx, &node.URI, indexName, fieldName, false, readerFunc) }(); err != nil { return err } diff --git a/http/client.go b/http/client.go index c32dd86ae..a3466adac 100644 --- a/http/client.go +++ b/http/client.go @@ -18,6 +18,7 @@ import ( "strings" "time" + "github.com/hashicorp/go-retryablehttp" pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/encoding/proto" "github.com/molecula/featurebase/v2/ingest" @@ -35,10 +36,9 @@ type InternalClient struct { log logger.Logger - retryPeriod time.Duration - // The client to use for HTTP communication. - httpClient *http.Client + httpClient *http.Client + retryableClient *retryablehttp.Client // the local node's API, used for operations that we can short-circuit that way api *pilosa.API } @@ -64,9 +64,13 @@ type InternalClientOption func(c *InternalClient) // WithClientRetryPeriod is the max amount of total time the client will // retry failed requests using exponential backoff. -func WithClientRetryPeriod(period time.Duration) InternalClientOption { +func WithClientRetryPeriod(waitMax time.Duration) InternalClientOption { return func(c *InternalClient) { - c.retryPeriod = period + c.retryableClient = &retryablehttp.Client{ + HTTPClient: c.httpClient, + RetryWaitMax: waitMax, + CheckRetry: retryWith400Policy, + } } } @@ -76,6 +80,21 @@ func WithClientLogger(log logger.Logger) InternalClientOption { } } +func noRetryPolicy(ctx context.Context, resp *http.Response, err error) (bool, error) { + return false, nil +} + +// retryWith400Policy wraps retryablehttp's default retry policy to +// also retry on 4XX errors which *should* be client errors and +// therefore useless to retry, but we have some incorrect status codes. +// TODO: fix the incorrect status codes so we can get rid of this. +func retryWith400Policy(ctx context.Context, resp *http.Response, err error) (bool, error) { + if resp != nil && resp.StatusCode > 400 { + return true, nil + } + return retryablehttp.DefaultRetryPolicy(ctx, resp, err) +} + func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client, opts ...InternalClientOption) *InternalClient { ic := &InternalClient{ defaultURI: defaultURI, @@ -87,6 +106,13 @@ func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client, o for _, opt := range opts { opt(ic) } + + if ic.retryableClient == nil { + ic.retryableClient = &retryablehttp.Client{ + HTTPClient: ic.httpClient, + CheckRetry: noRetryPolicy, + } + } return ic } @@ -1753,57 +1779,28 @@ func (n nopCloser) Close() error { return nil } -func (c *InternalClient) doWithRetry(req *http.Request) (*http.Response, error) { - sleepDuration := time.Second - var bod []byte - var err error - if req.Body != nil { - bod, err = ioutil.ReadAll(req.Body) - if err != nil { - return nil, errors.Wrap(err, "reading body") - } - req.Body = nopCloser{bytes.NewReader(bod)} - } - resp, err := c.httpClient.Do(req) - // start timer after first request, so if retryPeriod > 0 we - // pretty much always do at least one retry - start := time.Now() - for ; err != nil || resp.StatusCode < 200 || resp.StatusCode >= 300; resp, err = c.httpClient.Do(req) { - if req.Body != nil { - req.Body = nopCloser{bytes.NewReader(bod)} // can't seek due to races with http lib internals - } - if time.Since(start) > c.retryPeriod { - break - } - if err != nil { - c.log.Printf("retrying request due to error: '%v'", err) - } else { - if bod, readErr := ioutil.ReadAll(resp.Body); readErr != nil { - c.log.Printf("retrying request due to status: %d, error reading body: '%v', body: '%s'", resp.StatusCode, readErr, bod) - } else { - c.log.Printf("retrying request due to status: %d, body: '%s'", resp.StatusCode, bod) - } - } - - time.Sleep(sleepDuration) - sleepDuration *= 2 - } - return resp, err -} - // executeRequest executes the given request and checks the Response. For // responses with non-2XX status, the body is read and closed, and an error is // returned. If the error is nil, the caller must ensure that the response body // is closed. func (c *InternalClient) executeRequest(req *http.Request, opts ...executeRequestOption) (*http.Response, error) { + return c.executeRetryableRequest(&retryablehttp.Request{Request: req}, opts...) +} + +func (c *InternalClient) executeRetryableRequest(req *retryablehttp.Request, opts ...executeRequestOption) (*http.Response, error) { + tracing.GlobalTracer.InjectHTTPHeaders(req.Request) + req.Close = false eo := &executeOpts{} for _, opt := range opts { opt(eo) } - tracing.GlobalTracer.InjectHTTPHeaders(req) - req.Close = false - resp, err := c.doWithRetry(req) + resp, err := c.retryableClient.Do(req) + + return c.handleResponse(req.Request, eo, resp, err) +} + +func (c *InternalClient) handleResponse(req *http.Request, eo *executeOpts, resp *http.Response, err error) (*http.Response, error) { if err != nil { if resp != nil { resp.Body.Close() @@ -2083,7 +2080,7 @@ func (c *InternalClient) RetrieveTranslatePartitionFromURI(ctx context.Context, return resp.Body, nil } -func (c *InternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, rddbdata io.Reader) error { +func (c *InternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, index string, partitionID int, remote bool, readerFunc func() (io.Reader, error)) error { span, ctx := tracing.StartSpanFromContext(ctx, "InternalClient.ImportIndexKeys") defer span.Finish() @@ -2100,14 +2097,14 @@ func (c *InternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, ind url := fmt.Sprintf("%s/internal/translate/index/%s/%d", uri, index, partitionID) // Generate HTTP request. - httpReq, err := http.NewRequest("POST", url, rddbdata) + httpReq, err := retryablehttp.NewRequest("POST", url, readerFunc) if err != nil { return errors.Wrap(err, "creating request") } httpReq.Header.Set("User-Agent", "pilosa/"+pilosa.Version) // Execute request against the host. - resp, err := c.executeRequest(httpReq.WithContext(ctx)) + resp, err := c.executeRetryableRequest(httpReq.WithContext(ctx)) if err != nil { return err } @@ -2115,7 +2112,7 @@ func (c *InternalClient) ImportIndexKeys(ctx context.Context, uri *pnet.URI, ind return nil } -func (c *InternalClient) ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, rddbdata io.Reader) error { +func (c *InternalClient) ImportFieldKeys(ctx context.Context, uri *pnet.URI, index, field string, remote bool, readerFunc func() (io.Reader, error)) error { span, ctx := tracing.StartSpanFromContext(ctx, "InternalClient.ImportFieldKeys") defer span.Finish() @@ -2132,14 +2129,14 @@ func (c *InternalClient) ImportFieldKeys(ctx context.Context, uri *pnet.URI, ind url := fmt.Sprintf("%s/internal/translate/field/%s/%s", uri, index, field) // Generate HTTP request. - httpReq, err := http.NewRequest("POST", url, rddbdata) + httpReq, err := retryablehttp.NewRequest("POST", url, readerFunc) if err != nil { return errors.Wrap(err, "creating request") } httpReq.Header.Set("User-Agent", "pilosa/"+pilosa.Version) // Execute request against the host. - resp, err := c.executeRequest(httpReq.WithContext(ctx)) + resp, err := c.executeRetryableRequest(httpReq.WithContext(ctx)) if err != nil { return err } From ea59f14d5025138a91b6bfff154c8f9fa83dd43a Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Wed, 22 Dec 2021 11:21:11 -0600 Subject: [PATCH 49/58] must use retryablehttp.NewClient to get defaults otherwise it won't actually retry :( --- http/client.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/http/client.go b/http/client.go index a3466adac..7f7374d4d 100644 --- a/http/client.go +++ b/http/client.go @@ -66,11 +66,12 @@ type InternalClientOption func(c *InternalClient) // retry failed requests using exponential backoff. func WithClientRetryPeriod(waitMax time.Duration) InternalClientOption { return func(c *InternalClient) { - c.retryableClient = &retryablehttp.Client{ - HTTPClient: c.httpClient, - RetryWaitMax: waitMax, - CheckRetry: retryWith400Policy, - } + fmt.Println("client w/ retry policy", waitMax) + rc := retryablehttp.NewClient() + rc.HTTPClient = c.httpClient + rc.RetryWaitMax = waitMax + rc.CheckRetry = retryWith400Policy + c.retryableClient = rc } } @@ -108,10 +109,11 @@ func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client, o } if ic.retryableClient == nil { - ic.retryableClient = &retryablehttp.Client{ - HTTPClient: ic.httpClient, - CheckRetry: noRetryPolicy, - } + fmt.Println("no retry policy") + rc := retryablehttp.NewClient() + rc.HTTPClient = ic.httpClient + rc.CheckRetry = noRetryPolicy + ic.retryableClient = rc } return ic } From 295fab4892d81336b71c49a6d0a4ec6e790e6e7a Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Wed, 22 Dec 2021 12:21:11 -0600 Subject: [PATCH 50/58] retry on >= 400, not just greater. good catch --- ctl/restore.go | 2 +- http/client.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ctl/restore.go b/ctl/restore.go index d0c0b7ec7..f7d669f09 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -179,7 +179,7 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. } func retryWith400(ctx context.Context, resp *http.Response, err error) (bool, error) { - if resp != nil && resp.StatusCode > 400 { // we have some dumb status codes + if resp != nil && resp.StatusCode >= 400 { // we have some dumb status codes return true, nil } return retryablehttp.DefaultRetryPolicy(ctx, resp, err) diff --git a/http/client.go b/http/client.go index 7f7374d4d..222499794 100644 --- a/http/client.go +++ b/http/client.go @@ -90,7 +90,7 @@ func noRetryPolicy(ctx context.Context, resp *http.Response, err error) (bool, e // therefore useless to retry, but we have some incorrect status codes. // TODO: fix the incorrect status codes so we can get rid of this. func retryWith400Policy(ctx context.Context, resp *http.Response, err error) (bool, error) { - if resp != nil && resp.StatusCode > 400 { + if resp != nil && resp.StatusCode >= 400 { return true, nil } return retryablehttp.DefaultRetryPolicy(ctx, resp, err) From 9367a626095ecfb10d702f8abdab0415b807d9c1 Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 27 Dec 2021 09:34:43 -0700 Subject: [PATCH 51/58] Add /debug/rbf endpoint for debugging --- api.go | 16 ++++++++++++++++ api_test.go | 22 ++++++++++++++++++++++ http/handler.go | 15 +++++++++++++++ rbf/db.go | 17 +++++++++++++++++ rbf/db_test.go | 15 +++++++++++++++ rbf/tx.go | 17 +++++++++++++++++ 6 files changed, 102 insertions(+) diff --git a/api.go b/api.go index 47558678d..628feb16d 100644 --- a/api.go +++ b/api.go @@ -23,6 +23,7 @@ import ( "github.com/molecula/featurebase/v2/disco" "github.com/molecula/featurebase/v2/ingest" + "github.com/molecula/featurebase/v2/rbf" //"github.com/molecula/featurebase/v2/pg" "github.com/molecula/featurebase/v2/pql" @@ -3156,6 +3157,21 @@ func (api *API) Plan(ctx context.Context, q string) (*Stmt, error) { return api.server.PlanSQL(ctx, q) } +func (api *API) RBFDebugInfo() map[string]*rbf.DebugInfo { + infos := make(map[string]*rbf.DebugInfo) + + for key, dbShard := range api.holder.Txf().dbPerShard.Flatmap { + wrapper, ok := dbShard.W.(*RbfDBWrapper) + if !ok { + continue + } + + skey := fmt.Sprintf("%s/%d", key.index, key.shard) + infos[skey] = wrapper.db.DebugInfo() + } + return infos +} + type serverInfo struct { ShardWidth uint64 `json:"shardWidth"` ReplicaN int `json:"replicaN"` diff --git a/api_test.go b/api_test.go index 9ffdea841..d42aca667 100644 --- a/api_test.go +++ b/api_test.go @@ -1415,3 +1415,25 @@ func TestVariousApiTranslateCalls(t *testing.T) { */ } } + +func TestAPI_RBFDebugInfo(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c := test.MustRunCluster(t, 1, + []server.CommandOption{ + server.OptCommandServerOptions( + pilosa.OptServerNodeID("node0"), + pilosa.OptServerClusterHasher(&offsetModHasher{}), + pilosa.OptServerOpenTranslateReader(http.GetOpenTranslateReaderFunc(nil)), + )}, + ) + defer c.Close() + + coord := c.GetPrimary() + + if _, err := coord.API.CreateIndex(ctx, "i", pilosa.IndexOptions{}); err != nil { + t.Fatal(err) + } else if infos := coord.API.RBFDebugInfo(); infos == nil { + t.Fatal("expected info") + } +} diff --git a/http/handler.go b/http/handler.go index 958f29662..05b65aa58 100644 --- a/http/handler.go +++ b/http/handler.go @@ -441,6 +441,9 @@ func newRouter(handler *Handler) http.Handler { router.HandleFunc("/internal/idalloc/data", handler.handleIDAllocData).Methods("GET").Name("IDAllocData") router.HandleFunc("/internal/restore/{index}/{shardID}", handler.handlePostRestore).Methods("POST").Name("Restore") + + router.HandleFunc("/internal/debug/rbf", handler.handleGetInternalDebugRBFJSON).Methods("GET").Name("GetInternalDebugRBFJSON") + // endpoints for collecting cpu profiles from a chosen begin point to // when the client wants to stop. Used for profiling imports that // could be long or short. @@ -2064,6 +2067,18 @@ func validateProtobufHeader(r *http.Request) (error string, code int) { return } +// handleGetInternalDebugRBFJSON handles /internal/debug/rbf requests. +func (h *Handler) handleGetInternalDebugRBFJSON(w http.ResponseWriter, r *http.Request) { + buf, err := json.MarshalIndent(h.api.RBFDebugInfo(), "", " ") + if err != nil { + http.Error(w, "marshal json: "+err.Error(), http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", "application/json") + w.Write(buf) +} + // handleGetMetricsJSON handles /metrics.json requests, translating text metrics results to more consumable JSON. func (h *Handler) handleGetMetricsJSON(w http.ResponseWriter, r *http.Request) { if !validHeaderAcceptJSON(r.Header) { diff --git a/rbf/db.go b/rbf/db.go index 65dd4fbea..6e4955f94 100644 --- a/rbf/db.go +++ b/rbf/db.go @@ -7,6 +7,8 @@ import ( "io" "os" "path/filepath" + "runtime/debug" + "sort" "sync" "syscall" @@ -637,6 +639,7 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) { pageMap: db.pageMap, walPageN: db.walPageN, writable: writable, + stack: debug.Stack(), // DEBUG DeleteEmptyContainer: true, } @@ -815,6 +818,20 @@ func (db *DB) getCursor(tx *Tx) *Cursor { return c } +func (db *DB) DebugInfo() *DebugInfo { + info := &DebugInfo{Path: db.Path} + for tx := range db.txs { + info.Txs = append(info.Txs, tx.DebugInfo()) + } + sort.Slice(info.Txs, func(i, j int) bool { return info.Txs[i].Ptr < info.Txs[j].Ptr }) + return info +} + +type DebugInfo struct { + Path string `json:"path"` + Txs []*TxDebugInfo `json:"txs"` +} + // Shared pool for in-memory database pages. // These are used before being flushed to disk. var pagePool = &sync.Pool{ diff --git a/rbf/db_test.go b/rbf/db_test.go index c0eb3b3c7..8bae550bb 100644 --- a/rbf/db_test.go +++ b/rbf/db_test.go @@ -339,6 +339,21 @@ func TestDB_MultiTx(t *testing.T) { } } +func TestDB_DebugInfo(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + + tx := MustBegin(t, db, true) + defer tx.Rollback() + + info := db.DebugInfo() + if got, want := info.Path, db.Path; got != want { + t.Fatalf("Path=%q, want %q", got, want) + } else if got, want := len(info.Txs), 1; got != want { + t.Fatalf("len(Txs)=%d, want %d", got, want) + } +} + // premake pool of random values const randPool = (1 << 18) diff --git a/rbf/tx.go b/rbf/tx.go index 5c6c7f7c6..5d731fe4f 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -65,6 +65,9 @@ type Tx struct { // manages to trigger a *deallocation* (which I don't think should be // happening), we'll process that one after the current list is processed. pendingFreelistAdds []uint32 + + // DEBUG + stack []byte } func (tx *Tx) DBPath() string { @@ -2042,6 +2045,20 @@ func (tx *Tx) GetSortedFieldViewList() (fvs []txkey.FieldView, _ error) { return } +func (tx *Tx) DebugInfo() *TxDebugInfo { + return &TxDebugInfo{ + Ptr: fmt.Sprintf("%p", tx), + Writable: tx.writable, + Stack: string(tx.stack), + } +} + +type TxDebugInfo struct { + Ptr string `json:"ptr"` + Writable bool `json:"writable"` + Stack string `json:"stack,omitempty"` +} + // SnapshotReader returns a reader that provides a snapshot for the current database state. func (tx *Tx) SnapshotReader() (io.Reader, error) { if tx.db == nil { From 6638fa17eeb36ca4ffcbcf5c4fb0ab053291747d Mon Sep 17 00:00:00 2001 From: Travis Date: Mon, 27 Dec 2021 10:22:04 -0600 Subject: [PATCH 52/58] Expose `etcd.dir` configuration option The goal is to allow a user to separate FeatureBase and etcd I/O. --- ctl/server.go | 2 +- internal/clustertests/docker-compose.yml | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ctl/server.go b/ctl/server.go index 9867c35b9..086506f54 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -45,7 +45,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // Etcd // Etcd.Name used Config.Name for its value. - // Etcd.Dir defaults to a directory under the pilosa data directory. + flags.StringVar(&srv.Config.Etcd.Dir, "etcd.dir", srv.Config.Etcd.Dir, "Directory to store etcd data files. If not provided, a directory will be created under the main data-dir directory.") // Etcd.ClusterName uses Cluster.Name for its value flags.StringVar(&srv.Config.Etcd.LClientURL, "etcd.listen-client-address", srv.Config.Etcd.LClientURL, "Listen client address.") flags.StringVar(&srv.Config.Etcd.AClientURL, "etcd.advertise-client-address", srv.Config.Etcd.AClientURL, "Advertise client address. If not provided, uses the listen client address.") diff --git a/internal/clustertests/docker-compose.yml b/internal/clustertests/docker-compose.yml index 4192be6f8..46143a3f6 100644 --- a/internal/clustertests/docker-compose.yml +++ b/internal/clustertests/docker-compose.yml @@ -9,6 +9,7 @@ services: - "33455:10101" environment: - PILOSA_NAME=pilosa1 + - PILOSA_ETCD_DIR=/root/.etcd - PILOSA_ETCD_LISTEN_CLIENT_ADDRESS=http://0.0.0.0:10201 - PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS=http://pilosa1:10201 - PILOSA_ETCD_LISTEN_PEER_ADDRESS=http://0.0.0.0:10301 @@ -28,6 +29,7 @@ services: - "33456:10101" environment: - PILOSA_NAME=pilosa2 + - PILOSA_ETCD_DIR=/root/.etcd - PILOSA_ETCD_LISTEN_CLIENT_ADDRESS=http://0.0.0.0:10201 - PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS=http://pilosa2:10201 - PILOSA_ETCD_LISTEN_PEER_ADDRESS=http://0.0.0.0:10301 @@ -47,6 +49,7 @@ services: - "33457:10101" environment: - PILOSA_NAME=pilosa3 + - PILOSA_ETCD_DIR=/root/.etcd - PILOSA_ETCD_LISTEN_CLIENT_ADDRESS=http://0.0.0.0:10201 - PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS=http://pilosa3:10201 - PILOSA_ETCD_LISTEN_PEER_ADDRESS=http://0.0.0.0:10301 From 310584b0d8d69c9e3e5654bc2bf950109a90526a Mon Sep 17 00:00:00 2001 From: Ben Johnson Date: Mon, 27 Dec 2021 13:30:57 -0700 Subject: [PATCH 53/58] Add job & worker metrics --- executor.go | 20 ++++++++++++++++++-- server.go | 4 ++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/executor.go b/executor.go index 5e03e1a72..f0adc7bc7 100644 --- a/executor.go +++ b/executor.go @@ -179,11 +179,18 @@ func newExecutor(opts ...executorOption) *executor { func (e *executor) addWorker() { e.workersWG.Add(1) - atomic.AddInt64(&e.currentWorkers, 1) + n := atomic.AddInt64(&e.currentWorkers, 1) + if e.Holder != nil { + e.Holder.Stats.Gauge("worker_total", float64(n), 0) + } + go func() { defer e.workersWG.Done() e.worker(e.work) - atomic.AddInt64(&e.currentWorkers, -1) + n := atomic.AddInt64(&e.currentWorkers, -1) + if e.Holder != nil { + e.Holder.Stats.Gauge("worker_total", float64(n), 0) + } }() } @@ -204,6 +211,14 @@ func (e *executor) Close() error { return nil } +// InitStats initializes stats counters. Must be called after Holder set. +func (e *executor) InitStats() { + if e.Holder != nil { + e.Holder.Stats.Count("job_total", 0, 0) + e.Holder.Stats.Gauge("worker_total", float64(atomic.LoadInt64(&e.currentWorkers)), 0) + } +} + // Execute executes a PQL query. func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shards []uint64, opt *execOptions) (QueryResponse, error) { span, ctx := tracing.StartSpanFromContext(ctx, "Executor.Execute") @@ -5989,6 +6004,7 @@ type job struct { func (e *executor) worker(work chan job) { for j := range work { atomic.AddUint64(&e.workCounter, 1) + e.Holder.Stats.Count("job_total", 1, 0) if j.idleHands { return } diff --git a/server.go b/server.go index 0c74a2fa5..a5d363e80 100644 --- a/server.go +++ b/server.go @@ -506,6 +506,10 @@ func NewServer(opts ...ServerOption) (*Server, error) { s.holder.schemator = s.schemator s.holder.sharder = s.sharder s.holder.serializer = s.serializer + + // Initial stats must be invoked after the executor obtains reference to the holder. + s.executor.InitStats() + return s, nil } From bb39b05d0572e42d4b23090441a204e55b969aae Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 27 Dec 2021 13:02:22 -0600 Subject: [PATCH 54/58] remove leftover fmt.Println --- http/client.go | 1 - 1 file changed, 1 deletion(-) diff --git a/http/client.go b/http/client.go index 222499794..1dd2da51a 100644 --- a/http/client.go +++ b/http/client.go @@ -109,7 +109,6 @@ func NewInternalClientFromURI(defaultURI *pnet.URI, remoteClient *http.Client, o } if ic.retryableClient == nil { - fmt.Println("no retry policy") rc := retryablehttp.NewClient() rc.HTTPClient = ic.httpClient rc.CheckRetry = noRetryPolicy From fe54cbf8ae31f64f269157e483bb7df8abbb4b21 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Mon, 27 Dec 2021 14:27:59 -0600 Subject: [PATCH 55/58] remove other print and tweak backup test timings --- http/client.go | 1 - internal/clustertests/cluster_test.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/http/client.go b/http/client.go index 1dd2da51a..e343c65c9 100644 --- a/http/client.go +++ b/http/client.go @@ -66,7 +66,6 @@ type InternalClientOption func(c *InternalClient) // retry failed requests using exponential backoff. func WithClientRetryPeriod(waitMax time.Duration) InternalClientOption { return func(c *InternalClient) { - fmt.Println("client w/ retry policy", waitMax) rc := retryablehttp.NewClient() rc.HTTPClient = c.httpClient rc.RetryWaitMax = waitMax diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index af89abc2a..d3009dbda 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -153,7 +153,7 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("sending stop command: %v", err) } if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=0.5s"); err != nil { + "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=50ms"); err != nil { t.Fatalf("sending second backup command: %v", err) } time.Sleep(time.Millisecond * 5) // want the backup to get started, then fail From 1a8c10d5f3adebd45971a2ab770f0dbaf8778538 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 28 Dec 2021 10:31:05 -0600 Subject: [PATCH 56/58] fix backup fail test so it actually fails A few things were going wrong here. First, we take a "RetryPeriod" option on backup and restore which is meant to be roughly the total amount of time we spend retrying any given request before failing. However we were incorrectly passing that as the RetryMaxWait which is the maximum amount of time to sleep between any two attempts. We now do some fuzzy math to figure out approximately how many attempts we should make given a minimum sleep of 100ms and the fact that we double the sleep time every attempt. Second, during the backup test, if a host was totally stopped when we started the request, it would fail immediately and then retry, but if the host was stopped during the request (after DNS had resolved), then the request would wait for the DialTimeout which we default to 30s, so turning off the cluster for 5 seconds and turning it back on resulted in the backup completing rather than failing. Because of this, we change the commandClient to have a default dial timeout of 1 second. I was tempted to change the global default to 1s which I think would be fine, but didn't want to break anything too badly. --- ctl/common.go | 15 +++++++++++- ctl/restore.go | 33 ++++++++++++++++++++------- http/client.go | 16 +++++++++++-- http/handler.go | 24 ++++++++++++------- internal/clustertests/cluster_test.go | 11 +++++---- 5 files changed, 75 insertions(+), 24 deletions(-) diff --git a/ctl/common.go b/ctl/common.go index 7ba2df2f7..558cdece3 100644 --- a/ctl/common.go +++ b/ctl/common.go @@ -2,6 +2,11 @@ package ctl import ( + "net" + "time" + + gohttp "net/http" + "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/logger" "github.com/molecula/featurebase/v2/server" @@ -25,6 +30,14 @@ func SetTLSConfig(flags *pflag.FlagSet, prefix string, certificatePath *string, flags.BoolVarP(enableClientVerification, prefix+"tls.enable-client-verification", "", false, "Enable TLS certificate client verification for incoming connections") } +// default dial timeout is 30s for some reason which makes testing +// failures/retries really awkward. I don't think we need it that +// high, so I set it to 1s here... let's see what happens. +func clientOptions(client *gohttp.Client, dialer *net.Dialer) *gohttp.Client { + dialer.Timeout = time.Second * 1 + return client +} + // commandClient returns a pilosa.InternalHTTPClient for the command func commandClient(cmd CommandWithTLSSupport, opts ...http.InternalClientOption) (*http.InternalClient, error) { tls := cmd.TLSConfiguration() @@ -32,7 +45,7 @@ func commandClient(cmd CommandWithTLSSupport, opts ...http.InternalClientOption) if err != nil { return nil, errors.Wrap(err, "getting tls config") } - client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig), opts...) + client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig, clientOptions), opts...) if err != nil { return nil, errors.Wrap(err, "getting internal client") } diff --git a/ctl/restore.go b/ctl/restore.go index f7d669f09..b7f1fb2dc 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "math" "net/http" "os" "path/filepath" @@ -137,8 +138,7 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. if len(existingSchema) == 0 { cmd.Logger().Printf("Load Schema") url := primary.URI.Path("/schema") - client := retryablehttp.NewClient() - client.RetryWaitMax = cmd.RetryPeriod + client := cmd.newClient() _, err = client.Post(url, "application/json", f) } else { schema := &pilosa.Schema{} @@ -185,6 +185,27 @@ func retryWith400(ctx context.Context, resp *http.Response, err error) (bool, er return retryablehttp.DefaultRetryPolicy(ctx, resp, err) } +// This logic is taken from featurebase/http/client.go If this logic +// is not the same as what's there, that could be a problem. Ideally +// all network calls from restore would go through the client and this +// would not longer be needed. +func (cmd *RestoreCommand) newClient() *retryablehttp.Client { + min := time.Millisecond * 100 + + // do some math to figure out how many attempts we need to get our + // total sleep time close to the period + attempts := math.Log2(float64(cmd.RetryPeriod)) - math.Log2(float64(min)) + attempts += 0.3 // mmmm, fudge + if attempts < 1 { + attempts = 1 + } + client := retryablehttp.NewClient() + client.RetryWaitMin = min + client.RetryMax = int(attempts) + client.CheckRetry = retryWith400 + return client +} + func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology.Node) error { logger := cmd.Logger() @@ -200,9 +221,7 @@ func (cmd *RestoreCommand) restoreIDAlloc(ctx context.Context, primary *topology logger.Printf("Load idalloc") url := primary.URI.Path("/internal/idalloc/restore") - client := retryablehttp.NewClient() - client.RetryWaitMax = cmd.RetryPeriod - client.CheckRetry = retryWith400 + client := cmd.newClient() _, err = client.Post(url, "application/octet-stream", f) return err } @@ -279,9 +298,7 @@ func (cmd *RestoreCommand) restoreShard(ctx context.Context, filename string) er req = req.WithContext(ctx) req.Header.Set("Content-Type", "application/octet-stream") - client := retryablehttp.NewClient() - client.RetryWaitMax = cmd.RetryPeriod - client.CheckRetry = retryWith400 + client := cmd.newClient() resp, err := client.Do(req) if err != nil { return err diff --git a/http/client.go b/http/client.go index e343c65c9..8d3437bb5 100644 --- a/http/client.go +++ b/http/client.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "io/ioutil" + "math" "math/rand" "net/http" "net/url" @@ -64,11 +65,22 @@ type InternalClientOption func(c *InternalClient) // WithClientRetryPeriod is the max amount of total time the client will // retry failed requests using exponential backoff. -func WithClientRetryPeriod(waitMax time.Duration) InternalClientOption { +func WithClientRetryPeriod(period time.Duration) InternalClientOption { + min := time.Millisecond * 100 + + // do some math to figure out how many attempts we need to get our + // total sleep time close to the period + attempts := math.Log2(float64(period)) - math.Log2(float64(min)) + attempts += 0.3 // mmmm, fudge + if attempts < 1 { + attempts = 1 + } + fmt.Println("attempts: ", int(attempts)) return func(c *InternalClient) { rc := retryablehttp.NewClient() rc.HTTPClient = c.httpClient - rc.RetryWaitMax = waitMax + rc.RetryWaitMin = min + rc.RetryMax = int(attempts) rc.CheckRetry = retryWith400Policy c.retryableClient = rc } diff --git a/http/handler.go b/http/handler.go index 05b65aa58..7ef5eda37 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2593,14 +2593,17 @@ func (s queryValidationSpec) validate(query url.Values) error { return nil } -func GetHTTPClient(t *tls.Config) *http.Client { +type ClientOption func(client *http.Client, dialer *net.Dialer) *http.Client + +func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client { + dialer := &net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + DualStack: true, + } transport := &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - DualStack: true, - }).DialContext, + Proxy: http.ProxyFromEnvironment, + DialContext: dialer.DialContext, MaxIdleConns: 1000, MaxIdleConnsPerHost: 200, IdleConnTimeout: 90 * time.Second, @@ -2610,7 +2613,12 @@ func GetHTTPClient(t *tls.Config) *http.Client { if t != nil { transport.TLSClientConfig = t } - return &http.Client{Transport: transport} + + client := &http.Client{Transport: transport} + for _, opt := range opts { + client = opt(client, dialer) + } + return client } // handlePostImportAtomicRecord handles /import-atomic-record requests diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index d3009dbda..e23d10f8c 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -149,22 +149,23 @@ func TestClusterStuff(t *testing.T) { // now do backup with all nodes down and too short a timeout // so it fails. Has be to be all 3 because the cluster has // replicas=3 and the backup command will retry on replicas. - if err = sendCmd("docker", "stop", "clustertests_pilosa2_1"); err != nil { - t.Fatalf("sending stop command: %v", err) - } if backupCmd, err = startCmd( - "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=50ms"); err != nil { + "featurebase", "backup", "--host=pilosa1:10101", fmt.Sprintf("--output=%s", tmpdir+"/backuptest2"), "--retry-period=200ms"); err != nil { t.Fatalf("sending second backup command: %v", err) } - time.Sleep(time.Millisecond * 5) // want the backup to get started, then fail + time.Sleep(time.Millisecond * 10) // want the backup to get started, then fail if err = sendCmd("docker", "stop", "clustertests_pilosa1_1"); err != nil { t.Fatalf("sending stop command: %v", err) } + if err = sendCmd("docker", "stop", "clustertests_pilosa2_1"); err != nil { + t.Fatalf("sending stop command: %v", err) + } if err = sendCmd("docker", "stop", "clustertests_pilosa3_1"); err != nil { t.Fatalf("sending stop command: %v", err) } time.Sleep(time.Second * 5) + if err = sendCmd("docker", "start", "clustertests_pilosa1_1"); err != nil { t.Fatalf("sending start command: %v", err) } From ffd91137e1702de482bbe9ef00fe59410d743be5 Mon Sep 17 00:00:00 2001 From: Travis Date: Tue, 28 Dec 2021 13:34:07 -0600 Subject: [PATCH 57/58] Stop blocking API called when cluster is DOWN or DEGRADED This commit effectively removes the API-level validation that was blocking certain API methods when the cluster was in a particular state (namely DOWN and DEGRADED). The thinking is that we shouldn't be blocking these requests at the API level, but rather should let them pass through and allow the fact that a node is ACTUALLY down dictate the behavior. With this change, two tests were modified. They were previously expecting the error message from the API validation on DOWN, but now they check for a "shard unavailable" error, which is what gets returned for a particular query when the cluster is in an unhealthy state. --- api.go | 11 +++++- server/server_test.go | 86 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/api.go b/api.go index 628feb16d..fdee2f8f7 100644 --- a/api.go +++ b/api.go @@ -131,9 +131,16 @@ func (api *API) SetAPIOptions(opts ...apiOption) error { var validAPIMethods = map[disco.ClusterState]map[apiMethod]struct{}{ disco.ClusterStateStarting: methodsCommon, disco.ClusterStateNormal: appendMap(methodsCommon, methodsNormal), - disco.ClusterStateDegraded: appendMap(methodsCommon, methodsDegraded), + // Ideally, this would be just `appendMap(methodsCommon, methodsDegraded)`, + // but in an attempt to reduce the influence that state (determined by etcd) + // has on a node under load, this is set to effectively allow all requests + // in a DEGRADED state. + disco.ClusterStateDegraded: appendMap(methodsCommon, methodsNormal), disco.ClusterStateResizing: appendMap(methodsCommon, methodsResizing), - disco.ClusterStateDown: methodsCommon, + // Ideally, this would be just `methodsCommon`, but in an attempt to reduce + // the influence that state (determined by etcd) has on a node under load, + // this is set to effectively allow all requests in a DOWN state. + disco.ClusterStateDown: appendMap(methodsCommon, methodsNormal), } func appendMap(a, b map[apiMethod]struct{}) map[apiMethod]struct{} { diff --git a/server/server_test.go b/server/server_test.go index 014e2035e..551781ffb 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -17,7 +17,7 @@ import ( "testing" "time" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" "github.com/molecula/featurebase/v2/disco" "github.com/molecula/featurebase/v2/http" "github.com/molecula/featurebase/v2/pql" @@ -26,6 +26,7 @@ import ( "github.com/molecula/featurebase/v2/test" "github.com/molecula/featurebase/v2/testhook" "github.com/pkg/errors" + "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" ) @@ -504,6 +505,30 @@ func TestClusteringNodesReplica1(t *testing.T) { t.Fatalf("starting cluster: %v", err) } + indexName := "idx" + fieldName := "fld" + + // Create the schema. + if _, err := cluster.GetPrimary().API.CreateIndex(context.Background(), indexName, pilosa.IndexOptions{}); err != nil { + t.Fatalf("creating index: %v", err) + } + if _, err := cluster.GetPrimary().API.CreateField(context.Background(), indexName, fieldName); err != nil { + t.Fatalf("creating field: %v", err) + } + + // Set some columns across shards to ensure that the Row query will require + // data from all nodes. + data := []string{} + for rowID := 1; rowID < 2; rowID++ { + for columnID := 1; columnID < 10; columnID++ { + data = append(data, fmt.Sprintf(`Set(%d, %s=%d)`, columnID*pilosa.ShardWidth, fieldName, rowID)) + } + } + if _, err := cluster.GetPrimary().Query(t, indexName, "", strings.Join(data, "")); err != nil { + t.Fatalf("setting columns: %v", err) + } + + // Shut down a node. if err := cluster.GetNonPrimary().Command.Close(); err != nil { t.Fatalf("closing third node: %v", err) } @@ -513,7 +538,12 @@ func TestClusteringNodesReplica1(t *testing.T) { } // confirm that cluster stops accepting queries after one node closes - if _, err := cluster.GetPrimary().API.Query(context.Background(), &pilosa.QueryRequest{}); !strings.Contains(err.Error(), "not allowed in state DOWN") { + qry := &pilosa.QueryRequest{ + Index: "idx", + Query: fmt.Sprintf("Row(%s=1)", fieldName), + } + + if _, err := cluster.GetPrimary().API.Query(context.Background(), qry); !strings.Contains(err.Error(), "shard unavailable") { t.Fatalf("got unexpected error querying an incomplete cluster: %v", err) } } @@ -540,8 +570,34 @@ func TestClusteringNodesReplica2(t *testing.T) { } defer cluster.Close() + indexName := "idx" + fieldName := "fld" + coord, others := cluster.GetPrimary(), cluster.GetNonPrimaries() + // Create the schema. + if _, err := coord.API.CreateIndex(context.Background(), indexName, pilosa.IndexOptions{}); err != nil { + t.Fatalf("creating index: %v", err) + } + if _, err := coord.API.CreateField(context.Background(), indexName, fieldName); err != nil { + t.Fatalf("creating field: %v", err) + } + + // Set some columns across shards to ensure that the Row query will require + // data from all nodes. + data := []string{} + cols := []uint64{} + for rowID := 1; rowID < 2; rowID++ { + for columnID := 1; columnID < 30; columnID++ { + col := uint64(columnID * pilosa.ShardWidth) + cols = append(cols, col) + data = append(data, fmt.Sprintf(`Set(%d, %s=%d)`, col, fieldName, rowID)) + } + } + if _, err := coord.Query(t, indexName, "", strings.Join(data, "")); err != nil { + t.Fatalf("setting columns: %v", err) + } + if err := others[0].Close(); err != nil { t.Fatalf("closing third node: %v", err) } @@ -569,8 +625,30 @@ func TestClusteringNodesReplica2(t *testing.T) { t.Fatalf("after closing second server: %v", err) } - if _, err := coord.API.Query(context.Background(), &pilosa.QueryRequest{}); !strings.Contains(err.Error(), "not allowed in state DOWN") { - t.Fatalf("got unexpected error querying an incomplete cluster: %v", err) + qry := &pilosa.QueryRequest{ + Index: "idx", + Query: fmt.Sprintf("Row(%s=1)", fieldName), + } + + // Because we no longer block queries when the cluster is in state DOWN, + // there are cases where a DOWN cluster can still respond to a query. In + // that case, we want the test to pass. But if the unavailable node(s) cause + // the query to result in an error, we check that it's the error we expect. + resp, err := coord.API.Query(context.Background(), qry) + if err != nil { + if !strings.Contains(err.Error(), "shard unavailable") { + t.Fatalf("got unexpected error querying an incomplete cluster: %v", err) + } + } else { + if len(resp.Results) == 0 { + t.Fatal("got no results") + } + + row, ok := resp.Results[0].(*pilosa.Row) + if !ok { + t.Fatalf("expected a *pilosa.Row, but got %T", resp.Results[0]) + } + require.Equal(t, row.Columns(), cols) } } From 93b97b9831b9c099c549a6f595e6df7d79050bf0 Mon Sep 17 00:00:00 2001 From: Fletcher Haynes Date: Thu, 30 Dec 2021 17:00:59 -0800 Subject: [PATCH 58/58] Test push to see if pipeline is running on push to master --- .gitlab/.gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index a27c1012e..60d1a309d 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -13,6 +13,7 @@ include: variables: GOVERSION: "1.16.10" + stages: - lint - test