vbatts/cri-o
1
0
Fork 0
Code Issues Pull requests Releases Wiki Activity

Small refactor on AddContainer. Refactor common errors.

Browse source 
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 <mheon@redhat.com>
This commit is contained in:
Matthew Heon 2017-04-07 11:58:59 -04:00
parent f25187d3f2
commit b14e2d2937
4 changed files with 64 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

2
server/server.go
View file

@ -134,7 +134,7 @@ func (s *Server) removeSandbox(id string) error {
} }
func (s *Server) addContainer(c *oci.Container) 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) { func (s *Server) getContainer(id string) (*oci.Container, error) {

55
server/state/file_state.go
View file

@ -128,7 +128,6 @@ func (s *FileState) forceSyncWithDisk() error {
} }
for _, sb := range oldSandboxes { for _, sb := range oldSandboxes {
if !newState.HasSandbox(sb.ID()) && sb.NetNs() != nil { if !newState.HasSandbox(sb.ID()) && sb.NetNs() != nil {
if err := sb.NetNsRemove(); err != nil { if err := sb.NetNsRemove(); err != nil {
return fmt.Errorf("error closing network namespace of sandbox %v: %v", sb.ID(), err) 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 // Perform an initial sync with the disk
if err := state.forceSyncWithDisk(); err != nil { if err := state.forceSyncWithDisk(); err != nil {
return nil, fmt.Errorf("error syncing on-disk state: %v", err) return nil, errSyncWithDisk(err)
} }
return state, nil return state, nil
@ -668,18 +667,18 @@ func (s *FileState) AddSandbox(sb *sandbox.Sandbox) error {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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 { 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 err := s.putSandboxToDisk(sb); err != nil {
if err2 := s.memoryState.DeleteSandbox(sb.ID()); err2 != 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) 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 return nil
@ -705,15 +704,15 @@ func (s *FileState) DeleteSandbox(id string) error {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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 { 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 { if err := s.removeSandboxFromDisk(id); err != nil {
return err return fmt.Errorf("error removing sandbox ID %s from disk: %v", id, err)
} }
return nil return nil
@ -725,7 +724,7 @@ func (s *FileState) GetSandbox(id string) (*sandbox.Sandbox, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.GetSandbox(id)
@ -737,7 +736,7 @@ func (s *FileState) LookupSandboxByName(name string) (*sandbox.Sandbox, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.LookupSandboxByName(name)
@ -750,7 +749,7 @@ func (s *FileState) LookupSandboxByID(id string) (*sandbox.Sandbox, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.LookupSandboxByID(id)
@ -762,30 +761,30 @@ func (s *FileState) GetAllSandboxes() ([]*sandbox.Sandbox, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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() return s.memoryState.GetAllSandboxes()
} }
// AddContainer adds a container to the state // 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() s.lockfile.Lock()
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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 { if err := s.memoryState.AddContainer(c); err != nil {
return fmt.Errorf("error adding container %v to in-memory state: %v", c.ID(), err) return err
} }
if err := s.putContainerToDisk(c, true); err != nil { 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) 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 return nil
@ -811,15 +810,15 @@ func (s *FileState) DeleteContainer(id, sandboxID string) error {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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 { 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 { 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 return nil
@ -831,7 +830,7 @@ func (s *FileState) GetContainer(id, sandboxID string) (*oci.Container, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.GetContainer(id, sandboxID)
@ -843,7 +842,7 @@ func (s *FileState) GetContainerSandbox(id string) (string, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.GetContainerSandbox(id)
@ -855,7 +854,7 @@ func (s *FileState) LookupContainerByName(name string) (*oci.Container, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.LookupContainerByName(name)
@ -868,7 +867,7 @@ func (s *FileState) LookupContainerByID(id string) (*oci.Container, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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) return s.memoryState.LookupContainerByID(id)
@ -881,8 +880,12 @@ func (s *FileState) GetAllContainers() ([]*oci.Container, error) {
defer s.lockfile.Unlock() defer s.lockfile.Unlock()
if err := s.syncWithDisk(); err != nil { 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() return s.memoryState.GetAllContainers()
} }
func errSyncWithDisk(err error) error {
return fmt.Errorf("error syncing on-disk state: %v", err)
}

50
server/state/in_memory_state.go
View file

@ -42,6 +42,10 @@ func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) error {
s.lock.Lock() s.lock.Lock()
defer s.lock.Unlock() defer s.lock.Unlock()
if sandbox == nil {
return fmt.Errorf("nil passed as sandbox to AddSandbox")
}
if _, exist := s.sandboxes[sandbox.ID()]; exist { if _, exist := s.sandboxes[sandbox.ID()]; exist {
return fmt.Errorf("sandbox with ID %v already exists", sandbox.ID()) 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() defer s.lock.Unlock()
if _, exist := s.sandboxes[id]; !exist { 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() name := s.sandboxes[id].Name()
@ -128,7 +132,7 @@ func (s *InMemoryState) GetSandbox(id string) (*sandbox.Sandbox, error) {
sandbox, ok := s.sandboxes[id] sandbox, ok := s.sandboxes[id]
if !ok { if !ok {
return nil, fmt.Errorf("no sandbox with id %v exists", id) return nil, errNoSuchSandbox(id)
} }
return sandbox, nil return sandbox, nil
@ -146,7 +150,7 @@ func (s *InMemoryState) LookupSandboxByName(name string) (*sandbox.Sandbox, erro
sandbox, ok := s.sandboxes[id] sandbox, ok := s.sandboxes[id]
if !ok { 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) 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] sandbox, ok := s.sandboxes[fullID]
if !ok { 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) 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 // 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() s.lock.Lock()
defer s.lock.Unlock() defer s.lock.Unlock()
if c.Sandbox() != sandboxID { if c == nil {
return fmt.Errorf("cannot add container to sandbox %v as it is part of sandbox %v", sandboxID, c.Sandbox()) return fmt.Errorf("nil passed as container to AddContainer")
} }
sandbox, ok := s.sandboxes[sandboxID] sandbox, ok := s.sandboxes[c.Sandbox()]
if !ok { 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 { 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() { 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) sandbox.AddContainer(c)
@ -260,12 +264,12 @@ func (s *InMemoryState) DeleteContainer(id, sandboxID string) error {
sandbox, ok := s.sandboxes[sandboxID] sandbox, ok := s.sandboxes[sandboxID]
if !ok { if !ok {
return fmt.Errorf("sandbox with ID %v does not exist", sandboxID) return errNoSuchSandbox(sandboxID)
} }
ctr := sandbox.GetContainer(id) ctr := sandbox.GetContainer(id)
if ctr == nil { if ctr == nil {
return fmt.Errorf("sandbox %v has no container with ID %v", sandboxID, id) return errNoSuchContainerInSandbox(id, sandboxID)
} }
sandbox.RemoveContainer(id) sandbox.RemoveContainer(id)
@ -306,7 +310,7 @@ func (s *InMemoryState) GetContainerSandbox(id string) (string, error) {
ctr := s.containers.Get(id) ctr := s.containers.Get(id)
if ctr == nil { if ctr == nil {
return "", fmt.Errorf("no container with ID %v found", id) return "", errNoSuchContainer(id)
} }
return ctr.Sandbox(), nil return ctr.Sandbox(), nil
@ -350,7 +354,7 @@ func (s *InMemoryState) GetAllContainers() ([]*oci.Container, error) {
func (s *InMemoryState) getContainer(id string) (*oci.Container, error) { func (s *InMemoryState) getContainer(id string) (*oci.Container, error) {
ctr := s.containers.Get(id) ctr := s.containers.Get(id)
if ctr == nil { if ctr == nil {
return nil, fmt.Errorf("cannot find container with ID %v", id) return nil, errNoSuchContainer(id)
} }
return s.getContainerFromSandbox(id, ctr.Sandbox()) 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) { func (s *InMemoryState) getContainerFromSandbox(id, sandboxID string) (*oci.Container, error) {
sandbox, ok := s.sandboxes[sandboxID] sandbox, ok := s.sandboxes[sandboxID]
if !ok { if !ok {
return nil, fmt.Errorf("sandbox with ID %v does not exist", sandboxID) return nil, errNoSuchSandbox(sandboxID)
} }
ctr := sandbox.GetContainer(id) ctr := sandbox.GetContainer(id)
if ctr == nil { if ctr == nil {
return nil, fmt.Errorf("cannot find container %v in sandbox %v", id, sandboxID) return nil, errNoSuchContainerInSandbox(id, sandboxID)
} }
return ctr, nil 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)
}

2
server/state/state_store.go
View file

@ -11,7 +11,7 @@ type Store interface {
HasSandbox(id string) bool HasSandbox(id string) bool
DeleteSandbox(id string) error DeleteSandbox(id string) error
// These should modify the associated sandbox without prompting // 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 HasContainer(id, sandboxID string) bool
DeleteContainer(id, sandboxID string) error DeleteContainer(id, sandboxID string) error
// These two require full, explicit ID // These two require full, explicit ID