From d4325f42fb9ae7c61c409f16954cba0375308d8b Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Fri, 29 May 2020 12:39:22 -0700 Subject: [PATCH 1/5] Check for sandboxes before deleting the pod from apiserver --- pkg/kubelet/BUILD | 11 +--- pkg/kubelet/container/runtime.go | 1 - pkg/kubelet/cri/remote/fake/fake_runtime.go | 2 +- pkg/kubelet/kubelet_pods.go | 10 +++ pkg/kubelet/kubelet_pods_test.go | 68 +++++++++++++++++++++ 5 files changed, 81 insertions(+), 11 deletions(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 7efbf4a6927..ea8482725ed 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -254,20 +254,13 @@ go_test( "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", "//staging/src/k8s.io/component-base/version:go_default_library", "//vendor/github.com/golang/groupcache/lru:go_default_library", + "//staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2: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 index 30f815b0718..c280d5f57c7 100644 --- a/pkg/kubelet/container/runtime.go +++ b/pkg/kubelet/container/runtime.go @@ -299,7 +299,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 } diff --git a/pkg/kubelet/cri/remote/fake/fake_runtime.go b/pkg/kubelet/cri/remote/fake/fake_runtime.go index e49f311aa18..339561f9b8e 100644 --- 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_pods.go b/pkg/kubelet/kubelet_pods.go index 386f82d1591..2d56d3df6f7 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -969,6 +969,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 index 467c21f4440..f49301eae6f 100644 --- 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.ContainerStatus{ + {}, + }, + }, + }, + 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) + } + }) + } +} From 90cc954eed47badb8b404bc0f113c4c73bd2c244 Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Sun, 5 Jul 2020 15:54:02 -0700 Subject: [PATCH 2/5] add sandbox deletor to delete sandboxes on pod delete event --- pkg/kubelet/BUILD | 2 + pkg/kubelet/container/runtime.go | 4 + pkg/kubelet/container/testing/fake_runtime.go | 8 + pkg/kubelet/container/testing/runtime_mock.go | 5 + pkg/kubelet/kubelet.go | 17 ++ pkg/kubelet/kubelet_pods_test.go | 2 +- .../kuberuntime/kuberuntime_container.go | 1 + pkg/kubelet/kuberuntime/kuberuntime_gc.go | 19 +-- .../kuberuntime/kuberuntime_manager_test.go | 5 +- .../kuberuntime/kuberuntime_sandbox.go | 12 ++ .../kuberuntime/kuberuntime_sandbox_test.go | 38 +++++ pkg/kubelet/pod_sandbox_deleter.go | 83 +++++++++ pkg/kubelet/pod_sandbox_deleter_test.go | 160 ++++++++++++++++++ 13 files changed, 338 insertions(+), 18 deletions(-) create mode 100644 pkg/kubelet/pod_sandbox_deleter.go create mode 100644 pkg/kubelet/pod_sandbox_deleter_test.go diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index ea8482725ed..0ec01f97aee 100644 --- 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", diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go index c280d5f57c7..322187287e2 100644 --- 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 + // Delete a sandbox. If the container is still running, an error is returned. + DeleteSandbox(sandboxID string) error // ImageService provides methods to image-related methods. ImageService // UpdatePodCIDR sends a new podCIDR to the runtime. @@ -308,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 index 6598c727920..35cca0822b1 100644 --- 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 index d9e4b57c954..fca05bba3db 100644 --- 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/kubelet.go b/pkg/kubelet/kubelet.go index 051671a9fc8..7a470feb45d 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -654,6 +654,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) @@ -1095,6 +1096,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 @@ -1866,6 +1870,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 { @@ -2193,6 +2200,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_test.go b/pkg/kubelet/kubelet_pods_test.go index f49301eae6f..12e941cad2f 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -2454,7 +2454,7 @@ func TestPodResourcesAreReclaimed(t *testing.T) { pod: &v1.Pod{}, status: v1.PodStatus{}, runtimeStatus: kubecontainer.PodStatus{ - ContainerStatuses: []*kubecontainer.ContainerStatus{ + ContainerStatuses: []*kubecontainer.Status{ {}, }, }, diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index ec8d9fac03f..ac0513ed37b 100644 --- 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 index 8c4f786db9b..65bf7a9ab82 100644 --- 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 index e8e9512a73b..e38630615e5 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -318,7 +318,7 @@ 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) @@ -326,6 +326,9 @@ func TestGetPodStatus(t *testing.T) { 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 index 0978044f753..19606e1ce60 100644 --- 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) + // the stop sandbox is called as part of kill pod but the error is ignored. So, + // we have to call stop sandbox again to make sure that all the resources like + // netwrork 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 index d1533a5a6c6..0d52519df95 100644 --- 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 new file mode 100644 index 00000000000..dc52d3c724a --- /dev/null +++ b/pkg/kubelet/pod_sandbox_deleter.go @@ -0,0 +1,83 @@ +/* +Copyright 2020 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 kubelet + +import ( + "sort" + + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" + "k8s.io/klog/v2" + kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" +) + +const ( + // The number of sandboxes which can be deleted in parallel. + sandboxDeletionBufferLimit = 20 +) + +type sandboxStatusByCreatedList []*runtimeapi.PodSandboxStatus + +type podSandboxDeleter struct { + worker chan<- string +} + +func (a sandboxStatusByCreatedList) Len() int { return len(a) } +func (a sandboxStatusByCreatedList) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a sandboxStatusByCreatedList) Less(i, j int) bool { + return a[i].CreatedAt > a[j].CreatedAt +} + +func newPodSandboxDeleter(runtime kubecontainer.Runtime) *podSandboxDeleter { + buffer := make(chan string, sandboxDeletionBufferLimit) + go wait.Forever(func() { + for { + id := <-buffer + if err := runtime.DeleteSandbox(id); err != nil { + klog.Warningf("[pod_sandbox_deleter] DeleteSandbox returned error for (id=%v): %v", id, err) + } + } + }, 0) + + return &podSandboxDeleter{ + worker: buffer, + } +} + +// deleteSandboxesInPod issues sandbox deletion requests for all inactive sandboxes after sorting by creation time +// and skipping toKeep number of sandboxes +func (p *podSandboxDeleter) deleteSandboxesInPod(podStatus *kubecontainer.PodStatus, toKeep int) { + sandboxIDs := sets.NewString() + for _, containerStatus := range podStatus.ContainerStatuses { + sandboxIDs.Insert(containerStatus.PodSandboxId) + } + sandboxStatuses := podStatus.SandboxStatuses + if toKeep > 0 { + sort.Sort(sandboxStatusByCreatedList(sandboxStatuses)) + } + + for i := len(sandboxStatuses) - 1; i >= toKeep; i-- { + if _, ok := sandboxIDs[sandboxStatuses[i].Id]; !ok && sandboxStatuses[i].State != runtimeapi.PodSandboxState_SANDBOX_READY { + select { + case p.worker <- sandboxStatuses[i].Id: + default: + klog.Warningf("Failed to issue the request to remove sandbox %v", sandboxStatuses[i].Id) + } + } + } +} diff --git a/pkg/kubelet/pod_sandbox_deleter_test.go b/pkg/kubelet/pod_sandbox_deleter_test.go new file mode 100644 index 00000000000..16d59f0b5af --- /dev/null +++ b/pkg/kubelet/pod_sandbox_deleter_test.go @@ -0,0 +1,160 @@ +/* +Copyright 2020 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 kubelet + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/wait" + runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" + kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" +) + +type testPodSandboxDeleter struct { + podSandboxDeleter + deletedSandoxes []string +} + +func newTestPodSandboxDeleter() (*testPodSandboxDeleter, chan struct{}) { + buffer := make(chan string, 5) + stopCh := make(chan struct{}) + testSandboxDeleter := &testPodSandboxDeleter{ + podSandboxDeleter: podSandboxDeleter{ + worker: buffer, + }, + deletedSandoxes: []string{}, + } + go wait.Until(func() { + for { + id, ok := <-buffer + if !ok { + close(stopCh) + break + } + testSandboxDeleter.deletedSandoxes = append(testSandboxDeleter.deletedSandoxes, id) + } + }, 0, stopCh) + + return testSandboxDeleter, stopCh +} + +func Test_podSandboxDeleter_deleteSandboxesInPod(t *testing.T) { + type args struct { + podStatus *kubecontainer.PodStatus + toKeep int + } + tests := []struct { + name string + args args + want []string + }{ + { + name: "ready sandboxes shouldn't be deleted ever", + args: args{ + podStatus: &kubecontainer.PodStatus{ + SandboxStatuses: []*runtimeapi.PodSandboxStatus{ + { + Id: "testsandbox", + State: runtimeapi.PodSandboxState_SANDBOX_READY, + }, + }, + }, + toKeep: 0, + }, + want: []string{}, + }, + { + name: "all unready sandboxes should be deleted if to keep is 0", + args: args{ + podStatus: &kubecontainer.PodStatus{ + SandboxStatuses: []*runtimeapi.PodSandboxStatus{ + { + Id: "testsandbox", + State: runtimeapi.PodSandboxState_SANDBOX_READY, + }, + { + Id: "testsandbox1", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + }, + { + Id: "testsandbox2", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + }, + }, + }, + toKeep: 0, + }, + want: []string{"testsandbox1", "testsandbox2"}, + }, + { + name: "sandboxes with containers shouldn't be deleted", + args: args{ + podStatus: &kubecontainer.PodStatus{ + ContainerStatuses: []*kubecontainer.Status{ + { + PodSandboxId: "testsandbox1", + }, + }, + SandboxStatuses: []*runtimeapi.PodSandboxStatus{ + { + Id: "testsandbox1", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + }, + { + Id: "testsandbox2", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + }, + }, + }, + toKeep: 0, + }, + want: []string{"testsandbox2"}, + }, + { + name: "latest unready sandboxes shouldn't be deleted if to keep is 1", + args: args{ + podStatus: &kubecontainer.PodStatus{ + SandboxStatuses: []*runtimeapi.PodSandboxStatus{ + { + Id: "testsandbox1", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + CreatedAt: time.Now().Add(time.Second).UnixNano(), + }, + { + Id: "testsandbox2", + State: runtimeapi.PodSandboxState_SANDBOX_NOTREADY, + CreatedAt: time.Now().Add(2 * time.Second).UnixNano(), + }, + }, + }, + toKeep: 1, + }, + want: []string{"testsandbox1"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p, stopCh := newTestPodSandboxDeleter() + p.deleteSandboxesInPod(tt.args.podStatus, tt.args.toKeep) + close(p.worker) + <-stopCh + assert.ElementsMatch(t, tt.want, p.deletedSandoxes, tt.name) + }) + } +} From 851d7785310a404163c37049f4db3dc9fe3f2451 Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Mon, 6 Jul 2020 17:01:54 -0700 Subject: [PATCH 3/5] address review comments --- pkg/kubelet/container/runtime.go | 4 ++-- pkg/kubelet/kuberuntime/kuberuntime_container.go | 2 +- pkg/kubelet/kuberuntime/kuberuntime_sandbox.go | 8 ++++---- pkg/kubelet/pod_sandbox_deleter.go | 5 ++--- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go index 322187287e2..3ed1e4f3db5 100644 --- a/pkg/kubelet/container/runtime.go +++ b/pkg/kubelet/container/runtime.go @@ -114,7 +114,7 @@ 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 - // Delete a sandbox. If the container is still running, an error is returned. + // DeleteSandbox deletes a sandbox. DeleteSandbox(sandboxID string) error // ImageService provides methods to image-related methods. ImageService @@ -311,7 +311,7 @@ type Status struct { // Name of the container. Name string // ID of the sandbox to which this container belongs. - PodSandboxId string + PodSandboxID string // Status of the container. State State // Creation time of the container. diff --git a/pkg/kubelet/kuberuntime/kuberuntime_container.go b/pkg/kubelet/kuberuntime/kuberuntime_container.go index ac0513ed37b..30f3b585662 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_container.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_container.go @@ -474,7 +474,7 @@ func (m *kubeGenericRuntimeManager) getPodContainerStatuses(uid kubetypes.UID, n cStatus.Message += tMessage } } - cStatus.PodSandboxId = c.PodSandboxId + cStatus.PodSandboxID = c.PodSandboxId statuses[i] = cStatus } diff --git a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go index 19606e1ce60..a278ce0af03 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go @@ -305,12 +305,12 @@ func (m *kubeGenericRuntimeManager) GetPortForward(podName, podNamespace string, return url.Parse(resp.Url) } -// DeleteSandbox removes the sandbox by sandboxID.. +// DeleteSandbox removes the sandbox by sandboxID. func (m *kubeGenericRuntimeManager) DeleteSandbox(sandboxID string) error { klog.V(4).Infof("Removing sandbox %q", sandboxID) - // the stop sandbox is called as part of kill pod but the error is ignored. So, - // we have to call stop sandbox again to make sure that all the resources like - // netwrork are cleaned by runtime. + // 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 } diff --git a/pkg/kubelet/pod_sandbox_deleter.go b/pkg/kubelet/pod_sandbox_deleter.go index dc52d3c724a..95fd80d863d 100644 --- a/pkg/kubelet/pod_sandbox_deleter.go +++ b/pkg/kubelet/pod_sandbox_deleter.go @@ -46,8 +46,7 @@ func (a sandboxStatusByCreatedList) Less(i, j int) bool { func newPodSandboxDeleter(runtime kubecontainer.Runtime) *podSandboxDeleter { buffer := make(chan string, sandboxDeletionBufferLimit) go wait.Forever(func() { - for { - id := <-buffer + for id := range buffer { if err := runtime.DeleteSandbox(id); err != nil { klog.Warningf("[pod_sandbox_deleter] DeleteSandbox returned error for (id=%v): %v", id, err) } @@ -64,7 +63,7 @@ func newPodSandboxDeleter(runtime kubecontainer.Runtime) *podSandboxDeleter { func (p *podSandboxDeleter) deleteSandboxesInPod(podStatus *kubecontainer.PodStatus, toKeep int) { sandboxIDs := sets.NewString() for _, containerStatus := range podStatus.ContainerStatuses { - sandboxIDs.Insert(containerStatus.PodSandboxId) + sandboxIDs.Insert(containerStatus.PodSandboxID) } sandboxStatuses := podStatus.SandboxStatuses if toKeep > 0 { From 872859b422df7be354102f4bc48715c103911d3d Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Tue, 7 Jul 2020 11:07:49 -0700 Subject: [PATCH 4/5] correct the sandboxId attribute in unit tests --- pkg/kubelet/kuberuntime/kuberuntime_manager_test.go | 2 +- pkg/kubelet/pod_sandbox_deleter_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index e38630615e5..bf9af3e9efc 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -327,7 +327,7 @@ func TestGetPodStatus(t *testing.T) { 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) + assert.Equal(t, sandbox.Id, containerStatus.PodSandboxID) } } diff --git a/pkg/kubelet/pod_sandbox_deleter_test.go b/pkg/kubelet/pod_sandbox_deleter_test.go index 16d59f0b5af..189ec246774 100644 --- a/pkg/kubelet/pod_sandbox_deleter_test.go +++ b/pkg/kubelet/pod_sandbox_deleter_test.go @@ -108,7 +108,7 @@ func Test_podSandboxDeleter_deleteSandboxesInPod(t *testing.T) { podStatus: &kubecontainer.PodStatus{ ContainerStatuses: []*kubecontainer.Status{ { - PodSandboxId: "testsandbox1", + PodSandboxID: "testsandbox1", }, }, SandboxStatuses: []*runtimeapi.PodSandboxStatus{ From acac15c20e5effb079502f8cb047be5871853f80 Mon Sep 17 00:00:00 2001 From: "Keerthan Reddy,Mala" Date: Wed, 22 Jul 2020 14:12:27 -0700 Subject: [PATCH 5/5] fix bazel build file --- pkg/kubelet/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 0ec01f97aee..9e05e5c24cd 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -255,8 +255,8 @@ 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", - "//vendor/github.com/golang/groupcache/lru: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",