Hi @kensimon. 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 @k8s-bot 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.

/unassign
/assign @derekwaynecarr

i like the goal of not losing the source message due to ttl concerns. if you can add a test case as this evolves, it would help me know if this will not cause painful event spam issues to recur (which was extremely painful 2 yrs ago).

Yeah I'll definitely add the test case to prove the point a bit. I've tested this manually with local-up-cluster and deployed a broken container, and it would create the first 10 events as independent events, but after the 10th event, the first event would be modified (the count and lastseen bumped, and the message string changed to the latest event's string).

The only thing that's a bit confusing is that there's no clear sign that an event is being re-used as a canonical event. As an example, 15 events come in that are the same Reason but different Messages... For the first 10 you would see:

event1{count: 1}
event2{count: 1}
...
event10{count: 1}

and when the 11th event comes in, you'll see:

event2{count: 1}
event3{count: 1}
...
event1{count: 2}

And if the other events fall off from TTL (I simulated this by setting the event-ttl to 60s), and 4 more events come in, you'll eventually only see:

event1{count: 6}

which is a bit confusing, since the event actually happened a total of 15 times, but there's 9 events that expired off and only the first one was re-used. I think it's not worse than we have now, but it's still not ideal.

i would prefer a new event, and a message prefix that says something like "combined from similar : last-message known here"

@derekwaynecarr See update, this now preserves the behavior of still creating an aggregate event, but keeping its message up-to-date. The PR is now quite a bit simpler, and the tests basically work as-is (all I had to do was change the TestEventAggregatorByReasonMessageFunc test to check for a prefix instead of a complete match.)

I'm accomplishing this by having the EventAggregate function responsible for deciding what cache key to use for the event as it gets passed to observeEvent. If the event shouldn't be aggregated, it returns the complete getEventKey result. If the event should be aggregated, it returns the EventAggregatorByReasonFunc instead.

Since observeEvent now takes the cache key as a parameter, it is only responsible for "is there already an event I've seen with this cache key, if so replace, if not create".

This should now be ok to test (PR subject has been updated)

Removed some dead code from when the PR was more complex, should be a much simpler change now.

@derekwaynecarr poke

@pmorie @derekwaynecarr PTAL.

@k8s-bot test this #IGNORE

@k8s-bot ok to test

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

I've seen these failures before, not sure if it's issues with the environment or just flakes...

Thanks!

/lgtm
/approve

hmm, seems i cant approve this. i did verify the behavior works as expected on a run-away controller, and the new experience is preferred.

/assign deads2k

/assign @deads2k

build failing due to gcloud's api quota getting exceeded, I'll try and trigger a re-test once things cool down a bit

/approve

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, kensimon

The full list of commands accepted by this bot can be found here.

@deads2k is this going to get merged for 1.7, or?

@deads2k is this going to get merged for 1.7, or?

yeah, its in queue https://submit-queue.k8s.io/#/queue

Automatic merge from submit-queue (batch tested with PRs 46726, 41912, 46695, 46034, 46551)