From 627a5b73bf71b9161f42bd36fab240b3f858ee51 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Tue, 21 Feb 2023 10:42:55 -0500 Subject: [PATCH] Various fixes from code review Signed-off-by: Matt Heon --- libpod/sqlite_state.go | 52 ++++++++++--------- libpod/sqlite_state_internal.go | 90 ++++++++++++++++----------------- 2 files changed, 73 insertions(+), 69 deletions(-) diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index 7963ec6c0b..8178e0a950 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -49,12 +49,16 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { return nil, fmt.Errorf("cannot connect to database: %w", err) } + // Enable foreign keys constraints, which we use extensively in our + // tables. if _, err := state.conn.Exec("PRAGMA foreign_keys = ON;"); err != nil { return nil, fmt.Errorf("enabling foreign key support in database: %w", err) } + // Enable WAL mode for performance - https://www.sqlite.org/wal.html if _, err := state.conn.Exec("PRAGMA journal_mode = WAL;"); err != nil { return nil, fmt.Errorf("switching journal to WAL mode: %w", err) } + // Force WAL mode to fsync after every transaction, for reboot safety. if _, err := state.conn.Exec("PRAGMA synchronous = FULL;"); err != nil { return nil, fmt.Errorf("setting full fsync mode in db: %w", err) } @@ -97,7 +101,7 @@ func (s *SQLiteState) Refresh() (defErr error) { podStates := make(map[string]string) volumeStates := make(map[string]string) - ctrRows, err := s.conn.Query("SELECT Id, Json FROM ContainerState;") + ctrRows, err := s.conn.Query("SELECT ID, JSON FROM ContainerState;") if err != nil { return fmt.Errorf("querying for container states: %w", err) } @@ -128,7 +132,7 @@ func (s *SQLiteState) Refresh() (defErr error) { ctrStates[id] = string(newJSON) } - podRows, err := s.conn.Query("SELECT Id, Json FROM PodState;") + podRows, err := s.conn.Query("SELECT ID, JSON FROM PodState;") if err != nil { return fmt.Errorf("querying for pod states: %w", err) } @@ -159,7 +163,7 @@ func (s *SQLiteState) Refresh() (defErr error) { podStates[id] = string(newJSON) } - volRows, err := s.conn.Query("SELECT Name, Json FROM VolumeState;") + volRows, err := s.conn.Query("SELECT Name, JSON FROM VolumeState;") if err != nil { return fmt.Errorf("querying for volume states: %w", err) } @@ -201,23 +205,23 @@ func (s *SQLiteState) Refresh() (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to refresh database state: %v", err) + logrus.Errorf("Rolling back transaction to refresh database state: %v", err) } } }() for id, json := range ctrStates { - if _, err := tx.Exec("UPDATE TABLE ContainerState SET Json=? WHERE Id=?;", json, id); err != nil { + if _, err := tx.Exec("UPDATE TABLE ContainerState SET JSON=? WHERE ID=?;", json, id); err != nil { return fmt.Errorf("updating container state: %w", err) } } for id, json := range podStates { - if _, err := tx.Exec("UPDATE TABLE PodState SET Json=? WHERE Id=?;", json, id); err != nil { + if _, err := tx.Exec("UPDATE TABLE PodState SET JSON=? WHERE ID=?;", json, id); err != nil { return fmt.Errorf("updating pod state: %w", err) } } for name, json := range volumeStates { - if _, err := tx.Exec("UPDATE TABLE VolumeState SET Json=? WHERE Name=?;", json, name); err != nil { + if _, err := tx.Exec("UPDATE TABLE VolumeState SET JSON=? WHERE Name=?;", json, name); err != nil { return fmt.Errorf("updating volume state: %w", err) } } @@ -319,7 +323,7 @@ func (s *SQLiteState) ValidateDBConfig(runtime *Runtime) (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to create DB config: %v", err) + logrus.Errorf("Rolling back transaction to create DB config: %v", err) } } }() @@ -386,7 +390,7 @@ func (s *SQLiteState) GetContainerName(id string) (string, error) { var name string - row := s.conn.QueryRow("SELECT Name FROM ContainerConfig WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT Name FROM ContainerConfig WHERE ID=?;", id) if err := row.Scan(&name); err != nil { if errors.Is(err, sql.ErrNoRows) { return "", define.ErrNoSuchCtr @@ -411,7 +415,7 @@ func (s *SQLiteState) GetPodName(id string) (string, error) { var name string - row := s.conn.QueryRow("SELECT Name FROM PodConfig WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT Name FROM PodConfig WHERE ID=?;", id) if err := row.Scan(&name); err != nil { if errors.Is(err, sql.ErrNoRows) { return "", define.ErrNoSuchPod @@ -461,7 +465,7 @@ func (s *SQLiteState) LookupContainerID(idOrName string) (string, error) { return "", define.ErrDBClosed } - rows, err := s.conn.Query("SELECT Id FROM ContainerConfig WHERE ContainerConfig.Name=? OR (ContainerConfig.Id LIKE ?);", idOrName, idOrName) + rows, err := s.conn.Query("SELECT ID FROM ContainerConfig WHERE ContainerConfig.Name=? OR (ContainerConfig.ID LIKE ?);", idOrName, idOrName) if err != nil { return "", fmt.Errorf("looking up container %q in database: %w", idOrName, err) } @@ -496,7 +500,7 @@ func (s *SQLiteState) LookupContainer(idOrName string) (*Container, error) { return nil, define.ErrDBClosed } - rows, err := s.conn.Query("SELECT Json FROM ContainerConfig WHERE ContainerConfig.Name=? OR (ContainerConfig.Id LIKE ?);", idOrName, idOrName) + rows, err := s.conn.Query("SELECT JSON FROM ContainerConfig WHERE ContainerConfig.Name=? OR (ContainerConfig.ID LIKE ?);", idOrName, idOrName) if err != nil { return nil, fmt.Errorf("looking up container %q in database: %w", idOrName, err) } @@ -543,7 +547,7 @@ func (s *SQLiteState) HasContainer(id string) (bool, error) { return false, define.ErrDBClosed } - row := s.conn.QueryRow("SELECT 1 FROM ContainerConfig WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT 1 FROM ContainerConfig WHERE ID=?;", id) var check int if err := row.Scan(&check); err != nil { @@ -601,7 +605,7 @@ func (s *SQLiteState) UpdateContainer(ctr *Container) error { return define.ErrCtrRemoved } - row := s.conn.QueryRow("SELECT Json FROM ContainerState WHERE Id=?;", ctr.ID()) + row := s.conn.QueryRow("SELECT JSON FROM ContainerState WHERE ID=?;", ctr.ID()) var rawJSON string if err := row.Scan(&rawJSON); err != nil { @@ -644,12 +648,12 @@ func (s *SQLiteState) SaveContainer(ctr *Container) (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to save container %s state: %v", ctr.ID(), err) + logrus.Errorf("Rolling back transaction to save container %s state: %v", ctr.ID(), err) } } }() - result, err := tx.Exec("UPDATE ContainerState SET Json=? WHERE Id=?;", stateJSON, ctr.ID()) + result, err := tx.Exec("UPDATE ContainerState SET JSON=? WHERE ID=?;", stateJSON, ctr.ID()) if err != nil { return fmt.Errorf("writing container %s state: %w", ctr.ID(), err) } @@ -681,7 +685,7 @@ func (s *SQLiteState) ContainerInUse(ctr *Container) ([]string, error) { return nil, define.ErrCtrRemoved } - rows, err := s.conn.Query("SELECT Id FROM ContainerDependency WHERE DependencyID=?;", ctr.ID()) + rows, err := s.conn.Query("SELECT ID FROM ContainerDependency WHERE DependencyID=?;", ctr.ID()) if err != nil { return nil, fmt.Errorf("retrieving containers that depend on container %s: %w", ctr.ID(), err) } @@ -709,7 +713,7 @@ func (s *SQLiteState) AllContainers(loadState bool) ([]*Container, error) { ctrs := []*Container{} if loadState { - rows, err := s.conn.Query("SELECT ContainerConfig.Json, ContainerState.Json AS StateJson INNER JOIN ContainerState ON ContainerConfig.Id = ContainerState.Id;") + rows, err := s.conn.Query("SELECT ContainerConfig.JSON, ContainerState.JSON AS StateJSON INNER JOIN ContainerState ON ContainerConfig.ID = ContainerState.ID;") if err != nil { return nil, fmt.Errorf("retrieving all containers from database: %w", err) } @@ -736,7 +740,7 @@ func (s *SQLiteState) AllContainers(loadState bool) ([]*Container, error) { ctrs = append(ctrs, ctr) } } else { - rows, err := s.conn.Query("SELECT Json FROM ContainerConfig;") + rows, err := s.conn.Query("SELECT JSON FROM ContainerConfig;") if err != nil { return nil, fmt.Errorf("retrieving all containers from database: %w", err) } @@ -843,7 +847,7 @@ func (s *SQLiteState) AddContainerExitCode(id string, exitCode int32) (defErr er defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to add exit code: %v", err) + logrus.Errorf("Rolling back transaction to add exit code: %v", err) } } }() @@ -869,7 +873,7 @@ func (s *SQLiteState) GetContainerExitCode(id string) (int32, error) { return -1, define.ErrDBClosed } - row := s.conn.QueryRow("SELECT ExitCode FROM ContainerExitCode WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT ExitCode FROM ContainerExitCode WHERE ID=?;", id) var exitCode int32 if err := row.Scan(&exitCode); err != nil { @@ -893,7 +897,7 @@ func (s *SQLiteState) GetContainerExitCodeTimeStamp(id string) (*time.Time, erro return nil, define.ErrDBClosed } - row := s.conn.QueryRow("SELECT Timestamp FROM ContainerExitCode WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT Timestamp FROM ContainerExitCode WHERE ID=?;", id) var timestamp int64 if err := row.Scan(×tamp); err != nil { @@ -923,7 +927,7 @@ func (s *SQLiteState) PruneContainerExitCodes() (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to remove old timestamps: %v", err) + logrus.Errorf("Rolling back transaction to remove old timestamps: %v", err) } } }() @@ -1418,7 +1422,7 @@ func (s *SQLiteState) LookupPod(idOrName string) (*Pod, error) { return nil, define.ErrDBClosed } - rows, err := s.conn.Query("SELECT Json FROM PodConfig WHERE PodConfig.Name=? OR (PodConfig.Id LIKE ?);", idOrName, idOrName) + rows, err := s.conn.Query("SELECT JSON FROM PodConfig WHERE PodConfig.Name=? OR (PodConfig.ID LIKE ?);", idOrName, idOrName) if err != nil { return nil, fmt.Errorf("looking up pod %q in database: %w", idOrName, err) } diff --git a/libpod/sqlite_state_internal.go b/libpod/sqlite_state_internal.go index b0d3ff2098..862fd23f7b 100644 --- a/libpod/sqlite_state_internal.go +++ b/libpod/sqlite_state_internal.go @@ -51,60 +51,60 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { // run the SQL, but that seems unnecessary. const dbConfig = ` CREATE TABLE IF NOT EXISTS DBConfig( - Id INTEGER PRIMARY KEY NOT NULL, + ID INTEGER PRIMARY KEY NOT NULL, SchemaVersion INTEGER NOT NULL, - Os TEXT NOT NULL, + OS TEXT NOT NULL, StaticDir TEXT NOT NULL, TmpDir TEXT NOT NULL, GraphRoot TEXT NOT NULL, RunRoot TEXT NOT NULL, GraphDriver TEXT NOT NULL, VolumeDir TEXT NOT NULL, - CHECK (Id IN (1)) + CHECK (ID IN (1)) );` const idNamespace = ` CREATE TABLE IF NOT EXISTS IDNamespace( - Id TEXT PRIMARY KEY NOT NULL + ID TEXT PRIMARY KEY NOT NULL );` const containerConfig = ` CREATE TABLE IF NOT EXISTS ContainerConfig( - Id TEXT PRIMARY KEY NOT NULL, + ID TEXT PRIMARY KEY NOT NULL, Name TEXT UNIQUE NOT NULL, PodID TEXT, - Json TEXT NOT NULL, - FOREIGN KEY (Id) REFERENCES IDNamespace(Id) DEFERRABLE INITIALLY DEFERRED, - FOREIGN KEY (Id) REFERENCES ContainerState(Id) DEFERRABLE INITIALLY DEFERRED, - FOREIGN KEY (PodID) REFERENCES PodConfig(Id) + JSON TEXT NOT NULL, + FOREIGN KEY (ID) REFERENCES IDNamespace(ID) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (ID) REFERENCES ContainerState(ID) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (PodID) REFERENCES PodConfig(ID) );` const containerState = ` CREATE TABLE IF NOT EXISTS ContainerState( - Id TEXT PRIMARY KEY NOT NULL, + ID TEXT PRIMARY KEY NOT NULL, State INTEGER NOT NULL, ExitCode INTEGER, - Json TEXT NOT NULL, - FOREIGN KEY (Id) REFERENCES ContainerConfig(Id) DEFERRABLE INITIALLY DEFERRED, + JSON TEXT NOT NULL, + FOREIGN KEY (ID) REFERENCES ContainerConfig(ID) DEFERRABLE INITIALLY DEFERRED, CHECK (ExitCode BETWEEN 0 AND 255) );` const containerExecSession = ` CREATE TABLE IF NOT EXISTS ContainerExecSession( - Id TEXT PRIMARY KEY NOT NULL, + ID TEXT PRIMARY KEY NOT NULL, ContainerID TEXT NOT NULL, - Json TEXT NOT NULL, - FOREIGN KEY (ContainerID) REFERENCES ContainerConfig(Id) + JSON TEXT NOT NULL, + FOREIGN KEY (ContainerID) REFERENCES ContainerConfig(ID) );` const containerDependency = ` CREATE TABLE IF NOT EXISTS ContainerDependency( - Id TEXT NOT NULL, + ID TEXT NOT NULL, DependencyID TEXT NOT NULL, - PRIMARY KEY (Id, DependencyID), - FOREIGN KEY (Id) REFERENCES ContainerConfig(Id) DEFERRABLE INITIALLY DEFERRED, - FOREIGN KEY (DependencyID) REFERENCES ContainerConfig(Id), - CHECK (Id <> DependencyID) + PRIMARY KEY (ID, DependencyID), + FOREIGN KEY (ID) REFERENCES ContainerConfig(ID) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (DependencyID) REFERENCES ContainerConfig(ID), + CHECK (ID <> DependencyID) );` const containerVolume = ` @@ -112,13 +112,13 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { ContainerID TEXT NOT NULL, VolumeName TEXT NOT NULL, PRIMARY KEY (ContainerID, VolumeName), - FOREIGN KEY (ContainerID) REFERENCES ContainerConfig(Id) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (ContainerID) REFERENCES ContainerConfig(ID) DEFERRABLE INITIALLY DEFERRED, FOREIGN KEY (VolumeName) REFERENCES VolumeConfig(Name) );` const containerExitCode = ` CREATE TABLE IF NOT EXISTS ContainerExitCode( - Id TEXT PRIMARY KEY NOT NULL, + ID TEXT PRIMARY KEY NOT NULL, Timestamp INTEGER NOT NULL, ExitCode INTEGER NOT NULL, CHECK (ExitCode BETWEEN 0 AND 255) @@ -126,34 +126,34 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { const podConfig = ` CREATE TABLE IF NOT EXISTS PodConfig( - Id TEXT PRIMARY KEY NOT NULL, + ID TEXT PRIMARY KEY NOT NULL, Name TEXT UNIQUE NOT NULL, - Json TEXT NOT NULL, - FOREIGN KEY (Id) REFERENCES IDNamespace(Id) DEFERRABLE INITIALLY DEFERRED, - FOREIGN KEY (Id) REFERENCES PodState(Id) DEFERRABLE INITIALLY DEFERRED + JSON TEXT NOT NULL, + FOREIGN KEY (ID) REFERENCES IDNamespace(ID) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (ID) REFERENCES PodState(ID) DEFERRABLE INITIALLY DEFERRED );` const podState = ` CREATE TABLE IF NOT EXISTS PodState( - Id TEXT PRIMARY KEY NOT NULL, - InfraContainerId TEXT, - Json TEXT NOT NULL, - FOREIGN KEY (Id) REFERENCES PodConfig(Id) DEFERRABLE INITIALLY DEFERRED, - FOREIGN KEY (InfraContainerId) REFERENCES ContainerConfig(Id) DEFERRABLE INITIALLY DEFERRED + ID TEXT PRIMARY KEY NOT NULL, + InfraContainerID TEXT, + JSON TEXT NOT NULL, + FOREIGN KEY (ID) REFERENCES PodConfig(ID) DEFERRABLE INITIALLY DEFERRED, + FOREIGN KEY (InfraContainerID) REFERENCES ContainerConfig(ID) DEFERRABLE INITIALLY DEFERRED );` const volumeConfig = ` CREATE TABLE IF NOT EXISTS VolumeConfig( Name TEXT PRIMARY KEY NOT NULL, StorageID TEXT, - Json TEXT NOT NULL, + JSON TEXT NOT NULL, FOREIGN KEY (Name) REFERENCES VolumeState(Name) DEFERRABLE INITIALLY DEFERRED );` const volumeState = ` CREATE TABLE IF NOT EXISTS VolumeState( Name TEXT PRIMARY KEY NOT NULL, - Json TEXT NOT NULL, + JSON TEXT NOT NULL, FOREIGN KEY (Name) REFERENCES VolumeConfig(Name) DEFERRABLE INITIALLY DEFERRED );` @@ -169,7 +169,7 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { "PodConfig": podConfig, "PodState": podState, "VolumeConfig": volumeConfig, - "volumeState": volumeState, + "VolumeState": volumeState, } tx, err := conn.Begin() @@ -179,7 +179,7 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to create tables: %v", err) + logrus.Errorf("Rolling back transaction to create tables: %v", err) } } }() @@ -199,7 +199,7 @@ func sqliteInitTables(conn *sql.DB) (defErr error) { // Get the config of a container with the given ID from the database func (s *SQLiteState) getCtrConfig(id string) (*ContainerConfig, error) { - row := s.conn.QueryRow("SELECT Json FROM ContainerConfig WHERE Id=?;", id) + row := s.conn.QueryRow("SELECT JSON FROM ContainerConfig WHERE ID=?;", id) var rawJSON string if err := row.Scan(&rawJSON); err != nil { @@ -293,12 +293,12 @@ func (s *SQLiteState) rewriteContainerConfig(ctr *Container, newCfg *ContainerCo defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to rewrite container %s config: %v", ctr.ID(), err) + logrus.Errorf("Rolling back transaction to rewrite container %s config: %v", ctr.ID(), err) } } }() - results, err := tx.Exec("UPDATE TABLE ContainerConfig SET Name=?, Json=? WHERE Id=?;", newCfg.Name, json, ctr.ID()) + results, err := tx.Exec("UPDATE TABLE ContainerConfig SET Name=?, JSON=? WHERE ID=?;", newCfg.Name, json, ctr.ID()) if err != nil { return fmt.Errorf("updating container config table with new configuration for container %s: %w", ctr.ID(), err) } @@ -343,7 +343,7 @@ func (s *SQLiteState) addContainer(ctr *Container) (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to create container: %v", err) + logrus.Errorf("Rolling back transaction to create container: %v", err) } } }() @@ -360,7 +360,7 @@ func (s *SQLiteState) addContainer(ctr *Container) (defErr error) { for _, dep := range deps { // Check if the dependency is in the same pod var depPod sql.NullString - row := tx.QueryRow("SELECT PodID FROM ContainerConfig WHERE Id=?;", dep) + row := tx.QueryRow("SELECT PodID FROM ContainerConfig WHERE ID=?;", dep) if err := row.Scan(&depPod); err != nil { if errors.Is(err, sql.ErrNoRows) { return fmt.Errorf("container dependency %s does not exist in database: %w", dep, define.ErrNoSuchCtr) @@ -400,21 +400,21 @@ func (s *SQLiteState) removeContainer(ctr *Container) (defErr error) { defer func() { if defErr != nil { if err := tx.Rollback(); err != nil { - logrus.Errorf("Error rolling back transaction to remove container %s: %v", ctr.ID(), err) + logrus.Errorf("Rolling back transaction to remove container %s: %v", ctr.ID(), err) } } }() - if _, err := tx.Exec("DELETE FROM IDNamespace WHERE Id=?;", ctr.ID()); err != nil { + if _, err := tx.Exec("DELETE FROM IDNamespace WHERE ID=?;", ctr.ID()); err != nil { return fmt.Errorf("removing container %s id from database: %w", ctr.ID(), err) } - if _, err := tx.Exec("DELETE FROM ContainerConfig WHERE Id=?;", ctr.ID()); err != nil { + if _, err := tx.Exec("DELETE FROM ContainerConfig WHERE ID=?;", ctr.ID()); err != nil { return fmt.Errorf("removing container %s config from database: %w", ctr.ID(), err) } - if _, err := tx.Exec("DELETE FROM ContainerState WHERE Id=?;", ctr.ID()); err != nil { + if _, err := tx.Exec("DELETE FROM ContainerState WHERE ID=?;", ctr.ID()); err != nil { return fmt.Errorf("removing container %s state from database: %w", ctr.ID(), err) } - if _, err := tx.Exec("DELETE FROM ContainerDependency WHERE Id=?;", ctr.ID()); err != nil { + if _, err := tx.Exec("DELETE FROM ContainerDependency WHERE ID=?;", ctr.ID()); err != nil { return fmt.Errorf("removing container %s dependencies from database: %w", ctr.ID(), err) } if _, err := tx.Exec("DELETE FROM ContainerVolume WHERE ContainerID=?;", ctr.ID()); err != nil {