Merge pull request #94 from mrunalp/ctr_meta

Refactor to separate container IDs from container names

.tool/lint

@@ -14,6 +14,7 @@ for d in $(find . -type d -not -iwholename '*.git*' -a -not -iname '.tool' -a -n
 		--disable=gotype \
 		--disable=gas \
 		--cyclo-over=50 \
+		--dupl-threshold=100 \
 		--tests \
 		--deadline=30s "${d}"
 done

oci/oci.go

@@ -208,6 +208,7 @@ func (r *Runtime) ContainerStatus(c *Container) *ContainerState {
 
 // Container respresents a runtime container.
 type Container struct {
+	id         string
 	name       string
 	bundlePath string
 	logPath    string
@@ -228,8 +229,9 @@ type ContainerState struct {
 }
 
 // NewContainer creates a container object.
-func NewContainer(name string, bundlePath string, logPath string, labels map[string]string, sandbox string, terminal bool) (*Container, error) {
+func NewContainer(id string, name string, bundlePath string, logPath string, labels map[string]string, sandbox string, terminal bool) (*Container, error) {
 	c := &Container{
+		id:         id,
 		name:       name,
 		bundlePath: bundlePath,
 		logPath:    logPath,
@@ -245,6 +247,11 @@ func (c *Container) Name() string {
 	return c.name
 }
 
+// ID returns the id of the container.
+func (c *Container) ID() string {
+	return c.id
+}
+
 // BundlePath returns the bundlePath of the container.
 func (c *Container) BundlePath() string {
 	return c.bundlePath

server/container.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/docker/pkg/stringid"
 	"github.com/kubernetes-incubator/cri-o/oci"
 	"github.com/kubernetes-incubator/cri-o/utils"
 	"github.com/opencontainers/runtime-tools/generate"
@@ -23,6 +24,21 @@ const (
 	ContainerStateStopped = "stopped"
 )
 
+func (s *Server) generateContainerIDandName(podName string, name string, attempt uint32) (string, string, error) {
+	var (
+		err error
+		id  = stringid.GenerateNonCryptoID()
+	)
+	nameStr := fmt.Sprintf("%s-%s-%v", podName, name, attempt)
+	if name == "infra" {
+		nameStr = fmt.Sprintf("%s-%s", podName, name)
+	}
+	if name, err = s.reserveContainerName(id, nameStr); err != nil {
+		return "", "", err
+	}
+	return id, name, err
+}
+
 // CreateContainer creates a new container in specified PodSandbox
 func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerRequest) (*pb.CreateContainerResponse, error) {
 	sbID := req.GetPodSandboxId()
@@ -51,6 +67,12 @@ func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerReq
 		return nil, fmt.Errorf("CreateContainerRequest.ContainerConfig.Name is empty")
 	}
 
+	attempt := containerConfig.GetMetadata().GetAttempt()
+	containerID, containerName, err := s.generateContainerIDandName(sb.name, name, attempt)
+	if err != nil {
+		return nil, err
+	}
+
 	// containerDir is the dir for the container bundle.
 	containerDir := filepath.Join(s.runtime.ContainerDir(), name)
 
@@ -62,7 +84,7 @@ func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerReq
 		return nil, err
 	}
 
-	container, err := s.createSandboxContainer(name, sb, req.GetSandboxConfig(), containerDir, containerConfig)
+	container, err := s.createSandboxContainer(containerID, containerName, sb, req.GetSandboxConfig(), containerDir, containerConfig)
 	if err != nil {
 		return nil, err
 	}
@@ -78,11 +100,11 @@ func (s *Server) CreateContainer(ctx context.Context, req *pb.CreateContainerReq
 	s.addContainer(container)
 
 	return &pb.CreateContainerResponse{
-		ContainerId: &name,
+		ContainerId: &containerID,
 	}, nil
 }
 
-func (s *Server) createSandboxContainer(name string, sb *sandbox, SandboxConfig *pb.PodSandboxConfig, containerDir string, containerConfig *pb.ContainerConfig) (*oci.Container, error) {
+func (s *Server) createSandboxContainer(containerID string, containerName string, sb *sandbox, SandboxConfig *pb.PodSandboxConfig, containerDir string, containerConfig *pb.ContainerConfig) (*oci.Container, error) {
 	if sb == nil {
 		return nil, errors.New("createSandboxContainer needs a sandbox")
 	}
@@ -257,7 +279,7 @@ func (s *Server) createSandboxContainer(name string, sb *sandbox, SandboxConfig
 	}
 	// Join the namespace paths for the pod sandbox container.
 	podContainerName := sb.name + "-infra"
-	podInfraContainer := s.state.containers.Get(podContainerName)
+	podInfraContainer := sb.getContainer(podContainerName)
 	podInfraState := s.runtime.ContainerStatus(podInfraContainer)
 
 	logrus.Infof("pod container state %v", podInfraState)
@@ -293,7 +315,7 @@ func (s *Server) createSandboxContainer(name string, sb *sandbox, SandboxConfig
 		return nil, err
 	}
 
-	container, err := oci.NewContainer(name, containerDir, logPath, labels, sb.id, containerConfig.GetTty())
+	container, err := oci.NewContainer(containerID, containerName, containerDir, logPath, labels, sb.id, containerConfig.GetTty())
 	if err != nil {
 		return nil, err
 	}
@@ -303,18 +325,18 @@ func (s *Server) createSandboxContainer(name string, sb *sandbox, SandboxConfig
 
 // StartContainer starts the container.
 func (s *Server) StartContainer(ctx context.Context, req *pb.StartContainerRequest) (*pb.StartContainerResponse, error) {
-	containerName := req.ContainerId
+	containerID := req.GetContainerId()
 
-	if *containerName == "" {
+	if containerID == "" {
 		return nil, fmt.Errorf("container ID should not be empty")
 	}
-	c := s.state.containers.Get(*containerName)
+	c := s.state.containers.Get(containerID)
 	if c == nil {
-		return nil, fmt.Errorf("specified container not found: %s", *containerName)
+		return nil, fmt.Errorf("specified container not found: %s", containerID)
 	}
 
 	if err := s.runtime.StartContainer(c); err != nil {
-		return nil, fmt.Errorf("failed to start container %s in sandbox %s: %v", c.Name(), *containerName, err)
+		return nil, fmt.Errorf("failed to start container %s in sandbox %s: %v", c.Name(), containerID, err)
 	}
 
 	return &pb.StartContainerResponse{}, nil
@@ -322,18 +344,18 @@ func (s *Server) StartContainer(ctx context.Context, req *pb.StartContainerReque
 
 // StopContainer stops a running container with a grace period (i.e., timeout).
 func (s *Server) StopContainer(ctx context.Context, req *pb.StopContainerRequest) (*pb.StopContainerResponse, error) {
-	containerName := req.ContainerId
+	containerID := req.GetContainerId()
 
-	if *containerName == "" {
+	if containerID == "" {
 		return nil, fmt.Errorf("container ID should not be empty")
 	}
-	c := s.state.containers.Get(*containerName)
+	c := s.state.containers.Get(containerID)
 	if c == nil {
-		return nil, fmt.Errorf("specified container not found: %s", *containerName)
+		return nil, fmt.Errorf("specified container not found: %s", containerID)
 	}
 
 	if err := s.runtime.StopContainer(c); err != nil {
-		return nil, fmt.Errorf("failed to stop container %s: %v", *containerName, err)
+		return nil, fmt.Errorf("failed to stop container %s: %v", containerID, err)
 	}
 
 	return &pb.StopContainerResponse{}, nil
@@ -342,14 +364,14 @@ func (s *Server) StopContainer(ctx context.Context, req *pb.StopContainerRequest
 // RemoveContainer removes the container. If the container is running, the container
 // should be force removed.
 func (s *Server) RemoveContainer(ctx context.Context, req *pb.RemoveContainerRequest) (*pb.RemoveContainerResponse, error) {
-	containerName := req.ContainerId
+	containerID := req.GetContainerId()
 
-	if *containerName == "" {
+	if containerID == "" {
 		return nil, fmt.Errorf("container ID should not be empty")
 	}
-	c := s.state.containers.Get(*containerName)
+	c := s.state.containers.Get(containerID)
 	if c == nil {
-		return nil, fmt.Errorf("specified container not found: %s", *containerName)
+		return nil, fmt.Errorf("specified container not found: %s", containerID)
 	}
 
 	if err := s.runtime.UpdateStatus(c); err != nil {
@@ -359,19 +381,20 @@ func (s *Server) RemoveContainer(ctx context.Context, req *pb.RemoveContainerReq
 	cState := s.runtime.ContainerStatus(c)
 	if cState.Status == ContainerStateCreated || cState.Status == ContainerStateRunning {
 		if err := s.runtime.StopContainer(c); err != nil {
-			return nil, fmt.Errorf("failed to stop container %s: %v", *containerName, err)
+			return nil, fmt.Errorf("failed to stop container %s: %v", containerID, err)
 		}
 	}
 
 	if err := s.runtime.DeleteContainer(c); err != nil {
-		return nil, fmt.Errorf("failed to delete container %s: %v", *containerName, err)
+		return nil, fmt.Errorf("failed to delete container %s: %v", containerID, err)
 	}
 
-	containerDir := filepath.Join(s.runtime.ContainerDir(), *containerName)
+	containerDir := filepath.Join(s.runtime.ContainerDir(), containerID)
 	if err := os.RemoveAll(containerDir); err != nil {
-		return nil, fmt.Errorf("failed to remove container %s directory: %v", *containerName, err)
+		return nil, fmt.Errorf("failed to remove container %s directory: %v", containerID, err)
 	}
 
+	s.releaseContainerName(c.Name())
 	s.removeContainer(c)
 
 	return &pb.RemoveContainerResponse{}, nil
@@ -416,15 +439,15 @@ func (s *Server) ListContainers(ctx context.Context, req *pb.ListContainersReque
 
 // ContainerStatus returns status of the container.
 func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusRequest) (*pb.ContainerStatusResponse, error) {
-	containerName := req.ContainerId
+	containerID := req.GetContainerId()
 
-	if *containerName == "" {
+	if containerID == "" {
 		return nil, fmt.Errorf("container ID should not be empty")
 	}
-	c := s.state.containers.Get(*containerName)
+	c := s.state.containers.Get(containerID)
 
 	if c == nil {
-		return nil, fmt.Errorf("specified container not found: %s", *containerName)
+		return nil, fmt.Errorf("specified container not found: %s", containerID)
 	}
 
 	if err := s.runtime.UpdateStatus(c); err != nil {
@@ -433,7 +456,7 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq
 
 	csr := &pb.ContainerStatusResponse{
 		Status: &pb.ContainerStatus{
-			Id: containerName,
+			Id: &containerID,
 		},
 	}
 

server/sandbox.go

@@ -139,12 +139,14 @@ func (s *Server) RunPodSandbox(ctx context.Context, req *pb.RunPodSandboxRequest
 	if err != nil {
 		return nil, err
 	}
+
+	containerID, containerName, err := s.generateContainerIDandName(name, "infra", 0)
 	g.AddAnnotation("ocid/labels", string(labelsJSON))
 	g.AddAnnotation("ocid/annotations", string(annotationsJSON))
 	g.AddAnnotation("ocid/log_path", logDir)
 	g.AddAnnotation("ocid/name", name)
-	containerName := name + "-infra"
 	g.AddAnnotation("ocid/container_name", containerName)
+	g.AddAnnotation("ocid/container_id", containerID)
 	s.addSandbox(&sandbox{
 		id:          id,
 		name:        name,
@@ -202,7 +204,7 @@ func (s *Server) RunPodSandbox(ctx context.Context, req *pb.RunPodSandboxRequest
 		}
 	}
 
-	container, err := oci.NewContainer(containerName, podSandboxDir, podSandboxDir, labels, id, false)
+	container, err := oci.NewContainer(containerID, containerName, podSandboxDir, podSandboxDir, labels, id, false)
 	if err != nil {
 		return nil, err
 	}
@@ -331,6 +333,7 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR
 			return nil, fmt.Errorf("failed to remove container %s directory: %v", c.Name(), err)
 		}
 
+		s.releaseContainerName(c.Name())
 		s.removeContainer(c)
 	}
 
@@ -339,6 +342,7 @@ func (s *Server) RemovePodSandbox(ctx context.Context, req *pb.RemovePodSandboxR
 	if err := os.RemoveAll(podSandboxDir); err != nil {
 		return nil, fmt.Errorf("failed to remove sandbox %s directory: %v", sandboxID, err)
 	}
+	s.releaseContainerName(podInfraContainer.Name())
 	s.removeContainer(podInfraContainer)
 
 	s.releasePodName(sb.name)

server/server.go

@@ -33,6 +33,8 @@ type Server struct {
 	netPlugin    ocicni.CNIPlugin
 	podNameIndex *registrar.Registrar
 	podIDIndex   *truncindex.TruncIndex
+	ctrNameIndex *registrar.Registrar
+	ctrIDIndex   *truncindex.TruncIndex
 }
 
 func (s *Server) loadSandbox(id string) error {
@@ -61,7 +63,7 @@ func (s *Server) loadSandbox(id string) error {
 		containers: oci.NewMemoryStore(),
 	})
 	sandboxPath := filepath.Join(s.sandboxDir, id)
-	scontainer, err := oci.NewContainer(m.Annotations["ocid/container_name"], sandboxPath, sandboxPath, labels, id, false)
+	scontainer, err := oci.NewContainer(m.Annotations["ocid/container_id"], m.Annotations["ocid/container_name"], sandboxPath, sandboxPath, labels, id, false)
 	if err != nil {
 		return err
 	}
@@ -107,6 +109,25 @@ func (s *Server) releasePodName(name string) {
 	s.podNameIndex.Release(name)
 }
 
+func (s *Server) reserveContainerName(id, name string) (string, error) {
+	if err := s.ctrNameIndex.Reserve(name, id); err != nil {
+		if err == registrar.ErrNameReserved {
+			id, err := s.ctrNameIndex.Get(name)
+			if err != nil {
+				logrus.Warnf("name %s already reserved for %s", name, id)
+				return "", err
+			}
+			return "", fmt.Errorf("conflict, name %s already reserved", name)
+		}
+		return "", fmt.Errorf("error reserving name %s", name)
+	}
+	return name, nil
+}
+
+func (s *Server) releaseContainerName(name string) {
+	s.ctrNameIndex.Release(name)
+}
+
 // New creates a new Server with options provided
 func New(runtimePath, root, sandboxDir, containerDir, conmonPath, pausePath string) (*Server, error) {
 	// TODO: This will go away later when we have wrapper process or systemd acting as
@@ -146,8 +167,12 @@ func New(runtimePath, root, sandboxDir, containerDir, conmonPath, pausePath stri
 			containers: containers,
 		},
 	}
+
 	s.podIDIndex = truncindex.NewTruncIndex([]string{})
 	s.podNameIndex = registrar.NewRegistrar()
+	s.ctrIDIndex = truncindex.NewTruncIndex([]string{})
+	s.ctrNameIndex = registrar.NewRegistrar()
+
 	if err := s.restore(); err != nil {
 		logrus.Warnf("couldn't restore: %v", err)
 	}
@@ -192,13 +217,13 @@ func (s *Server) addContainer(c *oci.Container) {
 	sandbox := s.state.sandboxes[c.Sandbox()]
 	// TODO(runcom): handle !ok above!!! otherwise it panics!
 	sandbox.addContainer(c)
-	s.state.containers.Add(c.Name(), c)
+	s.state.containers.Add(c.ID(), c)
 	s.stateLock.Unlock()
 }
 
-func (s *Server) getContainer(name string) *oci.Container {
+func (s *Server) getContainer(id string) *oci.Container {
 	s.stateLock.Lock()
-	c := s.state.containers.Get(name)
+	c := s.state.containers.Get(id)
 	s.stateLock.Unlock()
 	return c
 }
@@ -207,6 +232,6 @@ func (s *Server) removeContainer(c *oci.Container) {
 	s.stateLock.Lock()
 	sandbox := s.state.sandboxes[c.Sandbox()]
 	sandbox.removeContainer(c)
-	s.state.containers.Delete(c.Name())
+	s.state.containers.Delete(c.ID())
 	s.stateLock.Unlock()
 }