Allow a custom recommender to be set via annotation or label on the namespace #725
Conversation
There was a problem hiding this comment.
I left some inline comments, but I also have some concerns about adding two completely separate pieces of functionality in a single PR.
-
Setting the recommender makes complete sense, and I am happy to add this feature with some of the smaller issues fixed
-
Not overwriting the current settings is something that needs further discussion. I know we have an open issue for "playing nicely with existing VPAs", however I believe that has been made obsolete by prefixing the VPA names with Goldilocks. Can you elaborate on the need for this, or link an issue that shows the desire/need for this change?
Ideally, these would be separate PRs.
pkg/vpa/vpa.go
Outdated
| @@ -22,9 +22,9 @@ import ( | |||
| "strconv" | |||
| "strings" | |||
|
|
|||
| "github.com/samber/lo" | |||
There was a problem hiding this comment.
this is unnecessary code churn, and no longer in alphabetical order, can we please revert this?
pkg/vpa/vpa.go
Outdated
| @@ -457,3 +484,11 @@ func vpaMinReplicasForResource(obj runtime.Object) (*int32, bool) { | |||
|
|
|||
| return &minReplicasInt32, explicit | |||
| } | |||
|
|
|||
| func vpaRecommenderNameForNamespace(ns *corev1.Namespace) (string, error) { | |||
| recommenderName, ok := ns.Labels["goldilocks.fairwinds.com/vpa-recommenders"] | |||
There was a problem hiding this comment.
I believe we currently support both labels and annotations for the other settings that affect the VPA configuration. Would it be more appropriate to support both here?
There was a problem hiding this comment.
I think that makes sense, I can update
pkg/vpa/vpa_test.go
Outdated
| @@ -16,6 +16,7 @@ package vpa | |||
|
|
|||
| import ( | |||
| "context" | |||
| autoscaling "k8s.io/api/autoscaling/v1" | |||
There was a problem hiding this comment.
Please follow the convention elsewhere in this repo and add this with the other k8s apis below, rather than in the built-in imports.
| @@ -126,6 +126,11 @@ Example of an annotation | |||
| "memory": "2048Mi" } }, { "containerName": "istio-proxy", "mode": "Off" } | |||
| ] } | |||
| ``` | |||
| #### VPA Custom Recommender | |||
There was a problem hiding this comment.
Thank you for adding docs, much appreciated
pkg/vpa/vpa.go
Outdated
| @@ -348,6 +367,7 @@ func (r Reconciler) getVPAObject(existingVPA *vpav1.VerticalPodAutoscaler, ns *c | |||
| Name: "goldilocks-" + controller.Name, | |||
| Namespace: ns.Name, | |||
| }, | |||
| Spec: vpav1.VerticalPodAutoscalerSpec{}, | |||
There was a problem hiding this comment.
is this added for a particular reason?
There was a problem hiding this comment.
Let me double check. I recall adding it when I added the label functionality, but it may have been a faulty test so Ill address in my second rev of this
I will update this PR to only include this
The idea was that if someone wanted to use goldilocks to get started and automatically have VPAs for a number of resources in a number namespaces, they could then fine tune some of those things manually (with recommeders being the inspiration for this) without goldilocks overwriting them or the need for new VPAs, but I understand there are pros and cons here and I think for my use case, the annotation/label is enough |
brocoppler
changed the title
|
@sudermanjr I updated the PR to only include the new label/annotation and addressed all the minor things as well |
…the goldilocks controller) and a namespace label to add a custom recommender
|
@transient1 @sudermanjr Hey just wanted to bump this again if you need anything else to get this merged |
Checklist
Description
What's the goal of this PR?
To allow recommenders to be set on VPAs without being overwritten and provide a label that can be used to have goldilocks manage the setting of a given custom recommender
What changes did you make?
The spec is no longer always overwritten by the goldilocks controller, but rather it is additive when not explicitly a goldilocks managed field
What alternative solution should we consider, if any?
The label can be the ONLY way to set a custom recommender if using goldilocks
No label can be provided by goldilocks, but allow recommenders to be added and not overwritten still