Merge pull request kubernetes#92817 from kmala/kubelet

k8s-ci-robot · web-flow · commit 88512be21315 · 2020-09-10T07:27:45.000-07:00
Check for sandboxes before deleting the pod from apiserver
diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD
@@ -25,6 +25,7 @@ go_library(
         "kubelet_resources.go",
         "kubelet_volumes.go",
         "pod_container_deletor.go",
+        "pod_sandbox_deleter.go",
         "pod_workers.go",
         "reason_cache.go",
         "runonce.go",
@@ -177,6 +178,7 @@ go_test(
         "kubelet_volumes_linux_test.go",
         "kubelet_volumes_test.go",
         "pod_container_deletor_test.go",
+        "pod_sandbox_deleter_test.go",
         "pod_workers_test.go",
         "reason_cache_test.go",
         "runonce_test.go",
@@ -253,21 +255,14 @@ go_test(
         "//staging/src/k8s.io/client-go/util/testing:go_default_library",
         "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
         "//staging/src/k8s.io/component-base/version:go_default_library",
+        "//staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2:go_default_library",
         "//vendor/github.com/golang/groupcache/lru:go_default_library",
         "//vendor/github.com/google/cadvisor/info/v1:go_default_library",
         "//vendor/github.com/google/cadvisor/info/v2:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",
         "//vendor/k8s.io/utils/mount:go_default_library",
-    ] + select({
-        "@io_bazel_rules_go//go/platform:android": [
-            "//staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2:go_default_library",
-        ],
-        "@io_bazel_rules_go//go/platform:linux": [
-            "//staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2:go_default_library",
-        ],
-        "//conditions:default": [],
-    }),
+    ],
 )
 
 filegroup(
diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go
@@ -114,6 +114,8 @@ type Runtime interface {
 	GetContainerLogs(ctx context.Context, pod *v1.Pod, containerID ContainerID, logOptions *v1.PodLogOptions, stdout, stderr io.Writer) (err error)
 	// Delete a container. If the container is still running, an error is returned.
 	DeleteContainer(containerID ContainerID) error
+	// DeleteSandbox deletes a sandbox.
+	DeleteSandbox(sandboxID string) error
 	// ImageService provides methods to image-related methods.
 	ImageService
 	// UpdatePodCIDR sends a new podCIDR to the runtime.
@@ -299,7 +301,6 @@ type PodStatus struct {
 	// Status of containers in the pod.
 	ContainerStatuses []*Status
 	// Status of the pod sandbox.
-	// Only for kuberuntime now, other runtime may keep it nil.
 	SandboxStatuses []*runtimeapi.PodSandboxStatus
 }
 
@@ -309,6 +310,8 @@ type Status struct {
 	ID ContainerID
 	// Name of the container.
 	Name string
+	// ID of the sandbox to which this container belongs.
+	PodSandboxID string
 	// Status of the container.
 	State State
 	// Creation time of the container.
diff --git a/pkg/kubelet/container/testing/fake_runtime.go b/pkg/kubelet/container/testing/fake_runtime.go
@@ -364,6 +364,14 @@ func (f *FakeRuntime) DeleteContainer(containerID kubecontainer.ContainerID) err
 	return f.Err
 }
 
+func (f *FakeRuntime) DeleteSandbox(sandboxID string) error {
+	f.Lock()
+	defer f.Unlock()
+
+	f.CalledFunctions = append(f.CalledFunctions, "DeleteSandbox")
+	return f.Err
+}
+
 func (f *FakeRuntime) ImageStats() (*kubecontainer.ImageStats, error) {
 	f.Lock()
 	defer f.Unlock()
diff --git a/pkg/kubelet/container/testing/runtime_mock.go b/pkg/kubelet/container/testing/runtime_mock.go
@@ -147,6 +147,11 @@ func (r *Mock) DeleteContainer(containerID kubecontainer.ContainerID) error {
 	return args.Error(0)
 }
 
+func (r *Mock) DeleteSandbox(sandboxID string) error {
+	args := r.Called(sandboxID)
+	return args.Error(0)
+}
+
 func (r *Mock) ImageStats() (*kubecontainer.ImageStats, error) {
 	args := r.Called()
 	return args.Get(0).(*kubecontainer.ImageStats), args.Error(1)
diff --git a/pkg/kubelet/cri/remote/fake/fake_runtime.go b/pkg/kubelet/cri/remote/fake/fake_runtime.go
@@ -112,7 +112,7 @@ func (f *RemoteRuntime) StopPodSandbox(ctx context.Context, req *kubeapi.StopPod
 // This call is idempotent, and must not return an error if the sandbox has
 // already been removed.
 func (f *RemoteRuntime) RemovePodSandbox(ctx context.Context, req *kubeapi.RemovePodSandboxRequest) (*kubeapi.RemovePodSandboxResponse, error) {
-	err := f.RuntimeService.StopPodSandbox(req.PodSandboxId)
+	err := f.RuntimeService.RemovePodSandbox(req.PodSandboxId)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
@@ -671,6 +671,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 	}
 	klet.containerGC = containerGC
 	klet.containerDeletor = newPodContainerDeletor(klet.containerRuntime, integer.IntMax(containerGCPolicy.MaxPerPodContainer, minDeadContainerInPod))
+	klet.sandboxDeleter = newPodSandboxDeleter(klet.containerRuntime)
 
 	// setup imageManager
 	imageManager, err := images.NewImageGCManager(klet.containerRuntime, klet.StatsProvider, kubeDeps.Recorder, nodeRef, imageGCPolicy, crOptions.PodSandboxImage)
@@ -1098,6 +1099,9 @@ type Kubelet struct {
 	// trigger deleting containers in a pod
 	containerDeletor *podContainerDeletor
 
+	// trigger deleting sandboxes in a pod
+	sandboxDeleter *podSandboxDeleter
+
 	// config iptables util rules
 	makeIPTablesUtilChains bool
 
@@ -1870,6 +1874,9 @@ func (kl *Kubelet) syncLoopIteration(configCh <-chan kubetypes.PodUpdate, handle
 				klog.V(4).Infof("SyncLoop (PLEG): ignore irrelevant event: %#v", e)
 			}
 		}
+		if e.Type == pleg.ContainerRemoved {
+			kl.deletePodSandbox(e.ID)
+		}
 
 		if e.Type == pleg.ContainerDied {
 			if containerID, ok := e.Data.(string); ok {
@@ -2197,6 +2204,16 @@ func (kl *Kubelet) fastStatusUpdateOnce() {
 	}
 }
 
+func (kl *Kubelet) deletePodSandbox(podID types.UID) {
+	if podStatus, err := kl.podCache.Get(podID); err == nil {
+		toKeep := 1
+		if kl.IsPodDeleted(podID) {
+			toKeep = 0
+		}
+		kl.sandboxDeleter.deleteSandboxesInPod(podStatus, toKeep)
+	}
+}
+
 // isSyncPodWorthy filters out events that are not worthy of pod syncing
 func isSyncPodWorthy(event *pleg.PodLifecycleEvent) bool {
 	// ContainerRemoved doesn't affect pod state
diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go
@@ -967,6 +967,16 @@ func (kl *Kubelet) PodResourcesAreReclaimed(pod *v1.Pod, status v1.PodStatus) bo
 		klog.V(3).Infof("Pod %q is terminated, but some containers have not been cleaned up: %s", format.Pod(pod), statusStr)
 		return false
 	}
+	// pod's sandboxes should be deleted
+	if len(runtimeStatus.SandboxStatuses) > 0 {
+		var sandboxStr string
+		for _, sandbox := range runtimeStatus.SandboxStatuses {
+			sandboxStr += fmt.Sprintf("%+v ", *sandbox)
+		}
+		klog.V(3).Infof("Pod %q is terminated, but some pod sandboxes have not been cleaned up: %s", format.Pod(pod), sandboxStr)
+		return false
+	}
+
 	if kl.podVolumesExist(pod.UID) && !kl.keepTerminatedPodVolumes {
 		// We shouldn't delete pods whose volumes have not been cleaned up if we are not keeping terminated pod volumes
 		klog.V(3).Infof("Pod %q is terminated, but some volumes have not been cleaned up", format.Pod(pod))
diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go
@@ -42,6 +42,7 @@ import (
 	// api.Registry.GroupOrDie(v1.GroupName).GroupVersions[0].String() is changed
 	// to "v1"?
 
+	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 	_ "k8s.io/kubernetes/pkg/apis/core/install"
 	"k8s.io/kubernetes/pkg/features"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
@@ -2421,3 +2422,70 @@ func TestTruncatePodHostname(t *testing.T) {
 		assert.Equal(t, test.output, output)
 	}
 }
+
+func TestPodResourcesAreReclaimed(t *testing.T) {
+
+	type args struct {
+		pod           *v1.Pod
+		status        v1.PodStatus
+		runtimeStatus kubecontainer.PodStatus
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			"pod with running containers",
+			args{
+				pod: &v1.Pod{},
+				status: v1.PodStatus{
+					ContainerStatuses: []v1.ContainerStatus{
+						runningState("containerA"),
+						runningState("containerB"),
+					},
+				},
+			},
+			false,
+		},
+		{
+			"pod with containers in runtime cache",
+			args{
+				pod:    &v1.Pod{},
+				status: v1.PodStatus{},
+				runtimeStatus: kubecontainer.PodStatus{
+					ContainerStatuses: []*kubecontainer.Status{
+						{},
+					},
+				},
+			},
+			false,
+		},
+		{
+			"pod with sandbox present",
+			args{
+				pod:    &v1.Pod{},
+				status: v1.PodStatus{},
+				runtimeStatus: kubecontainer.PodStatus{
+					SandboxStatuses: []*runtimeapi.PodSandboxStatus{
+						{},
+					},
+				},
+			},
+			false,
+		},
+	}
+
+	testKubelet := newTestKubelet(t, false)
+	defer testKubelet.Cleanup()
+	kl := testKubelet.kubelet
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testKubelet.fakeRuntime.PodStatus = tt.args.runtimeStatus
+			if got := kl.PodResourcesAreReclaimed(tt.args.pod, tt.args.status); got != tt.want {
+				t.Errorf("PodResourcesAreReclaimed() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go
@@ -474,6 +474,7 @@ func (m *kubeGenericRuntimeManager) getPodContainerStatuses(uid kubetypes.UID, n
 				cStatus.Message += tMessage
 			}
 		}
+		cStatus.PodSandboxID = c.PodSandboxId
 		statuses[i] = cStatus
 	}
 
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_gc.go b/pkg/kubelet/kuberuntime/kuberuntime_gc.go
@@ -161,26 +161,13 @@ func (cgc *containerGC) removeOldestNSandboxes(sandboxes []sandboxGCInfo, toRemo
 	// Remove from oldest to newest (last to first).
 	for i := len(sandboxes) - 1; i >= numToKeep; i-- {
 		if !sandboxes[i].active {
-			cgc.removeSandbox(sandboxes[i].id)
+			if err := cgc.manager.DeleteSandbox(sandboxes[i].id); err != nil {
+				klog.Errorf("Failed to remove sandbox %q: %v", sandboxes[i].id, err)
+			}
 		}
 	}
 }
 
-// removeSandbox removes the sandbox by sandboxID.
-func (cgc *containerGC) removeSandbox(sandboxID string) {
-	klog.V(4).Infof("Removing sandbox %q", sandboxID)
-	// In normal cases, kubelet should've already called StopPodSandbox before
-	// GC kicks in. To guard against the rare cases where this is not true, try
-	// stopping the sandbox before removing it.
-	if err := cgc.client.StopPodSandbox(sandboxID); err != nil {
-		klog.Errorf("Failed to stop sandbox %q before removing: %v", sandboxID, err)
-		return
-	}
-	if err := cgc.client.RemovePodSandbox(sandboxID); err != nil {
-		klog.Errorf("Failed to remove sandbox %q: %v", sandboxID, err)
-	}
-}
-
 // evictableContainers gets all containers that are evictable. Evictable containers are: not running
 // and created more than MinAge ago.
 func (cgc *containerGC) evictableContainers(minAge time.Duration) (containersByEvictUnit, error) {
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go
@@ -318,14 +318,17 @@ func TestGetPodStatus(t *testing.T) {
 	}
 
 	// Set fake sandbox and faked containers to fakeRuntime.
-	makeAndSetFakePod(t, m, fakeRuntime, pod)
+	sandbox, _ := makeAndSetFakePod(t, m, fakeRuntime, pod)
 
 	podStatus, err := m.GetPodStatus(pod.UID, pod.Name, pod.Namespace)
 	assert.NoError(t, err)
 	assert.Equal(t, pod.UID, podStatus.ID)
 	assert.Equal(t, pod.Name, podStatus.Name)
 	assert.Equal(t, pod.Namespace, podStatus.Namespace)
 	assert.Equal(t, apitest.FakePodSandboxIPs, podStatus.IPs)
+	for _, containerStatus := range podStatus.ContainerStatuses {
+		assert.Equal(t, sandbox.Id, containerStatus.PodSandboxID)
+	}
 }
 
 func TestGetPods(t *testing.T) {
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go
@@ -304,3 +304,15 @@ func (m *kubeGenericRuntimeManager) GetPortForward(podName, podNamespace string,
 	}
 	return url.Parse(resp.Url)
 }
+
+// DeleteSandbox removes the sandbox by sandboxID.
+func (m *kubeGenericRuntimeManager) DeleteSandbox(sandboxID string) error {
+	klog.V(4).Infof("Removing sandbox %q", sandboxID)
+	// stop sandbox is called as part of kill pod function but the error is ignored. So,
+	// we have to call stop sandbox again to make sure that all the resources like network
+	// are cleaned by runtime.
+	if err := m.runtimeService.StopPodSandbox(sandboxID); err != nil {
+		return err
+	}
+	return m.runtimeService.RemovePodSandbox(sandboxID)
+}
diff --git a/pkg/kubelet/kuberuntime/kuberuntime_sandbox_test.go b/pkg/kubelet/kuberuntime/kuberuntime_sandbox_test.go
@@ -28,6 +28,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
+	apitest "k8s.io/cri-api/pkg/apis/testing"
 	"k8s.io/kubernetes/pkg/features"
 	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
 	"k8s.io/kubernetes/pkg/kubelet/runtimeclass"
@@ -177,3 +178,40 @@ func newSeccompPod(podFieldProfile, containerFieldProfile *v1.SeccompProfile, po
 	}
 	return pod
 }
+
+// TestDeleteSandbox tests removing the sandbox.
+func TestDeleteSandbox(t *testing.T) {
+	fakeRuntime, _, m, err := createTestRuntimeManager()
+	require.NoError(t, err)
+	pod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       "12345678",
+			Name:      "bar",
+			Namespace: "new",
+		},
+		Spec: v1.PodSpec{
+			Containers: []v1.Container{
+				{
+					Name:            "foo",
+					Image:           "busybox",
+					ImagePullPolicy: v1.PullIfNotPresent,
+				},
+			},
+		},
+	}
+
+	sandbox := makeFakePodSandbox(t, m, sandboxTemplate{
+		pod:       pod,
+		createdAt: fakeCreatedAt,
+		state:     runtimeapi.PodSandboxState_SANDBOX_NOTREADY,
+	})
+	fakeRuntime.SetFakeSandboxes([]*apitest.FakePodSandbox{sandbox})
+
+	err = m.DeleteSandbox(sandbox.Id)
+	assert.NoError(t, err)
+	assert.Contains(t, fakeRuntime.Called, "StopPodSandbox")
+	assert.Contains(t, fakeRuntime.Called, "RemovePodSandbox")
+	containers, err := fakeRuntime.ListPodSandbox(&runtimeapi.PodSandboxFilter{Id: sandbox.Id})
+	assert.NoError(t, err)
+	assert.Empty(t, containers)
+}
diff --git a/pkg/kubelet/pod_sandbox_deleter.go b/pkg/kubelet/pod_sandbox_deleter.go
diff --git a/pkg/kubelet/pod_sandbox_deleter_test.go b/pkg/kubelet/pod_sandbox_deleter_test.go

Original file line number	Diff line number	Diff line change
`@@ -112,7 +112,7 @@ func (f RemoteRuntime) StopPodSandbox(ctx context.Context, req kubeapi.StopPod`
`112`	`112`	`// This call is idempotent, and must not return an error if the sandbox has`
`113`	`113`	`// already been removed.`
`114`	`114`	`func (f RemoteRuntime) RemovePodSandbox(ctx context.Context, req kubeapi.RemovePodSandboxRequest) (*kubeapi.RemovePodSandboxResponse, error) {`
`115`		`- err := f.RuntimeService.StopPodSandbox(req.PodSandboxId)`
	`115`	`+ err := f.RuntimeService.RemovePodSandbox(req.PodSandboxId)`
`116`	`116`	`if err != nil {`
`117`	`117`	`return nil, err`
`118`	`118`	`}`
Original file line number	Diff line number	Diff line change
`@@ -474,6 +474,7 @@ func (m *kubeGenericRuntimeManager) getPodContainerStatuses(uid kubetypes.UID, n`
`474`	`474`	`cStatus.Message += tMessage`
`475`	`475`	`}`
`476`	`476`	`}`
	`477`	`+ cStatus.PodSandboxID = c.PodSandboxId`
`477`	`478`	`statuses[i] = cStatus`
`478`	`479`	`}`
`479`	`480`