Clarify VPA logging

[jbartosik]

No description provided.

[bskiba]

Please make the values into variables, so that we don't have to modify multiple places.

[jbartosik]

Done

[bskiba]

Maybe just pod.Name?

[jbartosik]

I'll add pod.Name but I want to keep pod index too. If the check always fails for pod with index 0 then likely always all pods would fail the check. If the check sometimes fails for pod with a higher index then we know that the problem can affect only some of pods in a deployment.

[bskiba]

AFAIR, the Expect will already say:
expected: this much,
got: this much

I think the only piece missing here for clarity is what we are this value of.
So I would much rather see a message that says for example "pod sth: resource request incorrect" or "pod sth: cpu limit ratio incorrect (not capped to limitrange max)"

I might remember wrong though. WDYT?

[jbartosik]

AFAIR, the Expect will already say:
expected: this much,
got: this much

I think the only piece missing here for clarity is what we are this value of.
So I would much rather see a message that says for example "pod sth: resource request incorrect" or "pod sth: cpu limit ratio incorrect (not capped to limitrange max)"

I might remember wrong though. WDYT?

Errors look like:

Expected
    <resource.Quantity>: {
        i: {value: 100, scale: -3},
        d: {Dec: nil},
        s: "100m",
        Format: "DecimalSI",
    }
to equal
    <resource.Quantity>: {
        i: {value: 200, scale: -3},
        d: {Dec: nil},
        s: "200m",
        Format: "DecimalSI",
    }

For me it wasn't clear which is the actual value and which is expected value.

[bskiba]

Expected to equal seems quite clear to me (although it's the other way round than I remembered). I don't want to add too much there since the code gets cluttered IMO.

[jbartosik]

Expected to equal seems quite clear to me (although it's the other way round than I remembered). I don't want to add too much there since the code gets cluttered IMO.

Order isn't the only information missing from the log. It's also not saying what those values are (limit or request). I think we should log more details about the failure, looking it up in test code is painful.

I added another commit, reducing amount of cluter a bit while trying to preserve most of information

[jbartosik]

/hold
Rebased to resolve merge conflicts, putting on hold while I rerun tests.

[jbartosik]

/hold cancel

Tests seem to work after rebase

[bskiba]

Thanks a lot, this looks really good
/lgtm
/apprive

[bskiba]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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 [bskiba]

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