Merge pull request #34858 from Random-Liu/symlink-docker-log

pkg/kubelet/api/testing/fake_runtime_service.go

@@ -19,6 +19,7 @@ package testing
 import (
 	"fmt"
 	"io"
+	"reflect"
 	"sync"
 	"time"
 
@@ -77,6 +78,16 @@ func (r *FakeRuntimeService) SetFakeContainers(containers []*FakeContainer) {
 
 }
 
+func (r *FakeRuntimeService) AssertCalls(calls []string) error {
+	r.Lock()
+	defer r.Unlock()
+
+	if !reflect.DeepEqual(calls, r.Called) {
+		return fmt.Errorf("expected %#v, got %#v", calls, r.Called)
+	}
+	return nil
+}
+
 func NewFakeRuntimeService() *FakeRuntimeService {
 	return &FakeRuntimeService{
 		Called:     make([]string, 0),

pkg/kubelet/container/os.go

@@ -29,6 +29,7 @@ type OSInterface interface {
 	Symlink(oldname string, newname string) error
 	Stat(path string) (os.FileInfo, error)
 	Remove(path string) error
+	RemoveAll(path string) error
 	Create(path string) (*os.File, error)
 	Hostname() (name string, err error)
 	Chtimes(path string, atime time.Time, mtime time.Time) error
@@ -59,6 +60,11 @@ func (RealOS) Remove(path string) error {
 	return os.Remove(path)
 }
 
+// RemoveAll will call os.RemoveAll to remove the path and its children.
+func (RealOS) RemoveAll(path string) error {
+	return os.RemoveAll(path)
+}
+
 // Create will call os.Create to create and return a file
 // at path.
 func (RealOS) Create(path string) (*os.File, error) {

pkg/kubelet/container/testing/os.go

@@ -26,11 +26,13 @@ import (
 // If a member of the form `*Fn` is set, that function will be called in place
 // of the real call.
 type FakeOS struct {
-	StatFn    func(string) (os.FileInfo, error)
-	ReadDirFn func(string) ([]os.FileInfo, error)
-	HostName  string
-	Removes   []string
-	Files     map[string][]*os.FileInfo
+	StatFn     func(string) (os.FileInfo, error)
+	ReadDirFn  func(string) ([]os.FileInfo, error)
+	MkdirAllFn func(string, os.FileMode) error
+	SymlinkFn  func(string, string) error
+	HostName   string
+	Removes    []string
+	Files      map[string][]*os.FileInfo
 }
 
 func NewFakeOS() *FakeOS {
@@ -41,12 +43,18 @@ func NewFakeOS() *FakeOS {
 }
 
 // Mkdir is a fake call that just returns nil.
-func (FakeOS) MkdirAll(path string, perm os.FileMode) error {
+func (f *FakeOS) MkdirAll(path string, perm os.FileMode) error {
+	if f.MkdirAllFn != nil {
+		return f.MkdirAllFn(path, perm)
+	}
 	return nil
 }
 
 // Symlink is a fake call that just returns nil.
-func (FakeOS) Symlink(oldname string, newname string) error {
+func (f *FakeOS) Symlink(oldname string, newname string) error {
+	if f.SymlinkFn != nil {
+		return f.SymlinkFn(oldname, newname)
+	}
 	return nil
 }
 
@@ -64,6 +72,11 @@ func (f *FakeOS) Remove(path string) error {
 	return nil
 }
 
+// RemoveAll is a fake call that just returns nil.
+func (f *FakeOS) RemoveAll(path string) error {
+	return nil
+}
+
 // Create is a fake call that returns nil.
 func (FakeOS) Create(path string) (*os.File, error) {
 	return nil, nil
@@ -89,5 +102,5 @@ func (f *FakeOS) ReadDir(dirname string) ([]os.FileInfo, error) {
 	if f.ReadDirFn != nil {
 		return f.ReadDirFn(dirname)
 	}
-	return nil, errors.New("unimplemented testing mock")
+	return nil, nil
 }

pkg/kubelet/dockershim/docker_container.go

@@ -19,6 +19,8 @@ package dockershim
 import (
 	"fmt"
 	"io"
+	"os"
+	"path/filepath"
 	"time"
 
 	dockertypes "github.com/docker/engine-api/types"
@@ -91,6 +93,8 @@ func (ds *dockerService) CreateContainer(podSandboxID string, config *runtimeApi
 	labels := makeLabels(config.GetLabels(), config.GetAnnotations())
 	// Apply a the container type label.
 	labels[containerTypeLabelKey] = containerTypeLabelContainer
+	// Write the container log path in the labels.
+	labels[containerLogPathLabelKey] = filepath.Join(sandboxConfig.GetLogDirectory(), config.GetLogPath())
 	// Write the sandbox ID in the labels.
 	labels[sandboxIDLabelKey] = podSandboxID
 
@@ -181,9 +185,63 @@ func (ds *dockerService) CreateContainer(podSandboxID string, config *runtimeApi
 	return "", err
 }
 
+// getContainerLogPath returns the container log path specified by kubelet and the real
+// path where docker stores the container log.
+func (ds *dockerService) getContainerLogPath(containerID string) (string, string, error) {
+	info, err := ds.client.InspectContainer(containerID)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to inspect container %q: %v", containerID, err)
+	}
+	return info.Config.Labels[containerLogPathLabelKey], info.LogPath, nil
+}
+
+// createContainerLogSymlink creates the symlink for docker container log.
+func (ds *dockerService) createContainerLogSymlink(containerID string) error {
+	path, realPath, err := ds.getContainerLogPath(containerID)
+	if err != nil {
+		return fmt.Errorf("failed to get container %q log path: %v", containerID, err)
+	}
+	if path != "" {
+		// Only create the symlink when container log path is specified.
+		if err = ds.os.Symlink(realPath, path); err != nil {
+			return fmt.Errorf("failed to create symbolic link %q to the container log file %q for container %q: %v",
+				path, realPath, containerID, err)
+		}
+	}
+	return nil
+}
+
+// removeContainerLogSymlink removes the symlink for docker container log.
+func (ds *dockerService) removeContainerLogSymlink(containerID string) error {
+	path, _, err := ds.getContainerLogPath(containerID)
+	if err != nil {
+		return fmt.Errorf("failed to get container %q log path: %v", containerID, err)
+	}
+	if path != "" {
+		// Only remove the symlink when container log path is specified.
+		err := ds.os.Remove(path)
+		if err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("failed to remove container %q log symlink %q: %v", containerID, path, err)
+		}
+	}
+	return nil
+}
+
 // StartContainer starts the container.
 func (ds *dockerService) StartContainer(containerID string) error {
-	return ds.client.StartContainer(containerID)
+	err := ds.client.StartContainer(containerID)
+	if err != nil {
+		return fmt.Errorf("failed to start container %q: %v", containerID, err)
+	}
+	// Create container log symlink.
+	if err := ds.createContainerLogSymlink(containerID); err != nil {
+		// Do not stop the container if fail to create symlink, because:
+		// 1. This is not a critical failure.
+		// 2. We don't have enough information to properly stop container here.
+		// Kubelet will surface this error to user with event.
+		return err
+	}
+	return nil
 }
 
 // StopContainer stops a running container with a grace period (i.e., timeout).
@@ -194,7 +252,18 @@ func (ds *dockerService) StopContainer(containerID string, timeout int64) error
 // RemoveContainer removes the container.
 // TODO: If a container is still running, should we forcibly remove it?
 func (ds *dockerService) RemoveContainer(containerID string) error {
-	return ds.client.RemoveContainer(containerID, dockertypes.ContainerRemoveOptions{RemoveVolumes: true})
+	// Ideally, log lifecycle should be independent of container lifecycle.
+	// However, docker will remove container log after container is removed,
+	// we can't prevent that now, so we also cleanup the symlink here.
+	err := ds.removeContainerLogSymlink(containerID)
+	if err != nil {
+		return err
+	}
+	err = ds.client.RemoveContainer(containerID, dockertypes.ContainerRemoveOptions{RemoveVolumes: true})
+	if err != nil {
+		return fmt.Errorf("failed to remove container %q: %v", containerID, err)
+	}
+	return nil
 }
 
 func getContainerTimestamps(r *dockertypes.ContainerJSON) (time.Time, time.Time, time.Time, error) {

pkg/kubelet/dockershim/docker_container_test.go

@@ -18,12 +18,14 @@ package dockershim
 
 import (
 	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 
 	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
 )
 
 // A helper to create a basic config.
@@ -171,3 +173,45 @@ func TestContainerStatus(t *testing.T) {
 	status, err = ds.ContainerStatus(id)
 	assert.Error(t, err, fmt.Sprintf("status of container: %+v", status))
 }
+
+// TestContainerLogPath tests the container log creation logic.
+func TestContainerLogPath(t *testing.T) {
+	ds, fDocker, _ := newTestDockerService()
+	podLogPath := "/pod/1"
+	containerLogPath := "0"
+	kubeletContainerLogPath := filepath.Join(podLogPath, containerLogPath)
+	sConfig := makeSandboxConfig("foo", "bar", "1", 0)
+	sConfig.LogDirectory = &podLogPath
+	config := makeContainerConfig(sConfig, "pause", "iamimage", 0, nil, nil)
+	config.LogPath = &containerLogPath
+
+	const sandboxId = "sandboxid"
+	id, err := ds.CreateContainer(sandboxId, config, sConfig)
+
+	// Check internal container log label
+	c, err := fDocker.InspectContainer(id)
+	assert.NoError(t, err)
+	assert.Equal(t, c.Config.Labels[containerLogPathLabelKey], kubeletContainerLogPath)
+
+	// Set docker container log path
+	dockerContainerLogPath := "/docker/container/log"
+	c.LogPath = dockerContainerLogPath
+
+	// Verify container log symlink creation
+	fakeOS := ds.os.(*containertest.FakeOS)
+	fakeOS.SymlinkFn = func(oldname, newname string) error {
+		assert.Equal(t, dockerContainerLogPath, oldname)
+		assert.Equal(t, kubeletContainerLogPath, newname)
+		return nil
+	}
+	err = ds.StartContainer(id)
+	assert.NoError(t, err)
+
+	err = ds.StopContainer(id, 0)
+	assert.NoError(t, err)
+
+	// Verify container log symlink deletion
+	err = ds.RemoveContainer(id)
+	assert.NoError(t, err)
+	assert.Equal(t, fakeOS.Removes, []string{kubeletContainerLogPath})
+}

pkg/kubelet/dockershim/docker_sandbox.go

@@ -71,7 +71,7 @@ func (ds *dockerService) RunPodSandbox(config *runtimeApi.PodSandboxConfig) (str
 	// Step 3: Start the sandbox container.
 	// Assume kubelet's garbage collector would remove the sandbox later, if
 	// startContainer failed.
-	err = ds.StartContainer(createResp.ID)
+	err = ds.client.StartContainer(createResp.ID)
 	return createResp.ID, err
 }
 

pkg/kubelet/dockershim/docker_service.go

@@ -48,16 +48,18 @@ const (
 	containerTypeLabelKey       = "io.kubernetes.docker.type"
 	containerTypeLabelSandbox   = "podsandbox"
 	containerTypeLabelContainer = "container"
+	containerLogPathLabelKey    = "io.kubernetes.container.logpath"
 	sandboxIDLabelKey           = "io.kubernetes.sandbox.id"
 )
 
-var internalLabelKeys []string = []string{containerTypeLabelKey, sandboxIDLabelKey}
+var internalLabelKeys []string = []string{containerTypeLabelKey, containerLogPathLabelKey, sandboxIDLabelKey}
 
 // NOTE: Anything passed to DockerService should be eventually handled in another way when we switch to running the shim as a different process.
 func NewDockerService(client dockertools.DockerInterface, seccompProfileRoot string, podSandboxImage string) DockerLegacyService {
 	return &dockerService{
 		seccompProfileRoot: seccompProfileRoot,
 		client:             dockertools.NewInstrumentedDockerInterface(client),
+		os:                 kubecontainer.RealOS{},
 		podSandboxImage:    podSandboxImage,
 	}
 }
@@ -81,6 +83,7 @@ type DockerLegacyService interface {
 type dockerService struct {
 	seccompProfileRoot string
 	client             dockertools.DockerInterface
+	os                 kubecontainer.OSInterface
 	podSandboxImage    string
 }
 

pkg/kubelet/dockershim/docker_service_test.go

@@ -19,6 +19,7 @@ package dockershim
 import (
 	"time"
 
+	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
 	"k8s.io/kubernetes/pkg/kubelet/dockertools"
 	"k8s.io/kubernetes/pkg/util/clock"
 )
@@ -26,5 +27,5 @@ import (
 func newTestDockerService() (*dockerService, *dockertools.FakeDockerClient, *clock.FakeClock) {
 	fakeClock := clock.NewFakeClock(time.Time{})
 	c := dockertools.NewFakeDockerClientWithClock(fakeClock)
-	return &dockerService{client: c}, c, fakeClock
+	return &dockerService{client: c, os: &containertest.FakeOS{}}, c, fakeClock
 }

pkg/kubelet/kuberuntime/helpers.go

@@ -18,12 +18,14 @@ package kuberuntime
 
 import (
 	"fmt"
+	"path/filepath"
 	"strconv"
 
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
 	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/types"
 )
 
 const (
@@ -199,3 +201,13 @@ func getStableKey(pod *api.Pod, container *api.Container) string {
 	hash := strconv.FormatUint(kubecontainer.HashContainer(container), 16)
 	return fmt.Sprintf("%s_%s_%s_%s_%s", pod.Name, pod.Namespace, string(pod.UID), container.Name, hash)
 }
+
+// buildContainerLogsPath builds log path for container relative to pod logs directory.
+func buildContainerLogsPath(containerName string, restartCount int) string {
+	return fmt.Sprintf("%s_%d.log", containerName, restartCount)
+}
+
+// buildPodLogsDirectory builds absolute log directory path for a pod sandbox.
+func buildPodLogsDirectory(podUID types.UID) string {
+	return filepath.Join(podLogsRootDirectory, string(podUID))
+}

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -22,7 +22,7 @@ import (
 	"io/ioutil"
 	"math/rand"
 	"os"
-	"path"
+	"path/filepath"
 	"sort"
 	"sync"
 	"time"
@@ -97,6 +97,19 @@ func (m *kubeGenericRuntimeManager) startContainer(podSandboxID string, podSandb
 	}
 	m.recorder.Eventf(ref, api.EventTypeNormal, events.StartedContainer, "Started container with id %v", containerID)
 
+	// Symlink container logs to the legacy container log location for cluster logging
+	// support.
+	// TODO(random-liu): Remove this after cluster logging supports CRI container log path.
+	containerMeta := containerConfig.GetMetadata()
+	sandboxMeta := podSandboxConfig.GetMetadata()
+	legacySymlink := legacyLogSymlink(containerID, containerMeta.GetName(), sandboxMeta.GetName(),
+		sandboxMeta.GetNamespace())
+	containerLog := filepath.Join(podSandboxConfig.GetLogDirectory(), containerConfig.GetLogPath())
+	if err := m.osInterface.Symlink(containerLog, legacySymlink); err != nil {
+		glog.Errorf("Failed to create legacy symbolic link %q to container %q log %q: %v",
+			legacySymlink, containerID, containerLog, err)
+	}
+
 	// Step 4: execute the post start hook.
 	if container.Lifecycle != nil && container.Lifecycle.PostStart != nil {
 		kubeContainerID := kubecontainer.ContainerID{
@@ -115,11 +128,6 @@ func (m *kubeGenericRuntimeManager) startContainer(podSandboxID string, podSandb
 	return "", nil
 }
 
-// getContainerLogsPath gets log path for container.
-func getContainerLogsPath(containerName string, podUID kubetypes.UID) string {
-	return path.Join(podLogsRootDirectory, string(podUID), fmt.Sprintf("%s.log", containerName))
-}
-
 // generateContainerConfig generates container config for kubelet runtime api.
 func (m *kubeGenericRuntimeManager) generateContainerConfig(container *api.Container, pod *api.Pod, restartCount int, podIP string) (*runtimeApi.ContainerConfig, error) {
 	opts, err := m.runtimeHelper.GenerateRunContainerOptions(pod, container, podIP)
@@ -128,7 +136,7 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *api.Conta
 	}
 
 	command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
-	containerLogsPath := getContainerLogsPath(container.Name, pod.UID)
+	containerLogsPath := buildContainerLogsPath(container.Name, restartCount)
 	podHasSELinuxLabel := pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.SELinuxOptions != nil
 	restartCountUint32 := uint32(restartCount)
 	config := &runtimeApi.ContainerConfig{
@@ -269,7 +277,8 @@ func makeMounts(opts *kubecontainer.RunContainerOptions, container *api.Containe
 		// here we just add a random id to make the path unique for different instances
 		// of the same container.
 		cid := makeUID()
-		containerLogPath := path.Join(opts.PodContainerDir, cid)
+		containerLogPath := filepath.Join(opts.PodContainerDir, cid)
+		// TODO: We should try to use os interface here.
 		fs, err := os.Create(containerLogPath)
 		if err != nil {
 			glog.Errorf("Error on creating termination-log file %q: %v", containerLogPath, err)
@@ -690,7 +699,52 @@ func (m *kubeGenericRuntimeManager) ExecInContainer(containerID kubecontainer.Co
 	return fmt.Errorf("not implemented")
 }
 
+// removeContainer removes the container and the container logs.
+// Notice that we remove the container logs first, so that container will not be removed if
+// container logs are failed to be removed, and kubelet will retry this later. This guarantees
+// that container logs to be removed with the container.
+// Notice that we assume that the container should only be removed in non-running state, and
+// it will not write container logs anymore in that state.
+func (m *kubeGenericRuntimeManager) removeContainer(containerID string) error {
+	glog.V(4).Infof("Removing container %q", containerID)
+	// Remove the container log.
+	if err := m.removeContainerLog(containerID); err != nil {
+		return err
+	}
+	// Remove the container.
+	return m.runtimeService.RemoveContainer(containerID)
+}
+
+// removeContainerLog removes the container log.
+func (m *kubeGenericRuntimeManager) removeContainerLog(containerID string) error {
+	// Remove the container log.
+	status, err := m.runtimeService.ContainerStatus(containerID)
+	if err != nil {
+		glog.Errorf("ContainerStatus for %q error: %v", containerID, err)
+		return err
+	}
+	labeledInfo := getContainerInfoFromLabels(status.Labels)
+	annotatedInfo := getContainerInfoFromAnnotations(status.Annotations)
+	path := filepath.Join(buildPodLogsDirectory(labeledInfo.PodUID),
+		buildContainerLogsPath(labeledInfo.ContainerName, annotatedInfo.RestartCount))
+	if err := m.osInterface.Remove(path); err != nil && !os.IsNotExist(err) {
+		glog.Errorf("Failed to remove container %q log %q: %v", containerID, path, err)
+		return err
+	}
+
+	// Remove the legacy container log symlink.
+	// TODO(random-liu): Remove this after cluster logging supports CRI container log path.
+	legacySymlink := legacyLogSymlink(containerID, labeledInfo.ContainerName, labeledInfo.PodName,
+		labeledInfo.PodNamespace)
+	if err := m.osInterface.Remove(legacySymlink); err != nil && !os.IsNotExist(err) {
+		glog.Errorf("Failed to remove container %q log legacy symbolic link %q: %v",
+			containerID, legacySymlink, err)
+		return err
+	}
+	return nil
+}
+
 // DeleteContainer removes a container.
 func (m *kubeGenericRuntimeManager) DeleteContainer(containerID kubecontainer.ContainerID) error {
-	return m.runtimeService.RemoveContainer(containerID.ID)
+	return m.removeContainer(containerID.ID)
 }

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kuberuntime
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/kubernetes/pkg/api"
+	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
+)
+
+// TestRemoveContainer tests removing the container and its corresponding container logs.
+func TestRemoveContainer(t *testing.T) {
+	fakeRuntime, _, m, err := createTestRuntimeManager()
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			UID:       "12345678",
+			Name:      "bar",
+			Namespace: "new",
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Name:            "foo",
+					Image:           "busybox",
+					ImagePullPolicy: api.PullIfNotPresent,
+				},
+			},
+		},
+	}
+
+	// Create fake sandbox and container
+	_, fakeContainers, err := makeAndSetFakePod(m, fakeRuntime, pod)
+	assert.NoError(t, err)
+	assert.Equal(t, len(fakeContainers), 1)
+
+	containerId := fakeContainers[0].GetId()
+	fakeOS := m.osInterface.(*containertest.FakeOS)
+	err = m.removeContainer(containerId)
+	assert.NoError(t, err)
+	// Verify container log is removed
+	expectedContainerLogPath := filepath.Join(podLogsRootDirectory, "12345678", "foo_0.log")
+	expectedContainerLogSymlink := legacyLogSymlink(containerId, "foo", "bar", "new")
+	assert.Equal(t, fakeOS.Removes, []string{expectedContainerLogPath, expectedContainerLogSymlink})
+	// Verify container is removed
+	fakeRuntime.AssertCalls([]string{"RemoveContainer"})
+	containers, err := fakeRuntime.ListContainers(&runtimeApi.ContainerFilter{Id: &containerId})
+	assert.NoError(t, err)
+	assert.Empty(t, containers)
+}

pkg/kubelet/kuberuntime/kuberuntime_gc.go

@@ -17,6 +17,9 @@ limitations under the License.
 package kuberuntime
 
 import (
+	"fmt"
+	"os"
+	"path/filepath"
 	"sort"
 	"time"
 
@@ -116,26 +119,20 @@ func (cgc *containerGC) removeOldestN(containers []containerGCInfo, toRemove int
 	// Remove from oldest to newest (last to first).
 	numToKeep := len(containers) - toRemove
 	for i := numToKeep; i < len(containers); i++ {
-		cgc.removeContainer(containers[i].id, containers[i].name)
+		if err := cgc.manager.removeContainer(containers[i].id); err != nil {
+			glog.Errorf("Failed to remove container %q: %v", containers[i].id, err)
+		}
 	}
 
 	// Assume we removed the containers so that we're not too aggressive.
 	return containers[:numToKeep]
 }
 
-// removeContainer removes the container by containerID.
-func (cgc *containerGC) removeContainer(containerID, containerName string) {
-	glog.V(4).Infof("Removing container %q name %q", containerID, containerName)
-	if err := cgc.client.RemoveContainer(containerID); err != nil {
-		glog.Warningf("Failed to remove container %q: %v", containerID, err)
-	}
-}
-
 // removeSandbox removes the sandbox by sandboxID.
 func (cgc *containerGC) removeSandbox(sandboxID string) {
 	glog.V(4).Infof("Removing sandbox %q", sandboxID)
 	if err := cgc.client.RemovePodSandbox(sandboxID); err != nil {
-		glog.Warningf("Failed to remove sandbox %q: %v", sandboxID, err)
+		glog.Errorf("Failed to remove sandbox %q: %v", sandboxID, err)
 	}
 }
 
@@ -184,7 +181,7 @@ func (cgc *containerGC) evictableContainers(minAge time.Duration) (containersByE
 
 // evictableSandboxes gets all sandboxes that are evictable. Evictable sandboxes are: not running
 // and contains no containers at all.
-func (cgc *containerGC) evictableSandboxes() ([]string, error) {
+func (cgc *containerGC) evictableSandboxes(minAge time.Duration) ([]string, error) {
 	containers, err := cgc.manager.getKubeletContainers(true)
 	if err != nil {
 		return nil, err
@@ -196,7 +193,7 @@ func (cgc *containerGC) evictableSandboxes() ([]string, error) {
 	}
 
 	evictSandboxes := make([]string, 0)
-	newestGCTime := time.Now().Add(-sandboxMinGCAge)
+	newestGCTime := time.Now().Add(-minAge)
 	for _, sandbox := range sandboxes {
 		// Prune out ready sandboxes.
 		if sandbox.GetState() == runtimeApi.PodSandBoxState_READY {
@@ -234,6 +231,42 @@ func (cgc *containerGC) isPodDeleted(podUID types.UID) bool {
 	return !found
 }
 
+// evictPodLogsDirectories evicts all evictable pod logs directories. Pod logs directories
+// are evictable if there are no corresponding pods.
+func (cgc *containerGC) evictPodLogsDirectories(allSourcesReady bool) error {
+	osInterface := cgc.manager.osInterface
+	if allSourcesReady {
+		// Only remove pod logs directories when all sources are ready.
+		dirs, err := osInterface.ReadDir(podLogsRootDirectory)
+		if err != nil {
+			return fmt.Errorf("failed to read podLogsRootDirectory %q: %v", podLogsRootDirectory, err)
+		}
+		for _, dir := range dirs {
+			podUID := types.UID(dir.Name())
+			if !cgc.isPodDeleted(podUID) {
+				continue
+			}
+			err := osInterface.RemoveAll(filepath.Join(podLogsRootDirectory, dir.Name()))
+			if err != nil {
+				glog.Errorf("Failed to remove pod logs directory %q: %v", dir.Name(), err)
+			}
+		}
+	}
+
+	// Remove dead container log symlinks.
+	// TODO(random-liu): Remove this after cluster logging supports CRI container log path.
+	logSymlinks, _ := filepath.Glob(filepath.Join(legacyContainerLogsDir, fmt.Sprintf("*.%s", legacyLogSuffix)))
+	for _, logSymlink := range logSymlinks {
+		if _, err := osInterface.Stat(logSymlink); os.IsNotExist(err) {
+			err := osInterface.Remove(logSymlink)
+			if err != nil {
+				glog.Errorf("Failed to remove container log dead symlink %q: %v", logSymlink, err)
+			}
+		}
+	}
+	return nil
+}
+
 // GarbageCollect removes dead containers using the specified container gc policy.
 // Note that gc policy is not applied to sandboxes. Sandboxes are only removed when they are
 // not ready and containing no containers.
@@ -289,7 +322,7 @@ func (cgc *containerGC) GarbageCollect(gcPolicy kubecontainer.ContainerGCPolicy,
 	}
 
 	// Remove sandboxes with zero containers
-	evictSandboxes, err := cgc.evictableSandboxes()
+	evictSandboxes, err := cgc.evictableSandboxes(sandboxMinGCAge)
 	if err != nil {
 		return err
 	}
@@ -297,5 +330,7 @@ func (cgc *containerGC) GarbageCollect(gcPolicy kubecontainer.ContainerGCPolicy,
 		cgc.removeSandbox(sandbox)
 	}
 
-	return nil
+	// Remove pod sandbox log directory
+	// TODO(random-liu): Add legacy container log localtion cleanup.
+	return cgc.evictPodLogsDirectories(allSourcesReady)
 }

pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

@@ -38,6 +38,14 @@ func (m *kubeGenericRuntimeManager) createPodSandbox(pod *api.Pod, attempt uint3
 		return "", message, err
 	}
 
+	// Create pod logs directory
+	err = m.osInterface.MkdirAll(podSandboxConfig.GetLogDirectory(), 0755)
+	if err != nil {
+		message := fmt.Sprintf("Create pod log directory for pod %q failed: %v", format.Pod(pod), err)
+		glog.Errorf(message)
+		return "", message, err
+	}
+
 	podSandBoxID, err := m.runtimeService.RunPodSandbox(podSandboxConfig)
 	if err != nil {
 		message := fmt.Sprintf("CreatePodSandbox for pod %q failed: %v", format.Pod(pod), err)
@@ -82,6 +90,9 @@ func (m *kubeGenericRuntimeManager) generatePodSandboxConfig(pod *api.Pod, attem
 		podSandboxConfig.Hostname = &hostname
 	}
 
+	logDir := buildPodLogsDirectory(pod.UID)
+	podSandboxConfig.LogDirectory = &logDir
+
 	cgroupParent := ""
 	portMappings := []*runtimeApi.PortMapping{}
 	for _, c := range pod.Spec.Containers {

pkg/kubelet/kuberuntime/kuberuntime_sandbox_test.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kuberuntime
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/kubernetes/pkg/api"
+	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
+	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
+)
+
+// TestCreatePodSandbox tests creating sandbox and its corresponding pod log directory.
+func TestCreatePodSandbox(t *testing.T) {
+	fakeRuntime, _, m, err := createTestRuntimeManager()
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			UID:       "12345678",
+			Name:      "bar",
+			Namespace: "new",
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Name:            "foo",
+					Image:           "busybox",
+					ImagePullPolicy: api.PullIfNotPresent,
+				},
+			},
+		},
+	}
+
+	fakeOS := m.osInterface.(*containertest.FakeOS)
+	fakeOS.MkdirAllFn = func(path string, perm os.FileMode) error {
+		// Check pod logs root directory is created.
+		assert.Equal(t, filepath.Join(podLogsRootDirectory, "12345678"), path)
+		assert.Equal(t, os.FileMode(0755), perm)
+		return nil
+	}
+	id, _, err := m.createPodSandbox(pod, 1)
+	assert.NoError(t, err)
+	fakeRuntime.AssertCalls([]string{"RunPodSandbox"})
+	sandboxes, err := fakeRuntime.ListPodSandbox(&runtimeApi.PodSandboxFilter{Id: &id})
+	assert.NoError(t, err)
+	assert.Equal(t, len(sandboxes), 1)
+	// TODO Check pod sandbox configuration
+}

pkg/kubelet/kuberuntime/legacy.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kuberuntime
+
+import (
+	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
+)
+
+// This file implements the functions that are needed for backward
+// compatibility. Therefore, it imports various kubernetes packages
+// directly.
+
+const (
+	// legacyContainerLogsDir is the legacy location of container logs. It is the same with
+	// kubelet.containerLogsDir.
+	legacyContainerLogsDir = "/var/log/containers"
+	// legacyLogSuffix is the legacy log suffix.
+	legacyLogSuffix = dockertools.LogSuffix
+)
+
+// legacyLogSymlink composes the legacy container log path. It is only used for legacy cluster
+// logging support.
+func legacyLogSymlink(containerID string, containerName, podName, podNamespace string) string {
+	return dockertools.LogSymlink(legacyContainerLogsDir, kubecontainer.BuildPodFullName(podName, podNamespace),
+		containerName, containerID)
+}