From b14e2d29376c2ea86907f16fd38d48ab4460ade9 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 7 Apr 2017 11:58:59 -0400 Subject: [PATCH] Small refactor on AddContainer. Refactor common errors. Remove sandboxID parameter from AddContainer, we can get it from the container anyways. Add nil checks to AddSandbox and AddContainer to prevent panics. Refactor common errors into functions to reduce code duplication. Signed-off-by: Matthew Heon --- server/server.go | 2 +- server/state/file_state.go | 55 +++++++++++++++++---------------- server/state/in_memory_state.go | 50 ++++++++++++++++++++---------- server/state/state_store.go | 2 +- 4 files changed, 64 insertions(+), 45 deletions(-) diff --git a/server/server.go b/server/server.go index 3b4165a9..8e53b001 100644 --- a/server/server.go +++ b/server/server.go @@ -134,7 +134,7 @@ func (s *Server) removeSandbox(id string) error { } func (s *Server) addContainer(c *oci.Container) error { - return s.state.AddContainer(c, c.Sandbox()) + return s.state.AddContainer(c) } func (s *Server) getContainer(id string) (*oci.Container, error) { diff --git a/server/state/file_state.go b/server/state/file_state.go index f42f581d..69f5f735 100644 --- a/server/state/file_state.go +++ b/server/state/file_state.go @@ -128,7 +128,6 @@ func (s *FileState) forceSyncWithDisk() error { } for _, sb := range oldSandboxes { if !newState.HasSandbox(sb.ID()) && sb.NetNs() != nil { - if err := sb.NetNsRemove(); err != nil { return fmt.Errorf("error closing network namespace of sandbox %v: %v", sb.ID(), err) } @@ -656,7 +655,7 @@ func NewFileState(statePath string, runtime *oci.Runtime) (Store, error) { // Perform an initial sync with the disk if err := state.forceSyncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return state, nil @@ -668,18 +667,18 @@ func (s *FileState) AddSandbox(sb *sandbox.Sandbox) error { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return fmt.Errorf("error syncing with on-disk state: %v", err) + return errSyncWithDisk(err) } if err := s.memoryState.AddSandbox(sb); err != nil { - return fmt.Errorf("error adding sandbox %v to in-memory state: %v", sb.ID(), err) + return err } if err := s.putSandboxToDisk(sb); err != nil { if err2 := s.memoryState.DeleteSandbox(sb.ID()); err2 != nil { logrus.Errorf("error removing sandbox %s from in-memory state, states are desynced: %v", sb.ID(), err) } - return err + return fmt.Errorf("error saving sandbox %s to disk: %v", sb.ID(), err) } return nil @@ -705,15 +704,15 @@ func (s *FileState) DeleteSandbox(id string) error { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return fmt.Errorf("error syncing with on-disk state: %v", err) + return errSyncWithDisk(err) } if err := s.memoryState.DeleteSandbox(id); err != nil { - return fmt.Errorf("error removing sandbox %v from in-memory state: %v", id, err) + return err } if err := s.removeSandboxFromDisk(id); err != nil { - return err + return fmt.Errorf("error removing sandbox ID %s from disk: %v", id, err) } return nil @@ -725,7 +724,7 @@ func (s *FileState) GetSandbox(id string) (*sandbox.Sandbox, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.GetSandbox(id) @@ -737,7 +736,7 @@ func (s *FileState) LookupSandboxByName(name string) (*sandbox.Sandbox, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.LookupSandboxByName(name) @@ -750,7 +749,7 @@ func (s *FileState) LookupSandboxByID(id string) (*sandbox.Sandbox, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.LookupSandboxByID(id) @@ -762,30 +761,30 @@ func (s *FileState) GetAllSandboxes() ([]*sandbox.Sandbox, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.GetAllSandboxes() } // AddContainer adds a container to the state -func (s *FileState) AddContainer(c *oci.Container, sandboxID string) error { +func (s *FileState) AddContainer(c *oci.Container) error { s.lockfile.Lock() defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return fmt.Errorf("error syncing with on-disk state: %v", err) + return errSyncWithDisk(err) } - if err := s.memoryState.AddContainer(c, sandboxID); err != nil { - return fmt.Errorf("error adding container %v to in-memory state: %v", c.ID(), err) + if err := s.memoryState.AddContainer(c); err != nil { + return err } if err := s.putContainerToDisk(c, true); err != nil { - if err2 := s.memoryState.DeleteContainer(c.ID(), sandboxID); err2 != nil { + if err2 := s.memoryState.DeleteContainer(c.ID(), c.Sandbox()); err2 != nil { logrus.Errorf("error removing container %v from in-memory state, states are desynced: %v", c.ID(), err2) } - return err + return fmt.Errorf("error adding container %s to on-disk state: %v", c.ID(), err) } return nil @@ -811,15 +810,15 @@ func (s *FileState) DeleteContainer(id, sandboxID string) error { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return fmt.Errorf("error syncing with on-disk state: %v", err) + return errSyncWithDisk(err) } if err := s.memoryState.DeleteContainer(id, sandboxID); err != nil { - return fmt.Errorf("error removing container %v from in-memory state: %v", id, err) + return err } if err := s.removeContainerFromDisk(id, sandboxID); err != nil { - return err + return fmt.Errorf("error removing container %s from on-disk state: %v", id, err) } return nil @@ -831,7 +830,7 @@ func (s *FileState) GetContainer(id, sandboxID string) (*oci.Container, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.GetContainer(id, sandboxID) @@ -843,7 +842,7 @@ func (s *FileState) GetContainerSandbox(id string) (string, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return "", fmt.Errorf("error syncing with on-disk state: %v", err) + return "", errSyncWithDisk(err) } return s.memoryState.GetContainerSandbox(id) @@ -855,7 +854,7 @@ func (s *FileState) LookupContainerByName(name string) (*oci.Container, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.LookupContainerByName(name) @@ -868,7 +867,7 @@ func (s *FileState) LookupContainerByID(id string) (*oci.Container, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.LookupContainerByID(id) @@ -881,8 +880,12 @@ func (s *FileState) GetAllContainers() ([]*oci.Container, error) { defer s.lockfile.Unlock() if err := s.syncWithDisk(); err != nil { - return nil, fmt.Errorf("error syncing with on-disk state: %v", err) + return nil, errSyncWithDisk(err) } return s.memoryState.GetAllContainers() } + +func errSyncWithDisk(err error) error { + return fmt.Errorf("error syncing on-disk state: %v", err) +} diff --git a/server/state/in_memory_state.go b/server/state/in_memory_state.go index c563485f..b2d8d798 100644 --- a/server/state/in_memory_state.go +++ b/server/state/in_memory_state.go @@ -42,6 +42,10 @@ func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) error { s.lock.Lock() defer s.lock.Unlock() + if sandbox == nil { + return fmt.Errorf("nil passed as sandbox to AddSandbox") + } + if _, exist := s.sandboxes[sandbox.ID()]; exist { return fmt.Errorf("sandbox with ID %v already exists", sandbox.ID()) } @@ -93,7 +97,7 @@ func (s *InMemoryState) DeleteSandbox(id string) error { defer s.lock.Unlock() if _, exist := s.sandboxes[id]; !exist { - return fmt.Errorf("no sandbox with ID %v exists, cannot delete", id) + return errNoSuchSandbox(id) } name := s.sandboxes[id].Name() @@ -128,7 +132,7 @@ func (s *InMemoryState) GetSandbox(id string) (*sandbox.Sandbox, error) { sandbox, ok := s.sandboxes[id] if !ok { - return nil, fmt.Errorf("no sandbox with id %v exists", id) + return nil, errNoSuchSandbox(id) } return sandbox, nil @@ -146,7 +150,7 @@ func (s *InMemoryState) LookupSandboxByName(name string) (*sandbox.Sandbox, erro sandbox, ok := s.sandboxes[id] if !ok { - // This should never happen + // This should never happen - our internal state must be desynced return nil, fmt.Errorf("cannot find sandbox %v in sandboxes map", id) } @@ -166,7 +170,7 @@ func (s *InMemoryState) LookupSandboxByID(id string) (*sandbox.Sandbox, error) { sandbox, ok := s.sandboxes[fullID] if !ok { - // This should never happen + // This should never happen, internal state must be desynced return nil, fmt.Errorf("cannot find sandbox %v in sandboxes map", fullID) } @@ -187,25 +191,25 @@ func (s *InMemoryState) GetAllSandboxes() ([]*sandbox.Sandbox, error) { } // AddContainer adds a single container to a given sandbox in the state -func (s *InMemoryState) AddContainer(c *oci.Container, sandboxID string) error { +func (s *InMemoryState) AddContainer(c *oci.Container) error { s.lock.Lock() defer s.lock.Unlock() - if c.Sandbox() != sandboxID { - return fmt.Errorf("cannot add container to sandbox %v as it is part of sandbox %v", sandboxID, c.Sandbox()) + if c == nil { + return fmt.Errorf("nil passed as container to AddContainer") } - sandbox, ok := s.sandboxes[sandboxID] + sandbox, ok := s.sandboxes[c.Sandbox()] if !ok { - return fmt.Errorf("sandbox with ID %v does not exist, cannot add container", sandboxID) + return errNoSuchSandbox(c.Sandbox()) } if ctr := sandbox.GetContainer(c.ID()); ctr != nil { - return fmt.Errorf("container with ID %v already exists in sandbox %v", c.ID(), sandboxID) + return fmt.Errorf("container with ID %v already exists in sandbox %v", c.ID(), c.Sandbox()) } if sandbox.InfraContainer().ID() == c.ID() { - return fmt.Errorf("container is infra container of sandbox %s, refusing to add to containers list", sandboxID) + return fmt.Errorf("container is infra container of sandbox %s, refusing to add to containers list", c.Sandbox()) } sandbox.AddContainer(c) @@ -260,12 +264,12 @@ func (s *InMemoryState) DeleteContainer(id, sandboxID string) error { sandbox, ok := s.sandboxes[sandboxID] if !ok { - return fmt.Errorf("sandbox with ID %v does not exist", sandboxID) + return errNoSuchSandbox(sandboxID) } ctr := sandbox.GetContainer(id) if ctr == nil { - return fmt.Errorf("sandbox %v has no container with ID %v", sandboxID, id) + return errNoSuchContainerInSandbox(id, sandboxID) } sandbox.RemoveContainer(id) @@ -306,7 +310,7 @@ func (s *InMemoryState) GetContainerSandbox(id string) (string, error) { ctr := s.containers.Get(id) if ctr == nil { - return "", fmt.Errorf("no container with ID %v found", id) + return "", errNoSuchContainer(id) } return ctr.Sandbox(), nil @@ -350,7 +354,7 @@ func (s *InMemoryState) GetAllContainers() ([]*oci.Container, error) { func (s *InMemoryState) getContainer(id string) (*oci.Container, error) { ctr := s.containers.Get(id) if ctr == nil { - return nil, fmt.Errorf("cannot find container with ID %v", id) + return nil, errNoSuchContainer(id) } return s.getContainerFromSandbox(id, ctr.Sandbox()) @@ -361,13 +365,25 @@ func (s *InMemoryState) getContainer(id string) (*oci.Container, error) { func (s *InMemoryState) getContainerFromSandbox(id, sandboxID string) (*oci.Container, error) { sandbox, ok := s.sandboxes[sandboxID] if !ok { - return nil, fmt.Errorf("sandbox with ID %v does not exist", sandboxID) + return nil, errNoSuchSandbox(sandboxID) } ctr := sandbox.GetContainer(id) if ctr == nil { - return nil, fmt.Errorf("cannot find container %v in sandbox %v", id, sandboxID) + return nil, errNoSuchContainerInSandbox(id, sandboxID) } return ctr, nil } + +func errNoSuchSandbox(id string) error { + return fmt.Errorf("no sandbox with ID %s found", id) +} + +func errNoSuchContainer(id string) error { + return fmt.Errorf("no container with ID %s found", id) +} + +func errNoSuchContainerInSandbox(id string, sandboxID string) error { + return fmt.Errorf("no container with ID %s in sandbox %s", id, sandboxID) +} diff --git a/server/state/state_store.go b/server/state/state_store.go index d7662285..f96ce0de 100644 --- a/server/state/state_store.go +++ b/server/state/state_store.go @@ -11,7 +11,7 @@ type Store interface { HasSandbox(id string) bool DeleteSandbox(id string) error // These should modify the associated sandbox without prompting - AddContainer(c *oci.Container, sandboxID string) error + AddContainer(c *oci.Container) error HasContainer(id, sandboxID string) bool DeleteContainer(id, sandboxID string) error // These two require full, explicit ID