chore(vpa): update dependencies

[davidspek]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR updates the dependency package versions for the vertical pod autoscaler

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @davidspek. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[adrianmoisey]

/remove-kind feature
/kind cleanup
/ok-to-test

Thanks for the PR, can you also bump the VPA's dependencies, as this only does the e2e tests

[davidspek]

@adrianmoisey There are too many changes for GitHub to display them in the PR. But if you look at https://github.com/davidspek/autoscaler/tree/feat/update-vpa-k8s/vertical-pod-autoscaler the dependencies were updated for the regular VPA as well.

[adrianmoisey]

Wow! GitHub breaks is such wonderful ways when the commit is large.

[adrianmoisey]

So far this looks good to me. I am getting some failures on the e2e tests locally, but I think those aren't related. I'll spend some more time looking into that to make sure it's unrelated.

[davidspek]

Great, thanks!

[adrianmoisey]

There are two small linting issues:

$ golangci-lint run
pkg/recommender/input/cluster_feeder.go:165:25: SA1019: cache.NewIndexerInformer is deprecated: Use NewInformerWithOptions instead. (staticcheck)
	indexer, controller := cache.NewIndexerInformer(
	                       ^
pkg/utils/vpa/api.go:84:25: SA1019: cache.NewIndexerInformer is deprecated: Use NewInformerWithOptions instead. (staticcheck)
	indexer, controller := cache.NewIndexerInformer(vpaListWatch,
	                       ^

(if you run this locally, you may see more linting issues, but a PR was merged into master to take care of those, the ones I pasted above is the output after I merged master into your branch, locally).

[adrianmoisey]

/lgtm

e2e tests passed on my laptop.

Something to note: not all the dependencies have been upgraded. I don't mind doing those in a later PR, unless you want to do them now

[davidspek]

I can do them here. Which ones did I miss?

[davidspek]

I see I missed k8s.io/legacy-cloud-providers. Just pushed a commit to update that as well.

[adrianmoisey]

• github.com/fsnotify/fsnotify
• github.com/prometheus/client_golang
• github.com/prometheus/common
• github.com/stretchr/testify
• golang.org/x/time

[davidspek]

Oh I thought I'd scope this to just the k8s dependencies. Should I change it to all?

[adrianmoisey]

/lgtm

[raywainman]

Looks like we are losing the framework.TestContext.AllowedNotReadyNodes here, I haven't looked yet but do we know why? Is it OK?

[raywainman]

Found the original PR that removed this: kubernetes/kubernetes#124205

So looks generally harmless? Would love to validate that framework.TestContext.AllowedNotReadyNodes was always 0 in our case as well.

[adrianmoisey]

For what it's worth, I've run the e2e tests locally and everything passes

[raywainman]

/lgtm

The delta in changes here is crazy! Over 500k less lines of code!

2 super quick things:

1. Left a comment about the modification to the test, just want to quickly validate that this is intended.
2. Can we change the PR title to specify that this is for the e2e tests? Makes things easier when scanning changes for release notes.

[raywainman]

For whomever approves...
Do these dependencies relate to the compatibility chart in the readme:
https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/docs/installation.md#compatibility

My understanding is that this is partially affecting this - it means our tests with this VPA version will likely only work in the later Kubernetes versions. We'll have to be more careful about updating the compatibility table when we make the same changes in the main VPA code.

[adrianmoisey]

2. Can we change the PR title to specify that this is for the e2e tests? Makes things easier when scanning changes for release notes.

GitHub breaks on this change, it's both the VPA's dependencies and the e2e tests's dependencies.

For whomever approves...
Do these dependencies relate to the compatibility chart in the readme:
https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/docs/installation.md#compatibility

My understanding is that this is partially affecting this - it means our tests with this VPA version will likely only work in the later Kubernetes versions. We'll have to be more careful about updating the compatibility table when we make the same changes in the main VPA code.

I've been testing it locally using Kubernetes 1.26.3 and it works.

What I don't know is what the promises of client-go are regarding backwards compatibility, and how we determine when to update that compatibility table in the VPA's docs.

[davidspek]

@raywainman This PR also updates the dependencies for the main VPA, not just e2e. The PR is just to large to show it on GitHub. In terms of the change to the e2e test, those were necessary after the upgrade.

In terms of general compatibility, the cluster-autoscaler is already on v0.33.0-alpha.0. I didn't do that here and went to the latest released version instead. However, for a project like VPA I think it's very important to stay up to date with Kubernetes dependencies and test against the latest versions, similar to what the cluster-autoscaler does.

[ialidzhikov]

Wrt to all the comments that the diff is huge. We should remove the vendor/. I suggest to do it in another PR. The cluster-autoscaler seems to already remove the vendor/ dir.

[adrianmoisey]

What about:

1. Close this PR
2. Make a new PR that removes vendor/
3. Get that PR merged
4. Make a new PR to bump dependencies

It may make review much easier

[ialidzhikov]

I created issue for the vendor dir removal: #7570

[davidspek]

Once #7572 is merged I'll update this PR. There's a fix necessary after #7572 which relies on the dependencies being updated so I'd like to get that in quickly after this change. I'll create a new PR for the e2e dependency updates once #7573 is merged.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidspek
Once this PR has been reviewed and has the lgtm label, please ask for approval from raywainman. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• vertical-pod-autoscaler/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[adrianmoisey]

Is it worth removing this, and only scoping the PR to dependencies only?

[davidspek]

Sure