VPA e2e actuation, admission-controller, and updater test suites are failing

[jbartosik]

It seems to me that #5680 that broke VPA e2e tests. @wu0407 can you take a look?

• vpa-actuation tests are consistently failing. First failing run started on 2023-05-02 14:30 CEST, last passing run started on 2023-05-02 12:23 CEST.
• vpa-admission-controller tests are consistently failing. First failing run started on 2023-05-02 13:35 CEST, last passing run started on 2023-05-02 11:35 CEST.
• vpa-updater tests are consistently failing. First failing run started on 2023-05-02 14:01 CEST, last passing run started on 2023-05-02 12:01 CEST.

Two PRs labeled VPA merged on that day:

• add subresource status for vpa #5680 at 12:58 UTC+2 (so CEST) (matches when tests started to fail) and
• [vpa] [recommender] use factories for recommender and clusterStateFeeder #5676 at 15:46 UTC+2 (CEST) (failing tests already started by that time).

[jbartosik]

I think the problem is that tests are not creating VPA objects correctly now.

For example while raises request according to container min limit set in LimitRange test case was running I printed VPA it created and it has empty status[1]. But the test is supposed to set a recommendation (here).

It looks like after #5680 we can't set status like we used to. I think we should fix our e2e and mention that the change is breaking this way too but we might have to revert it.

@wu0407 can you fix those tests?

@mwielgus @kgolab what do you think?

[1]

$ kubectl get vpa --all-namespaces
NAMESPACE                       NAME          MODE   CPU   MEM   PROVIDED   AGE
vertical-pod-autoscaling-9363   hamster-vpa   Auto                          119s
jbartosik-macbookpro:~ jbartosik$ kubectl get vpa --all-namespaces -oyaml
apiVersion: v1
items:
- apiVersion: autoscaling.k8s.io/v1
  kind: VerticalPodAutoscaler
  metadata:
    creationTimestamp: "2023-05-08T12:31:57Z"
    generation: 1
    name: hamster-vpa
    namespace: vertical-pod-autoscaling-9363
    resourceVersion: "17919"
    uid: 3387c7dd-81e4-4909-9217-c367ada9b04d
  spec:
    resourcePolicy: {}
    targetRef:
      apiVersion: apps/v1
      kind: Deployment
      name: hamster-deployment
    updatePolicy:
      updateMode: Auto
kind: List
metadata:
  resourceVersion: ""

[voelzmo]

Wait, isn't raises request according to container min limit set in LimitRange only deploying an admission-controller and no recommender at all? How could the VPAs status be updated in a test like this?

[jbartosik]

@voelzmo the test is setting recommendation (here). That's why we don't want recommender running in those tests. We don't want recommendations written by the recommender, we want recommendations written by the test scenario.

EDIT: it seems to me that recommender was writing recommendations successfully, tests scenarios where we expected recommender to write a recommendation were passing. Tests where we wanted test scenario, not recommender, to write a recommendation were failing.

[jbartosik]

vpa-actuation, vpa-admission-controller, and vpa-updater tests passed after the revert PR merged.

[wu0407]

I will fix that

[voelzmo]

Ah, I see, that makes sense! When reviewing the PR, I didn't run the e2e tests (which in hindsight I probably should have), but checked that VPA works for a deployed example workload  – and that succeeded.
So this is about also allowing the test setup to set the VPA's status correctly, the VPA was working correctly (mostly?).

I think there's a few places where updating the status is currently ignored, like patching recommendations in v1beta2 and in v1 and in places where we upload an entire VPA with spec and status together, like the one you linked

Thanks for taking care of it, @wu0407!