diff --git a/go.mod b/go.mod index 0dc29cfbd3..149939ede6 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.12 require ( cloud.google.com/go v0.37.4 github.com/AbGuthrie/goquery/v2 v2.0.1 + github.com/DATA-DOG/go-sqlmock v1.5.0 github.com/VividCortex/gohistogram v1.0.0 // indirect github.com/VividCortex/mysqlerr v0.0.0-20170204212430-6c6b55f8796f github.com/WatchBeam/clock v0.0.0-20170901150240-b08e6b4da7ea diff --git a/go.sum b/go.sum index e9425f3668..a1de0541b2 100644 --- a/go.sum +++ b/go.sum @@ -6,6 +6,8 @@ github.com/AbGuthrie/goquery/v2 v2.0.1 h1:h0tIhmeRroyqYjT9zxXPXOrheNp1xqNTV+XFWu github.com/AbGuthrie/goquery/v2 v2.0.1/go.mod h1:xpDLF4kUr+TRFXogclRa7Zzc8bMAB/fYm1zG/XX1WOA= github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/DATA-DOG/go-sqlmock v1.5.0 h1:Shsta01QNfFxHCfpW6YH2STWB0MudeXXEWMr20OEh60= +github.com/DATA-DOG/go-sqlmock v1.5.0/go.mod h1:f/Ixk793poVmq4qj/V1dPUg2JEAKC73Q5eFN3EC/SaM= github.com/Microsoft/go-winio v0.4.9/go.mod h1:VhR8bwka0BXejwEJY73c50VrPtXAaKcyvVC4A4RozmA= github.com/Shopify/sarama v1.19.0/go.mod h1:FVkBWblsNy7DGZRfXLU0O9RCGt5g3g3yEuWXgklEdEo= github.com/Shopify/toxiproxy v2.1.4+incompatible/go.mod h1:OXgGpZ6Cli1/URJOF1DMxUHB2q5Ap20/P/eIdh4G0pI= diff --git a/server/datastore/mysql/mysql.go b/server/datastore/mysql/mysql.go index d9297aa536..b1ecdf24cb 100644 --- a/server/datastore/mysql/mysql.go +++ b/server/datastore/mysql/mysql.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/VividCortex/mysqlerr" "github.com/WatchBeam/clock" "github.com/cenkalti/backoff/v4" "github.com/fleetdm/fleet/server/config" @@ -61,12 +62,28 @@ func (d *Datastore) getTransaction(opts []kolide.OptionalArg) dbfunctions { type txFn func(*sqlx.Tx) error +// retryableError determines whether a MySQL error can be retried. By default +// errors are considered non-retryable. Only errors that we know have a +// possibility of succeeding on a retry should return true in this function. +func retryableError(err error) bool { + base := errors.Cause(err) + if b, ok := base.(*mysql.MySQLError); ok { + switch b.Number { + // Consider lock related errors to be retryable + case mysqlerr.ER_LOCK_DEADLOCK, mysqlerr.ER_LOCK_WAIT_TIMEOUT: + return true + } + } + + return false +} + // withRetryTxx provides a common way to commit/rollback a txFn wrapped in a retry with exponential backoff func (d *Datastore) withRetryTxx(fn txFn) (err error) { operation := func() error { tx, err := d.db.Beginx() if err != nil { - return errors.Wrap(err, "creating transaction") + return errors.Wrap(err, "create transaction") } defer func() { @@ -78,18 +95,29 @@ func (d *Datastore) withRetryTxx(fn txFn) (err error) { } }() - err = fn(tx) - if err != nil { + if err := fn(tx); err != nil { rbErr := tx.Rollback() if rbErr != nil && rbErr != sql.ErrTxDone { - return fmt.Errorf("got err '%s' rolling back after err '%s'", rbErr, err) + // Consider rollback errors to be non-retryable + return backoff.Permanent(errors.Wrapf(err, "got err '%s' rolling back after err", rbErr.Error())) } - return err - } else { - err = tx.Commit() - if err != nil { - return errors.Wrap(err, "committing transaction") + + if retryableError(err) { + return err } + + // Consider any other errors to be non-retryable + return backoff.Permanent(err) + } + + if err := tx.Commit(); err != nil { + err = errors.Wrap(err, "commit transaction") + + if retryableError(err) { + return err + } + + return backoff.Permanent(errors.Wrap(err, "commit transaction")) } return nil diff --git a/server/datastore/mysql/mysql_test.go b/server/datastore/mysql/mysql_test.go index d88f07e6b1..8ee86db0c8 100644 --- a/server/datastore/mysql/mysql_test.go +++ b/server/datastore/mysql/mysql_test.go @@ -3,7 +3,14 @@ package mysql import ( "testing" + "github.com/DATA-DOG/go-sqlmock" + "github.com/VividCortex/mysqlerr" + "github.com/go-kit/kit/log" + "github.com/go-sql-driver/mysql" + "github.com/jmoiron/sqlx" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSanitizeColumn(t *testing.T) { @@ -108,3 +115,119 @@ func TestSearchLike(t *testing.T) { }) } } + +func mockDatastore(t *testing.T) (sqlmock.Sqlmock, *Datastore) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + ds := &Datastore{ + db: sqlx.NewDb(db, "sqlmock"), + logger: log.NewNopLogger(), + } + + return mock, ds +} + +func TestWithRetryTxxSuccess(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnResult(sqlmock.NewResult(1, 1)) + mock.ExpectCommit() + + require.NoError(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestWithRetryTxxRollbackSuccess(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnError(errors.New("fail")) + mock.ExpectRollback() + + require.Error(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestWithRetryTxxRollbackError(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnError(errors.New("fail")) + mock.ExpectRollback().WillReturnError(errors.New("rollback failed")) + + require.Error(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestWithRetryTxxRetrySuccess(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + // Return a retryable error + mock.ExpectExec("SELECT 1").WillReturnError(&mysql.MySQLError{Number: mysqlerr.ER_LOCK_DEADLOCK}) + mock.ExpectRollback() + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnResult(sqlmock.NewResult(1, 1)) + mock.ExpectCommit() + + assert.NoError(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestWithRetryTxxCommitRetrySuccess(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnResult(sqlmock.NewResult(1, 1)) + // Return a retryable error + mock.ExpectCommit().WillReturnError(&mysql.MySQLError{Number: mysqlerr.ER_LOCK_DEADLOCK}) + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnResult(sqlmock.NewResult(1, 1)) + mock.ExpectCommit() + + assert.NoError(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestWithRetryTxxCommitError(t *testing.T) { + mock, ds := mockDatastore(t) + defer ds.Close() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1").WillReturnResult(sqlmock.NewResult(1, 1)) + // Return a retryable error + mock.ExpectCommit().WillReturnError(errors.New("fail")) + + assert.Error(t, ds.withRetryTxx(func(tx *sqlx.Tx) error { + _, err := tx.Exec("SELECT 1") + return err + })) + + require.NoError(t, mock.ExpectationsWereMet()) +}