chore: add recommender vpa-object-namespace integration test #8348

phuhung273 · 2025-07-23T13:14:21Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Integration test for recommender flags: vpa-object-namespace, ignored-vpa-object-namespaces
Moving some functions from e2e/v1 to e2e/utils for usage across integration and v1. Please let me know if this is appropriate.

Which issue(s) this PR fixes:

Relates #7723

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 · 2025-07-23T13:14:31Z

Hi @phuhung273. 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.

k8s-ci-robot · 2025-07-23T13:14:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: phuhung273
Once this PR has been reviewed and has the lgtm label, please assign raywainman for approval. For more information see the 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

omerap12 · 2025-07-23T17:13:46Z

/ok-to-test

adrianmoisey · 2025-07-28T19:46:01Z

Integration test for recommender flags: vpa-object-namespace, ignored-vpa-object-namespaces

Moving some functions from e2e/v1 to e2e/utils for usage across integration and v1. Please let me know if this is appropriate.

Hey

Would it be possible to split this PR into multiple commits for easier review?

phuhung273 · 2025-07-29T00:53:56Z

Sure @adrianmoisey, im thinking about:

1 commit only for moving functions from v1 -> utils
1 commit for new tests

How do you think ?

adrianmoisey · 2025-07-29T07:47:34Z

Sounds good to me

phuhung273 · 2025-07-29T15:04:36Z

I've splitted into 2 commits. Please help take a look @adrianmoisey. Thanks

omerap12

Couple of comments from my side. regarding the integration tests only

omerap12 · 2025-08-04T13:50:20Z

vertical-pod-autoscaler/e2e/integration/integration.go

