diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 8c5412b868c..b1dc644561b 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/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 } diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index b4481d06a7e..d8925fbf9e5 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/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)) + } + } + }) } }