Merge pull request #135551 from Jefftree/deployment

Add pod indexer to deployment controller

pkg/controller/deployment/deployment_controller.go

@@ -83,6 +83,8 @@ type DeploymentController struct {
 	rsLister appslisters.ReplicaSetLister
 	// podLister can list/get pods from the shared informer's store
 	podLister corelisters.PodLister
+	// podIndexer allows looking up pods by ControllerRef UID
+	podIndexer cache.Indexer
 
 	// dListerSynced returns true if the Deployment store has been synced at least once.
 	// Added as a member to the struct to allow injection for testing.
@@ -156,6 +158,12 @@ func NewDeploymentController(ctx context.Context, dInformer appsinformers.Deploy
 	dc.dListerSynced = dInformer.Informer().HasSynced
 	dc.rsListerSynced = rsInformer.Informer().HasSynced
 	dc.podListerSynced = podInformer.Informer().HasSynced
+
+	if err := controller.AddPodControllerIndexer(podInformer.Informer()); err != nil {
+		return nil, fmt.Errorf("adding Pod controller UID indexer: %w", err)
+	}
+	dc.podIndexer = podInformer.Informer().GetIndexer()
+
 	return dc, nil
 }
 
@@ -562,31 +570,16 @@ func (dc *DeploymentController) getReplicaSetsForDeployment(ctx context.Context,
 // NOTE: The pod pointers returned by this method point the pod objects in the cache and thus
 // shouldn't be modified in any way.
 func (dc *DeploymentController) getPodMapForDeployment(d *apps.Deployment, rsList []*apps.ReplicaSet) (map[types.UID][]*v1.Pod, error) {
-	// Get all Pods that potentially belong to this Deployment.
-	selector, err := metav1.LabelSelectorAsSelector(d.Spec.Selector)
-	if err != nil {
-		return nil, err
-	}
-	pods, err := dc.podLister.Pods(d.Namespace).List(selector)
-	if err != nil {
-		return nil, err
-	}
 	// Group Pods by their controller (if it's in rsList).
 	podMap := make(map[types.UID][]*v1.Pod, len(rsList))
+
 	for _, rs := range rsList {
-		podMap[rs.UID] = []*v1.Pod{}
-	}
-	for _, pod := range pods {
-		// Do not ignore inactive Pods because Recreate Deployments need to verify that no
-		// Pods from older versions are running before spinning up new Pods.
-		controllerRef := metav1.GetControllerOf(pod)
-		if controllerRef == nil {
-			continue
-		}
-		// Only append if we care about this UID.
-		if _, ok := podMap[controllerRef.UID]; ok {
-			podMap[controllerRef.UID] = append(podMap[controllerRef.UID], pod)
+		// list all pods managed by this ReplicaSet using the pod indexer
+		pods, err := controller.FilterPodsByOwner(dc.podIndexer, &rs.ObjectMeta, "ReplicaSet", false)
+		if err != nil {
+			return nil, err
 		}
+		podMap[rs.UID] = pods
 	}
 	return podMap, nil
 }

pkg/controller/deployment/deployment_controller_test.go

@@ -1005,49 +1005,74 @@ func TestDeleteReplicaSetOrphan(t *testing.T) {
 }
 
 func BenchmarkGetPodMapForDeployment(b *testing.B) {
-	_, ctx := ktesting.NewTestContext(b)
-
-	f := newFixture(b)
-
-	d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
-
-	rs1 := newReplicaSet(d, "rs1", 1)
-	rs2 := newReplicaSet(d, "rs2", 1)
-
-	var pods []*v1.Pod
-	var objects []runtime.Object
-	for i := 0; i < 100; i++ {
-		p1, p2 := generatePodFromRS(rs1), generatePodFromRS(rs2)
-		p1.Name, p2.Name = p1.Name+fmt.Sprintf("-%d", i), p2.Name+fmt.Sprintf("-%d", i)
-		pods = append(pods, p1, p2)
-		objects = append(objects, p1, p2)
+	cases := []struct {
+		name      string
+		numPods   int
+		extraPods int // Pods in namespace not owned by this deployment
+	}{
+		{name: "10-Pods-No-Noise", numPods: 10, extraPods: 0},
+		{name: "10-Pods-With-Noise", numPods: 10, extraPods: 100},
+		{name: "10000-Pods-No-Noise", numPods: 10000, extraPods: 0},
+		{name: "10000-Pods-With-Noise", numPods: 10000, extraPods: 100000},
 	}
 
-	f.dLister = append(f.dLister, d)
-	f.rsLister = append(f.rsLister, rs1, rs2)
-	f.podLister = append(f.podLister, pods...)
-	f.objects = append(f.objects, d, rs1, rs2)
-	f.objects = append(f.objects, objects...)
+	for _, tc := range cases {
+		b.Run(tc.name, func(b *testing.B) {
+			_, ctx := ktesting.NewTestContext(b)
 
-	// Start the fixture.
-	c, informers, err := f.newController(ctx)
-	if err != nil {
-		b.Fatalf("error creating Deployment controller: %v", err)
-	}
-	stopCh := make(chan struct{})
-	defer close(stopCh)
-	informers.Start(stopCh)
+			f := newFixture(b)
 
-	b.ReportAllocs()
-	b.ResetTimer()
-	for n := 0; n < b.N; n++ {
-		m, err := c.getPodMapForDeployment(d, f.rsLister)
-		if err != nil {
-			b.Fatalf("getPodMapForDeployment() error: %v", err)
-		}
-		if len(m) != 2 {
-			b.Errorf("Invalid map size, expected 2, got: %d", len(m))
-		}
+			d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
+
+			rs1 := newReplicaSet(d, "rs1", 1)
+			rs2 := newReplicaSet(d, "rs2", 1)
+
+			var pods []*v1.Pod
+			var objects []runtime.Object
+			for i := 0; i < tc.numPods; i++ {
+				p1, p2 := generatePodFromRS(rs1), generatePodFromRS(rs2)
+				p1.Name, p2.Name = p1.Name+fmt.Sprintf("-%d", i), p2.Name+fmt.Sprintf("-%d", i)
+				pods = append(pods, p1, p2)
+				objects = append(objects, p1, p2)
+			}
+
+			// Add extra pods that don't belong to the deployment
+			for i := 0; i < tc.extraPods; i++ {
+				p := generatePodFromRS(rs1)
+				p.Name = fmt.Sprintf("extra-pod-%d", i)
+				p.OwnerReferences = nil // Orphaned/unrelated
+				p.Labels = map[string]string{"foo": "notbar"}
+				pods = append(pods, p)
+				objects = append(objects, p)
+			}
+
+			f.dLister = append(f.dLister, d)
+			f.rsLister = append(f.rsLister, rs1, rs2)
+			f.podLister = append(f.podLister, pods...)
+			f.objects = append(f.objects, d, rs1, rs2)
+			f.objects = append(f.objects, objects...)
+
+			// Start the fixture.
+			c, informers, err := f.newController(ctx)
+			if err != nil {
+				b.Fatalf("error creating Deployment controller: %v", err)
+			}
+			stopCh := make(chan struct{})
+			defer close(stopCh)
+			informers.Start(stopCh)
+
+			b.ReportAllocs()
+			b.ResetTimer()
+			for n := 0; n < b.N; n++ {
+				m, err := c.getPodMapForDeployment(d, f.rsLister)
+				if err != nil {
+					b.Fatalf("getPodMapForDeployment() error: %v", err)
+				}
+				if len(m) != 2 {
+					b.Errorf("Invalid map size, expected 2, got: %d", len(m))
+				}
+			}
+		})
 	}
 }