From cb28a1d284c8e9ec3539a11e472590686ccbd5f8 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 7 Feb 2018 15:05:17 -0500 Subject: Fix bugs identified by unit tests Signed-off-by: Matthew Heon Closes: #268 Approved by: rhatdan --- libpod/in_memory_state.go | 69 ++++++++++++++++++++++++++++++----- libpod/pod.go | 1 + libpod/sql_state.go | 31 ++++++++++------ libpod/sql_state_internal.go | 86 +++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 163 insertions(+), 24 deletions(-) diff --git a/libpod/in_memory_state.go b/libpod/in_memory_state.go index c057f6804..4a2d5a311 100644 --- a/libpod/in_memory_state.go +++ b/libpod/in_memory_state.go @@ -123,6 +123,17 @@ func (s *InMemoryState) AddContainer(ctr *Container) error { return errors.Wrapf(ErrInvalidArg, "cannot add a container that is in a pod with AddContainer, use AddContainerToPod") } + // There are potential race conditions with this + // But in-memory state is intended purely for testing and not production + // use, so this should be fine. + depCtrs := ctr.Dependencies() + for _, depCtr := range depCtrs { + _, ok = s.containers[depCtr] + if !ok { + return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depCtr) + } + } + if err := s.nameIndex.Reserve(ctr.Name(), ctr.ID()); err != nil { return errors.Wrapf(err, "error registering container name %s", ctr.Name()) } @@ -135,7 +146,6 @@ func (s *InMemoryState) AddContainer(ctr *Container) error { s.containers[ctr.ID()] = ctr // Add containers this container depends on - depCtrs := ctr.Dependencies() for _, depCtr := range depCtrs { s.addCtrToDependsMap(ctr.ID(), depCtr) } @@ -157,6 +167,7 @@ func (s *InMemoryState) RemoveContainer(ctr *Container) error { } if _, ok := s.containers[ctr.ID()]; !ok { + ctr.valid = false return errors.Wrapf(ErrNoSuchCtr, "no container exists in state with ID %s", ctr.ID()) } @@ -301,6 +312,10 @@ func (s *InMemoryState) PodHasContainer(pod *Pod, ctrID string) (bool, error) { return false, errors.Wrapf(ErrPodRemoved, "pod %s is not valid") } + if ctrID == "" { + return false, ErrEmptyID + } + podCtrs, ok := s.podContainers[pod.ID()] if !ok { pod.valid = false @@ -398,10 +413,12 @@ func (s *InMemoryState) RemovePod(pod *Pod) error { // pods out of the state if _, ok := s.pods[pod.ID()]; !ok { + pod.valid = false return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID()) } podCtrs, ok := s.podContainers[pod.ID()] if !ok { + pod.valid = false return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID()) } if len(podCtrs) != 0 { @@ -430,20 +447,19 @@ func (s *InMemoryState) RemovePodContainers(pod *Pod) error { // Get pod containers podCtrs, ok := s.podContainers[pod.ID()] if !ok { + pod.valid = false return errors.Wrapf(ErrNoSuchPod, "no pod exists in state with ID %s", pod.ID()) } // Go through container dependencies. Check to see if any are outside the pod. for ctr := range podCtrs { ctrDeps, ok := s.ctrDepends[ctr] - if !ok { - return errors.Wrapf(ErrInternal, "cannot find dependencies for container %s", ctr) - } - - for _, dep := range ctrDeps { - _, ok := podCtrs[dep] - if !ok { - return errors.Wrapf(ErrCtrExists, "container %s has dependency %s outside of pod %s", ctr, dep, pod.ID()) + if ok { + for _, dep := range ctrDeps { + _, ok := podCtrs[dep] + if !ok { + return errors.Wrapf(ErrCtrExists, "container %s has dependency %s outside of pod %s", ctr, dep, pod.ID()) + } } } } @@ -490,6 +506,17 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error { return errors.Wrapf(ErrCtrExists, "container with ID %s already exists in pod %s", ctr.ID(), pod.ID()) } + // There are potential race conditions with this + // But in-memory state is intended purely for testing and not production + // use, so this should be fine. + depCtrs := ctr.Dependencies() + for _, depCtr := range depCtrs { + _, ok = s.containers[depCtr] + if !ok { + return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depCtr) + } + } + // Add container to state _, ok = s.containers[ctr.ID()] if ok { @@ -510,6 +537,11 @@ func (s *InMemoryState) AddContainerToPod(pod *Pod, ctr *Container) error { // Add container to pod containers podCtrs[ctr.ID()] = ctr + // Add containers this container depends on + for _, depCtr := range depCtrs { + s.addCtrToDependsMap(ctr.ID(), depCtr) + } + return nil } @@ -523,6 +555,13 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error { return errors.Wrapf(ErrCtrRemoved, "container %s is not valid and cannot be removed from the pod", ctr.ID()) } + // Ensure we don't remove a container which other containers depend on + deps, ok := s.ctrDepends[ctr.ID()] + if ok && len(deps) != 0 { + depsStr := strings.Join(deps, ", ") + return errors.Wrapf(ErrCtrExists, "the following containers depend on container %s: %s", ctr.ID(), depsStr) + } + // Retrieve pod containers podCtrs, ok := s.podContainers[pod.ID()] if !ok { @@ -530,6 +569,12 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error { return errors.Wrapf(ErrPodRemoved, "pod %s has been removed", pod.ID()) } + // Does the container exist? + if _, ok := s.containers[ctr.ID()]; !ok { + ctr.valid = false + return errors.Wrapf(ErrNoSuchCtr, "container %s does not exist in state", ctr.ID()) + } + // Is the container in the pod? if _, ok := podCtrs[ctr.ID()]; !ok { return errors.Wrapf(ErrNoSuchCtr, "container with ID %s not found in pod %s", ctr.ID(), pod.ID()) @@ -549,6 +594,12 @@ func (s *InMemoryState) RemoveContainerFromPod(pod *Pod, ctr *Container) error { // Remove the container from the pod delete(podCtrs, ctr.ID()) + // Remove us from container dependencies + depCtrs := ctr.Dependencies() + for _, depCtr := range depCtrs { + s.removeCtrFromDependsMap(ctr.ID(), depCtr) + } + return nil } diff --git a/libpod/pod.go b/libpod/pod.go index 4f5521ccb..ecef8554e 100644 --- a/libpod/pod.go +++ b/libpod/pod.go @@ -45,6 +45,7 @@ func newPod(lockDir string, runtime *Runtime) (*Pod, error) { pod := new(Pod) pod.id = stringid.GenerateNonCryptoID() pod.name = namesgenerator.GetRandomName(0) + pod.labels = make(map[string]string) pod.runtime = runtime // Path our lock file will reside at diff --git a/libpod/sql_state.go b/libpod/sql_state.go index a5809fe58..ff04d6d63 100644 --- a/libpod/sql_state.go +++ b/libpod/sql_state.go @@ -14,7 +14,7 @@ import ( // DBSchema is the current DB schema version // Increments every time a change is made to the database's tables -const DBSchema = 9 +const DBSchema = 10 // SQLState is a state implementation backed by a persistent SQLite3 database type SQLState struct { @@ -252,7 +252,7 @@ func (s *SQLState) AddContainer(ctr *Container) (err error) { return errors.Wrapf(ErrPodExists, "cannot add container that belongs to a pod, use AddContainerToPod instead") } - return s.addContainer(ctr) + return s.addContainer(ctr, nil) } // UpdateContainer updates a container's state from the database @@ -381,7 +381,7 @@ func (s *SQLState) UpdateContainer(ctr *Container) error { } // SaveContainer updates a container's state in the database -func (s *SQLState) SaveContainer(ctr *Container) error { +func (s *SQLState) SaveContainer(ctr *Container) (err error) { const update = `UPDATE containerState SET State=?, ConfigPath=?, @@ -508,7 +508,7 @@ func (s *SQLState) RemoveContainer(ctr *Container) error { return errors.Wrapf(ErrPodExists, "container %s belongs to a pod, use RemoveContainerFromPod", ctr.ID()) } - return s.removeContainer(ctr) + return s.removeContainer(ctr, nil) } // AllContainers retrieves all the containers presently in the state @@ -801,7 +801,7 @@ func (s *SQLState) AddPod(pod *Pod) (err error) { // RemovePod removes a pod from the state // Only empty pods can be removed -func (s *SQLState) RemovePod(pod *Pod) error { +func (s *SQLState) RemovePod(pod *Pod) (err error) { const ( removePod = "DELETE FROM pods WHERE ID=?;" removeRegistry = "DELETE FROM registry WHERE Id=?;" @@ -832,6 +832,7 @@ func (s *SQLState) RemovePod(pod *Pod) error { if err != nil { return errors.Wrapf(err, "error retrieving number of rows in transaction removing pod %s", pod.ID()) } else if rows == 0 { + pod.valid = false return ErrNoSuchPod } @@ -851,11 +852,11 @@ func (s *SQLState) RemovePod(pod *Pod) error { // This can avoid issues with dependencies within the pod // The operation will fail if any container in the pod has a dependency from // outside the pod -func (s *SQLState) RemovePodContainers(pod *Pod) error { +func (s *SQLState) RemovePodContainers(pod *Pod) (err error) { const ( getPodCtrs = "SELECT Id FROM containers WHERE pod=?;" removeCtr = "DELETE FROM containers WHERE pod=?;" - removeCtrState = "DELETE FROM containerState WHERE ID=ANY(SELECT Id FROM containers WHERE pod=?);" + removeCtrState = "DELETE FROM containerState WHERE ID IN (SELECT Id FROM containers WHERE pod=?);" ) if !s.valid { @@ -880,6 +881,16 @@ func (s *SQLState) RemovePodContainers(pod *Pod) error { } }() + // Check if the pod exists + exists, err := podExistsTx(pod.ID(), tx) + if err != nil { + return err + } + if !exists { + pod.valid = false + return ErrNoSuchPod + } + // First get all containers in the pod rows, err := tx.Query(getPodCtrs, pod.ID()) if err != nil { @@ -920,7 +931,7 @@ func (s *SQLState) RemovePodContainers(pod *Pod) error { } if err := tx.Commit(); err != nil { - return errors.Wrapf(err, "error committing transaction to add pod %s", pod.ID()) + return errors.Wrapf(err, "error committing transaction remove pod %s containers", pod.ID()) } committed = true @@ -963,7 +974,7 @@ func (s *SQLState) AddContainerToPod(pod *Pod, ctr *Container) error { return errors.Wrapf(ErrInvalidArg, "container's pod ID does not match given pod's ID") } - return s.addContainer(ctr) + return s.addContainer(ctr, pod) } // RemoveContainerFromPod removes a container from the given pod @@ -972,7 +983,7 @@ func (s *SQLState) RemoveContainerFromPod(pod *Pod, ctr *Container) error { return errors.Wrapf(ErrInvalidArg, "container %s is not in pod %s", ctr.ID(), pod.ID()) } - return s.removeContainer(ctr) + return s.removeContainer(ctr, pod) } // AllPods retrieves all pods presently in the state diff --git a/libpod/sql_state_internal.go b/libpod/sql_state_internal.go index f2b0d603c..5a1b68615 100644 --- a/libpod/sql_state_internal.go +++ b/libpod/sql_state_internal.go @@ -37,6 +37,7 @@ const ( FROM containers INNER JOIN containerState ON containers.Id = containerState.Id ` + ExistsQuery = "SELECT 1 FROM pods WHERE Id=?;" ) // Checks that the DB configuration matches the runtime's configuration @@ -326,9 +327,7 @@ func prepareDB(db *sql.DB) (err error) { // Check if given pod exists // Internal-only version of hasPod func (s *SQLState) podExists(id string) (bool, error) { - const query = "SELECT 1 FROM pods WHERE Id=?;" - - row := s.db.QueryRow(query, id) + row := s.db.QueryRow(ExistsQuery, id) var check int err := row.Scan(&check) @@ -343,7 +342,26 @@ func (s *SQLState) podExists(id string) (bool, error) { } return true, nil +} + +// Check if given pod exists within a transaction +// Transaction-based version of podExists +func podExistsTx(id string, tx *sql.Tx) (bool, error) { + row := tx.QueryRow(ExistsQuery, id) + + var check int + err := row.Scan(&check) + if err != nil { + if err == sql.ErrNoRows { + return false, nil + } + return false, errors.Wrapf(err, "error questing database for existence of pod %s", id) + } else if check != 1 { + return false, errors.Wrapf(ErrInternal, "check digit for podExists query incorrect") + } + + return true, nil } // Get filename for OCI spec on disk @@ -717,7 +735,7 @@ func (s *SQLState) podFromScannable(row scannable) (*Pod, error) { } // Internal function for adding containers -func (s *SQLState) addContainer(ctr *Container) (err error) { +func (s *SQLState) addContainer(ctr *Container, pod *Pod) (err error) { const ( addCtr = `INSERT INTO containers VALUES ( ?, ?, ?, ?, ?, @@ -800,6 +818,19 @@ func (s *SQLState) addContainer(ctr *Container) (err error) { } }() + // First check if pod exists, if one is given + if pod != nil { + exists, err := podExistsTx(pod.ID(), tx) + if err != nil { + return err + } + + if !exists { + pod.valid = false + return errors.Wrapf(ErrNoSuchPod, "pod %s does not exist in state, cannot add container to it", pod.ID()) + } + } + // Add container to registry if _, err := tx.Exec(addRegistry, ctr.ID(), ctr.Name()); err != nil { return errors.Wrapf(err, "error adding container %s to name/ID registry", ctr.ID()) @@ -905,11 +936,13 @@ func (s *SQLState) addContainer(ctr *Container) (err error) { } // Internal functions for removing containers -func (s *SQLState) removeContainer(ctr *Container) error { +func (s *SQLState) removeContainer(ctr *Container, pod *Pod) (err error) { const ( removeCtr = "DELETE FROM containers WHERE Id=?;" removeState = "DELETE FROM containerState WHERE Id=?;" removeRegistry = "DELETE FROM registry WHERE Id=?;" + existsInPod = "SELECT 1 FROM containers WHERE Id=? AND Pod=?;" + ctrExists = "SELECT 1 FROM containers WHERE Id=?;" ) if !s.valid { @@ -930,6 +963,48 @@ func (s *SQLState) removeContainer(ctr *Container) error { } }() + if pod != nil { + // Check to see if the pod exists + exists, err := podExistsTx(pod.ID(), tx) + if err != nil { + return err + } + + if !exists { + pod.valid = false + return errors.Wrapf(ErrNoSuchPod, "pod %s does not exist in state, cannot add container to it", pod.ID()) + } + + var check int + + // Check to see if the container exists + row := tx.QueryRow(ctrExists, ctr.ID()) + err = row.Scan(&check) + if err != nil { + if err == sql.ErrNoRows { + ctr.valid = false + return errors.Wrapf(ErrNoSuchCtr, "container %s does not exist", ctr.ID()) + } + + return errors.Wrapf(err, "error querying database for container %s existence", ctr.ID()) + } else if check != 1 { + return errors.Wrapf(ErrInternal, "check digit for ctr exists query incorrect") + } + + // Check to see if the container is in the pod + row = tx.QueryRow(existsInPod, ctr.ID(), pod.ID()) + err = row.Scan(&check) + if err != nil { + if err == sql.ErrNoRows { + return errors.Wrapf(ErrNoSuchCtr, "container %s is not in pod %s", ctr.ID(), pod.ID()) + } + + return errors.Wrapf(err, "error querying database for container %s existence", ctr.ID()) + } else if check != 1 { + return errors.Wrapf(ErrInternal, "check digit for ctr exists in pod query incorrect") + } + } + // Check rows acted on for the first transaction, verify we actually removed something result, err := tx.Exec(removeCtr, ctr.ID()) if err != nil { @@ -939,6 +1014,7 @@ func (s *SQLState) removeContainer(ctr *Container) error { if err != nil { return errors.Wrapf(err, "error retrieving number of rows in transaction removing container %s", ctr.ID()) } else if rows == 0 { + ctr.valid = false return ErrNoSuchCtr } -- cgit v1.2.3-54-g00ecf