Merge pull request #35541 from foxish/deletions-safe-again

Automatic merge from submit-queue

Moving some force deletion logic from the NC into the PodGC

**What this PR does / why we need it**: Moves some pod force-deletion behavior into the PodGC, which is a better place for these.

This should be a NOP because we're just moving functionality
around and thanks to #35476, the podGC controller should always
run.

Related: https://github.com/kubernetes/kubernetes/pull/34160, https://github.com/kubernetes/kubernetes/issues/35145

cc @gmarek @kubernetes/sig-apps

pkg/controller/node/controller_utils.go

@@ -135,12 +135,6 @@ func (nc *NodeController) maybeDeleteTerminatingPod(obj interface{}) {
 		return
 	}
 
-	// delete terminating pods that have not yet been scheduled
-	if len(pod.Spec.NodeName) == 0 {
-		utilruntime.HandleError(nc.forcefullyDeletePod(pod))
-		return
-	}
-
 	nodeObj, found, err := nc.nodeStore.Store.GetByKey(pod.Spec.NodeName)
 	if err != nil {
 		// this can only happen if the Store.KeyFunc has a problem creating
@@ -150,11 +144,8 @@ func (nc *NodeController) maybeDeleteTerminatingPod(obj interface{}) {
 		return
 	}
 
-	// delete terminating pods that have been scheduled on
-	// nonexistent nodes
+	// if there is no such node, do nothing and let the podGC clean it up.
 	if !found {
-		glog.Warningf("Unable to find Node: %v, deleting all assigned Pods.", pod.Spec.NodeName)
-		utilruntime.HandleError(nc.forcefullyDeletePod(pod))
 		return
 	}
 

pkg/controller/node/nodecontroller_test.go

@@ -1723,14 +1723,14 @@ func TestCheckPod(t *testing.T) {
 				ObjectMeta: api.ObjectMeta{DeletionTimestamp: &unversioned.Time{}},
 				Spec:       api.PodSpec{NodeName: ""},
 			},
-			prune: true,
+			prune: false,
 		},
 		{
 			pod: api.Pod{
 				ObjectMeta: api.ObjectMeta{DeletionTimestamp: &unversioned.Time{}},
 				Spec:       api.PodSpec{NodeName: "nonexistant"},
 			},
-			prune: true,
+			prune: false,
 		},
 	}
 

pkg/controller/podgc/BUILD

@@ -43,6 +43,7 @@ go_test(
         "//pkg/api/unversioned:go_default_library",
         "//pkg/client/cache:go_default_library",
         "//pkg/client/clientset_generated/internalclientset/fake:go_default_library",
+        "//pkg/labels:go_default_library",
         "//pkg/util/sets:go_default_library",
     ],
 )

pkg/controller/podgc/gc_controller.go

@@ -125,6 +125,7 @@ func (gcc *PodGCController) gc() {
 		gcc.gcTerminated(pods)
 	}
 	gcc.gcOrphaned(pods)
+	gcc.gcUnscheduledTerminating(pods)
 }
 
 func isPodTerminated(pod *api.Pod) bool {
@@ -168,7 +169,7 @@ func (gcc *PodGCController) gcTerminated(pods []*api.Pod) {
 	wait.Wait()
 }
 
-// cleanupOrphanedPods deletes pods that are bound to nodes that don't exist.
+// gcOrphaned deletes pods that are bound to nodes that don't exist.
 func (gcc *PodGCController) gcOrphaned(pods []*api.Pod) {
 	glog.V(4).Infof("GC'ing orphaned")
 
@@ -183,7 +184,25 @@ func (gcc *PodGCController) gcOrphaned(pods []*api.Pod) {
 		if err := gcc.deletePod(pod.Namespace, pod.Name); err != nil {
 			utilruntime.HandleError(err)
 		} else {
-			glog.V(4).Infof("Forced deletion of oprhaned Pod %s succeeded", pod.Name)
+			glog.V(0).Infof("Forced deletion of orphaned Pod %s succeeded", pod.Name)
+		}
+	}
+}
+
+// gcUnscheduledTerminating deletes pods that are terminating and haven't been scheduled to a particular node.
+func (gcc *PodGCController) gcUnscheduledTerminating(pods []*api.Pod) {
+	glog.V(4).Infof("GC'ing unscheduled pods which are terminating.")
+
+	for _, pod := range pods {
+		if pod.DeletionTimestamp == nil || len(pod.Spec.NodeName) > 0 {
+			continue
+		}
+
+		glog.V(2).Infof("Found unscheduled terminating Pod %v not assigned to any Node. Deleting.", pod.Name)
+		if err := gcc.deletePod(pod.Namespace, pod.Name); err != nil {
+			utilruntime.HandleError(err)
+		} else {
+			glog.V(0).Infof("Forced deletion of unscheduled terminating Pod %s succeeded", pod.Name)
 		}
 	}
 }

pkg/controller/podgc/gc_controller_test.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/client/cache"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/labels"
 	"k8s.io/kubernetes/pkg/util/sets"
 )
 
