From ce7a0171d156709bc8bbf2ac1138b8022bb08054 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 14 Feb 2018 09:51:24 -0500 Subject: Squash logged errors from failed SQL rollbacks Currently we unconditionally roll back transactions after error, even if a commit has already been attempted. Commit is guaranteed to end a transaction, though, whether by successfully committing or by rolling back if that fails. As such, we attempt a double rollback if a transaction fails at commit (for example, for a constraint violation), which doesn't error but does log angry warning messages. Ensure we don't try rolling back after commit runs to prevent this. Signed-off-by: Matthew Heon Closes: #327 Approved by: rhatdan --- libpod/sql_state.go | 28 ++++++++++++++++++++++------ libpod/sql_state_internal.go | 22 +++++++++++++++++----- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/libpod/sql_state.go b/libpod/sql_state.go index a1830c468..9efa12111 100644 --- a/libpod/sql_state.go +++ b/libpod/sql_state.go @@ -108,12 +108,14 @@ func (s *SQLState) Refresh() (err error) { return ErrDBClosed } + committed := false + tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to refresh state: %v", err2) } @@ -135,6 +137,8 @@ func (s *SQLState) Refresh() (err error) { return errors.Wrapf(err, "error refreshing database state") } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to refresh database") } @@ -407,12 +411,14 @@ func (s *SQLState) SaveContainer(ctr *Container) (err error) { return ErrDBClosed } + committed := false + tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to add container %s: %v", ctr.ID(), err2) } @@ -447,6 +453,8 @@ func (s *SQLState) SaveContainer(ctr *Container) (err error) { return ErrNoSuchCtr } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to update container %s", ctr.ID()) } @@ -769,12 +777,14 @@ func (s *SQLState) AddPod(pod *Pod) (err error) { return errors.Wrapf(err, "error marshaling pod %s labels to JSON", pod.ID()) } + committed := false + tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to add pod %s: %v", pod.ID(), err2) } @@ -789,6 +799,8 @@ func (s *SQLState) AddPod(pod *Pod) (err error) { return errors.Wrapf(err, "error adding pod %s to database", pod.ID()) } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to add pod %s", pod.ID()) } @@ -808,12 +820,14 @@ func (s *SQLState) RemovePod(pod *Pod) (err error) { return ErrDBClosed } + committed := false + tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to remove pod %s: %v", pod.ID(), err2) } @@ -838,6 +852,8 @@ func (s *SQLState) RemovePod(pod *Pod) (err error) { return errors.Wrapf(err, "error removing pod %s from name/ID registry", pod.ID()) } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to remove pod %s", pod.ID()) } @@ -925,12 +941,12 @@ func (s *SQLState) RemovePodContainers(pod *Pod) (err error) { return errors.Wrapf(err, "error removing pod %s containers from containers table", pod.ID()) } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction remove pod %s containers", pod.ID()) } - committed = true - // Remove JSON files from the containers in question hasError := false for _, ctr := range containers { diff --git a/libpod/sql_state_internal.go b/libpod/sql_state_internal.go index 3881305be..9cc4cd861 100644 --- a/libpod/sql_state_internal.go +++ b/libpod/sql_state_internal.go @@ -71,12 +71,14 @@ func checkDB(db *sql.DB, r *Runtime) (err error) { const checkRuntimeExists = "SELECT name FROM sqlite_master WHERE type='table' AND name='runtime';" + committed := false + tx, err := db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to check runtime table: %v", err2) } @@ -165,6 +167,8 @@ func checkDB(db *sql.DB, r *Runtime) (err error) { graphDriverName, r.config.StorageConfig.GraphDriverName) } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing runtime table transaction in database") } @@ -291,13 +295,15 @@ func prepareDB(db *sql.DB) (err error) { ); ` + committed := false + // Create the tables tx, err := db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to create tables: %v", err2) } @@ -318,6 +324,8 @@ func prepareDB(db *sql.DB) (err error) { return errors.Wrapf(err, "error creating pods table in database") } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing table creation transaction in database") } @@ -810,12 +818,14 @@ func (s *SQLState) addContainer(ctr *Container, pod *Pod) (err error) { } } + committed := false + tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") } defer func() { - if err != nil { + if err != nil && !committed { if err2 := tx.Rollback(); err2 != nil { logrus.Errorf("Error rolling back transaction to add container %s: %v", ctr.ID(), err2) } @@ -947,6 +957,8 @@ func (s *SQLState) addContainer(ctr *Container, pod *Pod) (err error) { }() } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to add container %s", ctr.ID()) } @@ -1047,12 +1059,12 @@ func (s *SQLState) removeContainer(ctr *Container, pod *Pod) (err error) { return errors.Wrapf(err, "error removing container %s from name/ID registry", ctr.ID()) } + committed = true + if err := tx.Commit(); err != nil { return errors.Wrapf(err, "error committing transaction to remove container %s", ctr.ID()) } - committed = true - // Remove the container's JSON from disk jsonPath := getSpecPath(s.specsDir, ctr.ID()) if err := os.Remove(jsonPath); err != nil { -- cgit v1.2.3-54-g00ecf