From b2c383892c79a525039dcb22f0a5218de53e3a21 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 4 Oct 2016 14:17:15 -0700 Subject: [PATCH 1/5] Add id field to container Signed-off-by: Mrunal Patel --- oci/oci.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/oci/oci.go b/oci/oci.go index 66c9dc86..7a449d09 100644 --- a/oci/oci.go +++ b/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 @@ -245,6 +246,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 From 1783342841d0b875f7f8d0668eb1ceae7964becf Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 4 Oct 2016 15:59:46 -0700 Subject: [PATCH 2/5] Increase the duplicate threshold Signed-off-by: Mrunal Patel --- .tool/lint | 1 + 1 file changed, 1 insertion(+) diff --git a/.tool/lint b/.tool/lint index c629f766..06dae83f 100755 --- a/.tool/lint +++ b/.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 From 3e1954923200c90578f9ea2da85f1cc443a6fce0 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 4 Oct 2016 16:00:04 -0700 Subject: [PATCH 3/5] Add name and id indexes for containers Signed-off-by: Mrunal Patel --- server/server.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/server/server.go b/server/server.go index 5c62f554..18ca5692 100644 --- a/server/server.go +++ b/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 { @@ -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) } From 484719c8fe6167974fb475f8a4e574afce0bf113 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 4 Oct 2016 16:01:22 -0700 Subject: [PATCH 4/5] Add a function to generate container id and name Signed-off-by: Mrunal Patel --- server/container.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/server/container.go b/server/container.go index b3d274f8..efd0a332 100644 --- a/server/container.go +++ b/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() From 0482a4281a2a0af6d599b590d54b386e971e9258 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 4 Oct 2016 16:50:29 -0700 Subject: [PATCH 5/5] Separate container IDs from container names Signed-off-by: Mrunal Patel --- oci/oci.go | 3 +- server/container.go | 67 +++++++++++++++++++++++++-------------------- server/sandbox.go | 8 ++++-- server/server.go | 10 +++---- 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/oci/oci.go b/oci/oci.go index 7a449d09..38e62d0d 100644 --- a/oci/oci.go +++ b/oci/oci.go @@ -229,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, diff --git a/server/container.go b/server/container.go index efd0a332..48ad9363 100644 --- a/server/container.go +++ b/server/container.go @@ -29,9 +29,9 @@ func (s *Server) generateContainerIDandName(podName string, name string, attempt err error id = stringid.GenerateNonCryptoID() ) - nameStr = fmt.Sprintf("%s-%s-%v", podName, name, attempt) + nameStr := fmt.Sprintf("%s-%s-%v", podName, name, attempt) if name == "infra" { - nameStr := fmt.Sprintf("%s-%s", podName, name) + nameStr = fmt.Sprintf("%s-%s", podName, name) } if name, err = s.reserveContainerName(id, nameStr); err != nil { return "", "", err @@ -67,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) @@ -78,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 } @@ -94,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") } @@ -273,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) @@ -309,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 } @@ -319,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 @@ -338,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 @@ -358,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 { @@ -375,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 @@ -432,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 { @@ -449,7 +456,7 @@ func (s *Server) ContainerStatus(ctx context.Context, req *pb.ContainerStatusReq csr := &pb.ContainerStatusResponse{ Status: &pb.ContainerStatus{ - Id: containerName, + Id: &containerID, }, } diff --git a/server/sandbox.go b/server/sandbox.go index 5d37c1ca..579db4ef 100644 --- a/server/sandbox.go +++ b/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) diff --git a/server/server.go b/server/server.go index 18ca5692..4be9a23b 100644 --- a/server/server.go +++ b/server/server.go @@ -63,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 } @@ -217,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 } @@ -232,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() }