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