From e9e8fe6c32f28a79dfc13b648d0632c795ce841e Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Mon, 6 Mar 2017 14:14:40 -0800 Subject: [PATCH] RC/RS: Fix ignoring inactive Pods. --- pkg/controller/replicaset/replica_set.go | 4 ++-- pkg/controller/replicaset/replica_set_test.go | 8 +++++++- pkg/controller/replication/replication_controller.go | 6 +++--- .../replication/replication_controller_test.go | 10 ++++++++-- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/pkg/controller/replicaset/replica_set.go b/pkg/controller/replicaset/replica_set.go index 91a1b87d33..c20906cfae 100644 --- a/pkg/controller/replicaset/replica_set.go +++ b/pkg/controller/replicaset/replica_set.go @@ -553,13 +553,13 @@ func (rsc *ReplicaSetController) syncReplicaSet(key string) error { // list all pods to include the pods that don't match the rs`s selector // anymore but has the stale controller ref. // TODO: Do the List and Filter in a single pass, or use an index. - pods, err := rsc.podLister.Pods(rs.Namespace).List(labels.Everything()) + allPods, err := rsc.podLister.Pods(rs.Namespace).List(labels.Everything()) if err != nil { return err } // Ignore inactive pods. var filteredPods []*v1.Pod - for _, pod := range pods { + for _, pod := range allPods { if controller.IsPodActive(pod) { filteredPods = append(filteredPods, pod) } diff --git a/pkg/controller/replicaset/replica_set_test.go b/pkg/controller/replicaset/replica_set_test.go index ae79874dd9..e34e07ed85 100644 --- a/pkg/controller/replicaset/replica_set_test.go +++ b/pkg/controller/replicaset/replica_set_test.go @@ -305,10 +305,16 @@ func TestSyncReplicaSetCreates(t *testing.T) { defer close(stopCh) manager, informers := testNewReplicaSetControllerFromClient(client, stopCh, BurstReplicas) - // A controller with 2 replicas and no pods in the store, 2 creates expected + // A controller with 2 replicas and no active pods in the store. + // Inactive pods should be ignored. 2 creates expected. labelMap := map[string]string{"foo": "bar"} rs := newReplicaSet(2, labelMap) informers.Extensions().V1beta1().ReplicaSets().Informer().GetIndexer().Add(rs) + failedPod := newPod("failed-pod", rs, v1.PodFailed, nil, true) + deletedPod := newPod("deleted-pod", rs, v1.PodRunning, nil, true) + deletedPod.DeletionTimestamp = &metav1.Time{Time: time.Now()} + informers.Core().V1().Pods().Informer().GetIndexer().Add(failedPod) + informers.Core().V1().Pods().Informer().GetIndexer().Add(deletedPod) fakePodControl := controller.FakePodControl{} manager.podControl = &fakePodControl diff --git a/pkg/controller/replication/replication_controller.go b/pkg/controller/replication/replication_controller.go index 8818f2e1d2..9ef5ff0e0f 100644 --- a/pkg/controller/replication/replication_controller.go +++ b/pkg/controller/replication/replication_controller.go @@ -571,13 +571,13 @@ func (rm *ReplicationManager) syncReplicationController(key string) error { // list all pods to include the pods that don't match the rc's selector // anymore but has the stale controller ref. // TODO: Do the List and Filter in a single pass, or use an index. - pods, err := rm.podLister.Pods(rc.Namespace).List(labels.Everything()) + allPods, err := rm.podLister.Pods(rc.Namespace).List(labels.Everything()) if err != nil { return err } // Ignore inactive pods. var filteredPods []*v1.Pod - for _, pod := range pods { + for _, pod := range allPods { if controller.IsPodActive(pod) { filteredPods = append(filteredPods, pod) } @@ -585,7 +585,7 @@ func (rm *ReplicationManager) syncReplicationController(key string) error { cm := controller.NewPodControllerRefManager(rm.podControl, rc, labels.Set(rc.Spec.Selector).AsSelectorPreValidated(), controllerKind) // NOTE: filteredPods are pointing to objects from cache - if you need to // modify them, you need to copy it first. - filteredPods, err = cm.ClaimPods(pods) + filteredPods, err = cm.ClaimPods(filteredPods) if err != nil { return err } diff --git a/pkg/controller/replication/replication_controller_test.go b/pkg/controller/replication/replication_controller_test.go index 23ce15afa9..0353aea169 100644 --- a/pkg/controller/replication/replication_controller_test.go +++ b/pkg/controller/replication/replication_controller_test.go @@ -259,11 +259,17 @@ func TestDeleteFinalStateUnknown(t *testing.T) { func TestSyncReplicationControllerCreates(t *testing.T) { c := clientset.NewForConfigOrDie(&restclient.Config{Host: "", ContentConfig: restclient.ContentConfig{GroupVersion: &api.Registry.GroupOrDie(v1.GroupName).GroupVersion}}) - manager, _, rcInformer := newReplicationManagerFromClient(c, BurstReplicas) + manager, podInformer, rcInformer := newReplicationManagerFromClient(c, BurstReplicas) - // A controller with 2 replicas and no pods in the store, 2 creates expected + // A controller with 2 replicas and no active pods in the store. + // Inactive pods should be ignored. 2 creates expected. rc := newReplicationController(2) rcInformer.Informer().GetIndexer().Add(rc) + failedPod := newPod("failed-pod", rc, v1.PodFailed, nil, true) + deletedPod := newPod("deleted-pod", rc, v1.PodRunning, nil, true) + deletedPod.DeletionTimestamp = &metav1.Time{Time: time.Now()} + podInformer.Informer().GetIndexer().Add(failedPod) + podInformer.Informer().GetIndexer().Add(deletedPod) fakePodControl := controller.FakePodControl{} manager.podControl = &fakePodControl