Eventing: [regression] Upgrade to latest breaks receiving events or produce event doubles

[cardil]

Describe the bug

In PR #2388 I was observing a couple (~2%) of lost events during upgrade & downgrade. Recently (after merging master) I see that after doing upgrade to latest all subsequent events sent to default broker were lost.

--- FAIL: TestProbe (343.99s)
    test_runner.go:95: namespace is : "test-probe-p4xhq"
    prober.go:89: There ware 398 errors. Listing them below.
    prober.go:92: expecting to have 827 unique events received, but received 431 unique events

Such error can be seen here: https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/2388/pull-knative-eventing-integration-tests/1232974021964861442

Expected behavior

No events should be lost.

To Reproduce

Run test/e2e-upgrade-tests.sh from PR #2388

Knative release version

v0.12.0-203-g206b8ea6

Additional context

As this is a regression from previous behavior it should have High priority.

[vaikas]

Thank you!!!
From the logs, there are all kinds of errors (can't mount volumes, etc.) and it's tricky to see the end state of the objects during the failure. Would it be possible to add use of Tracker for the created resources (like in the eventing e2e tests for resources created), so we can see the status of all the objects on a failed test.

[cardil]

What is totally strange is that latest changes in #2388 are now back to reporting an only couple of missing events instead of dropping all subsequent.

Somehow described here behavior must be lurking somewhere between 206b8ea...daed3c3

[cardil]

I notice that after tests there are left the 2 triggers I create, and a default broker. All of them should be deleted by test, but they have finalizers field that prevents them to be deleted. Manually editing them to remove the field, release them.

[cardil]

What is totally strange is that latest changes in #2388 are now back to reporting an only couple of missing events instead of dropping all subsequent.

Somehow described here behavior must be lurking somewhere between 206b8ea...daed3c3

I think that's not true. Sometimes only couple events get lost, but sometimes it totally breaks.

[cardil]

After #2388 been updated to the latest master, instead of lost events now we have event received twice.

[cardil]

I've observe interesting thing in latest code. In PR #3260 I restore original upgrade prober setting to have 10ms interval between events.

Much more events were propagated: 18.4k and there was only 12 missing ones. No doubles. That's 0.065% error rate.

It looks like we lost 3 events at the beginning, and couple more somehow in the middle.

There were 18478 events propagated, but 12 errors occurred. Listing them below.
FAIL: expecting to have 18489 unique events received, but received 18478 unique events
FAIL: event #1 should be received once, but was received 0 times
FAIL: event #2 should be received once, but was received 0 times
FAIL: event #5 should be received once, but was received 0 times
FAIL: event #10569 should be received once, but was received 0 times
FAIL: event #10570 should be received once, but was received 0 times
FAIL: event #10571 should be received once, but was received 0 times
FAIL: event #10572 should be received once, but was received 0 times
FAIL: event #10573 should be received once, but was received 0 times
FAIL: event #10576 should be received once, but was received 0 times
FAIL: event #10577 should be received once, but was received 0 times
FAIL: event #10578 should be received once, but was received 0 times 

Log: https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/3260/pull-knative-eventing-upgrade-tests/1268473590890631168#1:build-log.txt%3A1217

[pierDipi]

For the record after #2932 we see

event #number should be received once, but was received 2 times

Latest run from testgrid: https://prow.knative.dev/view/gcs/knative-prow/logs/ci-knative-eventing-continuous/1285558395314638849

@cardil What is your goal here, exactly once?

I think it's acceptable to have duplicated events and the test now should fail if there are events lost.

[cardil]

I think the goal should be to send every event just once. Doubles should not be default behavior, as users may produce systems that will not tolerate doubles well enough.

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[cardil]

/reopen
/remove-lifecycle stale

[knative-prow-robot]

@cardil: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle stale

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.

[slinkydeveloper]

I think the goal should be to send every event just once. Doubles should not be default behavior, as users may produce systems that will not tolerate doubles well enough.

That is not a guarantee we support, we support only at least once, while volatile components (in memory channel) supports only best effort guarantee. Hence the behaviour of the upgrade tests is "as expected".

Since the release 0.12 is very old and we don't support it anymore, i'm going to close this issue. If we need to take an action item related to this topic on a recent release, please open another issue