VPA: Create event for VPA object when Pod is evicted

[adrianmoisey]

What type of PR is this?

/kind feature

What this PR does / why we need it:

A user asked for the VPA to emit an event that is related to the VPA object, in addition to the event related to the Pod object.
This makes sense, I see it as a "log" of sorts of what a particular VPA is up to.

Which issue(s) this PR fixes:

Fixes #7149

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Create VPA Event (in addition to a Pod event) when a Pod is evicted

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

[omerap12]

Just my humble opinion.

[omerap12]

Is there any reason why not doing something like:

basicVpa := getBasicVpa()
for _, pod := range pods[:2] {
    err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
    assert.Nil(t, err, "Should evict with no error")
}
for _, pod := range pods[2:] {
    err := eviction.Evict(pod, basicVpa, test.FakeEventRecorder())
    assert.Error(t, err, "Error expected")
}

[adrianmoisey]

Fixed in 5cbf777

[omerap12]

Can we do something like?

const (
    maxRetries = 5
    retryDelay = 100 * time.Millisecond
    contextTimeout = 5*time.Second
)

[adrianmoisey]

Good idea. I've made them variables instead, I hope that's fine: d16c5e7

[omerap12]

Instead of context.TODO() I suggest something like:

ctx, cancel := context.WithTimeout(context.Background(), contextTimeout)
defer cancel()

[adrianmoisey]

Great, thanks. Done in d16c5e7

[omerap12]

Check for error?

if err := corescheme.AddToScheme(vpascheme); err != nil {
        klog.Fatalf("Error adding core scheme: %v", err)
    }

[adrianmoisey]

Whoops! Thanks for catching that. Fixed in f6ada00

[omerap12]

Just a couple more comments.

[omerap12]

/lgtm

[kwiesmueller]

/lgtm
/approve

/hold
Feel free to unhold, just want to unblock.
WDYT about putting the extra event behind a flag? Not sure if people may be surprised about an extra event (load wise) for each evict. On the other hand load of those shouldn't be this high.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, kwiesmueller

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

[adrianmoisey]

WDYT about putting the extra event behind a flag? Not sure if people may be surprised about an extra event (load wise) for each evict. On the other hand load of those shouldn't be this high.

Hmmm, I'm not too sure.

I like it "on by default", and I think the load won't be too high. Kubernetes seems to handle events pretty well.
But I'm unsure at what scale people will be running the VPA.

At a guess this may happen roughly as often as the HPA scales Deployments, so my gut feel is that it doesn't need to be behind a flag.

[adrianmoisey]

What about putting it behind a flag, that is default to "on"?
That way users can opt-out if it's too noisy.

[adrianmoisey]

I thought about it a bit more. The act of scheduling a Pod often creates a few Events, ie:

Events:
  Type    Reason     Age   From               Message
  ----    ------     ----  ----               -------
  Normal  Scheduled  2m6s  default-scheduler  Successfully assigned default/hamster-65cd4dd797-rdplp to kind-control-plane
  Normal  Pulled     2m6s  kubelet            Container image "registry.k8s.io/ubuntu-slim:0.1" already present on machine
  Normal  Created    2m6s  kubelet            Created container hamster
  Normal  Started    2m6s  kubelet            Started container hamster

I think there also may be an event on the ReplicaSet, and if this was backed by an HPA, one there too.

I'm happy to merge this as-is if you're fine with it

[kwiesmueller]

sgtm
/unhold