+// Run a test container to try and contact the Kubernetes api-server from a pod, wait for it
+// to flip to Ready, log its output and delete it.
+func runKubernetesServiceTestContainer(c clientset.Interface, ns string) {
+	path := "test/images/clusterapi-tester/pod.yaml"


what is this path?

I have no idea actually... This file was copied from e2e/v1. Should I also move these functions to utils ?

cc @adrianmoisey

What was the reason for copying it, rather than reusing it?

No reason, I forgot this file at first 😅 Sorry for this.

IMO, we should reuse it.

By reusing, i mean moving it to utils. We cannot just making it public as importing e2e/v1 will trigger all tests.

vertical-pod-autoscaler/e2e/integration/recommender.go

omerap12 · 2025-08-04T13:56:26Z

vertical-pod-autoscaler/e2e/integration/integration.go

+
+// Run a test container to try and contact the Kubernetes api-server from a pod, wait for it
+// to flip to Ready, log its output and delete it.
+func runKubernetesServiceTestContainer(c clientset.Interface, ns string) {


I don't understand why we need this test. can you please elaborate?

This file was copied from e2e/v1. Can we discuss in #8348 (comment) ?

omerap12 · 2025-08-04T16:36:47Z

vertical-pod-autoscaler/e2e/integration/integration_test.go

+	if framework.TestContext.ListImages {
+		for _, v := range image.GetImageConfigs() {
+			fmt.Println(v.GetE2EImage())
+		}
+		os.Exit(0)
+	}


I'm not sure why this is here. Actually, I don't really understand what's happening in this whole file. Can you please explain?

I tried to remove this and the test still work. Will remove it.

About the whole file, I can confirm that we need TestMain and handleFlags since it will fail parsing params like --disable-log-dump in go test command https://github.com/phuhung273/autoscaler/blob/e90e2f377cb8ab74948e287817a79cb5636dff16/vertical-pod-autoscaler/hack/run-integration-locally.sh#L114

Understood. we can just remove this section then

omerap12 · 2025-08-04T16:39:45Z

vertical-pod-autoscaler/e2e/integration/recommender.go

+	})
+
+	ginkgo.AfterEach(func() {
+		f.ClientSet.AppsV1().Deployments(utils.RecommenderNamespace).Delete(context.TODO(), utils.RecommenderDeploymentName, metav1.DeleteOptions{})


Don't we need to delete the namespaces we are creating in every test?
like we have in line 54:

ignoredNamespace, err := f.CreateNamespace(context.TODO(), "ignored-namespace", nil)

From what i've seen, namespaces created by f.CreateNamespace are automatically cleaned up after every test.

I added delete deployment here since VPA (kube-system) is not automatically cleaned.

omerap12 · 2025-08-04T16:46:03Z

vertical-pod-autoscaler/e2e/integration/recommender.go

+	ginkgo.By("Ignored namespace should not be recommended")
+	ignoredVpa, err := vpaClientSet.AutoscalingV1().VerticalPodAutoscalers(ignoredNamespace).Get(context.TODO(), ignoredVpaCRD.Name, metav1.GetOptions{})
+	gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	gomega.Expect(ignoredVpa.Status.Conditions).Should(gomega.HaveLen(0))


Can you please add a comment explaining the logic of this test? It looks like you're creating two namespaces and two VPAs with the same targetRef. One VPA uses the --ignore-namespace flag, so it shouldn't generate any recommendations, while the other should. A short comment would help make this more understandable.

Yes your summary is correct. Two VPAs have the same targetRef but theyre in different namespaces.

I just added a comment to the function. Please let me know if it is clear enough. Thanks man.

omerap12 · 2025-08-04T16:47:08Z

vertical-pod-autoscaler/e2e/utils/common.go

@@ -145,6 +145,25 @@ func PatchVpaRecommendation(f *framework.Framework, vpa *vpa_types.VerticalPodAu
 	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Failed to patch VPA.")
 }

+// NewVPADeployment creates a VPA deployment with n containers
+// for e2e test purposes.
+func NewVPADeployment(f *framework.Framework, flags []string) *appsv1.Deployment {


TODO (future PRs)- update this function to receive args as well

Absolutely, right now i have no special config.

adrianmoisey · 2025-08-04T19:11:45Z

vertical-pod-autoscaler/e2e/utils/common.go

+
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
+	vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned"
+	framework_deployment "k8s.io/kubernetes/test/e2e/framework/deployment"


Nit: this should be in the section above

Sure i've moved it to above section. Hope I understand your point correctly ?

adrianmoisey · 2025-08-04T19:16:03Z

vertical-pod-autoscaler/e2e/integration/integration.go

This whole file already lives in e2e/v1/e2e.go with a few minor changes

Yes you're correct. Can we discuss in #8348 (comment) to also move functions in this file to utils ?

k8s-ci-robot removed the do-not-merge/needs-area label Jul 23, 2025

k8s-ci-robot requested review from jbartosik and kwiesmueller July 23, 2025 13:14

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 23, 2025

phuhung273 force-pushed the vpa-recommender-namespace-test branch 2 times, most recently from 038a8b6 to b123353 Compare July 23, 2025 13:58

phuhung273 mentioned this pull request Jul 23, 2025

VPA: Missing Flag Validation in VPA e2e Tests #7723

Open

1 task

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2025

move common functions from v1 to utils

656d363

phuhung273 force-pushed the vpa-recommender-namespace-test branch from b123353 to 656d363 Compare July 29, 2025 12:44

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2025

add integration test

e90e2f3

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 29, 2025

omerap12 reviewed Aug 4, 2025

View reviewed changes

adrianmoisey reviewed Aug 4, 2025

View reviewed changes

remove unused, add comment

f7b9af7

chore: add recommender vpa-object-namespace integration test #8348

Are you sure you want to change the base?

chore: add recommender vpa-object-namespace integration test #8348

Conversation

phuhung273 commented Jul 23, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented Jul 23, 2025

Uh oh!

k8s-ci-robot commented Jul 23, 2025

Uh oh!

omerap12 commented Jul 23, 2025

Uh oh!

adrianmoisey commented Jul 28, 2025

Uh oh!

phuhung273 commented Jul 29, 2025

Uh oh!

adrianmoisey commented Jul 29, 2025

Uh oh!

phuhung273 commented Jul 29, 2025

Uh oh!

omerap12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phuhung273 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phuhung273 Aug 5, 2025 •

edited

Loading