From 8140bbc4f5119268c715ae6adaf1884c394a0db4 Mon Sep 17 00:00:00 2001 From: Matt Matejczyk Date: Tue, 9 Jul 2019 14:22:02 +0200 Subject: [PATCH] Deployment Controller - don't copy pods in getPodMapForDeployment As the benchmark shows it speeds up the method~x4 and reduces memory consumption ~x20. ``` benchmark old ns/op new ns/op delta BenchmarkGetPodMapForDeployment-12 276121 72591 -73.71% benchmark old allocs new allocs delta BenchmarkGetPodMapForDeployment-12 241 238 -1.24% benchmark old bytes new bytes delta BenchmarkGetPodMapForDeployment-12 554025 28956 -94.77% ``` --- .../deployment/deployment_controller.go | 14 +-- .../deployment/deployment_controller_test.go | 10 +- pkg/controller/deployment/recreate.go | 6 +- pkg/controller/deployment/recreate_test.go | 92 ++++++++----------- 4 files changed, 54 insertions(+), 68 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 702e49d89c8..cea0156ecc4 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -366,7 +366,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) { } numPods := 0 for _, podList := range podMap { - numPods += len(podList.Items) + numPods += len(podList) } if numPods == 0 { dc.enqueueDeployment(d) @@ -525,7 +525,9 @@ func (dc *DeploymentController) getReplicaSetsForDeployment(d *apps.Deployment) // // It returns a map from ReplicaSet UID to a list of Pods controlled by that RS, // according to the Pod's ControllerRef. -func (dc *DeploymentController) getPodMapForDeployment(d *apps.Deployment, rsList []*apps.ReplicaSet) (map[types.UID]*v1.PodList, error) { +// NOTE: The pod pointers returned by this method point the 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 { @@ -536,9 +538,9 @@ func (dc *DeploymentController) getPodMapForDeployment(d *apps.Deployment, rsLis return nil, err } // Group Pods by their controller (if it's in rsList). - podMap := make(map[types.UID]*v1.PodList, len(rsList)) + podMap := make(map[types.UID][]*v1.Pod, len(rsList)) for _, rs := range rsList { - podMap[rs.UID] = &v1.PodList{} + podMap[rs.UID] = []*v1.Pod{} } for _, pod := range pods { // Do not ignore inactive Pods because Recreate Deployments need to verify that no @@ -548,8 +550,8 @@ func (dc *DeploymentController) getPodMapForDeployment(d *apps.Deployment, rsLis continue } // Only append if we care about this UID. - if podList, ok := podMap[controllerRef.UID]; ok { - podList.Items = append(podList.Items, *pod) + if _, ok := podMap[controllerRef.UID]; ok { + podMap[controllerRef.UID] = append(podMap[controllerRef.UID], pod) } } return podMap, nil diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 79ce9ae853e..2638599c9e3 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -633,7 +633,7 @@ func TestGetPodMapForReplicaSets(t *testing.T) { } podCount := 0 for _, podList := range podMap { - podCount += len(podList.Items) + podCount += len(podList) } if got, want := podCount, 3; got != want { t.Errorf("podCount = %v, want %v", got, want) @@ -642,19 +642,19 @@ func TestGetPodMapForReplicaSets(t *testing.T) { if got, want := len(podMap), 2; got != want { t.Errorf("len(podMap) = %v, want %v", got, want) } - if got, want := len(podMap[rs1.UID].Items), 2; got != want { + if got, want := len(podMap[rs1.UID]), 2; got != want { t.Errorf("len(podMap[rs1]) = %v, want %v", got, want) } expect := map[string]struct{}{"rs1-pod": {}, "pod4": {}} - for _, pod := range podMap[rs1.UID].Items { + for _, pod := range podMap[rs1.UID] { if _, ok := expect[pod.Name]; !ok { t.Errorf("unexpected pod name for rs1: %s", pod.Name) } } - if got, want := len(podMap[rs2.UID].Items), 1; got != want { + if got, want := len(podMap[rs2.UID]), 1; got != want { t.Errorf("len(podMap[rs2]) = %v, want %v", got, want) } - if got, want := podMap[rs2.UID].Items[0].Name, "rs2-pod"; got != want { + if got, want := podMap[rs2.UID][0].Name, "rs2-pod"; got != want { t.Errorf("podMap[rs2] = [%v], want [%v]", got, want) } } diff --git a/pkg/controller/deployment/recreate.go b/pkg/controller/deployment/recreate.go index fdf50442f69..7ac5b2c4519 100644 --- a/pkg/controller/deployment/recreate.go +++ b/pkg/controller/deployment/recreate.go @@ -25,7 +25,7 @@ import ( ) // rolloutRecreate implements the logic for recreating a replica set. -func (dc *DeploymentController) rolloutRecreate(d *apps.Deployment, rsList []*apps.ReplicaSet, podMap map[types.UID]*v1.PodList) error { +func (dc *DeploymentController) rolloutRecreate(d *apps.Deployment, rsList []*apps.ReplicaSet, podMap map[types.UID][]*v1.Pod) error { // Don't create a new RS if not already existed, so that we avoid scaling up before scaling down. newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, false) if err != nil { @@ -95,7 +95,7 @@ func (dc *DeploymentController) scaleDownOldReplicaSetsForRecreate(oldRSs []*app } // oldPodsRunning returns whether there are old pods running or any of the old ReplicaSets thinks that it runs pods. -func oldPodsRunning(newRS *apps.ReplicaSet, oldRSs []*apps.ReplicaSet, podMap map[types.UID]*v1.PodList) bool { +func oldPodsRunning(newRS *apps.ReplicaSet, oldRSs []*apps.ReplicaSet, podMap map[types.UID][]*v1.Pod) bool { if oldPods := util.GetActualReplicaCountForReplicaSets(oldRSs); oldPods > 0 { return true } @@ -104,7 +104,7 @@ func oldPodsRunning(newRS *apps.ReplicaSet, oldRSs []*apps.ReplicaSet, podMap ma if newRS != nil && newRS.UID == rsUID { continue } - for _, pod := range podList.Items { + for _, pod := range podList { switch pod.Status.Phase { case v1.PodFailed, v1.PodSucceeded: // Don't count pods in terminal state. diff --git a/pkg/controller/deployment/recreate_test.go b/pkg/controller/deployment/recreate_test.go index dc9d3b04aa4..04ce1507119 100644 --- a/pkg/controller/deployment/recreate_test.go +++ b/pkg/controller/deployment/recreate_test.go @@ -88,7 +88,7 @@ func TestOldPodsRunning(t *testing.T) { newRS *apps.ReplicaSet oldRSs []*apps.ReplicaSet - podMap map[types.UID]*v1.PodList + podMap map[types.UID][]*v1.Pod hasOldPodsRunning bool }{ @@ -115,18 +115,16 @@ func TestOldPodsRunning(t *testing.T) { { name: "old RSs with zero status replicas but pods in terminal state are present", oldRSs: []*apps.ReplicaSet{newRSWithStatus("rs-1", 0, 0, nil)}, - podMap: map[types.UID]*v1.PodList{ + podMap: map[types.UID][]*v1.Pod{ "uid-1": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodFailed, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodFailed, }, - { - Status: v1.PodStatus{ - Phase: v1.PodSucceeded, - }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodSucceeded, }, }, }, @@ -136,13 +134,11 @@ func TestOldPodsRunning(t *testing.T) { { name: "old RSs with zero status replicas but pod in unknown phase present", oldRSs: []*apps.ReplicaSet{newRSWithStatus("rs-1", 0, 0, nil)}, - podMap: map[types.UID]*v1.PodList{ + podMap: map[types.UID][]*v1.Pod{ "uid-1": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodUnknown, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodUnknown, }, }, }, @@ -152,13 +148,11 @@ func TestOldPodsRunning(t *testing.T) { { name: "old RSs with zero status replicas with pending pod present", oldRSs: []*apps.ReplicaSet{newRSWithStatus("rs-1", 0, 0, nil)}, - podMap: map[types.UID]*v1.PodList{ + podMap: map[types.UID][]*v1.Pod{ "uid-1": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodPending, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodPending, }, }, }, @@ -168,13 +162,11 @@ func TestOldPodsRunning(t *testing.T) { { name: "old RSs with zero status replicas with running pod present", oldRSs: []*apps.ReplicaSet{newRSWithStatus("rs-1", 0, 0, nil)}, - podMap: map[types.UID]*v1.PodList{ + podMap: map[types.UID][]*v1.Pod{ "uid-1": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodRunning, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodRunning, }, }, }, @@ -184,30 +176,24 @@ func TestOldPodsRunning(t *testing.T) { { name: "old RSs with zero status replicas but pods in terminal state and pending are present", oldRSs: []*apps.ReplicaSet{newRSWithStatus("rs-1", 0, 0, nil)}, - podMap: map[types.UID]*v1.PodList{ + podMap: map[types.UID][]*v1.Pod{ "uid-1": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodFailed, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodFailed, }, - { - Status: v1.PodStatus{ - Phase: v1.PodSucceeded, - }, + }, + { + Status: v1.PodStatus{ + Phase: v1.PodSucceeded, }, }, }, - "uid-2": { - Items: []v1.Pod{}, - }, + "uid-2": {}, "uid-3": { - Items: []v1.Pod{ - { - Status: v1.PodStatus{ - Phase: v1.PodPending, - }, + { + Status: v1.PodStatus{ + Phase: v1.PodPending, }, }, }, @@ -232,14 +218,12 @@ func rsWithUID(uid string) *apps.ReplicaSet { return rs } -func podMapWithUIDs(uids []string) map[types.UID]*v1.PodList { - podMap := make(map[types.UID]*v1.PodList) +func podMapWithUIDs(uids []string) map[types.UID][]*v1.Pod { + podMap := make(map[types.UID][]*v1.Pod) for _, uid := range uids { - podMap[types.UID(uid)] = &v1.PodList{ - Items: []v1.Pod{ - { /* supposedly a pod */ }, - { /* supposedly another pod pod */ }, - }, + podMap[types.UID(uid)] = []*v1.Pod{ + { /* supposedly a pod */ }, + { /* supposedly another pod pod */ }, } } return podMap