Skip to content

Commit

Permalink
Allow setting minimum replicas for eviction on VPAs (#628)
Browse files Browse the repository at this point in the history 
* Allow setting minimum replicas for eviction on VPAs

* copy pasta

* Fix test
  • Loading branch information
@sudermanjr
sudermanjr authored Aug 10, 2023
1 parent d36cf59 commit b222c8a
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 22 deletions.
2 changes: 2 additions & 0 deletions pkg/utils/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ var (
VpaEnabledLabel = LabelOrAnnotationBase + "/" + "enabled"
// VpaUpdateModeKey is the label used to indicate the vpa update mode.
VpaUpdateModeKey = LabelOrAnnotationBase + "/" + "vpa-update-mode"
// VpaMinReplicas is the annotation to use to define minimum replicas for eviction of a VPA
VpaMinReplicasAnnotation = LabelOrAnnotationBase + "/" + "vpa-min-replicas"
// DeploymentExcludeContainersAnnotation is the label used to exclude container names from being reported.
WorkloadExcludeContainersAnnotation = LabelOrAnnotationBase + "/" + "exclude-containers"
// VpaResourcePolicyAnnotation is the annotation use to define the json configuration of PodResourcePolicy section of a vpa
Expand Down
42 changes: 38 additions & 4 deletions pkg/vpa/vpa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ func (r Reconciler) namespaceIsManaged(namespace *corev1.Namespace) bool {
func (r Reconciler) reconcileControllersAndVPAs(ns *corev1.Namespace, vpas []vpav1.VerticalPodAutoscaler, controllers []Controller) error {
defaultUpdateMode, _ := vpaUpdateModeForResource(ns)
defaultResourcePolicy, _ := vpaResourcePolicyForResource(ns)
defaultMinReplicas, _ := vpaMinReplicasForResource(ns)

// these keys will eventually contain the leftover vpas that do not have a matching controller associated
vpaHasAssociatedController := map[string]bool{}
Expand All @@ -181,7 +182,7 @@ func (r Reconciler) reconcileControllersAndVPAs(ns *corev1.Namespace, vpas []vpa
vpaName = cvpa.Name
}
klog.V(2).Infof("Reconciling Namespace/%s for %s/%s with VPA/%s", ns.Name, controller.Kind, controller.Name, vpaName)
err := r.reconcileControllerAndVPA(ns, controller, cvpa, defaultUpdateMode, defaultResourcePolicy)
err := r.reconcileControllerAndVPA(ns, controller, cvpa, defaultUpdateMode, defaultResourcePolicy, defaultMinReplicas)
if err != nil {
return err
}
Expand All @@ -201,7 +202,7 @@ func (r Reconciler) reconcileControllersAndVPAs(ns *corev1.Namespace, vpas []vpa
return nil
}

func (r Reconciler) reconcileControllerAndVPA(ns *corev1.Namespace, controller Controller, vpa *vpav1.VerticalPodAutoscaler, vpaUpdateMode *vpav1.UpdateMode, vpaResourcePolicy *vpav1.PodResourcePolicy) error {
func (r Reconciler) reconcileControllerAndVPA(ns *corev1.Namespace, controller Controller, vpa *vpav1.VerticalPodAutoscaler, vpaUpdateMode *vpav1.UpdateMode, vpaResourcePolicy *vpav1.PodResourcePolicy, minReplicas *int32) error {
controllerObj := controller.Unstructured.DeepCopyObject()
if vpaUpdateModeOverride, explicit := vpaUpdateModeForResource(controllerObj); explicit {
vpaUpdateMode = vpaUpdateModeOverride
Expand All @@ -213,7 +214,7 @@ func (r Reconciler) reconcileControllerAndVPA(ns *corev1.Namespace, controller C
klog.V(5).Infof("%s/%s has custom vpa-resource-policy", controller.Kind, controller.Name)
}

desiredVPA := r.getVPAObject(vpa, ns, controller, vpaUpdateMode, vpaResourcePolicy)
desiredVPA := r.getVPAObject(vpa, ns, controller, vpaUpdateMode, vpaResourcePolicy, minReplicas)

if vpa == nil {
klog.V(5).Infof("%s/%s does not have a VPA currently, creating VPA/%s", controller.Kind, controller.Name, controller.Name)
Expand Down Expand Up @@ -330,7 +331,7 @@ func (r Reconciler) updateVPA(vpa vpav1.VerticalPodAutoscaler) error {
return nil
}

func (r Reconciler) getVPAObject(existingVPA *vpav1.VerticalPodAutoscaler, ns *corev1.Namespace, controller Controller, updateMode *vpav1.UpdateMode, resourcePolicy *vpav1.PodResourcePolicy) vpav1.VerticalPodAutoscaler {
func (r Reconciler) getVPAObject(existingVPA *vpav1.VerticalPodAutoscaler, ns *corev1.Namespace, controller Controller, updateMode *vpav1.UpdateMode, resourcePolicy *vpav1.PodResourcePolicy, minReplicas *int32) vpav1.VerticalPodAutoscaler {
var desiredVPA vpav1.VerticalPodAutoscaler

// create a brand new vpa with the correct information
Expand Down Expand Up @@ -362,6 +363,13 @@ func (r Reconciler) getVPAObject(existingVPA *vpav1.VerticalPodAutoscaler, ns *c
ResourcePolicy: resourcePolicy,
}

if minReplicas != nil {
if *minReplicas > 0 {
desiredVPA.Spec.UpdatePolicy.MinReplicas = minReplicas
}

}

return desiredVPA
}

Expand Down Expand Up @@ -412,3 +420,29 @@ func vpaResourcePolicyForResource(obj runtime.Object) (*vpav1.PodResourcePolicy,

return &resourcePol, explicit
}

// vpaMinReplicas sets the VPA minimum replicas required for eviction
func vpaMinReplicasForResource(obj runtime.Object) (*int32, bool) {
explicit := false

minReplicasString := ""
accessor, _ := meta.Accessor(obj)
if val, ok := accessor.GetAnnotations()[utils.VpaMinReplicasAnnotation]; ok {
minReplicasString = val
}

if minReplicasString == "" {
return nil, explicit
}

explicit = true
minReplicas, err := strconv.ParseInt(minReplicasString, 10, 32)
if err != nil {
klog.Error(err.Error())
return nil, explicit
}

minReplicasInt32 := int32(minReplicas)

return &minReplicasInt32, explicit
}
46 changes: 28 additions & 18 deletions pkg/vpa/vpa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,9 @@ func Test_getVPAObject(t *testing.T) {
t.Parallel()

mode, _ := vpaUpdateModeForResource(test.ns)
resoucePolicy, _ := vpaResourcePolicyForResource(test.ns)
vpa := rec.getVPAObject(test.vpa, test.ns, test.controller, mode, resoucePolicy)
resourcePolicy, _ := vpaResourcePolicyForResource(test.ns)
minReplicas, _ := vpaMinReplicasForResource(test.ns)
vpa := rec.getVPAObject(test.vpa, test.ns, test.controller, mode, resourcePolicy, minReplicas)

// expected ObjectMeta
assert.Equal(t, "goldilocks-test-vpa", vpa.Name)
Expand Down Expand Up @@ -175,8 +176,9 @@ func Test_createVPA(t *testing.T) {
}

updateMode, _ := vpaUpdateModeForResource(&nsTesting)
resoucePolicy, _ := vpaResourcePolicyForResource(&nsTesting)
testVPA := rec.getVPAObject(nil, &nsTesting, controller, updateMode, resoucePolicy)
resourcePolicy, _ := vpaResourcePolicyForResource(&nsTesting)
minReplicas, _ := vpaMinReplicasForResource(&nsTesting)
testVPA := rec.getVPAObject(nil, &nsTesting, controller, updateMode, resourcePolicy, minReplicas)

err := rec.createVPA(testVPA)
assert.NoError(t, err)
Expand Down Expand Up @@ -207,8 +209,9 @@ func Test_createVPAWithResourcePolicy(t *testing.T) {
rec.DryRun = false

updateMode, _ := vpaUpdateModeForResource(&nsLabeledResourcePolicy)
resoucePolicy, _ := vpaResourcePolicyForResource(&nsLabeledResourcePolicy)
testVPA := rec.getVPAObject(nil, &nsLabeledResourcePolicy, controller, updateMode, resoucePolicy)
resourcePolicy, _ := vpaResourcePolicyForResource(&nsLabeledResourcePolicy)
minReplicas, _ := vpaMinReplicasForResource(&nsLabeledResourcePolicy)
testVPA := rec.getVPAObject(nil, &nsLabeledResourcePolicy, controller, updateMode, resourcePolicy, minReplicas)

errCreate := rec.createVPA(testVPA)
newVPA, _ := VPAClient.Client.AutoscalingV1().VerticalPodAutoscalers(nsLabeledResourcePolicy.Name).Get(context.TODO(), "goldilocks-test-vpa", metav1.GetOptions{})
Expand All @@ -232,8 +235,10 @@ func Test_deleteVPA(t *testing.T) {
Unstructured: nil,
}
updateMode, _ := vpaUpdateModeForResource(&nsTesting)
resoucePolicy, _ := vpaResourcePolicyForResource(&nsTesting)
testVPA := rec.getVPAObject(nil, &nsTesting, controller, updateMode, resoucePolicy)
resourcePolicy, _ := vpaResourcePolicyForResource(&nsTesting)
minReplicas, _ := vpaMinReplicasForResource(&nsTesting)
testVPA := rec.getVPAObject(nil, &nsTesting, controller, updateMode, resourcePolicy, minReplicas)

_, err := VPAClient.Client.AutoscalingV1().VerticalPodAutoscalers(nsTesting.Name).Create(context.TODO(), &testVPA, metav1.CreateOptions{})
assert.NoError(t, err)

Expand Down Expand Up @@ -268,8 +273,10 @@ func Test_updateVPA(t *testing.T) {
Unstructured: nil,
}
updateMode, _ := vpaUpdateModeForResource(testNS)
resoucePolicy, _ := vpaResourcePolicyForResource(testNS)
testVPA := rec.getVPAObject(nil, testNS, controller, updateMode, resoucePolicy)
resourcePolicy, _ := vpaResourcePolicyForResource(testNS)
minReplicas, _ := vpaMinReplicasForResource(testNS)
testVPA := rec.getVPAObject(nil, testNS, controller, updateMode, resourcePolicy, minReplicas)

_, err := VPAClient.Client.AutoscalingV1().VerticalPodAutoscalers(testNS.Name).Create(context.TODO(), &testVPA, metav1.CreateOptions{})
assert.NoError(t, err)

Expand All @@ -290,8 +297,9 @@ func Test_updateVPA(t *testing.T) {
// change the update mode
testNS.Labels["goldilocks.fairwinds.com/vpa-update-mode"] = "auto"
updateMode, _ = vpaUpdateModeForResource(testNS)
resoucePolicy, _ = vpaResourcePolicyForResource(testNS)
newVPA := rec.getVPAObject(nil, testNS, controller, updateMode, resoucePolicy)
resourcePolicy, _ = vpaResourcePolicyForResource(testNS)
minReplicas, _ = vpaMinReplicasForResource(testNS)
newVPA := rec.getVPAObject(nil, testNS, controller, updateMode, resourcePolicy, minReplicas)

errUpdate2 := rec.updateVPA(newVPA)
assert.NoError(t, errUpdate2)
Expand Down Expand Up @@ -335,12 +343,14 @@ func Test_listVPA(t *testing.T) {
// test vpas
updateMode1, _ := vpaUpdateModeForResource(testNS1)
updateMode2, _ := vpaUpdateModeForResource(testNS2)
resoucePolicy1, _ := vpaResourcePolicyForResource(testNS1)
resoucePolicy2, _ := vpaResourcePolicyForResource(testNS2)

vpa1 := rec.getVPAObject(nil, testNS1, controller1, updateMode1, resoucePolicy1)
vpa2 := rec.getVPAObject(nil, testNS1, controller2, updateMode1, resoucePolicy1)
vpa3 := rec.getVPAObject(nil, testNS2, controller3, updateMode2, resoucePolicy2)
resourcePolicy1, _ := vpaResourcePolicyForResource(testNS1)
resourcePolicy2, _ := vpaResourcePolicyForResource(testNS2)
minReplicas1, _ := vpaMinReplicasForResource(testNS1)
minReplicas2, _ := vpaMinReplicasForResource(testNS2)

vpa1 := rec.getVPAObject(nil, testNS1, controller1, updateMode1, resourcePolicy1, minReplicas1)
vpa2 := rec.getVPAObject(nil, testNS1, controller2, updateMode1, resourcePolicy1, minReplicas1)
vpa3 := rec.getVPAObject(nil, testNS2, controller3, updateMode2, resourcePolicy2, minReplicas2)

// create vpas
_ = rec.createVPA(vpa1)
Expand Down

0 comments on commit b222c8a

Please sign in to comment.