From abfd18b0db8cb0ea331d90d702eab19e2157b973 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 24 Nov 2017 13:39:29 -0500 Subject: Move containers to file locks from c/storage Signed-off-by: Matthew Heon --- libpod/container.go | 28 +++++++++++----- libpod/runtime.go | 14 +++++++- libpod/runtime_ctr.go | 2 +- libpod/sql_state.go | 18 ++++++++--- libpod/sql_state_internal.go | 17 +++++++++- libpod/sql_state_test.go | 77 ++++++++++++++++++++++++++++++-------------- 6 files changed, 117 insertions(+), 39 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index bd7455147..f93b214b9 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -8,7 +8,6 @@ import ( "os" "path/filepath" "strconv" - "sync" "syscall" "time" @@ -61,9 +60,8 @@ type Container struct { state *containerRuntimeInfo - // TODO move to storage.Locker from sync.Mutex valid bool - lock sync.Mutex + lock storage.Locker runtime *Runtime } @@ -340,7 +338,7 @@ func (c *Container) syncContainer() error { } // Make a new container -func newContainer(rspec *spec.Spec) (*Container, error) { +func newContainer(rspec *spec.Spec, logDir string) (*Container, error) { if rspec == nil { return nil, errors.Wrapf(ErrInvalidArg, "must provide a valid runtime spec to create container") } @@ -357,6 +355,20 @@ func newContainer(rspec *spec.Spec) (*Container, error) { ctr.config.CreatedTime = time.Now() + // Path our lock file will reside at + lockPath := filepath.Join(logDir, ctr.config.ID) + // Ensure there is no conflict - file does not exist + _, err := os.Stat(lockPath) + if err == nil || !os.IsNotExist(err) { + return nil, errors.Wrapf(ErrCtrExists, "lockfile for container ID %s already exists", ctr.config.ID) + } + // Grab a lockfile at the given path + lock, err := storage.GetLockfile(lockPath) + if err != nil { + return nil, errors.Wrapf(err, "error creating lockfile for new container") + } + ctr.lock = lock + return ctr, nil } @@ -807,8 +819,8 @@ func (c *Container) mountStorage() (err error) { } }() - if err := c.runtime.state.SaveContainer(c); err != nil { - return errors.Wrapf(err, "error saving container %s state", c.ID()) + if err := c.save(); err != nil { + return err } return nil @@ -847,8 +859,8 @@ func (c *Container) cleanupStorage() error { c.state.Mountpoint = "" c.state.Mounted = false - if err := c.runtime.state.SaveContainer(c); err != nil { - return errors.Wrapf(err, "error saving container %s state", c.ID()) + if err := c.save(); err != nil { + return err } return nil diff --git a/libpod/runtime.go b/libpod/runtime.go index b86e06546..616641fc1 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -25,6 +25,7 @@ type Runtime struct { storageService *storageService imageContext *types.SystemContext ociRuntime *OCIRuntime + locksDir string valid bool lock sync.RWMutex } @@ -136,6 +137,17 @@ func NewRuntime(options ...RuntimeOption) (runtime *Runtime, err error) { } } + // Make a directory to hold container lockfiles + lockPath := filepath.Join(runtime.config.StaticDir, "lock") + if err := os.MkdirAll(lockPath, 0755); err != nil { + // The directory is allowed to exist + if !os.IsExist(err) { + return nil, errors.Wrapf(err, "error creating runtime lockfiles directory %s", + lockPath) + } + } + runtime.locksDir = lockPath + // Make the per-boot files directory if it does not exist if err := os.MkdirAll(runtime.config.TmpDir, 0755); err != nil { // The directory is allowed to exist @@ -166,7 +178,7 @@ func NewRuntime(options ...RuntimeOption) (runtime *Runtime, err error) { } } - state, err := NewSQLState(dbPath, lockPath, specsDir, runtime) + state, err := NewSQLState(dbPath, lockPath, specsDir, runtime.locksDir, runtime) if err != nil { return nil, err } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 5fb29abb5..793bb3e65 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -30,7 +30,7 @@ func (r *Runtime) NewContainer(spec *spec.Spec, options ...CtrCreateOption) (c * return nil, ErrRuntimeStopped } - ctr, err := newContainer(spec) + ctr, err := newContainer(spec, r.locksDir) if err != nil { return nil, err } diff --git a/libpod/sql_state.go b/libpod/sql_state.go index fb6be85d7..8640ca9c3 100644 --- a/libpod/sql_state.go +++ b/libpod/sql_state.go @@ -22,13 +22,14 @@ const DBSchema = 2 type SQLState struct { db *sql.DB specsDir string + locksDir string runtime *Runtime lock storage.Locker valid bool } // NewSQLState initializes a SQL-backed state, created the database if necessary -func NewSQLState(dbPath, lockPath, specsDir string, runtime *Runtime) (State, error) { +func NewSQLState(dbPath, lockPath, specsDir, locksDir string, runtime *Runtime) (State, error) { state := new(SQLState) state.runtime = runtime @@ -49,6 +50,15 @@ func NewSQLState(dbPath, lockPath, specsDir string, runtime *Runtime) (State, er } state.specsDir = specsDir + // Make the directory that will hold container lockfiles + if err := os.MkdirAll(locksDir, 0750); err != nil { + // The directory is allowed to exist + if !os.IsExist(err) { + return nil, errors.Wrapf(err, "error creating lockfiles dir %s", locksDir) + } + } + state.locksDir = locksDir + // Acquire the lock while we open the database and perform initial setup state.lock.Lock() defer state.lock.Unlock() @@ -130,7 +140,7 @@ func (s *SQLState) Container(id string) (*Container, error) { row := s.db.QueryRow(query, id) - ctr, err := ctrFromScannable(row, s.runtime, s.specsDir) + ctr, err := ctrFromScannable(row, s.runtime, s.specsDir, s.locksDir) if err != nil { return nil, errors.Wrapf(err, "error retrieving container %s from database", id) } @@ -177,7 +187,7 @@ func (s *SQLState) LookupContainer(idOrName string) (*Container, error) { } var err error - ctr, err = ctrFromScannable(rows, s.runtime, s.specsDir) + ctr, err = ctrFromScannable(rows, s.runtime, s.specsDir, s.locksDir) if err != nil { return nil, errors.Wrapf(err, "error retrieving container %s from database", idOrName) } @@ -576,7 +586,7 @@ func (s *SQLState) AllContainers() ([]*Container, error) { containers := []*Container{} for rows.Next() { - ctr, err := ctrFromScannable(rows, s.runtime, s.specsDir) + ctr, err := ctrFromScannable(rows, s.runtime, s.specsDir, s.locksDir) if err != nil { return nil, err } diff --git a/libpod/sql_state_internal.go b/libpod/sql_state_internal.go index 3aee8f638..e4af280ae 100644 --- a/libpod/sql_state_internal.go +++ b/libpod/sql_state_internal.go @@ -4,9 +4,11 @@ import ( "database/sql" "encoding/json" "io/ioutil" + "os" "path/filepath" "time" + "github.com/containers/storage" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -266,7 +268,7 @@ type scannable interface { } // Read a single container from a single row result in the database -func ctrFromScannable(row scannable, runtime *Runtime, specsDir string) (*Container, error) { +func ctrFromScannable(row scannable, runtime *Runtime, specsDir string, locksDir string) (*Container, error) { var ( id string name string @@ -384,6 +386,19 @@ func ctrFromScannable(row scannable, runtime *Runtime, specsDir string) (*Contai ctr.valid = true ctr.runtime = runtime + // Ensure the lockfile exists + lockPath := filepath.Join(locksDir, id) + _, err = os.Stat(lockPath) + if err != nil { + return nil, errors.Wrapf(err, "error performing stat on container %s lockfile", id) + } + // Open and set the lockfile + lock, err := storage.GetLockfile(lockPath) + if err != nil { + return nil, errors.Wrapf(err, "error retrieving lockfile for container %s", id) + } + ctr.lock = lock + // Retrieve the spec from disk ociSpec := new(spec.Spec) specPath := getSpecPath(specsDir, id) diff --git a/libpod/sql_state_test.go b/libpod/sql_state_test.go index 124959544..47760ed89 100644 --- a/libpod/sql_state_test.go +++ b/libpod/sql_state_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/assert" ) -func getTestContainer(id, name string) *Container { +func getTestContainer(id, name, locksDir string) (*Container, error) { ctr := &Container{ config: &ContainerConfig{ ID: id, @@ -44,7 +44,15 @@ func getTestContainer(id, name string) *Container { ctr.config.Labels["test"] = "testing" - return ctr + // Must make lockfile or container will error on being retrieved from DB + lockPath := filepath.Join(locksDir, id) + lock, err := storage.GetLockfile(lockPath) + if err != nil { + return nil, err + } + ctr.lock = lock + + return ctr, nil } // This horrible hack tests if containers are equal in a way that should handle @@ -107,7 +115,7 @@ func getEmptyState() (s State, p string, err error) { runtime.config = new(RuntimeConfig) runtime.config.StorageConfig = storage.StoreOptions{} - state, err := NewSQLState(dbPath, lockPath, tmpDir, runtime) + state, err := NewSQLState(dbPath, lockPath, tmpDir, tmpDir, runtime) if err != nil { return nil, "", err } @@ -121,7 +129,8 @@ func TestAddAndGetContainer(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -142,8 +151,10 @@ func TestAddAndGetContainerFromMultiple(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr1 := getTestContainer("11111111111111111111111111111111", "test1") - testCtr2 := getTestContainer("22222222222222222222222222222222", "test2") + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + assert.NoError(t, err) + testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", path) + assert.NoError(t, err) err = state.AddContainer(testCtr1) assert.NoError(t, err) @@ -177,8 +188,10 @@ func TestAddDuplicateIDFails(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr1 := getTestContainer("11111111111111111111111111111111", "test1") - testCtr2 := getTestContainer(testCtr1.ID(), "test2") + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + assert.NoError(t, err) + testCtr2, err := getTestContainer(testCtr1.ID(), "test2", path) + assert.NoError(t, err) err = state.AddContainer(testCtr1) assert.NoError(t, err) @@ -193,8 +206,10 @@ func TestAddDuplicateNameFails(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr1 := getTestContainer("11111111111111111111111111111111", "test1") - testCtr2 := getTestContainer("22222222222222222222222222222222", testCtr1.Name()) + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + assert.NoError(t, err) + testCtr2, err := getTestContainer("22222222222222222222222222222222", testCtr1.Name(), path) + assert.NoError(t, err) err = state.AddContainer(testCtr1) assert.NoError(t, err) @@ -248,7 +263,8 @@ func TestLookupContainerByFullID(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -269,7 +285,8 @@ func TestLookupContainerByUniquePartialID(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -290,8 +307,10 @@ func TestLookupContainerByNonUniquePartialIDFails(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr1 := getTestContainer("00000000000000000000000000000000", "test1") - testCtr2 := getTestContainer("00000000000000000000000000000001", "test2") + testCtr1, err := getTestContainer("00000000000000000000000000000000", "test1", path) + assert.NoError(t, err) + testCtr2, err := getTestContainer("00000000000000000000000000000001", "test2", path) + assert.NoError(t, err) err = state.AddContainer(testCtr1) assert.NoError(t, err) @@ -309,7 +328,8 @@ func TestLookupContainerByName(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -351,7 +371,8 @@ func TestHasContainerFindsContainer(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -367,7 +388,8 @@ func TestSaveAndUpdateContainer(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -398,7 +420,8 @@ func TestUpdateContainerNotInDatabaseReturnsError(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.UpdateContainer(testCtr) assert.Error(t, err) @@ -431,7 +454,8 @@ func TestSaveContainerNotInStateReturnsError(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.SaveContainer(testCtr) assert.Error(t, err) @@ -443,7 +467,8 @@ func TestRemoveContainer(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -466,7 +491,8 @@ func TestRemoveNonexistantContainerFails(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.RemoveContainer(testCtr) assert.Error(t, err) @@ -489,7 +515,8 @@ func TestGetAllContainersWithOneContainer(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test") + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + assert.NoError(t, err) err = state.AddContainer(testCtr) assert.NoError(t, err) @@ -511,8 +538,10 @@ func TestGetAllContainersTwoContainers(t *testing.T) { defer os.RemoveAll(path) defer state.Close() - testCtr1 := getTestContainer("11111111111111111111111111111111", "test1") - testCtr2 := getTestContainer("22222222222222222222222222222222", "test2") + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + assert.NoError(t, err) + testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", path) + assert.NoError(t, err) err = state.AddContainer(testCtr1) assert.NoError(t, err) -- cgit v1.2.3-54-g00ecf From e9298a533adf30c3dd1990098665be23a0f29d1a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 27 Nov 2017 13:42:45 -0500 Subject: Remove SQL state locking and rely on sqlite locking Also renames some parameters from locksDir -> lockDir for consistency. Signed-off-by: Matthew Heon --- libpod/container.go | 4 ++-- libpod/runtime.go | 13 ++++++------- libpod/runtime_ctr.go | 2 +- libpod/sql_state.go | 41 ++++++++--------------------------------- libpod/sql_state_internal.go | 4 ++-- 5 files changed, 19 insertions(+), 45 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index f93b214b9..4f8c148d1 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -338,7 +338,7 @@ func (c *Container) syncContainer() error { } // Make a new container -func newContainer(rspec *spec.Spec, logDir string) (*Container, error) { +func newContainer(rspec *spec.Spec, lockDir string) (*Container, error) { if rspec == nil { return nil, errors.Wrapf(ErrInvalidArg, "must provide a valid runtime spec to create container") } @@ -356,7 +356,7 @@ func newContainer(rspec *spec.Spec, logDir string) (*Container, error) { ctr.config.CreatedTime = time.Now() // Path our lock file will reside at - lockPath := filepath.Join(logDir, ctr.config.ID) + lockPath := filepath.Join(lockDir, ctr.config.ID) // Ensure there is no conflict - file does not exist _, err := os.Stat(lockPath) if err == nil || !os.IsNotExist(err) { diff --git a/libpod/runtime.go b/libpod/runtime.go index 616641fc1..ac7db51ca 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -25,7 +25,7 @@ type Runtime struct { storageService *storageService imageContext *types.SystemContext ociRuntime *OCIRuntime - locksDir string + lockDir string valid bool lock sync.RWMutex } @@ -138,15 +138,15 @@ func NewRuntime(options ...RuntimeOption) (runtime *Runtime, err error) { } // Make a directory to hold container lockfiles - lockPath := filepath.Join(runtime.config.StaticDir, "lock") - if err := os.MkdirAll(lockPath, 0755); err != nil { + lockDir := filepath.Join(runtime.config.StaticDir, "lock") + if err := os.MkdirAll(lockDir, 0755); err != nil { // The directory is allowed to exist if !os.IsExist(err) { return nil, errors.Wrapf(err, "error creating runtime lockfiles directory %s", - lockPath) + lockDir) } } - runtime.locksDir = lockPath + runtime.lockDir = lockDir // Make the per-boot files directory if it does not exist if err := os.MkdirAll(runtime.config.TmpDir, 0755); err != nil { @@ -166,7 +166,6 @@ func NewRuntime(options ...RuntimeOption) (runtime *Runtime, err error) { runtime.state = state } else { dbPath := filepath.Join(runtime.config.StaticDir, "state.sql") - lockPath := filepath.Join(runtime.config.TmpDir, "state.lck") specsDir := filepath.Join(runtime.config.StaticDir, "ocispec") // Make a directory to hold JSON versions of container OCI specs @@ -178,7 +177,7 @@ func NewRuntime(options ...RuntimeOption) (runtime *Runtime, err error) { } } - state, err := NewSQLState(dbPath, lockPath, specsDir, runtime.locksDir, runtime) + state, err := NewSQLState(dbPath, specsDir, runtime.lockDir, runtime) if err != nil { return nil, err } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 793bb3e65..f314b302f 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -30,7 +30,7 @@ func (r *Runtime) NewContainer(spec *spec.Spec, options ...CtrCreateOption) (c * return nil, ErrRuntimeStopped } - ctr, err := newContainer(spec, r.locksDir) + ctr, err := newContainer(spec, r.lockDir) if err != nil { return nil, err } diff --git a/libpod/sql_state.go b/libpod/sql_state.go index 8640ca9c3..cdf8dd707 100644 --- a/libpod/sql_state.go +++ b/libpod/sql_state.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" - "github.com/containers/storage" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -22,25 +21,17 @@ const DBSchema = 2 type SQLState struct { db *sql.DB specsDir string - locksDir string + lockDir string runtime *Runtime - lock storage.Locker valid bool } // NewSQLState initializes a SQL-backed state, created the database if necessary -func NewSQLState(dbPath, lockPath, specsDir, locksDir string, runtime *Runtime) (State, error) { +func NewSQLState(dbPath, specsDir, lockDir string, runtime *Runtime) (State, error) { state := new(SQLState) state.runtime = runtime - // Make our lock file - lock, err := storage.GetLockfile(lockPath) - if err != nil { - return nil, errors.Wrapf(err, "error creating lockfile for state") - } - state.lock = lock - // Make the directory that will hold JSON copies of container runtime specs if err := os.MkdirAll(specsDir, 0750); err != nil { // The directory is allowed to exist @@ -51,17 +42,13 @@ func NewSQLState(dbPath, lockPath, specsDir, locksDir string, runtime *Runtime) state.specsDir = specsDir // Make the directory that will hold container lockfiles - if err := os.MkdirAll(locksDir, 0750); err != nil { + if err := os.MkdirAll(lockDir, 0750); err != nil { // The directory is allowed to exist if !os.IsExist(err) { - return nil, errors.Wrapf(err, "error creating lockfiles dir %s", locksDir) + return nil, errors.Wrapf(err, "error creating lockfiles dir %s", lockDir) } } - state.locksDir = locksDir - - // Acquire the lock while we open the database and perform initial setup - state.lock.Lock() - defer state.lock.Unlock() + state.lockDir = lockDir // TODO add a separate temporary database for per-boot container // state @@ -97,9 +84,6 @@ func NewSQLState(dbPath, lockPath, specsDir, locksDir string, runtime *Runtime) // Close the state's database connection func (s *SQLState) Close() error { - s.lock.Lock() - defer s.lock.Unlock() - if !s.valid { return ErrDBClosed } @@ -140,7 +124,7 @@ func (s *SQLState) Container(id string) (*Container, error) { row := s.db.QueryRow(query, id) - ctr, err := ctrFromScannable(row, s.runtime, s.specsDir, s.locksDir) + ctr, err := ctrFromScannable(row, s.runtime, s.specsDir, s.lockDir) if err != nil { return nil, errors.Wrapf(err, "error retrieving container %s from database", id) } @@ -187,7 +171,7 @@ func (s *SQLState) LookupContainer(idOrName string) (*Container, error) { } var err error - ctr, err = ctrFromScannable(rows, s.runtime, s.specsDir, s.locksDir) + ctr, err = ctrFromScannable(rows, s.runtime, s.specsDir, s.lockDir) if err != nil { return nil, errors.Wrapf(err, "error retrieving container %s from database", idOrName) } @@ -260,9 +244,6 @@ func (s *SQLState) AddContainer(ctr *Container) (err error) { return errors.Wrapf(err, "error marshaling container %s labels to JSON", ctr.ID()) } - s.lock.Lock() - defer s.lock.Unlock() - tx, err := s.db.Begin() if err != nil { return errors.Wrapf(err, "error beginning database transaction") @@ -441,9 +422,6 @@ func (s *SQLState) SaveContainer(ctr *Container) error { Pid=? WHERE Id=?;` - s.lock.Lock() - defer s.lock.Unlock() - if !s.valid { return ErrDBClosed } @@ -501,9 +479,6 @@ func (s *SQLState) RemoveContainer(ctr *Container) error { removeState = "DELETE FROM containerState WHERE ID=?;" ) - s.lock.Lock() - defer s.lock.Unlock() - if !s.valid { return ErrDBClosed } @@ -586,7 +561,7 @@ func (s *SQLState) AllContainers() ([]*Container, error) { containers := []*Container{} for rows.Next() { - ctr, err := ctrFromScannable(rows, s.runtime, s.specsDir, s.locksDir) + ctr, err := ctrFromScannable(rows, s.runtime, s.specsDir, s.lockDir) if err != nil { return nil, err } diff --git a/libpod/sql_state_internal.go b/libpod/sql_state_internal.go index e4af280ae..dfcd97c63 100644 --- a/libpod/sql_state_internal.go +++ b/libpod/sql_state_internal.go @@ -268,7 +268,7 @@ type scannable interface { } // Read a single container from a single row result in the database -func ctrFromScannable(row scannable, runtime *Runtime, specsDir string, locksDir string) (*Container, error) { +func ctrFromScannable(row scannable, runtime *Runtime, specsDir string, lockDir string) (*Container, error) { var ( id string name string @@ -387,7 +387,7 @@ func ctrFromScannable(row scannable, runtime *Runtime, specsDir string, locksDir ctr.runtime = runtime // Ensure the lockfile exists - lockPath := filepath.Join(locksDir, id) + lockPath := filepath.Join(lockDir, id) _, err = os.Stat(lockPath) if err != nil { return nil, errors.Wrapf(err, "error performing stat on container %s lockfile", id) -- cgit v1.2.3-54-g00ecf From 43c51c58ad620fe544ca43dac003e113f7bbd8bd Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 27 Nov 2017 14:07:41 -0500 Subject: Fix unit tests and provide more realistic test environment Signed-off-by: Matthew Heon --- libpod/sql_state_test.go | 107 ++++++++++++++++++++++++----------------------- 1 file changed, 54 insertions(+), 53 deletions(-) (limited to 'libpod') diff --git a/libpod/sql_state_test.go b/libpod/sql_state_test.go index 47760ed89..0bb0cd517 100644 --- a/libpod/sql_state_test.go +++ b/libpod/sql_state_test.go @@ -97,10 +97,10 @@ func testContainersEqual(a, b *Container) bool { // Get an empty state for use in tests // An empty Runtime is provided -func getEmptyState() (s State, p string, err error) { +func getEmptyState() (s State, p string, p2 string, err error) { tmpDir, err := ioutil.TempDir("", "libpod_state_test_") if err != nil { - return nil, "", err + return nil, "", "", err } defer func() { if err != nil { @@ -109,27 +109,28 @@ func getEmptyState() (s State, p string, err error) { }() dbPath := filepath.Join(tmpDir, "db.sql") - lockPath := filepath.Join(tmpDir, "db.lck") + specsDir := filepath.Join(tmpDir, "specs") + lockDir := filepath.Join(tmpDir, "locks") runtime := new(Runtime) runtime.config = new(RuntimeConfig) runtime.config.StorageConfig = storage.StoreOptions{} - state, err := NewSQLState(dbPath, lockPath, tmpDir, tmpDir, runtime) + state, err := NewSQLState(dbPath, specsDir, lockDir, runtime) if err != nil { - return nil, "", err + return nil, "", "", err } - return state, tmpDir, nil + return state, tmpDir, lockDir, nil } func TestAddAndGetContainer(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -146,14 +147,14 @@ func TestAddAndGetContainer(t *testing.T) { } func TestAddAndGetContainerFromMultiple(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", lockPath) assert.NoError(t, err) - testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", path) + testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr1) @@ -173,7 +174,7 @@ func TestAddAndGetContainerFromMultiple(t *testing.T) { } func TestAddInvalidContainerFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -183,14 +184,14 @@ func TestAddInvalidContainerFails(t *testing.T) { } func TestAddDuplicateIDFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", lockPath) assert.NoError(t, err) - testCtr2, err := getTestContainer(testCtr1.ID(), "test2", path) + testCtr2, err := getTestContainer(testCtr1.ID(), "test2", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr1) @@ -201,14 +202,14 @@ func TestAddDuplicateIDFails(t *testing.T) { } func TestAddDuplicateNameFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", lockPath) assert.NoError(t, err) - testCtr2, err := getTestContainer("22222222222222222222222222222222", testCtr1.Name(), path) + testCtr2, err := getTestContainer("22222222222222222222222222222222", testCtr1.Name(), lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr1) @@ -219,7 +220,7 @@ func TestAddDuplicateNameFails(t *testing.T) { } func TestGetNonexistantContainerFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -229,7 +230,7 @@ func TestGetNonexistantContainerFails(t *testing.T) { } func TestGetContainerWithEmptyIDFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -239,7 +240,7 @@ func TestGetContainerWithEmptyIDFails(t *testing.T) { } func TestLookupContainerWithEmptyIDFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -249,7 +250,7 @@ func TestLookupContainerWithEmptyIDFails(t *testing.T) { } func TestLookupNonexistantContainerFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) @@ -258,12 +259,12 @@ func TestLookupNonexistantContainerFails(t *testing.T) { } func TestLookupContainerByFullID(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -280,12 +281,12 @@ func TestLookupContainerByFullID(t *testing.T) { } func TestLookupContainerByUniquePartialID(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -302,14 +303,14 @@ func TestLookupContainerByUniquePartialID(t *testing.T) { } func TestLookupContainerByNonUniquePartialIDFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr1, err := getTestContainer("00000000000000000000000000000000", "test1", path) + testCtr1, err := getTestContainer("00000000000000000000000000000000", "test1", lockPath) assert.NoError(t, err) - testCtr2, err := getTestContainer("00000000000000000000000000000001", "test2", path) + testCtr2, err := getTestContainer("00000000000000000000000000000001", "test2", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr1) @@ -323,12 +324,12 @@ func TestLookupContainerByNonUniquePartialIDFails(t *testing.T) { } func TestLookupContainerByName(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -345,7 +346,7 @@ func TestLookupContainerByName(t *testing.T) { } func TestHasContainerEmptyIDFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -355,7 +356,7 @@ func TestHasContainerEmptyIDFails(t *testing.T) { } func TestHasContainerNoSuchContainerReturnsFalse(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -366,12 +367,12 @@ func TestHasContainerNoSuchContainerReturnsFalse(t *testing.T) { } func TestHasContainerFindsContainer(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -383,12 +384,12 @@ func TestHasContainerFindsContainer(t *testing.T) { } func TestSaveAndUpdateContainer(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -415,12 +416,12 @@ func TestSaveAndUpdateContainer(t *testing.T) { } func TestUpdateContainerNotInDatabaseReturnsError(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.UpdateContainer(testCtr) @@ -429,7 +430,7 @@ func TestUpdateContainerNotInDatabaseReturnsError(t *testing.T) { } func TestUpdateInvalidContainerReturnsError(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -439,7 +440,7 @@ func TestUpdateInvalidContainerReturnsError(t *testing.T) { } func TestSaveInvalidContainerReturnsError(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -449,12 +450,12 @@ func TestSaveInvalidContainerReturnsError(t *testing.T) { } func TestSaveContainerNotInStateReturnsError(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.SaveContainer(testCtr) @@ -462,12 +463,12 @@ func TestSaveContainerNotInStateReturnsError(t *testing.T) { } func TestRemoveContainer(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -486,12 +487,12 @@ func TestRemoveContainer(t *testing.T) { } func TestRemoveNonexistantContainerFails(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.RemoveContainer(testCtr) @@ -499,7 +500,7 @@ func TestRemoveNonexistantContainerFails(t *testing.T) { } func TestGetAllContainersOnNewStateIsEmpty(t *testing.T) { - state, path, err := getEmptyState() + state, path, _, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() @@ -510,12 +511,12 @@ func TestGetAllContainersOnNewStateIsEmpty(t *testing.T) { } func TestGetAllContainersWithOneContainer(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", path) + testCtr, err := getTestContainer("0123456789ABCDEF0123456789ABCDEF", "test", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr) @@ -533,14 +534,14 @@ func TestGetAllContainersWithOneContainer(t *testing.T) { } func TestGetAllContainersTwoContainers(t *testing.T) { - state, path, err := getEmptyState() + state, path, lockPath, err := getEmptyState() assert.NoError(t, err) defer os.RemoveAll(path) defer state.Close() - testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", path) + testCtr1, err := getTestContainer("11111111111111111111111111111111", "test1", lockPath) assert.NoError(t, err) - testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", path) + testCtr2, err := getTestContainer("22222222222222222222222222222222", "test2", lockPath) assert.NoError(t, err) err = state.AddContainer(testCtr1) -- cgit v1.2.3-54-g00ecf From eb942b1b47235ffddc37c6e353fbafcfa3a60a15 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 4 Dec 2017 14:18:48 -0500 Subject: Fix lint Signed-off-by: Matthew Heon --- libpod/container.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index 4f8c148d1..df8d63fdc 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -819,11 +819,7 @@ func (c *Container) mountStorage() (err error) { } }() - if err := c.save(); err != nil { - return err - } - - return nil + return c.save() } // CleanupStorage unmounts all mount points in container and cleans up container storage @@ -859,9 +855,5 @@ func (c *Container) cleanupStorage() error { c.state.Mountpoint = "" c.state.Mounted = false - if err := c.save(); err != nil { - return err - } - - return nil + return c.save() } -- cgit v1.2.3-54-g00ecf From 70da705c364bf5f32c85966e15bfa56039b0f6f6 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 5 Dec 2017 09:21:50 -0500 Subject: Remove check for existance of lockfile when retrieving containers Signed-off-by: Matthew Heon --- libpod/sql_state_internal.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'libpod') diff --git a/libpod/sql_state_internal.go b/libpod/sql_state_internal.go index dfcd97c63..515b5b6ad 100644 --- a/libpod/sql_state_internal.go +++ b/libpod/sql_state_internal.go @@ -4,7 +4,6 @@ import ( "database/sql" "encoding/json" "io/ioutil" - "os" "path/filepath" "time" @@ -386,13 +385,8 @@ func ctrFromScannable(row scannable, runtime *Runtime, specsDir string, lockDir ctr.valid = true ctr.runtime = runtime - // Ensure the lockfile exists - lockPath := filepath.Join(lockDir, id) - _, err = os.Stat(lockPath) - if err != nil { - return nil, errors.Wrapf(err, "error performing stat on container %s lockfile", id) - } // Open and set the lockfile + lockPath := filepath.Join(lockDir, id) lock, err := storage.GetLockfile(lockPath) if err != nil { return nil, errors.Wrapf(err, "error retrieving lockfile for container %s", id) -- cgit v1.2.3-54-g00ecf From 190b05209f95cf611eb5cafc4bf4cce875c74e9e Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 5 Dec 2017 19:02:32 -0500 Subject: Small comment update to make use of syncContainer more obvious Signed-off-by: Matthew Heon --- libpod/container.go | 2 ++ 1 file changed, 2 insertions(+) (limited to 'libpod') diff --git a/libpod/container.go b/libpod/container.go index df8d63fdc..f1dd34131 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -314,6 +314,8 @@ func (c *Container) attachSocketPath() string { // Sync this container with on-disk state and runc status // Should only be called with container lock held +// This function should suffice to ensure a container's state is accurate and +// it is valid for use. func (c *Container) syncContainer() error { if err := c.runtime.state.UpdateContainer(c); err != nil { return err -- cgit v1.2.3-54-g00ecf