From 16fb71fea06d619018f921ff328c5ff155111046 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 7 Apr 2017 11:58:59 -0400 Subject: [PATCH] Make state and sandbox operations atomic. Minor refactors. Small API change to state.Store to remove unnecessary parameter from AddContainer. Signed-off-by: Matthew Heon --- server/server.go | 2 +- server/state/in_memory_state.go | 195 +++++++++++++++++++++++--------- server/state/state_store.go | 2 +- 3 files changed, 145 insertions(+), 54 deletions(-) diff --git a/server/server.go b/server/server.go index 373842c8..aac056fc 100644 --- a/server/server.go +++ b/server/server.go @@ -576,7 +576,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/in_memory_state.go b/server/state/in_memory_state.go index 48cd6df2..21c914de 100644 --- a/server/state/in_memory_state.go +++ b/server/state/in_memory_state.go @@ -4,6 +4,7 @@ import ( "fmt" "sync" + "github.com/Sirupsen/logrus" "github.com/docker/docker/pkg/registrar" "github.com/docker/docker/pkg/truncindex" "github.com/kubernetes-incubator/cri-o/oci" @@ -38,35 +39,52 @@ func NewInMemoryState() Store { } // AddSandbox adds a sandbox and any containers in it to the state -func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) error { +func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) (err 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()) } - // We shouldn't share ID with any containers, either + // We shouldn't share ID with any containers // Our pod infra container will share our ID and we don't want it to conflict with anything if ctrCheck := s.containers.Get(sandbox.ID()); ctrCheck != nil { return fmt.Errorf("requested sandbox ID %v conflicts with existing container ID", sandbox.ID()) } - s.sandboxes[sandbox.ID()] = sandbox - if err := s.podNameIndex.Reserve(sandbox.Name(), sandbox.ID()); err != nil { - return fmt.Errorf("error registering sandbox name: %v", err) - } - if err := s.podIDIndex.Add(sandbox.ID()); err != nil { - return fmt.Errorf("error registering sandbox ID: %v", err) + if err := s.addSandboxMappings(sandbox); err != nil { + return err } + defer func() { + if err != nil { + if err2 := s.deleteSandboxMappings(sandbox); err2 != nil { + logrus.Errorf("Error removing mappings for incompletely-added sandbox %s: %v", sandbox.ID(), err) + } + } + }() // If there are containers in the sandbox add them to the mapping - containers := sandbox.Containers() - for _, ctr := range containers { + addedCtrs := make([]*oci.Container, 0, len(sandbox.Containers())) + for _, ctr := range sandbox.Containers() { if err := s.addContainerMappings(ctr, true); err != nil { return fmt.Errorf("error adding container %v mappings in sandbox %v", ctr.ID(), sandbox.ID()) } + addedCtrs = append(addedCtrs, ctr) } + defer func() { + if err != nil { + for _, ctr := range addedCtrs { + if err2 := s.deleteContainerMappings(ctr, true); err2 != nil { + logrus.Errorf("Error removing container %s mappings: %v", ctr.ID(), err2) + } + } + } + }() // Add the pod infrastructure container to mappings // TODO: Right now, we don't add it to the all containers listing. We may want to change this. @@ -77,6 +95,24 @@ func (s *InMemoryState) AddSandbox(sandbox *sandbox.Sandbox) error { return nil } +// Add sandbox name, ID to appropriate mappings +func (s *InMemoryState) addSandboxMappings(sb *sandbox.Sandbox) error { + if _, exist := s.sandboxes[sb.ID()]; exist { + return fmt.Errorf("sandbox with ID %s already exists in sandboxes map", sb.ID()) + } + + if err := s.podNameIndex.Reserve(sb.Name(), sb.ID()); err != nil { + return fmt.Errorf("error registering sandbox name: %v", err) + } + if err := s.podIDIndex.Add(sb.ID()); err != nil { + s.podNameIndex.Release(sb.Name()) + return fmt.Errorf("error registering sandbox ID: %v", err) + } + s.sandboxes[sb.ID()] = sb + + return nil +} + // HasSandbox determines if a given sandbox exists in the state func (s *InMemoryState) HasSandbox(id string) bool { s.lock.Lock() @@ -88,39 +124,68 @@ func (s *InMemoryState) HasSandbox(id string) bool { } // DeleteSandbox removes a sandbox from the state -func (s *InMemoryState) DeleteSandbox(id string) error { +func (s *InMemoryState) DeleteSandbox(id string) (err error) { s.lock.Lock() 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() - containers := s.sandboxes[id].Containers() - infraContainer := s.sandboxes[id].InfraContainer() + sb := s.sandboxes[id] - delete(s.sandboxes, id) - s.podNameIndex.Release(name) - if err := s.podIDIndex.Delete(id); err != nil { - return fmt.Errorf("error unregistering sandbox ID: %v", err) + if err := s.deleteSandboxMappings(sb); err != nil { + return err } + defer func() { + if err != nil { + if err2 := s.addSandboxMappings(sb); err2 != nil { + logrus.Errorf("Error re-adding sandbox mappings: %v", err2) + } + } + }() + removedCtrs := make([]*oci.Container, 0, len(sb.Containers())) // If there are containers left in the sandbox delete them from the mappings - for _, ctr := range containers { + for _, ctr := range sb.Containers() { if err := s.deleteContainerMappings(ctr, true); err != nil { return fmt.Errorf("error removing container %v mappings: %v", ctr.ID(), err) } + removedCtrs = append(removedCtrs, ctr) } + defer func() { + if err != nil { + for _, ctr := range removedCtrs { + if err2 := s.addContainerMappings(ctr, true); err2 != nil { + logrus.Errorf("Error re-adding mappings for container %s: %v", ctr.ID(), err2) + } + } + } + }() // Delete infra container from mappings - if err := s.deleteContainerMappings(infraContainer, false); err != nil { - return fmt.Errorf("error removing infra container %v from mappings: %v", infraContainer.ID(), err) + if err := s.deleteContainerMappings(sb.InfraContainer(), false); err != nil { + return fmt.Errorf("error removing infra container %v from mappings: %v", sb.InfraContainer().ID(), err) } return nil } +// Remove sandbox name, ID to appropriate mappings +func (s *InMemoryState) deleteSandboxMappings(sb *sandbox.Sandbox) error { + if _, exist := s.sandboxes[sb.ID()]; !exist { + return fmt.Errorf("sandbox with ID %s does not exist in sandboxes map", sb.ID()) + } + + if err := s.podIDIndex.Delete(sb.ID()); err != nil { + return fmt.Errorf("error unregistering sandbox %s: %v", sb.ID(), err) + } + delete(s.sandboxes, sb.ID()) + s.podNameIndex.Release(sb.Name()) + + return nil +} + // GetSandbox returns a sandbox given its full ID func (s *InMemoryState) GetSandbox(id string) (*sandbox.Sandbox, error) { s.lock.Lock() @@ -128,7 +193,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 +211,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 +231,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,26 +252,34 @@ 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", c.Sandbox()) + } + + if err := s.addContainerMappings(c, true); err != nil { + return err } sandbox.AddContainer(c) - return s.addContainerMappings(c, true) + return nil } // Add container ID, Name and Sandbox mappings @@ -218,18 +291,18 @@ func (s *InMemoryState) addContainerMappings(c *oci.Container, addToContainers b // TODO: if not a pod infra container, check if it conflicts with existing sandbox ID? // Does this matter? + if err := s.ctrNameIndex.Reserve(c.Name(), c.ID()); err != nil { + return fmt.Errorf("error registering name for container %s: %v", c.ID(), err) + } + + if err := s.ctrIDIndex.Add(c.ID()); err != nil { + s.ctrNameIndex.Release(c.ID()) + return fmt.Errorf("error registering ID for container %s: %v", c.ID(), err) + } + if addToContainers { s.containers.Add(c.ID(), c) } - if err := s.ctrNameIndex.Reserve(c.Name(), c.ID()); err != nil { - s.containers.Delete(c.ID()) - return fmt.Errorf("error registering container name: %v", err) - } - if err := s.ctrIDIndex.Add(c.ID()); err != nil { - s.containers.Delete(c.ID()) - s.ctrNameIndex.Release(c.ID()) - return fmt.Errorf("error registering container ID: %v", err) - } return nil } @@ -256,17 +329,21 @@ 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) + } + + if err := s.deleteContainerMappings(ctr, true); err != nil { + return nil } sandbox.RemoveContainer(id) - return s.deleteContainerMappings(ctr, true) + return nil } // Deletes container from the ID and Name mappings and optionally from the global containers list @@ -275,13 +352,15 @@ func (s *InMemoryState) deleteContainerMappings(ctr *oci.Container, deleteFromCo return fmt.Errorf("container ID %v does not exist in containers store", ctr.ID()) } + if err := s.ctrIDIndex.Delete(ctr.ID()); err != nil { + return fmt.Errorf("error unregistering container ID for %s: %v", ctr.ID(), err) + } + + s.ctrNameIndex.Release(ctr.Name()) + if deleteFromContainers { s.containers.Delete(ctr.ID()) } - s.ctrNameIndex.Release(ctr.Name()) - if err := s.ctrIDIndex.Delete(ctr.ID()); err != nil { - return fmt.Errorf("error unregistering container ID: %v", err) - } return nil } @@ -302,7 +381,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 @@ -346,7 +425,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()) @@ -357,13 +436,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