diff --git a/pkg/controller/node/controller_utils.go b/pkg/controller/node/controller_utils.go index 0d7bae3a75ea..c237b6bdccc6 100644 --- a/pkg/controller/node/controller_utils.go +++ b/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 } diff --git a/pkg/controller/node/nodecontroller_test.go b/pkg/controller/node/nodecontroller_test.go index 8449ccd1919d..0e66f142394e 100644 --- a/pkg/controller/node/nodecontroller_test.go +++ b/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, }, } diff --git a/pkg/controller/podgc/BUILD b/pkg/controller/podgc/BUILD index c8af1ea1dc6f..e3e7af3a5529 100644 --- a/pkg/controller/podgc/BUILD +++ b/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", ], ) diff --git a/pkg/controller/podgc/gc_controller.go b/pkg/controller/podgc/gc_controller.go index f640e1d856a8..dea856ca97d1 100644 --- a/pkg/controller/podgc/gc_controller.go +++ b/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) } } } diff --git a/pkg/controller/podgc/gc_controller_test.go b/pkg/controller/podgc/gc_controller_test.go index ab94d874cca3..02558a80eb0b 100644 --- a/pkg/controller/podgc/gc_controller_test.go +++ b/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) + } + } +}