From aae8a2847aa4831b4e8514ca061d391b3b163bcd Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Wed, 25 Mar 2020 14:55:18 -0700 Subject: [PATCH 1/5] Check for sandboxes before deleting the pod from apiserver --- pkg/kubelet/kubelet_pods.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 345566d1026..50eacfb09c2 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -940,6 +940,24 @@ 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 + filter := &runtimeapi.PodSandboxFilter{ + LabelSelector: map[string]string{kubetypes.KubernetesPodUIDLabel: string(pod.UID)}, + } + sandboxes, err := kl.runtimeService.ListPodSandbox(filter) + if err != nil { + klog.V(3).Infof("Pod %q is terminated, Error getting pod sandboxes from the runtime service: %s", format.Pod(pod), err) + return false + } + if len(sandboxes) > 0 { + var sandboxStr string + for _, sandbox := range sandboxes { + 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)) From 1e42737e5819f586ccc628fec567d75bc669a5da Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Mon, 30 Mar 2020 09:32:15 -0700 Subject: [PATCH 2/5] add unit tests --- pkg/kubelet/kubelet_pods_test.go | 81 ++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index a542845c46e..88744d4394e 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io/ioutil" + kubetypes "k8s.io/kubernetes/pkg/kubelet/types" "os" "path/filepath" "sort" @@ -42,6 +43,8 @@ import ( // api.Registry.GroupOrDie(v1.GroupName).GroupVersions[0].String() is changed // to "v1"? + runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" + apitest "k8s.io/cri-api/pkg/apis/testing" _ "k8s.io/kubernetes/pkg/apis/core/install" "k8s.io/kubernetes/pkg/features" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" @@ -2385,3 +2388,81 @@ func TestTruncatePodHostname(t *testing.T) { assert.Equal(t, test.output, output) } } + +func TestKubelet_PodResourcesAreReclaimed(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.ContainerStatus{ + {}, + }, + }, + }, + false, + }, + { + "pod with sandbox present", + args{ + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + UID: types.UID("fakesandbox"), + }, + }, + status: v1.PodStatus{}, + }, + false, + }, + } + + testKubelet := newTestKubelet(t, false) + defer testKubelet.Cleanup() + kl := testKubelet.kubelet + + runtimeService := apitest.NewFakeRuntimeService() + runtimeService.SetFakeSandboxes([]*apitest.FakePodSandbox{ + { + PodSandboxStatus: runtimeapi.PodSandboxStatus{ + Id: "fakesandbox", + Labels: map[string]string{ + kubetypes.KubernetesPodUIDLabel: "fakesandbox", + }, + }, + }, + }) + kl.runtimeService = runtimeService + 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) + } + }) + } +} From c24349e9f288d8789046a2db125d6a60807e7b41 Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Tue, 31 Mar 2020 16:01:23 -0700 Subject: [PATCH 3/5] update the build file for bazel --- pkg/kubelet/BUILD | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 64106eac63b..451381353ad 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -250,20 +250,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", + "//staging/src/k8s.io/cri-api/pkg/apis/testing: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( From 70e2559acab102553b340600f905f0f08840fa1a Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Wed, 1 Apr 2020 14:30:12 -0700 Subject: [PATCH 4/5] use runtime sandbox status instead of calling cri --- pkg/kubelet/BUILD | 1 - pkg/kubelet/container/runtime.go | 1 - pkg/kubelet/kubelet_pods.go | 14 +++----------- pkg/kubelet/kubelet_pods_test.go | 25 ++++++------------------- 4 files changed, 9 insertions(+), 32 deletions(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 451381353ad..61433e13f62 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -251,7 +251,6 @@ go_test( "//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", - "//staging/src/k8s.io/cri-api/pkg/apis/testing: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", diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go index 71f6dc20f7e..367d7560991 100644 --- a/pkg/kubelet/container/runtime.go +++ b/pkg/kubelet/container/runtime.go @@ -281,7 +281,6 @@ type PodStatus struct { // Status of containers in the pod. ContainerStatuses []*ContainerStatus // Status of the pod sandbox. - // Only for kuberuntime now, other runtime may keep it nil. SandboxStatuses []*runtimeapi.PodSandboxStatus } diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 50eacfb09c2..33e47a73c09 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -941,18 +941,10 @@ func (kl *Kubelet) PodResourcesAreReclaimed(pod *v1.Pod, status v1.PodStatus) bo return false } // pod's sandboxes should be deleted - filter := &runtimeapi.PodSandboxFilter{ - LabelSelector: map[string]string{kubetypes.KubernetesPodUIDLabel: string(pod.UID)}, - } - sandboxes, err := kl.runtimeService.ListPodSandbox(filter) - if err != nil { - klog.V(3).Infof("Pod %q is terminated, Error getting pod sandboxes from the runtime service: %s", format.Pod(pod), err) - return false - } - if len(sandboxes) > 0 { + if len(runtimeStatus.SandboxStatuses) > 0 { var sandboxStr string - for _, sandbox := range sandboxes { - sandboxStr += fmt.Sprintf("%+v ", sandbox) + 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 diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index 88744d4394e..18135f8db1e 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "io/ioutil" - kubetypes "k8s.io/kubernetes/pkg/kubelet/types" "os" "path/filepath" "sort" @@ -44,7 +43,6 @@ import ( // to "v1"? runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" - apitest "k8s.io/cri-api/pkg/apis/testing" _ "k8s.io/kubernetes/pkg/apis/core/install" "k8s.io/kubernetes/pkg/features" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" @@ -2389,7 +2387,7 @@ func TestTruncatePodHostname(t *testing.T) { } } -func TestKubelet_PodResourcesAreReclaimed(t *testing.T) { +func TestPodResourcesAreReclaimed(t *testing.T) { type args struct { pod *v1.Pod @@ -2430,12 +2428,13 @@ func TestKubelet_PodResourcesAreReclaimed(t *testing.T) { { "pod with sandbox present", args{ - pod: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - UID: types.UID("fakesandbox"), + pod: &v1.Pod{}, + status: v1.PodStatus{}, + runtimeStatus: kubecontainer.PodStatus{ + SandboxStatuses: []*runtimeapi.PodSandboxStatus{ + {}, }, }, - status: v1.PodStatus{}, }, false, }, @@ -2445,18 +2444,6 @@ func TestKubelet_PodResourcesAreReclaimed(t *testing.T) { defer testKubelet.Cleanup() kl := testKubelet.kubelet - runtimeService := apitest.NewFakeRuntimeService() - runtimeService.SetFakeSandboxes([]*apitest.FakePodSandbox{ - { - PodSandboxStatus: runtimeapi.PodSandboxStatus{ - Id: "fakesandbox", - Labels: map[string]string{ - kubetypes.KubernetesPodUIDLabel: "fakesandbox", - }, - }, - }, - }) - kl.runtimeService = runtimeService for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { testKubelet.fakeRuntime.PodStatus = tt.args.runtimeStatus From 9b9cf33771a7797570cd917f5ca404a2457a99c5 Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Mon, 13 Apr 2020 13:39:43 -0700 Subject: [PATCH 5/5] fake remote runtime should call correct method on remove pod sandbox --- pkg/kubelet/remote/fake/fake_runtime.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/remote/fake/fake_runtime.go b/pkg/kubelet/remote/fake/fake_runtime.go index 71845820750..23067b4fded 100644 --- a/pkg/kubelet/remote/fake/fake_runtime.go +++ b/pkg/kubelet/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 }