Refactor parse.Run to use an event funnel #1272

karlkfi · 2024-06-13T03:30:46Z

Replace the timers in parse.Run with an set of event Producers that
send events into a Funnel which synchronizes them for an EventHandler
which executes the RunFunc as needed for specific event types.

The PublishingGroupBuilder takes the timer periods and generates
Publishers, one for each type of event. Then the event Funnel starts
all the Publishers and runs a select loop that calls the Publisher's
Publish method when that event is received. The Publish method then
calls the Subscriber.Handle method.

Motivation:

Fixes a bug that was causing unnecessary back-to-back sync
attempts.
Validates retry backoff behavior.
Validates sync attempt scheduling behavior.
Hides hard to test delay timers with an easier to test/read event-based scheduling system.
Unblocks future refactoring into a control-loop scheduling system.

Dependencies:

Inject Parser.Clock for testing #1299

Possible future cleanup:

Drive namespace events with a channel from the namespace controller
Drive retry events with a channel so they only ever happen when a retry is needed, instead of checking inside the event whether a retry is needed.
Don't start retry events until a run attempt has errored
Combine the SyncEventType and SyncWithReimportEventType, and just use a lastImportTime check to decide whether to re-import or not.

pkg/parse/run_test.go

pkg/parse/events/timed_event_producer.go

pkg/parse/run.go

karlkfi · 2024-06-28T17:50:00Z

/retest

karlkfi · 2024-07-01T23:40:56Z

/retest

karlkfi · 2024-07-02T17:20:49Z

/retest

pkg/parse/events/events.go

pkg/parse/run.go

karlkfi · 2024-08-06T21:12:52Z

pkg/reconciler/reconciler.go

+		SyncPeriod:         opts.PollingPeriod,
+		FullSyncPeriod:     opts.ResyncPeriod,
+		StatusUpdatePeriod: opts.StatusUpdatePeriod,
+		// TODO: Shouldn't this use opts.RetryPeriod as the initial duration?


This seems like an existing bug. The Backoff wasn't being built with the RetryPeriod option.

karlkfi · 2024-08-16T23:12:20Z

/cc @Camila-B

google-oss-prow · 2024-08-16T23:12:23Z

@karlkfi: GitHub didn't allow me to request PR reviews from the following users: Camila-B.

Note that only GoogleContainerTools members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Camila-B

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.

pkg/parse/run.go

pkg/parse/events/events.go

pkg/parse/run.go

pkg/reconciler/reconciler.go

pkg/parse/events/sources.go

pkg/parse/run.go

pkg/parse/events/sources.go

pkg/parse/events/funnel_test.go

pkg/parse/events/publisher.go

pkg/parse/events/doc.go

pkg/parse/events/builder.go

pkg/parse/events/doc.go

nan-yu

One nit comment. Otherwise LGTM

pkg/parse/events/funnel_test.go

pkg/parse/events/builder.go

Replace the timers in parse.Run with an set of event Producers that send events into a Funnel which synchronizes them for an EventHandler which executes the RunFunc as needed for specific event types. The PublishingGroupBuilder takes the timer periods and generates Publishers, one for each type of event. Then the event Funnel starts all the Publishers and runs a select loop that calls the Publisher's Publish method when that event is received. The Publish method then calls the Subscriber.Handle method. Motivation: - Allows for testing retry backoff in the event funnel, separate from the parser logic, using a FakeClock. - This should make parse.Run look a little more like a controller's Reconcile loop from controller-runtime, which paves the way for further refactoring. - This is not an ideal solution yet, just a next step towards something that's easier to test and easier to change.

nan-yu

/lgtm

google-oss-prow · 2024-09-23T20:59:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nan-yu

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:

~~OWNERS~~ [nan-yu]

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

karlkfi requested a review from sdowell June 13, 2024 03:30

google-oss-prow bot added the do-not-merge/work-in-progress label Jun 13, 2024

google-oss-prow bot requested review from nan-yu and tiffanny29631 June 13, 2024 03:30

google-oss-prow bot added the size/XL label Jun 13, 2024

karlkfi commented Jun 13, 2024

View reviewed changes

pkg/parse/run_test.go Outdated Show resolved Hide resolved

karlkfi commented Jun 13, 2024

View reviewed changes

pkg/parse/events/timed_event_producer.go Outdated Show resolved Hide resolved

karlkfi commented Jun 13, 2024

View reviewed changes

pkg/parse/events/timed_event_producer.go Outdated Show resolved Hide resolved

karlkfi commented Jun 13, 2024

View reviewed changes

pkg/parse/run.go Outdated Show resolved Hide resolved

karlkfi force-pushed the karl-backoff-test branch 2 times, most recently from 92bbc57 to f6887f9 Compare June 13, 2024 20:32

karlkfi force-pushed the karl-backoff-test branch 4 times, most recently from c00a18c to cad7935 Compare June 28, 2024 02:26