@@ -194,7 +195,12 @@ func TestGCOrphaned(t *testing.T) {
 		gcc.podController = &FakeController{}
 		gcc.nodeController = &FakeController{}
 
-		gcc.gc()
+		pods, err := gcc.podStore.List(labels.Everything())
+		if err != nil {
+			t.Errorf("Error while listing all Pods: %v", err)
+			return
+		}
+		gcc.gcOrphaned(pods)
 
 		pass := true
 		for _, pod := range deletedPodNames {
@@ -210,3 +216,86 @@ func TestGCOrphaned(t *testing.T) {
 		}
 	}
 }
+
+func TestGCUnscheduledTerminating(t *testing.T) {
+	type nameToPhase struct {
+		name              string
+		phase             api.PodPhase
+		deletionTimeStamp *unversioned.Time
+		nodeName          string
+	}
+
+	testCases := []struct {
+		name            string
+		pods            []nameToPhase
+		deletedPodNames sets.String
+	}{
+		{
+			name: "Unscheduled pod in any phase must be deleted",
+			pods: []nameToPhase{
+				{name: "a", phase: api.PodFailed, deletionTimeStamp: &unversioned.Time{}, nodeName: ""},
+				{name: "b", phase: api.PodSucceeded, deletionTimeStamp: &unversioned.Time{}, nodeName: ""},
+				{name: "c", phase: api.PodRunning, deletionTimeStamp: &unversioned.Time{}, nodeName: ""},
+			},
+			deletedPodNames: sets.NewString("a", "b", "c"),
+		},
+		{
+			name: "Scheduled pod in any phase must not be deleted",
+			pods: []nameToPhase{
+				{name: "a", phase: api.PodFailed, deletionTimeStamp: nil, nodeName: ""},
+				{name: "b", phase: api.PodSucceeded, deletionTimeStamp: nil, nodeName: "node"},
+				{name: "c", phase: api.PodRunning, deletionTimeStamp: &unversioned.Time{}, nodeName: "node"},
+			},
+			deletedPodNames: sets.NewString(),
+		},
+	}
+
+	for i, test := range testCases {
+		client := fake.NewSimpleClientset()
+		gcc := 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 {
+			creationTime = creationTime.Add(1 * time.Hour)
+			gcc.podStore.Indexer.Add(&api.Pod{
+				ObjectMeta: api.ObjectMeta{Name: pod.name, CreationTimestamp: unversioned.Time{Time: creationTime},
+					DeletionTimestamp: pod.deletionTimeStamp},
+				Status: api.PodStatus{Phase: pod.phase},
+				Spec:   api.PodSpec{NodeName: pod.nodeName},
+			})
+		}
+
+		store := cache.NewStore(cache.MetaNamespaceKeyFunc)
+		gcc.nodeStore = cache.StoreToNodeLister{Store: store}
+		gcc.podController = &FakeController{}
+		gcc.nodeController = &FakeController{}
+
+		pods, err := gcc.podStore.List(labels.Everything())
+		if err != nil {
+			t.Errorf("Error while listing all Pods: %v", err)
+			return
+		}
+		gcc.gcUnscheduledTerminating(pods)
+
+		pass := true
+		for _, pod := range deletedPodNames {
+			if !test.deletedPodNames.Has(pod) {
+				pass = false
+			}
+		}
+		if len(deletedPodNames) != len(test.deletedPodNames) {
+			pass = false
+		}
+		if !pass {
+			t.Errorf("[%v]pod's deleted expected and actual did not match.\n\texpected: %v\n\tactual: %v, test: %v", i, test.deletedPodNames, deletedPodNames, test.name)
+		}
+	}
+}