Move vpa reconcile logic from generic controller to vpa controller

cmd/vpa_butler/main.go

@@ -82,13 +82,12 @@ func main() {
 	})
 
 	handleError(err, "unable to start manager")
-	handleError(controllers.SetupForAppsV1(mgr, controllers.GenericControllerParams{
+	handleError(controllers.SetupForAppsV1(mgr), "unable to setup apps/v1 controllers")
+	vpaController := controllers.VpaController{
+		Client:           mgr.GetClient(),
+		Version:          Version,
 		MinAllowedCPU:    minAllowedCPU,
 		MinAllowedMemory: minAllowedMemory,
-	}), "unable to setup apps/v1 controllers")
-	vpaController := controllers.VpaController{
-		Client:  mgr.GetClient(),
-		Version: Version,
 	}
 	handleError(vpaController.SetupWithManager(mgr), "unable to setup vpa controller")
 	vpaRunnable := controllers.VpaRunnable{

go.mod

@@ -31,7 +31,6 @@ require (
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
-	github.com/google/gnostic v0.5.7-v3refs // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect

internal/common/vpa.go

@@ -1,7 +1,9 @@
 package common
 
 import (
+	autoscaling "k8s.io/api/autoscaling/v1"
 	vpav1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -21,3 +23,18 @@ func ManagedByButler(vpa *vpav1.VerticalPodAutoscaler) bool {
 	v, ok := vpa.Annotations[AnnotationManagedBy]
 	return ok && v == AnnotationVpaButler
 }
+
+func ConfigureVpaBaseline(vpa *vpav1.VerticalPodAutoscaler, owner client.Object, updateMode vpav1.UpdateMode) {
+	vpa.Spec.TargetRef = &autoscaling.CrossVersionObjectReference{
+		Kind:       owner.GetObjectKind().GroupVersionKind().Kind,
+		Name:       owner.GetName(),
+		APIVersion: owner.GetObjectKind().GroupVersionKind().Version,
+	}
+	vpa.Spec.UpdatePolicy = &vpav1.PodUpdatePolicy{
+		UpdateMode: &updateMode,
+	}
+	if vpa.Annotations == nil {
+		vpa.Annotations = make(map[string]string, 0)
+	}
+	vpa.Annotations[AnnotationManagedBy] = AnnotationVpaButler
+}

internal/controllers/constants.go

@@ -0,0 +1,7 @@
+package controllers
+
+const (
+	DaemonSetStr   string = "DaemonSet"
+	StatefulSetStr string = "StatefulSet"
+	DeploymentStr  string = "Deployment"
+)

internal/controllers/generic_controller.go

@@ -7,22 +7,15 @@ import (
 	"strings"
 
 	"github.com/go-logr/logr"
-	"github.com/pkg/errors"
 	"github.com/sapcc/vpa_butler/internal/common"
 	appsv1 "k8s.io/api/apps/v1"
-	autoscaling "k8s.io/api/autoscaling/v1"
-	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/api/meta"
-	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	vpav1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 const (
@@ -31,15 +24,9 @@ const (
 	maxNameLength = 63
 )
 
-type GenericControllerParams struct {
-	MinAllowedCPU    resource.Quantity
-	MinAllowedMemory resource.Quantity
-}
-
 type GenericController struct {
 	client.Client
 	typeName string
-	params   GenericControllerParams
 	Log      logr.Logger
 	Scheme   *runtime.Scheme
 	instance client.Object
@@ -68,11 +55,6 @@ func (v *GenericController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	v.Log.Info("Reconciling potential vpa target",
-		"namespace", instance.GetNamespace(),
-		"name", instance.GetName(),
-		"kind", instance.GetObjectKind().GroupVersionKind().Kind,
-	)
 	serve, err := v.shouldServeVpa(ctx, instance)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -81,11 +63,18 @@ func (v *GenericController) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		err = v.ensureVpaDeleted(ctx, instance)
 		return ctrl.Result{}, err
 	}
-
-	if v.reconcileVpa(ctx, instance) != nil {
-		return ctrl.Result{}, err
+	var vpa = new(vpav1.VerticalPodAutoscaler)
+	vpa.Namespace = instance.GetNamespace()
+	vpa.Name = getVpaName(instance)
+	if err := v.Client.Get(ctx, client.ObjectKeyFromObject(vpa), vpa); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return ctrl.Result{}, err
+		}
+		// vpa does not exist so create it
+		// set off here, as the vpa is to be fully configured by the VpaController
+		common.ConfigureVpaBaseline(vpa, instance, vpav1.UpdateModeOff)
+		return ctrl.Result{}, v.Client.Create(ctx, vpa)
 	}
-
 	return ctrl.Result{}, nil
 }
 
@@ -116,83 +105,6 @@ func (v *GenericController) shouldServeVpa(ctx context.Context, vpaOwner client.
 	return true, nil
 }
 
-func (v *GenericController) reconcileVpa(ctx context.Context, vpaOwner client.Object) error {
-	var vpa = new(vpav1.VerticalPodAutoscaler)
-	vpa.Namespace = vpaOwner.GetNamespace()
-	vpa.Name = getVpaName(vpaOwner)
-	exists := true
-	if err := v.Client.Get(ctx, client.ObjectKeyFromObject(vpa), vpa); err != nil {
-		// Return any other error.
-		if !apierrors.IsNotFound(err) {
-			return err
-		}
-		exists = false
-	}
-
-	if o, err := meta.Accessor(vpa); err == nil {
-		if o.GetDeletionTimestamp() != nil {
-			return fmt.Errorf("the resource %s/%s already exists but is marked for deletion",
-				o.GetNamespace(), o.GetName())
-		}
-	}
-
-	before, ok := vpa.DeepCopyObject().(client.Object)
-	if !ok {
-		return fmt.Errorf("failed to cast object to client.Object")
-	}
-	if err := v.configureVpa(vpaOwner, vpa); err != nil {
-		return errors.Wrap(err, "mutating object failed")
-	}
-
-	if !exists {
-		v.Log.Info("Creating vpa", "name", vpa.Name, "namespace", vpa.Namespace)
-		return v.Client.Create(ctx, vpa)
-	}
-
-	if equality.Semantic.DeepEqual(before, vpa) {
-		return nil
-	}
-	patch := client.MergeFrom(before)
-	v.Log.Info("Patching vpa", "name", vpa.Name, "namespace", vpa.Namespace)
-	if err := v.Client.Patch(ctx, vpa, patch); err != nil {
-		return err
-	}
-	return nil
-}
-
-func (v *GenericController) configureVpa(vpaOwner client.Object, vpa *vpav1.VerticalPodAutoscaler) error {
-	vpaSpec := &vpa.Spec
-	vpaSpec.TargetRef = &autoscaling.CrossVersionObjectReference{
-		Kind:       vpaOwner.GetObjectKind().GroupVersionKind().Kind,
-		Name:       vpaOwner.GetName(),
-		APIVersion: vpaOwner.GetObjectKind().GroupVersionKind().Version,
-	}
-	vpaSpec.UpdatePolicy = &vpav1.PodUpdatePolicy{
-		UpdateMode: &common.VpaUpdateMode,
-	}
-
-	resourceList := []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}
-	vpaSpec.ResourcePolicy = &vpav1.PodResourcePolicy{
-		ContainerPolicies: []vpav1.ContainerResourcePolicy{
-			{
-				ContainerName:       "*",
-				ControlledResources: &resourceList,
-				ControlledValues:    &common.VpaControlledValues,
-				MinAllowed: corev1.ResourceList{
-					corev1.ResourceCPU:    v.params.MinAllowedCPU,
-					corev1.ResourceMemory: v.params.MinAllowedMemory,
-				},
-			},
-		},
-	}
-	if vpa.Annotations == nil {
-		vpa.Annotations = make(map[string]string, 0)
-	}
-	vpa.Annotations[common.AnnotationManagedBy] = common.AnnotationVpaButler
-
-	return controllerutil.SetOwnerReference(vpaOwner, vpa, v.Scheme)
-}
-
 func (v *GenericController) ensureVpaDeleted(ctx context.Context, vpaOwner client.Object) error {
 	var vpa vpav1.VerticalPodAutoscaler
 	ref := types.NamespacedName{Namespace: vpaOwner.GetNamespace(), Name: getVpaName(vpaOwner)}
@@ -215,26 +127,23 @@ func getVpaName(vpaOwner client.Object) string {
 	return fmt.Sprintf("%s-%s", name, kind)
 }
 
-func SetupForAppsV1(mgr ctrl.Manager, params GenericControllerParams) error {
+func SetupForAppsV1(mgr ctrl.Manager) error {
 	deploymentController := GenericController{
 		Client: mgr.GetClient(),
-		params: params,
 	}
 	err := deploymentController.SetupWithManager(mgr, &appsv1.Deployment{})
 	if err != nil {
 		return fmt.Errorf("unable to setup deployment controller: %w", err)
 	}
 	daemonsetController := GenericController{
 		Client: mgr.GetClient(),
-		params: params,
 	}
 	err = daemonsetController.SetupWithManager(mgr, &appsv1.DaemonSet{})
 	if err != nil {
 		return fmt.Errorf("unable to setup daemonset controller: %w", err)
 	}
 	statefulSetController := GenericController{
 		Client: mgr.GetClient(),
-		params: params,
 	}
 	err = statefulSetController.SetupWithManager(mgr, &appsv1.StatefulSet{})
 	if err != nil {

internal/controllers/generic_controller_test.go

@@ -3,12 +3,11 @@ package controllers_test
 import (
 	"context"
 	"fmt"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/sapcc/vpa_butler/internal/common"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+	"github.com/sapcc/vpa_butler/internal/controllers"
 
 	appsv1 "k8s.io/api/apps/v1"
 	autoscalingv1 "k8s.io/api/autoscaling/v1"
@@ -57,6 +56,13 @@ func expectVpa(name string) {
 		if mangedBy != common.AnnotationVpaButler {
 			return fmt.Errorf("vpa has wrong managed-by annotation")
 		}
+		// the min resources stuff technically belongs to vpa_controller.go
+		if vpa.Spec.ResourcePolicy == nil {
+			return fmt.Errorf("vpa resource policy is nil")
+		}
+		if len(vpa.Spec.ResourcePolicy.ContainerPolicies) != 1 {
+			return fmt.Errorf("vpa has wrong amount of container policies")
+		}
 		minAllowed := vpa.Spec.ResourcePolicy.ContainerPolicies[0].MinAllowed
 		if !minAllowed.Cpu().Equal(testMinAllowedCPU) {
 			return fmt.Errorf("vpa minAllowed CPU does not match")
@@ -149,27 +155,6 @@ var _ = Describe("GenericController", func() {
 		It("should create a vpa", func() {
 			expectVpa("test-deployment-deployment")
 		})
-
-		It("patches the served vpa", func() {
-			// need to sleep here to ensure the a vpa is created before the update
-			// to this global variable
-			time.Sleep(100 * time.Millisecond)
-			common.VpaUpdateMode = vpav1.UpdateModeAuto
-			unmodified := deployment.DeepCopy()
-			deployment.Spec.Replicas = ptr.To[int32](2)
-			Expect(k8sClient.Patch(context.Background(), deployment, client.MergeFrom(unmodified))).To(Succeed())
-			Eventually(func() vpav1.UpdateMode {
-				var vpa vpav1.VerticalPodAutoscaler
-				err := k8sClient.Get(context.Background(), types.NamespacedName{
-					Name:      "test-deployment-deployment",
-					Namespace: metav1.NamespaceDefault,
-				}, &vpa)
-				if err != nil {
-					return vpav1.UpdateMode("")
-				}
-				return *vpa.Spec.UpdatePolicy.UpdateMode
-			}).Should(Equal(common.VpaUpdateMode))
-		})
 	})
 
 	Context("when creating a statefulset", func() {
@@ -218,7 +203,7 @@ var _ = Describe("GenericController", func() {
 			vpa.Namespace = metav1.NamespaceDefault
 			vpa.Spec.TargetRef = &autoscalingv1.CrossVersionObjectReference{
 				Name:       deploymentName,
-				Kind:       "Deployment",
+				Kind:       controllers.DeploymentStr,
 				APIVersion: "v1",
 			}
 			Expect(k8sClient.Create(context.Background(), vpa)).To(Succeed())

internal/controllers/suite_test.go

@@ -59,17 +59,16 @@ var _ = BeforeSuite(func() {
 	Expect(err).ToNot(HaveOccurred())
 
 	err = (&controllers.VpaController{
-		Client:  k8sManager.GetClient(),
-		Log:     GinkgoLogr.WithName("vpa-controller"),
-		Scheme:  k8sManager.GetScheme(),
-		Version: "test",
+		Client:           k8sManager.GetClient(),
+		Log:              GinkgoLogr.WithName("vpa-controller"),
+		Scheme:           k8sManager.GetScheme(),
+		Version:          "test",
+		MinAllowedCPU:    testMinAllowedCPU,
+		MinAllowedMemory: testMinAllowedMemory,
 	}).SetupWithManager(k8sManager)
 	Expect(err).ToNot(HaveOccurred())
 
-	Expect(controllers.SetupForAppsV1(k8sManager, controllers.GenericControllerParams{
-		MinAllowedCPU:    testMinAllowedCPU,
-		MinAllowedMemory: testMinAllowedMemory,
-	})).To(Succeed())
+	Expect(controllers.SetupForAppsV1(k8sManager)).To(Succeed())
 
 	Expect(k8sManager.Add(&controllers.VpaRunnable{
 		Client:          k8sManager.GetClient(),