karlkfi force-pushed the karl-backoff-test branch from cad7935 to 5fed287 Compare July 1, 2024 21:46

karlkfi force-pushed the karl-backoff-test branch from 5fed287 to b142636 Compare July 3, 2024 00:23

karlkfi force-pushed the karl-backoff-test branch 3 times, most recently from dfe215c to 6461a8f Compare August 6, 2024 20:19

google-oss-prow bot added size/XXL and removed size/XL labels Aug 6, 2024

karlkfi changed the title ~~[WIP] Refactor parse.Run to use an event generator~~ [WIP] Refactor parse.Run to use an event scheduler Aug 6, 2024

karlkfi changed the title ~~[WIP] Refactor parse.Run to use an event scheduler~~ [WIP] Refactor parse.Run to use event schedulers Aug 6, 2024

karlkfi force-pushed the karl-backoff-test branch 2 times, most recently from 222ec76 to f5f428e Compare August 6, 2024 20:52

karlkfi commented Aug 6, 2024

View reviewed changes

pkg/parse/events/events.go Outdated Show resolved Hide resolved

karlkfi commented Aug 6, 2024

View reviewed changes

pkg/parse/run.go Outdated Show resolved Hide resolved

karlkfi commented Aug 6, 2024

View reviewed changes

karlkfi force-pushed the karl-backoff-test branch 3 times, most recently from bd2d7bd to 1f9da0a Compare August 16, 2024 23:09

karlkfi changed the title ~~[WIP] Refactor parse.Run to use event schedulers~~ Refactor parse.Run to use an event funnel Aug 16, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Aug 16, 2024

karlkfi removed the request for review from tiffanny29631 August 16, 2024 23:11

nan-yu reviewed Aug 20, 2024

View reviewed changes

sdowell reviewed Aug 21, 2024

View reviewed changes

pkg/parse/events/sources.go Outdated Show resolved Hide resolved

karlkfi force-pushed the karl-backoff-test branch 2 times, most recently from 8bbb464 to f7b9406 Compare September 19, 2024 21:16

nan-yu reviewed Sep 20, 2024

View reviewed changes

pkg/parse/events/funnel_test.go Outdated Show resolved Hide resolved

pkg/parse/events/publisher.go Outdated Show resolved Hide resolved

pkg/parse/events/publisher.go Outdated Show resolved Hide resolved

karlkfi force-pushed the karl-backoff-test branch 2 times, most recently from 852cc5a to 2716f78 Compare September 20, 2024 21:03

nan-yu reviewed Sep 20, 2024

View reviewed changes

pkg/parse/events/doc.go Outdated Show resolved Hide resolved

pkg/parse/events/builder.go Show resolved Hide resolved

pkg/parse/events/doc.go Outdated Show resolved Hide resolved

karlkfi force-pushed the karl-backoff-test branch 2 times, most recently from 997373e to 55cb26e Compare September 23, 2024 18:13

nan-yu reviewed Sep 23, 2024

View reviewed changes

pkg/parse/events/funnel_test.go Outdated Show resolved Hide resolved

pkg/parse/events/builder.go Show resolved Hide resolved

karlkfi force-pushed the karl-backoff-test branch from 55cb26e to 66eaba4 Compare September 23, 2024 20:56

nan-yu approved these changes Sep 23, 2024

View reviewed changes

google-oss-prow bot assigned nan-yu Sep 23, 2024

google-oss-prow bot added the lgtm label Sep 23, 2024

google-oss-prow bot added the approved label Sep 23, 2024

google-oss-prow bot merged commit 9780d88 into GoogleContainerTools:main Sep 23, 2024
6 checks passed

karlkfi deleted the karl-backoff-test branch September 23, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor parse.Run to use an event funnel #1272

Refactor parse.Run to use an event funnel #1272

karlkfi commented Jun 13, 2024 •

edited

Loading

karlkfi commented Jun 28, 2024

karlkfi commented Jul 1, 2024

karlkfi commented Jul 2, 2024

karlkfi Aug 6, 2024

karlkfi commented Aug 16, 2024

google-oss-prow bot commented Aug 16, 2024

nan-yu left a comment

nan-yu left a comment

google-oss-prow bot commented Sep 23, 2024

Refactor parse.Run to use an event funnel #1272

Refactor parse.Run to use an event funnel #1272

Conversation

karlkfi commented Jun 13, 2024 • edited Loading

karlkfi commented Jun 28, 2024

karlkfi commented Jul 1, 2024

karlkfi commented Jul 2, 2024

karlkfi Aug 6, 2024

Choose a reason for hiding this comment

karlkfi commented Aug 16, 2024

google-oss-prow bot commented Aug 16, 2024

nan-yu left a comment

Choose a reason for hiding this comment

nan-yu left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Sep 23, 2024

karlkfi commented Jun 13, 2024 •

edited

Loading