This is an automated email from the ASF dual-hosted git repository.
rob pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new d3a01a0 add configurable timeouts to highly used DB queries
d3a01a0 is described below
commit d3a01a0c2840f0b0a992cc7a896d4474dadd7396
Author: Dylan Volz <dylan_v...@comcast.com>
AuthorDate: Wed Aug 8 12:09:28 2018 -0600
add configurable timeouts to highly used DB queries
---
traffic_ops/app/conf/cdn.conf | 1 +
traffic_ops/traffic_ops_golang/api/api.go | 8 ++++++--
.../traffic_ops_golang/api/shared_handlers_test.go | 2 +-
traffic_ops/traffic_ops_golang/auth/authorize.go | 4 ++--
traffic_ops/traffic_ops_golang/auth/login.go | 19 +++++++++++++------
traffic_ops/traffic_ops_golang/config/config.go | 5 +++++
.../server/servers_update_status.go | 8 +++++++-
.../server/servers_update_status_test.go | 4 +++-
.../traffic_ops_golang/systeminfo/system_info.go | 22 +++++++++++++++-------
.../systeminfo/system_info_test.go | 2 +-
traffic_ops/traffic_ops_golang/wrappers.go | 7 ++++++-
traffic_ops/traffic_ops_golang/wrappers_test.go | 6 ++++--
12 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/traffic_ops/app/conf/cdn.conf b/traffic_ops/app/conf/cdn.conf
index 4410552..c21a847 100644
--- a/traffic_ops/app/conf/cdn.conf
+++ b/traffic_ops/app/conf/cdn.conf
@@ -27,6 +27,7 @@
"max_db_connections": 20,
"db_max_idle_connections": 15,
"db_conn_max_lifetime_seconds": 60,
+ "db_query_timeout_seconds": 20,
"backend_max_connections": {
"mojolicious": 4
},
diff --git a/traffic_ops/traffic_ops_golang/api/api.go
b/traffic_ops/traffic_ops_golang/api/api.go
index c10646e..b2191d9 100644
--- a/traffic_ops/traffic_ops_golang/api/api.go
+++ b/traffic_ops/traffic_ops_golang/api/api.go
@@ -287,8 +287,8 @@ func NewInfo(r *http.Request, requiredParams []string,
intParamNames []string) (
if userErr != nil || sysErr != nil {
return nil, userErr, sysErr, errCode
}
- dbCtx, _ := context.WithTimeout(r.Context(), time.Second*15) //only
place we could call cancel here is in APIInfo.Close(), which already will
rollback the transaction (which is all cancel will do.)
- tx, err := db.BeginTxx(dbCtx, nil) // must be
last, MUST not return an error if this succeeds, without closing the tx
+ dbCtx, _ := context.WithTimeout(r.Context(),
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second) //only place we could
call cancel here is in APIInfo.Close(), which already will rollback the
transaction (which is all cancel will do.)
+ tx, err := db.BeginTxx(dbCtx, nil)
// must be last, MUST not return an error if this
succeeds, without closing the tx
if err != nil {
return nil, userErr, errors.New("could not begin transaction: "
+ err.Error()), http.StatusInternalServerError
}
@@ -336,6 +336,10 @@ func getConfig(ctx context.Context) (*config.Config,
error) {
return nil, errors.New("No config found in Context")
}
+func GetConfig(ctx context.Context) (*config.Config, error) {
+ return getConfig(ctx)
+}
+
func getReqID(ctx context.Context) (uint64, error) {
val := ctx.Value(ReqIDContextKey)
if val != nil {
diff --git a/traffic_ops/traffic_ops_golang/api/shared_handlers_test.go
b/traffic_ops/traffic_ops_golang/api/shared_handlers_test.go
index c13543d..12942e2 100644
--- a/traffic_ops/traffic_ops_golang/api/shared_handlers_test.go
+++ b/traffic_ops/traffic_ops_golang/api/shared_handlers_test.go
@@ -49,7 +49,7 @@ func GetTypeSingleton() func(apiInfo *APIInfo) CRUDer {
}
}
-var cfg = config.Config{}
+var cfg = config.Config{ConfigTrafficOpsGolang:
config.ConfigTrafficOpsGolang{DBQueryTimeoutSeconds: 20}}
func (i tester) GetKeyFieldsInfo() []KeyFieldInfo {
return []KeyFieldInfo{{"id", GetIntKey}}
diff --git a/traffic_ops/traffic_ops_golang/auth/authorize.go
b/traffic_ops/traffic_ops_golang/auth/authorize.go
index 0cbae8e..f8b17e3 100644
--- a/traffic_ops/traffic_ops_golang/auth/authorize.go
+++ b/traffic_ops/traffic_ops_golang/auth/authorize.go
@@ -64,13 +64,13 @@ type key int
const CurrentUserKey key = iota
// GetCurrentUserFromDB - returns the id and privilege level of the given
user along with the username, or -1 as the id, - as the userName and
PrivLevelInvalid if the user doesn't exist.
-func GetCurrentUserFromDB(DB *sqlx.DB, CurrentUserStmt, user string)
CurrentUser {
+func GetCurrentUserFromDB(DB *sqlx.DB, CurrentUserStmt, user string, timeout
time.Duration) CurrentUser {
var currentUserInfo CurrentUser
if DB == nil {
log.Errorf("no db provided to GetCurrentUserFromDB")
return CurrentUser{"-", -1, PrivLevelInvalid, TenantIDInvalid,
[]string{}}
}
- dbCtx, dbClose := context.WithTimeout(context.Background(),
time.Second*20)
+ dbCtx, dbClose := context.WithTimeout(context.Background(), timeout)
defer dbClose()
err := DB.GetContext(dbCtx, ¤tUserInfo, CurrentUserStmt, user)
diff --git a/traffic_ops/traffic_ops_golang/auth/login.go
b/traffic_ops/traffic_ops_golang/auth/login.go
index 26eb85c..45011cc 100644
--- a/traffic_ops/traffic_ops_golang/auth/login.go
+++ b/traffic_ops/traffic_ops_golang/auth/login.go
@@ -20,6 +20,7 @@ package auth
*/
import (
+ "context"
"crypto/sha1"
"encoding/hex"
"encoding/json"
@@ -56,12 +57,12 @@ func LoginHandler(db *sqlx.DB, cfg config.Config)
http.HandlerFunc {
resp := struct {
tc.Alerts
}{}
- userAllowed, err := CheckLocalUserIsAllowed(form, db)
+ userAllowed, err := CheckLocalUserIsAllowed(form, db,
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
if err != nil {
log.Errorf("error checking local user: %s\n",
err.Error())
}
if userAllowed {
- authenticated, err = checkLocalUserPassword(form, db)
+ authenticated, err = checkLocalUserPassword(form, db,
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
if err != nil {
log.Errorf("error checking local user password:
%s\n", err.Error())
}
@@ -105,9 +106,12 @@ func LoginHandler(db *sqlx.DB, cfg config.Config)
http.HandlerFunc {
}
}
-func CheckLocalUserIsAllowed(form passwordForm, db *sqlx.DB) (bool, error) {
+func CheckLocalUserIsAllowed(form passwordForm, db *sqlx.DB, timeout
time.Duration) (bool, error) {
var roleName string
- err := db.Get(&roleName, "SELECT role.name FROM role INNER JOIN tm_user
ON tm_user.role = role.id where username=$1", form.Username)
+ dbCtx, dbClose := context.WithTimeout(context.Background(), timeout)
+ defer dbClose()
+
+ err := db.GetContext(dbCtx, &roleName, "SELECT role.name FROM role
INNER JOIN tm_user ON tm_user.role = role.id where username=$1", form.Username)
if err != nil {
return false, err
}
@@ -119,9 +123,12 @@ func CheckLocalUserIsAllowed(form passwordForm, db
*sqlx.DB) (bool, error) {
return false, nil
}
-func checkLocalUserPassword(form passwordForm, db *sqlx.DB) (bool, error) {
+func checkLocalUserPassword(form passwordForm, db *sqlx.DB, timeout
time.Duration) (bool, error) {
var hashedPassword string
- err := db.Get(&hashedPassword, "SELECT local_passwd FROM tm_user WHERE
username=$1", form.Username)
+ dbCtx, dbClose := context.WithTimeout(context.Background(), timeout)
+ defer dbClose()
+
+ err := db.GetContext(dbCtx, &hashedPassword, "SELECT local_passwd FROM
tm_user WHERE username=$1", form.Username)
if err != nil {
return false, err
}
diff --git a/traffic_ops/traffic_ops_golang/config/config.go
b/traffic_ops/traffic_ops_golang/config/config.go
index 9ead04f..1f4d482 100644
--- a/traffic_ops/traffic_ops_golang/config/config.go
+++ b/traffic_ops/traffic_ops_golang/config/config.go
@@ -78,6 +78,7 @@ type ConfigTrafficOpsGolang struct {
DBMaxIdleConnections int `json:"db_max_idle_connections"`
DBConnMaxLifetimeSeconds int
`json:"db_conn_max_lifetime_seconds"`
BackendMaxConnections map[string]int `json:"backend_max_connections"`
+ DBQueryTimeoutSeconds int
`json:"db_query_timeout_seconds"`
ProfilingEnabled bool `json:"profiling_enabled"`
ProfilingLocation string `json:"profiling_location"`
}
@@ -105,6 +106,7 @@ type ConfigLDAP struct {
}
const DefaultLDAPTimeoutSecs = 60
+const DefaultDBQueryTimeoutSecs = 20
// ErrorLog - critical messages
func (c Config) ErrorLog() log.LogLocation {
@@ -262,6 +264,9 @@ func ParseConfig(cfg Config) (Config, error) {
if cfg.DBConnMaxLifetimeSeconds == 0 {
cfg.DBConnMaxLifetimeSeconds = DBConnMaxLifetimeSecondsDefault
}
+ if cfg.DBQueryTimeoutSeconds == 0 {
+ cfg.DBQueryTimeoutSeconds = DefaultDBQueryTimeoutSecs
+ }
invalidTOURLStr := ""
var err error
diff --git a/traffic_ops/traffic_ops_golang/server/servers_update_status.go
b/traffic_ops/traffic_ops_golang/server/servers_update_status.go
index 6b85f33..32a2d65 100644
--- a/traffic_ops/traffic_ops_golang/server/servers_update_status.go
+++ b/traffic_ops/traffic_ops_golang/server/servers_update_status.go
@@ -75,7 +75,13 @@ func getServerUpdateStatus(hostName string, db *sqlx.DB, ctx
context.Context) ([
groupBy := ` GROUP BY s.id, s.host_name, type.name,
server_reval_pending, use_reval_pending.value, s.upd_pending, status.name ORDER
BY s.id;`
- dbCtx, dbClose := context.WithTimeout(ctx, time.Second*10)
+ cfg, ctxErr := api.GetConfig(ctx)
+ if ctxErr != nil {
+ log.Errorln("unable to retrieve config from context")
+ return nil, ctxErr
+ }
+
+ dbCtx, dbClose := context.WithTimeout(ctx,
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
defer dbClose()
updateStatuses := []tc.ServerUpdateStatus{}
diff --git
a/traffic_ops/traffic_ops_golang/server/servers_update_status_test.go
b/traffic_ops/traffic_ops_golang/server/servers_update_status_test.go
index f8bfada..cb05099 100644
--- a/traffic_ops/traffic_ops_golang/server/servers_update_status_test.go
+++ b/traffic_ops/traffic_ops_golang/server/servers_update_status_test.go
@@ -25,6 +25,8 @@ import (
"testing"
"github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/config"
"github.com/jmoiron/sqlx"
sqlmock "gopkg.in/DATA-DOG/go-sqlmock.v1"
)
@@ -44,7 +46,7 @@ func TestGetServerUpdateStatus(t *testing.T) {
mock.ExpectQuery("SELECT").WillReturnRows(serverStatusRow)
- result, err := getServerUpdateStatus("host_name_1", db,
context.Background())
+ result, err := getServerUpdateStatus("host_name_1", db,
context.WithValue(context.Background(), api.ConfigContextKey,
&config.Config{ConfigTrafficOpsGolang:
config.ConfigTrafficOpsGolang{DBQueryTimeoutSeconds: 20}}))
if err != nil {
t.Errorf("getServerUpdateStatus: %v", err)
}
diff --git a/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
b/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
index 22972ab..1adaa6f 100644
--- a/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
+++ b/traffic_ops/traffic_ops_golang/systeminfo/system_info.go
@@ -22,14 +22,16 @@ package systeminfo
import (
"context"
"encoding/json"
+ "errors"
"fmt"
"net/http"
+ "time"
+ "github.com/apache/trafficcontrol/lib/go-log"
tc "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth"
- "time"
-
"github.com/jmoiron/sqlx"
)
@@ -45,7 +47,13 @@ func Handler(db *sqlx.DB) http.HandlerFunc {
}
privLevel := user.PrivLevel
- resp, err := getSystemInfoResponse(db, privLevel)
+ cfg, ctxErr := api.GetConfig(ctx)
+ if ctxErr != nil {
+ log.Errorln("unable to retrieve config from context: ",
ctxErr)
+ handleErrs(http.StatusInternalServerError,
errors.New("no config found in context"))
+ }
+
+ resp, err := getSystemInfoResponse(db, privLevel,
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
if err != nil {
handleErrs(http.StatusInternalServerError, err)
return
@@ -61,8 +69,8 @@ func Handler(db *sqlx.DB) http.HandlerFunc {
fmt.Fprintf(w, "%s", respBts)
}
}
-func getSystemInfoResponse(db *sqlx.DB, privLevel int)
(*tc.SystemInfoResponse, error) {
- info, err := getSystemInfo(db, privLevel)
+func getSystemInfoResponse(db *sqlx.DB, privLevel int, timeout time.Duration)
(*tc.SystemInfoResponse, error) {
+ info, err := getSystemInfo(db, privLevel, timeout)
if err != nil {
return nil, fmt.Errorf("getting SystemInfo: %v", err)
}
@@ -72,7 +80,7 @@ func getSystemInfoResponse(db *sqlx.DB, privLevel int)
(*tc.SystemInfoResponse,
return &resp, nil
}
-func getSystemInfo(db *sqlx.DB, privLevel int) (map[string]string, error) {
+func getSystemInfo(db *sqlx.DB, privLevel int, timeout time.Duration)
(map[string]string, error) {
// system info returns all global parameters
query := `SELECT
p.name,
@@ -81,7 +89,7 @@ p.last_updated,
p.value
FROM parameter p
WHERE p.config_file='global'`
- ctx, cancel := context.WithTimeout(context.Background(), time.Second*10)
+ ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
rows, err := db.QueryxContext(ctx, query)
diff --git a/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
b/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
index bf3ec6b..ec36710 100644
--- a/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
+++ b/traffic_ops/traffic_ops_golang/systeminfo/system_info_test.go
@@ -93,7 +93,7 @@ func TestGetSystemInfo(t *testing.T) {
}
mock.ExpectQuery("SELECT.*WHERE
p.config_file='global'").WillReturnRows(rows)
- sysinfo, err := getSystemInfo(db, auth.PrivLevelReadOnly)
+ sysinfo, err := getSystemInfo(db, auth.PrivLevelReadOnly,
20*time.Second)
if err != nil {
t.Errorf("getSystemInfo expected: nil error, actual: %v", err)
}
diff --git a/traffic_ops/traffic_ops_golang/wrappers.go
b/traffic_ops/traffic_ops_golang/wrappers.go
index bcac30d..17dc81b 100644
--- a/traffic_ops/traffic_ops_golang/wrappers.go
+++ b/traffic_ops/traffic_ops_golang/wrappers.go
@@ -112,7 +112,12 @@ func (a AuthBase) GetWrapper(privLevelRequired int)
Middleware {
handleErr(http.StatusInternalServerError,
errors.New("No DB found"))
}
- currentUserInfo := auth.GetCurrentUserFromDB(DB,
a.getCurrentUserInfoStmt, username)
+ cfg, err := api.GetConfig(r.Context())
+ if err != nil {
+ handleErr(http.StatusInternalServerError,
errors.New("No config found"))
+ }
+
+ currentUserInfo := auth.GetCurrentUserFromDB(DB,
a.getCurrentUserInfoStmt, username,
time.Duration(cfg.DBQueryTimeoutSeconds)*time.Second)
if currentUserInfo.PrivLevel < privLevelRequired {
handleErr(http.StatusForbidden,
errors.New("Forbidden."))
return
diff --git a/traffic_ops/traffic_ops_golang/wrappers_test.go
b/traffic_ops/traffic_ops_golang/wrappers_test.go
index 2a7fdcc..e170e37 100644
--- a/traffic_ops/traffic_ops_golang/wrappers_test.go
+++ b/traffic_ops/traffic_ops_golang/wrappers_test.go
@@ -31,12 +31,13 @@ import (
"testing"
"time"
+ "github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/config"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tocookie"
- "github.com/jmoiron/sqlx"
- "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/jmoiron/sqlx"
sqlmock "gopkg.in/DATA-DOG/go-sqlmock.v1"
)
@@ -184,6 +185,7 @@ func TestWrapAuth(t *testing.T) {
}
r = r.WithContext(context.WithValue(context.Background(),
api.DBContextKey, db))
+ r = r.WithContext(context.WithValue(r.Context(), api.ConfigContextKey,
&config.Config{ConfigTrafficOpsGolang:
config.ConfigTrafficOpsGolang{DBQueryTimeoutSeconds: 20}}))
f(w, r)
