Pass the whole VPA into cappingRecommendationProcessor.Apply() #7527
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,9 @@ limitations under the License. | |
| package api | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| v1 "k8s.io/api/core/v1" | ||
| vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" | ||
| ) | ||
|
|
||
|
|
@@ -31,19 +33,28 @@ type sequentialRecommendationProcessor struct { | |
| } | ||
|
|
||
| // Apply chains calls to underlying RecommendationProcessors in order provided on object construction | ||
| func (p *sequentialRecommendationProcessor) Apply( | ||
| vpa *vpa_types.VerticalPodAutoscaler, | ||
| pod *v1.Pod) (*vpa_types.RecommendedPodResources, ContainerToAnnotationsMap, error) { | ||
|
|
||
|
There was a problem hiding this comment. Same for here ( not sure if needed but just my opinion ): if vpa == nil {
return nil, nil, fmt.Errorf("cannot process nil vpa")
}
if vpa.Status.Recommendation == nil {
return nil, nil, nil
}There was a problem hiding this comment. Should be fixed in 13d6ffa There was a problem hiding this comment. Should we check pod here as well? There was a problem hiding this comment. I thought about adding a check there, but all that function does is pass Pod on to another function call. |
||
| if vpa == nil { | ||
| return nil, nil, fmt.Errorf("cannot process nil vpa") | ||
| } | ||
| if vpa.Status.Recommendation == nil { | ||
| return nil, nil, nil | ||
|
There was a problem hiding this comment. I actually wonder if this should be an error case... The processors are generally "adjusting" the recommendation one at a time. If there is no recommendation to adjust then this feels like an error case to me. @omerap12 or @adrianmoisey are you seeing a case where this should be allowed? There was a problem hiding this comment. Oh wait I see that this is being done in the capping processor. Perhaps it is best to be consistent. However I wonder if this can eventually be made more strict and turned into an error. There was a problem hiding this comment. I am assuming that this case should never happen |
||
| } | ||
|
|
||
| recommendation := vpa.Status.Recommendation | ||
|
|
||
| accumulatedContainerToAnnotationsMap := ContainerToAnnotationsMap{} | ||
|
|
||
| for _, processor := range p.processors { | ||
| var ( | ||
| err error | ||
| containerToAnnotationsMap ContainerToAnnotationsMap | ||
| ) | ||
| recommendation, containerToAnnotationsMap, err = processor.Apply(vpa, pod) | ||
| vpa.Status.Recommendation = recommendation | ||
|
|
||
| for container, newAnnotations := range containerToAnnotationsMap { | ||
| annotations, found := accumulatedContainerToAnnotationsMap[container] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the back-and-forth, but should we really add these nilchecks? You're changing the signature coming from explicitly passing the recommendations, policy and conditions to passing an entire VPA object. So the context where all this is currently called from already ensures that the VPA and Pod aren't
nil: https://github.com/kubernetes/autoscaler/pull/7527/files#diff-b21ffa4a9ddd85d3bdd971cf8ff4cdb5050b05f8c349137a3f7e3152d4634d6eR108-R110There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is something I had considered, but I landed on the fact that this function should be able to handle any possible input.
I don't actually know if there's a Go or Kubernetes best practice around this. I figured that more defensive code is a good thing, but I'm also happy to change, but I'll wait for consensus before changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, since this is an exported function, we should include the nil check as to prevent panics from invalid usage. But, I do agree that we're only changing the signature to so it's unlikely the VPA object would ever be nil in current usage patterns. I'm comfortable with whatever you decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for defensive approach here.