From 1c1b99d5546f17bbb6d38e7b7312daab18ea22e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Kulczy=C5=84ski?= Date: Fri, 2 Feb 2018 14:20:49 +0100 Subject: [PATCH] Delete VPA pointer from PodState --- .../recommender/model/cluster.go | 11 ++---- .../recommender/model/cluster_test.go | 39 +++++++++++-------- .../recommender/model/vpa.go | 14 ------- 3 files changed, 27 insertions(+), 37 deletions(-) diff --git a/vertical-pod-autoscaler/recommender/model/cluster.go b/vertical-pod-autoscaler/recommender/model/cluster.go index 159c529b4219..9b9d9c3e23f9 100644 --- a/vertical-pod-autoscaler/recommender/model/cluster.go +++ b/vertical-pod-autoscaler/recommender/model/cluster.go @@ -41,8 +41,6 @@ type PodState struct { Labels labels.Set // Containers that belong to the Pod, keyed by the container name. Containers map[string]*ContainerState - // VPA managing this pod (can be nil). - Vpa *Vpa // All VPA objects that match this Pod. While it is incorrect to let // multiple VPA objects match the same pod, the model has no means to // prevent such situation. In such case the pod is controlled by one of the @@ -188,10 +186,9 @@ func (cluster *ClusterState) DeleteVpa(vpaID VpaID) error { func newPod(id PodID) *PodState { return &PodState{ - id, - make(map[string]string), // empty labels. - make(map[string]*ContainerState), // empty containers. - nil, // Vpa. - make(map[VpaID]*Vpa), // empty MatchingVpas. + ID: id, + Labels: make(map[string]string), + Containers: make(map[string]*ContainerState), + MatchingVpas: make(map[VpaID]*Vpa), } } diff --git a/vertical-pod-autoscaler/recommender/model/cluster_test.go b/vertical-pod-autoscaler/recommender/model/cluster_test.go index 5fa3296866ff..1fd7497ea316 100644 --- a/vertical-pod-autoscaler/recommender/model/cluster_test.go +++ b/vertical-pod-autoscaler/recommender/model/cluster_test.go @@ -79,7 +79,7 @@ func TestAddVpaThenAddPod(t *testing.T) { vpa := addTestVpa(cluster) pod := addTestPod(cluster) assert.Equal(t, pod, vpa.Pods[testPodID]) - assert.Equal(t, vpa, pod.Vpa) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas) } // Creates a pod followed by a matching VPA. Verifies that the links between the @@ -89,7 +89,7 @@ func TestAddPodThenAddVpa(t *testing.T) { pod := addTestPod(cluster) vpa := addTestVpa(cluster) assert.Equal(t, pod, vpa.Pods[testPodID]) - assert.Equal(t, vpa, pod.Vpa) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas) } // Creates a VPA and a matching pod. Verifies that after deleting the VPA the @@ -99,7 +99,7 @@ func TestDeleteVpa(t *testing.T) { vpa := addTestVpa(cluster) pod := addTestPod(cluster) cluster.DeleteVpa(vpa.ID) - assert.Nil(t, pod.Vpa) + assert.Empty(t, pod.MatchingVpas) assert.NotContains(t, cluster.Vpas, vpa.ID) } @@ -123,7 +123,7 @@ func TestChangePodLabels(t *testing.T) { // Update Pod labels to no longer match the VPA. cluster.AddOrUpdatePod(testPodID, emptyLabels) assert.Empty(t, vpa.Pods) - assert.Nil(t, pod.Vpa) + assert.Empty(t, pod.MatchingVpas) } // Creates a VPA and a matching pod, then change the VPA pod selector 3 times: @@ -139,24 +139,24 @@ func TestUpdatePodSelector(t *testing.T) { assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 in (value-1,value-2)")) vpa = cluster.Vpas[testVpaID] assert.Equal(t, pod, vpa.Pods[testPodID]) - assert.Equal(t, vpa, pod.Vpa) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas) // Update the VPA selector to no longer match the Pod. assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 = value-2")) vpa = cluster.Vpas[testVpaID] assert.Empty(t, vpa.Pods) - assert.Nil(t, pod.Vpa) + assert.Empty(t, pod.MatchingVpas) // Update the VPA selector to match the Pod again. assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 = value-1")) vpa = cluster.Vpas[testVpaID] assert.Equal(t, pod, vpa.Pods[testPodID]) - assert.Equal(t, vpa, pod.Vpa) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas) } // Creates a VPA and a matching pod, then add another VPA matching the same pod. -// Verifies that the pod is controlled by the first VPA. -// Next deletes the first VPA and verfies that the pod is controlled by the +// Verifies that the pod is knows about both of them. +// Next deletes one of them and verfies that the pod is controlled by the // remaning VPA. Finally deletes the other VPA and verifies that the pod is no // longer controlled by any VPA. func TestTwoVpasForPod(t *testing.T) { @@ -164,14 +164,19 @@ func TestTwoVpasForPod(t *testing.T) { cluster.AddOrUpdateVpa(VpaID{"namespace-1", "vpa-1"}, "label-1 = value-1") pod := addTestPod(cluster) cluster.AddOrUpdateVpa(VpaID{"namespace-1", "vpa-2"}, "label-1 in (value-1,value-2)") - assert.Equal(t, cluster.Vpas[VpaID{"namespace-1", "vpa-1"}], pod.Vpa) - // Delete the VPA that currently controls the Pod. Expect that it will - // switch to the remaining one. + assert.Equal(t, map[VpaID]*Vpa{ + {"namespace-1", "vpa-1"}: cluster.Vpas[VpaID{"namespace-1", "vpa-1"}], + {"namespace-1", "vpa-2"}: cluster.Vpas[VpaID{"namespace-1", "vpa-2"}]}, + pod.MatchingVpas) + // Delete the first VPA from the Pod. Expect that it will still + // have the other one. assert.NoError(t, cluster.DeleteVpa(VpaID{"namespace-1", "vpa-1"})) - assert.Equal(t, cluster.Vpas[VpaID{"namespace-1", "vpa-2"}], pod.Vpa) + assert.Equal(t, map[VpaID]*Vpa{ + {"namespace-1", "vpa-2"}: cluster.Vpas[VpaID{"namespace-1", "vpa-2"}]}, + pod.MatchingVpas) // Delete the other VPA. The Pod is no longer vertically-scaled by anyone. assert.NoError(t, cluster.DeleteVpa(VpaID{"namespace-1", "vpa-2"})) - assert.Nil(t, pod.Vpa) + assert.Empty(t, pod.MatchingVpas) } // Verifies that a VPA with an empty selector (matching all pods) matches a pod @@ -186,6 +191,8 @@ func TestEmptySelector(t *testing.T) { anotherPodID := PodID{"namespace-1", "pod-2"} cluster.AddOrUpdatePod(anotherPodID, emptyLabels) // Both pods should be matched by the VPA. - assert.Equal(t, cluster.Vpas[testVpaID], cluster.Pods[testPodID].Vpa) - assert.Equal(t, cluster.Vpas[testVpaID], cluster.Pods[anotherPodID].Vpa) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: cluster.Vpas[testVpaID]}, + cluster.Pods[testPodID].MatchingVpas) + assert.Equal(t, map[VpaID]*Vpa{testVpaID: cluster.Vpas[testVpaID]}, + cluster.Pods[anotherPodID].MatchingVpas) } diff --git a/vertical-pod-autoscaler/recommender/model/vpa.go b/vertical-pod-autoscaler/recommender/model/vpa.go index fa04dc9ee28d..8b9a72f845b8 100644 --- a/vertical-pod-autoscaler/recommender/model/vpa.go +++ b/vertical-pod-autoscaler/recommender/model/vpa.go @@ -83,24 +83,10 @@ func (vpa *Vpa) UpdatePodLink(pod *PodState) bool { // Create links between VPA and pod. vpa.Pods[pod.ID] = pod pod.MatchingVpas[vpa.ID] = vpa - if pod.Vpa == nil { - pod.Vpa = vpa - } } else { // Delete the links between VPA and pod. delete(vpa.Pods, pod.ID) delete(pod.MatchingVpas, vpa.ID) - if pod.Vpa == vpa { - pod.Vpa = getAnyVpa(pod.MatchingVpas) - } } return true } - -// Returns any VPA from the map or nil if the map is empty. -func getAnyVpa(vpas map[VpaID]*Vpa) *Vpa { - for _, vpa := range vpas { - return vpa - } - return nil -}