From 4a3d51359a7e2962d36d0222425c8ab158dcb07a Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Mon, 11 Jul 2022 17:14:10 +0200 Subject: [PATCH 1/3] Refact GC controller to do not use stub deletePod --- pkg/controller/podgc/gc_controller.go | 12 +++-- pkg/controller/podgc/gc_controller_test.go | 55 +++++++++------------- 2 files changed, 28 insertions(+), 39 deletions(-) diff --git a/pkg/controller/podgc/gc_controller.go b/pkg/controller/podgc/gc_controller.go index bf64bd2208e..7be51b837f5 100644 --- a/pkg/controller/podgc/gc_controller.go +++ b/pkg/controller/podgc/gc_controller.go @@ -52,6 +52,7 @@ const ( type PodGCController struct { kubeClient clientset.Interface + ctx context.Context podLister corelisters.PodLister podListerSynced cache.InformerSynced @@ -60,7 +61,6 @@ type PodGCController struct { nodeQueue workqueue.DelayingInterface - deletePod func(namespace, name string) error terminatedPodThreshold int } @@ -71,21 +71,23 @@ func NewPodGC(ctx context.Context, kubeClient clientset.Interface, podInformer c } gcc := &PodGCController{ kubeClient: kubeClient, + ctx: ctx, terminatedPodThreshold: terminatedPodThreshold, podLister: podInformer.Lister(), podListerSynced: podInformer.Informer().HasSynced, nodeLister: nodeInformer.Lister(), nodeListerSynced: nodeInformer.Informer().HasSynced, nodeQueue: workqueue.NewNamedDelayingQueue("orphaned_pods_nodes"), - deletePod: func(namespace, name string) error { - klog.InfoS("PodGC is force deleting Pod", "pod", klog.KRef(namespace, name)) - return kubeClient.CoreV1().Pods(namespace).Delete(ctx, name, *metav1.NewDeleteOptions(0)) - }, } return gcc } +func (gcc *PodGCController) deletePod(namespace, name string) error { + klog.InfoS("PodGC is force deleting Pod", "pod", klog.KRef(namespace, name)) + return gcc.kubeClient.CoreV1().Pods(namespace).Delete(gcc.ctx, name, *metav1.NewDeleteOptions(0)) +} + func (gcc *PodGCController) Run(ctx context.Context) { defer utilruntime.HandleCrash() diff --git a/pkg/controller/podgc/gc_controller_test.go b/pkg/controller/podgc/gc_controller_test.go index 4805f36293f..05aa212d221 100644 --- a/pkg/controller/podgc/gc_controller_test.go +++ b/pkg/controller/podgc/gc_controller_test.go @@ -18,7 +18,6 @@ package podgc import ( "context" - "sync" "testing" "time" @@ -32,6 +31,7 @@ import ( coreinformers "k8s.io/client-go/informers/core/v1" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/util/workqueue" featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/controller" @@ -63,6 +63,17 @@ func compareStringSetToList(set sets.String, list []string) bool { return true } +func getDeletedPodNames(client *fake.Clientset) []string { + deletedPodNames := make([]string, 0) + for _, action := range client.Actions() { + if action.GetVerb() == "delete" && action.GetResource().Resource == "pods" { + deleteAction := action.(clienttesting.DeleteAction) + deletedPodNames = append(deletedPodNames, deleteAction.GetName()) + } + } + return deletedPodNames +} + func TestGCTerminated(t *testing.T) { type nameToPhase struct { name string @@ -129,14 +140,6 @@ func TestGCTerminated(t *testing.T) { t.Run(test.name, func(t *testing.T) { client := fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{*testutil.NewNode("node")}}) gcc, podInformer, _ := NewFromClient(client, test.threshold) - deletedPodNames := make([]string, 0) - var lock sync.Mutex - gcc.deletePod = func(_, name string) error { - lock.Lock() - defer lock.Unlock() - deletedPodNames = append(deletedPodNames, name) - return nil - } creationTime := time.Unix(0, 0) for _, pod := range test.pods { @@ -150,6 +153,8 @@ func TestGCTerminated(t *testing.T) { gcc.gc(context.TODO()) + deletedPodNames := getDeletedPodNames(client) + if pass := compareStringSetToList(test.deletedPodNames, deletedPodNames); !pass { t.Errorf("[%v]pod's deleted expected and actual did not match.\n\texpected: %v\n\tactual: %v", i, test.deletedPodNames.List(), deletedPodNames) @@ -329,17 +334,10 @@ func TestGCOrphaned(t *testing.T) { gcc.nodeQueue.ShutDown() gcc.nodeQueue = workqueue.NewDelayingQueueWithCustomClock(fakeClock, "podgc_test_queue") - deletedPodNames := make([]string, 0) - var lock sync.Mutex - gcc.deletePod = func(_, name string) error { - lock.Lock() - defer lock.Unlock() - deletedPodNames = append(deletedPodNames, name) - return nil - } - // First GC of orphaned pods gcc.gc(context.TODO()) + deletedPodNames := getDeletedPodNames(client) + if len(deletedPodNames) > 0 { t.Errorf("no pods should be deleted at this point.\n\tactual: %v", deletedPodNames) } @@ -371,6 +369,7 @@ func TestGCOrphaned(t *testing.T) { // Actual pod deletion gcc.gc(context.TODO()) + deletedPodNames = getDeletedPodNames(client) if pass := compareStringSetToList(test.deletedPodNames, deletedPodNames); !pass { t.Errorf("pod's deleted expected and actual did not match.\n\texpected: %v\n\tactual: %v", @@ -417,14 +416,6 @@ func TestGCUnscheduledTerminating(t *testing.T) { t.Run(test.name, func(t *testing.T) { client := fake.NewSimpleClientset() gcc, podInformer, _ := NewFromClient(client, -1) - deletedPodNames := make([]string, 0) - var lock sync.Mutex - gcc.deletePod = func(_, name string) error { - lock.Lock() - defer lock.Unlock() - deletedPodNames = append(deletedPodNames, name) - return nil - } creationTime := time.Unix(0, 0) for _, pod := range test.pods { @@ -443,6 +434,7 @@ func TestGCUnscheduledTerminating(t *testing.T) { return } gcc.gcUnscheduledTerminating(pods) + deletedPodNames := getDeletedPodNames(client) if pass := compareStringSetToList(test.deletedPodNames, deletedPodNames); !pass { t.Errorf("[%v]pod's deleted expected and actual did not match.\n\texpected: %v\n\tactual: %v, test: %v", @@ -557,14 +549,7 @@ func TestGCTerminating(t *testing.T) { t.Run(test.name, func(t *testing.T) { client := fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{*testutil.NewNode("node-a")}}) gcc, podInformer, nodeInformer := NewFromClient(client, -1) - deletedPodNames := make([]string, 0) - var lock sync.Mutex - gcc.deletePod = func(_, name string) error { - lock.Lock() - defer lock.Unlock() - deletedPodNames = append(deletedPodNames, name) - return nil - } + creationTime := time.Unix(0, 0) for _, node := range test.nodes { creationTime = creationTime.Add(2 * time.Hour) @@ -595,6 +580,8 @@ func TestGCTerminating(t *testing.T) { } gcc.gc(context.TODO()) + deletedPodNames := getDeletedPodNames(client) + if pass := compareStringSetToList(test.deletedPodNames, deletedPodNames); !pass { t.Errorf("[%v]pod's deleted expected and actual did not match.\n\texpected: %v\n\tactual: %v", i, test.deletedPodNames.List(), deletedPodNames) From 2730d285cf723f58ac4edd4d258a8b7929d755f8 Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Mon, 11 Jul 2022 17:43:40 +0200 Subject: [PATCH 2/3] do not store context --- pkg/controller/podgc/gc_controller.go | 25 +++++++++++----------- pkg/controller/podgc/gc_controller_test.go | 2 +- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/pkg/controller/podgc/gc_controller.go b/pkg/controller/podgc/gc_controller.go index 7be51b837f5..c87d01aff7d 100644 --- a/pkg/controller/podgc/gc_controller.go +++ b/pkg/controller/podgc/gc_controller.go @@ -71,7 +71,6 @@ func NewPodGC(ctx context.Context, kubeClient clientset.Interface, podInformer c } gcc := &PodGCController{ kubeClient: kubeClient, - ctx: ctx, terminatedPodThreshold: terminatedPodThreshold, podLister: podInformer.Lister(), podListerSynced: podInformer.Informer().HasSynced, @@ -83,9 +82,9 @@ func NewPodGC(ctx context.Context, kubeClient clientset.Interface, podInformer c return gcc } -func (gcc *PodGCController) deletePod(namespace, name string) error { +func (gcc *PodGCController) deletePod(ctx context.Context, namespace, name string) error { klog.InfoS("PodGC is force deleting Pod", "pod", klog.KRef(namespace, name)) - return gcc.kubeClient.CoreV1().Pods(namespace).Delete(gcc.ctx, name, *metav1.NewDeleteOptions(0)) + return gcc.kubeClient.CoreV1().Pods(namespace).Delete(ctx, name, *metav1.NewDeleteOptions(0)) } func (gcc *PodGCController) Run(ctx context.Context) { @@ -116,13 +115,13 @@ func (gcc *PodGCController) gc(ctx context.Context) { return } if gcc.terminatedPodThreshold > 0 { - gcc.gcTerminated(pods) + gcc.gcTerminated(ctx, pods) } if utilfeature.DefaultFeatureGate.Enabled(features.NodeOutOfServiceVolumeDetach) { - gcc.gcTerminating(pods) + gcc.gcTerminating(ctx, pods) } gcc.gcOrphaned(ctx, pods, nodes) - gcc.gcUnscheduledTerminating(pods) + gcc.gcUnscheduledTerminating(ctx, pods) } func isPodTerminated(pod *v1.Pod) bool { @@ -137,7 +136,7 @@ func isPodTerminating(pod *v1.Pod) bool { return pod.ObjectMeta.DeletionTimestamp != nil } -func (gcc *PodGCController) gcTerminating(pods []*v1.Pod) { +func (gcc *PodGCController) gcTerminating(ctx context.Context, pods []*v1.Pod) { klog.V(4).Info("GC'ing terminating pods that are on out-of-service nodes") terminatingPods := []*v1.Pod{} for _, pod := range pods { @@ -170,7 +169,7 @@ func (gcc *PodGCController) gcTerminating(pods []*v1.Pod) { wait.Add(1) go func(namespace string, name string) { defer wait.Done() - if err := gcc.deletePod(namespace, name); err != nil { + if err := gcc.deletePod(ctx, namespace, name); err != nil { // ignore not founds utilruntime.HandleError(err) } @@ -179,7 +178,7 @@ func (gcc *PodGCController) gcTerminating(pods []*v1.Pod) { wait.Wait() } -func (gcc *PodGCController) gcTerminated(pods []*v1.Pod) { +func (gcc *PodGCController) gcTerminated(ctx context.Context, pods []*v1.Pod) { terminatedPods := []*v1.Pod{} for _, pod := range pods { if isPodTerminated(pod) { @@ -202,7 +201,7 @@ func (gcc *PodGCController) gcTerminated(pods []*v1.Pod) { wait.Add(1) go func(namespace string, name string) { defer wait.Done() - if err := gcc.deletePod(namespace, name); err != nil { + if err := gcc.deletePod(ctx, namespace, name); err != nil { // ignore not founds defer utilruntime.HandleError(err) } @@ -235,7 +234,7 @@ func (gcc *PodGCController) gcOrphaned(ctx context.Context, pods []*v1.Pod, node continue } klog.V(2).InfoS("Found orphaned Pod assigned to the Node, deleting.", "pod", klog.KObj(pod), "node", pod.Spec.NodeName) - if err := gcc.deletePod(pod.Namespace, pod.Name); err != nil { + if err := gcc.deletePod(ctx, pod.Namespace, pod.Name); err != nil { utilruntime.HandleError(err) } else { klog.V(0).InfoS("Forced deletion of orphaned Pod succeeded", "pod", klog.KObj(pod)) @@ -275,7 +274,7 @@ func (gcc *PodGCController) checkIfNodeExists(ctx context.Context, name string) } // gcUnscheduledTerminating deletes pods that are terminating and haven't been scheduled to a particular node. -func (gcc *PodGCController) gcUnscheduledTerminating(pods []*v1.Pod) { +func (gcc *PodGCController) gcUnscheduledTerminating(ctx context.Context, pods []*v1.Pod) { klog.V(4).Infof("GC'ing unscheduled pods which are terminating.") for _, pod := range pods { @@ -284,7 +283,7 @@ func (gcc *PodGCController) gcUnscheduledTerminating(pods []*v1.Pod) { } klog.V(2).InfoS("Found unscheduled terminating Pod not assigned to any Node, deleting.", "pod", klog.KObj(pod)) - if err := gcc.deletePod(pod.Namespace, pod.Name); err != nil { + if err := gcc.deletePod(ctx, pod.Namespace, pod.Name); err != nil { utilruntime.HandleError(err) } else { klog.V(0).InfoS("Forced deletion of unscheduled terminating Pod succeeded", "pod", klog.KObj(pod)) diff --git a/pkg/controller/podgc/gc_controller_test.go b/pkg/controller/podgc/gc_controller_test.go index 05aa212d221..75906cf5bbd 100644 --- a/pkg/controller/podgc/gc_controller_test.go +++ b/pkg/controller/podgc/gc_controller_test.go @@ -433,7 +433,7 @@ func TestGCUnscheduledTerminating(t *testing.T) { t.Errorf("Error while listing all Pods: %v", err) return } - gcc.gcUnscheduledTerminating(pods) + gcc.gcUnscheduledTerminating(context.TODO(), pods) deletedPodNames := getDeletedPodNames(client) if pass := compareStringSetToList(test.deletedPodNames, deletedPodNames); !pass { From 778b8300bcb5d365420523d9fbd3088519caff70 Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Tue, 12 Jul 2022 10:16:00 +0200 Subject: [PATCH 3/3] fix nits --- pkg/controller/podgc/gc_controller.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/controller/podgc/gc_controller.go b/pkg/controller/podgc/gc_controller.go index c87d01aff7d..34dfbd760be 100644 --- a/pkg/controller/podgc/gc_controller.go +++ b/pkg/controller/podgc/gc_controller.go @@ -52,7 +52,6 @@ const ( type PodGCController struct { kubeClient clientset.Interface - ctx context.Context podLister corelisters.PodLister podListerSynced cache.InformerSynced @@ -82,11 +81,6 @@ func NewPodGC(ctx context.Context, kubeClient clientset.Interface, podInformer c return gcc } -func (gcc *PodGCController) deletePod(ctx context.Context, namespace, name string) error { - klog.InfoS("PodGC is force deleting Pod", "pod", klog.KRef(namespace, name)) - return gcc.kubeClient.CoreV1().Pods(namespace).Delete(ctx, name, *metav1.NewDeleteOptions(0)) -} - func (gcc *PodGCController) Run(ctx context.Context) { defer utilruntime.HandleCrash() @@ -303,3 +297,8 @@ func (o byCreationTimestamp) Less(i, j int) bool { } return o[i].CreationTimestamp.Before(&o[j].CreationTimestamp) } + +func (gcc *PodGCController) deletePod(ctx context.Context, namespace, name string) error { + klog.InfoS("PodGC is force deleting Pod", "pod", klog.KRef(namespace, name)) + return gcc.kubeClient.CoreV1().Pods(namespace).Delete(ctx, name, *metav1.NewDeleteOptions(0)) +}