Fix VPA e2e test failures #6391
Conversation
k8s-ci-robot
added
kind/bug
|
ping @laoj2 this fixes the tests and should enable us to cut a new release soon |
| @@ -248,7 +248,12 @@ func (b *verticalPodAutoscalerBuilder) Get() *vpa_types.VerticalPodAutoscaler { | |||
| Mode: b.scalingMode[containerName], | |||
| }) | |||
| } | |||
| recommendation = b.recommendation.WithContainer(b.containerNames[0]).Get() | |||
| // VPAs with a single container may still use the old/implicit way of adding recommendations | |||
There was a problem hiding this comment.
As a newbie to this code, I found this a bit confusing. Can you add more details to this comment? (i.e., describe that there are two ways of adding recommendations, (1) setting recommendations field via some builders, (2) another one setting appendedRecommendations, through AppendRecommendation(...))
There was a problem hiding this comment.
I think this weird split was introduced a long time ago when the option was added to provide more than one recommendation to the test VPA object: https://github.com/kubernetes/autoscaler/pull/2866/files#diff-294b6f5855b82faa084f2866e8d3542c0c129776ecbbfd44f8d921f210706490
You're right, ideally this should be simplified to work with a single way to set the recommendations. We probably should do this in a follow-up PR to get the tests green again first?
| @@ -248,7 +248,12 @@ func (b *verticalPodAutoscalerBuilder) Get() *vpa_types.VerticalPodAutoscaler { | |||
| Mode: b.scalingMode[containerName], | |||
| }) | |||
| } | |||
| recommendation = b.recommendation.WithContainer(b.containerNames[0]).Get() | |||
| // VPAs with a single container may still use the old/implicit way of adding recommendations | |||
| r := b.recommendation.WithContainer(b.containerNames[0]).Get() | |||
There was a problem hiding this comment.
Why do we hardcode the container name index here and append to recommendation.ContainerRecommendations instead of doing this logic for all containers in the loop above?
There was a problem hiding this comment.
See comment above, probably the same issue/root cause?
|
Hey @laoj2 thanks for the review! I agree that this part is a mess in the test code and we should clean it up. I'd rather do this in a follow-up PR and get the tests green as soon as possible, does that sound good? |
|
ping @laoj2 can we merge this and get the tests back to green again, so we can cut a new release, or do you want me to do any additional changes on this PR? |
|
Yes, sounds good @voelzmo. Thanks for the fix! /lgtm |
|
@laoj2: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kwiesmueller, voelzmo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes failing e2e tests for VPA. After the refactorings done in #5599, the tests started failing – but nobody realized this until now: #6389
This PR fixes the tests by doing two things:
Build()on the VPA object. This interferes with the pattern ofAppendRecommendation(), which lead to a second entry in thecontainerRecommendationssection, where one of those entries was empty.Which issue(s) this PR fixes:
Fixes #6389
Does this PR introduce a user-facing change?