diff --git a/pkg/controller/replicaset/replica_set.go b/pkg/controller/replicaset/replica_set.go index e26ce810c8c..6d6ddffb51b 100644 --- a/pkg/controller/replicaset/replica_set.go +++ b/pkg/controller/replicaset/replica_set.go @@ -326,12 +326,8 @@ func (rsc *ReplicaSetController) updatePod(old, cur interface{}) { } curPod := cur.(*api.Pod) oldPod := old.(*api.Pod) - glog.V(4).Infof("Pod %s updated %+v -> %+v.", curPod.Name, oldPod, curPod) - rs := rsc.getPodReplicaSet(curPod) - if rs == nil { - return - } - + glog.V(4).Infof("Pod %s updated, objectMeta %+v -> %+v.", curPod.Name, oldPod.ObjectMeta, curPod.ObjectMeta) + labelChanged := !reflect.DeepEqual(curPod.Labels, oldPod.Labels) if curPod.DeletionTimestamp != nil { // when a pod is deleted gracefully it's deletion timestamp is first modified to reflect a grace period, // and after such time has passed, the kubelet actually deletes it from the store. We receive an update @@ -339,11 +335,17 @@ func (rsc *ReplicaSetController) updatePod(old, cur interface{}) { // until the kubelet actually deletes the pod. This is different from the Phase of a pod changing, because // an rs never initiates a phase change, and so is never asleep waiting for the same. rsc.deletePod(curPod) + if labelChanged { + // we don't need to check the oldPod.DeletionTimestamp because DeletionTimestamp cannot be unset. + rsc.deletePod(oldPod) + } return } - rsc.enqueueReplicaSet(rs) - if !reflect.DeepEqual(curPod.Labels, oldPod.Labels) { + if rs := rsc.getPodReplicaSet(curPod); rs != nil { + rsc.enqueueReplicaSet(rs) + } + if labelChanged { // If the old and new ReplicaSet are the same, the first one that syncs // will set expectations preventing any damage from the second. if oldRS := rsc.getPodReplicaSet(oldPod); oldRS != nil { diff --git a/pkg/controller/replicaset/replica_set_test.go b/pkg/controller/replicaset/replica_set_test.go index d8324ebb0a8..fbdfc497fbe 100644 --- a/pkg/controller/replicaset/replica_set_test.go +++ b/pkg/controller/replicaset/replica_set_test.go @@ -547,15 +547,13 @@ func TestUpdatePods(t *testing.T) { testRSSpec2.Name = "barfoo" manager.rsStore.Store.Add(&testRSSpec2) - // Put one pod in the podStore + // case 1: We put in the podStore a pod with labels matching testRSSpec1, + // then update its labels to match testRSSpec2. We expect to receive a sync + // request for both replica sets. pod1 := newPodList(manager.podStore.Indexer, 1, api.PodRunning, labelMap1, testRSSpec1, "pod").Items[0] pod2 := pod1 pod2.Labels = labelMap2 - - // Send an update of the same pod with modified labels, and confirm we get a sync request for - // both controllers manager.updatePod(&pod1, &pod2) - expected := sets.NewString(testRSSpec1.Name, testRSSpec2.Name) for _, name := range expected.List() { t.Logf("Expecting update for %+v", name) @@ -568,6 +566,24 @@ func TestUpdatePods(t *testing.T) { t.Errorf("Expected update notifications for replica sets within 100ms each") } } + + // case 2: pod1 in the podStore has labels matching testRSSpec1. We update + // its labels to match no replica set. We expect to receive a sync request + // for testRSSpec1. + pod2.Labels = make(map[string]string) + manager.updatePod(&pod1, &pod2) + expected = sets.NewString(testRSSpec1.Name) + for _, name := range expected.List() { + t.Logf("Expecting update for %+v", name) + select { + case got := <-received: + if !expected.Has(got) { + t.Errorf("Expected keys %#v got %v", expected, got) + } + case <-time.After(wait.ForeverTestTimeout): + t.Errorf("Expected update notifications for replica sets within 100ms each") + } + } } func TestControllerUpdateRequeue(t *testing.T) { diff --git a/pkg/controller/replication/replication_controller.go b/pkg/controller/replication/replication_controller.go index 6fda7ce8969..2b8406a1219 100644 --- a/pkg/controller/replication/replication_controller.go +++ b/pkg/controller/replication/replication_controller.go @@ -353,12 +353,9 @@ func (rm *ReplicationManager) updatePod(old, cur interface{}) { return } curPod := cur.(*api.Pod) - rc := rm.getPodController(curPod) - if rc == nil { - return - } oldPod := old.(*api.Pod) - + glog.V(4).Infof("Pod %s updated, objectMeta %+v -> %+v.", curPod.Name, oldPod.ObjectMeta, curPod.ObjectMeta) + labelChanged := !reflect.DeepEqual(curPod.Labels, oldPod.Labels) if curPod.DeletionTimestamp != nil { // when a pod is deleted gracefully it's deletion timestamp is first modified to reflect a grace period, // and after such time has passed, the kubelet actually deletes it from the store. We receive an update @@ -366,11 +363,18 @@ func (rm *ReplicationManager) updatePod(old, cur interface{}) { // until the kubelet actually deletes the pod. This is different from the Phase of a pod changing, because // an rc never initiates a phase change, and so is never asleep waiting for the same. rm.deletePod(curPod) + if labelChanged { + // we don't need to check the oldPod.DeletionTimestamp because DeletionTimestamp cannot be unset. + rm.deletePod(oldPod) + } return } - rm.enqueueController(rc) + + if rc := rm.getPodController(curPod); rc != nil { + rm.enqueueController(rc) + } // Only need to get the old controller if the labels changed. - if !reflect.DeepEqual(curPod.Labels, oldPod.Labels) { + if labelChanged { // If the old and new rc are the same, the first one that syncs // will set expectations preventing any damage from the second. if oldRC := rm.getPodController(oldPod); oldRC != nil { diff --git a/pkg/controller/replication/replication_controller_test.go b/pkg/controller/replication/replication_controller_test.go index 376e5a4b9c9..52254f27074 100644 --- a/pkg/controller/replication/replication_controller_test.go +++ b/pkg/controller/replication/replication_controller_test.go @@ -531,15 +531,13 @@ func TestUpdatePods(t *testing.T) { testControllerSpec2.Name = "barfoo" manager.rcStore.Indexer.Add(&testControllerSpec2) - // Put one pod in the podStore + // case 1: We put in the podStore a pod with labels matching + // testControllerSpec1, then update its labels to match testControllerSpec2. + // We expect to receive a sync request for both controllers. pod1 := newPodList(manager.podStore.Indexer, 1, api.PodRunning, testControllerSpec1, "pod").Items[0] pod2 := pod1 pod2.Labels = testControllerSpec2.Spec.Selector - - // Send an update of the same pod with modified labels, and confirm we get a sync request for - // both controllers manager.updatePod(&pod1, &pod2) - expected := sets.NewString(testControllerSpec1.Name, testControllerSpec2.Name) for _, name := range expected.List() { t.Logf("Expecting update for %+v", name) @@ -552,6 +550,24 @@ func TestUpdatePods(t *testing.T) { t.Errorf("Expected update notifications for controllers within 100ms each") } } + + // case 2: pod1 in the podStore has labels matching testControllerSpec1. + // We update its labels to match no replication controller. We expect to + // receive a sync request for testControllerSpec1. + pod2.Labels = make(map[string]string) + manager.updatePod(&pod1, &pod2) + expected = sets.NewString(testControllerSpec1.Name) + for _, name := range expected.List() { + t.Logf("Expecting update for %+v", name) + select { + case got := <-received: + if !expected.Has(got) { + t.Errorf("Expected keys %#v got %v", expected, got) + } + case <-time.After(wait.ForeverTestTimeout): + t.Errorf("Expected update notifications for controllers within 100ms each") + } + } } func TestControllerUpdateRequeue(t *testing.T) {