VPA: Try make recommender e2e a little faster

[adrianmoisey]

By exiting early if the successful state exists

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The e2e tests have a few moments where it sleeps, waiting on a change. It's not obvious to the user if it's stuck or sleeping.
This PR tries to make it clear that it's sleeping.
For the 15 minute sleep, I made it periodically look for the successful state, and exist early if needed.

Which issue(s) this PR fixes:

Related to #7455

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adrianmoisey
Once this PR has been reviewed and has the lgtm label, please assign voelzmo for approval. 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

[omerap12]

Can we use logger instead of println?

[adrianmoisey]

I think we can. I'm not sure what is recommended for tests though. There seems to be a mix of Print and klog.
I guess klog makes sense?

[omerap12]

Yes . Klog seems better

[adrianmoisey]

Fixed in 56afad2

[omerap12]

Doesn't it set in line 156?
If so we can drop line 156

[omerap12]

And should we break earlier if the vpa was created within the max retries?

[adrianmoisey]

Doesn't it set in line 156? If so we can drop line 156

Yup, but it seems that the list and err in the loop is only scoped to that loop.

This is another option:

		var list *vpa_types.VerticalPodAutoscalerCheckpointList
		
		for i := 0; i < maxRetries; i++ {
			list, err = vpaClientSet.AutoscalingV1().VerticalPodAutoscalerCheckpoints(ns).List(context.TODO(), metav1.ListOptions{})
			if err == nil && len(list.Items) == 0 {
				break
			}
			klog.InfoS("Still waiting...")
			time.Sleep(retryDelay)
		}

		gomega.Expect(err).NotTo(gomega.HaveOccurred())
		gomega.Expect(list.Items).To(gomega.BeEmpty())

Honestly, I quite like the way it is now, the first declaration is to test if the loop should exit, the second is to test if the test passes.

However, I'm not strongly opinionated on either option. What do you think?

[adrianmoisey]

And should we break earlier if the vpa was created within the max retries?

Not sure what you mean here. This test is looking to see if a VPA Checkpoint is deleted within 15 minutes.

[omerap12]

And should we break earlier if the vpa was created within the max retries?

Not sure what you mean here. This test is looking to see if a VPA Checkpoint is deleted within 15 minutes.

My mistake. ignore it :)

[omerap12]

Doesn't it set in line 156? If so we can drop line 156

Yup, but it seems that the list and err in the loop is only scoped to that loop.

This is another option:

		var list *vpa_types.VerticalPodAutoscalerCheckpointList
		
		for i := 0; i < maxRetries; i++ {
			list, err = vpaClientSet.AutoscalingV1().VerticalPodAutoscalerCheckpoints(ns).List(context.TODO(), metav1.ListOptions{})
			if err == nil && len(list.Items) == 0 {
				break
			}
			klog.InfoS("Still waiting...")
			time.Sleep(retryDelay)
		}

		gomega.Expect(err).NotTo(gomega.HaveOccurred())
		gomega.Expect(list.Items).To(gomega.BeEmpty())

Honestly, I quite like the way it is now, the first declaration is to test if the loop should exit, the second is to test if the test passes.

However, I'm not strongly opinionated on either option. What do you think?

I see. so leave it as it is, it's just one more api call so I don't mind

[adrianmoisey]

Doesn't it set in line 156? If so we can drop line 156

Yup, but it seems that the list and err in the loop is only scoped to that loop.
This is another option:

		var list *vpa_types.VerticalPodAutoscalerCheckpointList
		
		for i := 0; i < maxRetries; i++ {
			list, err = vpaClientSet.AutoscalingV1().VerticalPodAutoscalerCheckpoints(ns).List(context.TODO(), metav1.ListOptions{})
			if err == nil && len(list.Items) == 0 {
				break
			}
			klog.InfoS("Still waiting...")
			time.Sleep(retryDelay)
		}

		gomega.Expect(err).NotTo(gomega.HaveOccurred())
		gomega.Expect(list.Items).To(gomega.BeEmpty())

Honestly, I quite like the way it is now, the first declaration is to test if the loop should exit, the second is to test if the test passes.
However, I'm not strongly opinionated on either option. What do you think?

I see. so leave it as it is, it's just one more api call so I don't mind

Yup, that's what I thought too.

I'm happy to change if any reviewer has any strong opinions.

[omerap12]

/lgtm