Add more precise logging for VPA resource recommendations

[nikimanoledaki]

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This PR adds more precise logging for VPA recommendations.

• Logs the exact processedRecommendation from the VPA Updater, specifically, the capped and uncapped target resources (CPU/mem).

Which issue(s) this PR fixes:

Helps to debug #6705

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The VPA Updater logs the processed recommendations, specifically, the target and uncapped target CPU/memory.

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

[k8s-ci-robot]

Welcome @nikimanoledaki!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @nikimanoledaki. 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/test-infra repository.

[leonardpahlke]

/ok-to-test

[nikimanoledaki]

/test

[k8s-ci-robot]

@nikimanoledaki: The /test command needs one or more targets.
The following commands are available to trigger optional jobs:

• /test pull-cluster-autoscaler-e2e-azure

In response to this:

/test

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.

[voelzmo]

Hey @nikimanoledaki thanks for the PR!

A few thoughts:

• do you care about all the recommendation values including the bounds? I guess given the issue at hand, we only really care about target and uncappedTarget (it might be interesting to see what the recommendation would be without minAllowed). So maybe let's remove upperBound and lowerBound here to reduce noise
• As the format string you're re-using doesn't have a placeholder for the recommendations, they're inserted at the end with this ugly !(EXTRA stuff. Maybe you could put a proper placeholder in the format string
• It might be the simplest solution to add a String() method to * RecommendedPodResources similar to this one (note that this doesn't account for any nil values yet)

func (r *RecommendedPodResources) String() string {
	sb := &strings.Builder{}
	for _, cr := range r.ContainerRecommendations {
		sb.WriteString(fmt.Sprintf("%s: [ target: %sK, %vm; uncappedTarget: %sK, %vm ]\n", cr.ContainerName, cr.Target.Memory().AsDec(), cr.Target.Cpu().MilliValue(), cr.UncappedTarget.Memory().AsDec(), cr.UncappedTarget.Cpu().MilliValue()))
	}
	return sb.String()
}

• Do we maybe want to add additional logging for that issue? I was integrating Add logging in case we have minimum memory recommendations gardener/autoscaler#299 into our fork to debug why recommender would give so low recommendations, even when the workload had been running for a long time. In theory, the in-memory model should be backfilled from the VPACheckpoint, such that even a small period with non-existing memory samples (caused e.g. by the KeyError that you observed) should not lead to recommendations below minAllowed.

I really appreciate that you're putting work into this!

[nikimanoledaki]

Hi @voelzmo thank you so much for your review! Apologies for the long delay in getting back to this. Responding to your feedback as soon as possible, first thing tomorrow. I needed to wrap up a few other things and finally made time to get back into this. Thank you for your understanding!

[nikimanoledaki]

/retest

[voelzmo]

This looks very good to me! We now have one remaining linter error, that's why the test-and-verify job is failing. Could you please take care of this:

/home/runner/work/autoscaler/autoscaler/go/src/k8s.io/autoscaler/vertical-pod-autoscaler/pkg/updater/priority/update_priority_calculator.go:168:1: exported method UpdatePriorityCalculator.GetProcessedRecommendationTargets should have comment or be unexported

[voelzmo]

I think we need to scale this in order to be Kilobytes, otherwise AsDec() returns the memory value in Bytes

Suggested change

	sb.WriteString(fmt.Sprintf("%sK ", cr.Target.Memory().AsDec()))
	sb.WriteString(fmt.Sprintf("%sk ", cr.Target.Memory().ScaledValue(resource.Kilo)))

whereas resource is imported from
"k8s.io/apimachinery/pkg/api/resource"

[nikimanoledaki]

Good point - added this in the latest commit + updated the tests!

[voelzmo]

/lgtm

[voelzmo]

@kwiesmueller this looks good from my side and will hopefully help investigating a long-standing issue. Can you please approve? Thanks!

[kwiesmueller]

Just a nit that the string building could be done without fmt which might be more efficient. (using the string builder directly and strconv).

Otherwise lgtm
/assign @raywainman

[kwiesmueller]

As you're already using the string builder here you could write this without fmt.Sprintf to be more efficient.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwiesmueller, nikimanoledaki, 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:

• vertical-pod-autoscaler/OWNERS [kwiesmueller]

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

[kwiesmueller]

Oh well, I guess I should have done a hold if I cared more about the nit.
This one is on me :#

[kwiesmueller]

@nikimanoledaki up to you if you want to follow up with another PR to reply to the nit

[nikimanoledaki]

@kwiesmueller thank you - agreed that it would be nice to make it more efficient! Opened a PR: #6883