✨ Implement warm replica support for controllers #3192

godwinpang · 2025-04-09T07:12:08Z

This change implements the proposal for warm replicas as proposed in #3121.
It adds an EnableWarmup option for controllers to optionally start as warmed replicas, which means that sources will be started before leader election has been won by the instance.

It also adds a new internal runnable interface called warmupRunnable with a single Warmup method that will be called before leader election. There is no guarantee that the Warmup method returns before leader election, just that it is called.

The controller's implementation of the Warmup method for the warmupRunnable starts the sources with a threadsafe method startEventSourcesAndQueueLocked that also adds events to the queue. In the case of a non-leader elected controller, it is intended for the Warmup method to race with the Start method to start the sources. The methods are synchronized by the didStartEventSourcesOnce sync.Once used in startEventSourcesAndQueueLocked.

For the most part, Warmup behaves exactly the same as Start with the following differences

There is no restriction as to how many times Warmup can be called, vs. Start which can only be called once.
Warmup doesn't initialize metrics for the controller, nor does it start the worker goroutines.

Shutdown

Warmup runnables are shutdown in a separate goroutine from the one that stops the leader election runnables, because there is no guarantee as to whether or not the Warmup or the Start method is holding the lock for the didStartEventSourcesOnce sync.Once instance.

Testing

On top of the unit + integration tests, this PR was tested with an internal project that ran this branch of controller-runtime for a week with (EnableWarmup=true`, and verified that the controllers on the non-leader elected replica had a populated workqueue.

k8s-ci-robot · 2025-04-09T07:12:18Z

Hi @godwinpang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

sbueringer · 2025-04-09T07:35:44Z

/ok-to-test

godwinpang · 2025-04-09T08:10:51Z

/retest

pkg/controller/controller_test.go

pkg/config/controller.go

pkg/controller/controller.go

pkg/manager/internal.go

pkg/manager/manager.go

pkg/manager/runnable_group.go

godwinpang · 2025-04-14T06:08:14Z

/retest

godwinpang · 2025-04-14T06:32:06Z

/retest

godwinpang · 2025-04-14T06:54:13Z

/retest

godwinpang · 2025-05-02T05:51:26Z

/retest

godwinpang · 2025-05-02T06:00:24Z

/retest

godwinpang · 2025-05-02T06:16:43Z

/retest

sbueringer · 2025-05-22T10:43:53Z

@godwinpang Pleae keep the conversations open (going forward). It's really hard to find the just resolved/fixed conversations to check that everything was addressed

pkg/internal/controller/controller.go

sbueringer · 2025-05-22T16:32:28Z

@godwinpang Heyho, discussed this with @alvaroaleman.

I think we're making really good progress on this PR and getting very close to merging it. We would just like to take a bit more time to ensure we don't break anything (I"ll also try to test it with Cluster API).

So we'll go ahead and cut the CR v0.21 release now already (a few folks are waiting for that :)). Once this PR merges we'll cherry-pick and cut a v0.21.1 patch release.

(I also modified the PR title as this PR is not breaking)

godwinpang · 2025-05-25T21:16:14Z

A heads up that I am OOO this week so there will not be much progress on this PR. We're going to enable this warmup feature internally on June 2nd when I get back for some soak time.

godwinpang · 2025-06-04T19:42:19Z

We've been running a forked version of c-r with EnableWarmup set to true for the past 2 days or so, and everything is working fine. The workqueue_depth metric shows that the non leader-elected pod's workqueues are being populated as expected.

I'll take a stab at

Adding a few more tests around Warmup behavior to make sure it has parity with Start esp around context timeout/cancellation.
Updating the PR description + design with what the latest implementation is.

Anything else we are looking to do to gain more confidence around this feature and get it merged? 😄

sbueringer · 2025-06-05T04:51:52Z

Sounds fine to me

alvaroaleman · 2025-06-19T16:26:49Z

@godwinpang any plans on wrapping this up?

godwinpang · 2025-06-24T08:28:04Z

@godwinpang any plans on wrapping this up?

Sorry, I missed this Github notification. I've updated the PR description and also duplicated the tests between Start and Warmup as appropriate.

alvaroaleman

thanks!
/hold

in case Stefan has anything to add

k8s-ci-robot · 2025-06-26T17:06:17Z

LGTM label has been added.

Git tree hash: 781742318cfc9c5c72173ae51ebe9fb41673044d

k8s-ci-robot · 2025-06-26T17:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, godwinpang

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~~ [alvaroaleman]

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

sbueringer · 2025-06-28T05:12:39Z

pkg/internal/controller/controller.go

+				case err := <-sourceStartErrChan:
+					return err
+				case <-sourceStartCtx.Done():
+					defer func() { <-hasAccessedQueueChan }() // Ensure that watch.Start has been called to avoid prematurely releasing lock before accessing c.Queue


Do we still need this (i.e. hasAccessedQueueChan) since we are starting the queue inside the sync.Once? (xref previous discussion: #3192 (comment))

We write c.Queue only once and directly above, I'm not sure if there is still a race condition around this field

If we actually need it, would it be a simpler way to achieve the same by passing in c.Queue as a parameter into the func in l.357?

nit for the godoc: this does not ensure that watch.Start was called only that q := c.Queue is done

Yes I agree we don't need this synchronization anymore with the move into sync.Once, looks like this was leftover as a change before the suggestions were taken into account from the linked discussion. I'll take out the code that protects against races on c.Queue access

sbueringer · 2025-06-28T05:32:21Z

pkg/manager/manager_test.go

+		}()
+
+		<-m.Elected()
+		Expect(<-runnableExecutionOrderChan).To(Equal(warmupRunnableName))


Sorry we might have discussed this before, how does this proof that warmupRunnable was run before leader election was won?
(This checks the order but not before/after leader election was won, correct?)

sbueringer · 2025-06-28T05:38:21Z

pkg/controller/controller_integration_test.go

@@ -48,30 +53,46 @@ var _ = Describe("controller", func() {
 	Describe("controller", func() {
 		// TODO(directxman12): write a whole suite of controller-client interaction tests

-		It("should reconcile", func() {
+		// Since all tests are run in parallel and share the same testenv, we namespace the objects


Are tests actually run in parallel? I thought they are run sequentially only tests of different packages are run in parallel (but each package has its own testenv)

sbueringer · 2025-06-28T05:49:23Z

pkg/internal/controller/controller_test.go

+
+			err := ctrl.startEventSourcesAndQueueLocked(ctx)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(ctrl.startWatches).To(BeNil(), "startWatches should be reset to nil after returning")


While we're here, maybe let's also check startedEventSourcesAndQueue is true

sbueringer · 2025-06-28T05:59:35Z

pkg/internal/controller/controller_test.go

+			}()
+
+			<-m.Elected()
+			Expect(<-runnableExecutionOrderChan).To(Equal(warmupRunnableName))


Same question here, how does this verify warmupRunnable was run before leader election is done?

sbueringer · 2025-06-28T06:01:14Z

pkg/internal/controller/controller_test.go

+			}
+
+			By("calling Warmup and Start concurrently")
+			go func() {


Should we also add a channel for start and block on it at the end of the test to ensure it was actually called?

sbueringer · 2025-06-28T06:01:41Z

pkg/internal/controller/controller_test.go

+			ctrl.CacheSyncTimeout = time.Second
+
+			var watchStartedCount atomic.Int32
+			ctrl.startWatches = []source.TypedSource[reconcile.Request]{


WDYT about adding more watches for some easy additional coverage? (e.g. add 10 of these in a for loop)

sbueringer · 2025-06-28T06:25:39Z

@godwinpang Thank you very much! We're almost there (only one idea for simplification in the prod code, otherwise just a few comments on tests)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 9, 2025

k8s-ci-robot requested review from joelanford and sbueringer April 9, 2025 07:12

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 9, 2025

godwinpang marked this pull request as draft April 9, 2025 07:12

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 9, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2025

godwinpang force-pushed the warm-replica-impl branch from 5d1be84 to 5db61c7 Compare April 9, 2025 07:44

alvaroaleman reviewed Apr 9, 2025

View reviewed changes

godwinpang added 3 commits April 13, 2025 22:31

[Warm Replicas] Implement warm replica support for controllers.

8239300

Remove irrelevant runnable_group.go code.

73fc8fa

Rename ShouldWarmup.

be1b1c2

godwinpang force-pushed the warm-replica-impl branch from 5db61c7 to be1b1c2 Compare April 14, 2025 05:58

fmt

c9b99eb

Change to atomic.Bool to avoid race in test.

e7a2bbf

godwinpang added 2 commits April 29, 2025 12:36

Address comments.

854987c

Add ready check to block controller startup until warmup is complete.

072ad4b

godwinpang force-pushed the warm-replica-impl branch from e05677a to 1d07efc Compare May 2, 2025 05:56

godwinpang force-pushed the warm-replica-impl branch from 1c32d37 to 7cc29dc Compare May 2, 2025 06:26

godwinpang added 3 commits May 22, 2025 01:50

Fix test races by ensuring goroutines do not outlive their It blocks.

ea2aa0e

Block on source start on context cancel.

730b30e

Guard access to c.Queue explicitly.

bca3e2a

sbueringer reviewed May 22, 2025

View reviewed changes

pkg/internal/controller/controller.go Outdated Show resolved Hide resolved

pkg/internal/controller/controller.go Outdated Show resolved Hide resolved

sbueringer changed the title ~~⚠️ [Warm Replicas] Implement warm replica support for controllers.~~ ✨ [Warm Replicas] Implement warm replica support for controllers. May 22, 2025

godwinpang added 4 commits May 22, 2025 14:49

Initialize queue in warmup with test.

12e938c

Fix watch comment.

de4232d

Add warmup to manager and controller integration tests.

a3dc13b

fmt + lint.

84d2053

sbueringer mentioned this pull request Jun 10, 2025

Controller Checkpointing #3234

Open

godwinpang added 2 commits June 23, 2025 22:52

Add tests for Warmup for parity with Start.

cefd22d

golangci-lint

8c7b12f

alvaroaleman approved these changes Jun 26, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2025

k8s-ci-robot assigned alvaroaleman Jun 26, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2025

alvaroaleman changed the title ~~✨ [Warm Replicas] Implement warm replica support for controllers.~~ ✨ Implement warm replica support for controllers Jun 26, 2025

godwinpang requested a review from sbueringer June 26, 2025 23:14

sbueringer reviewed Jun 28, 2025

View reviewed changes

✨ Implement warm replica support for controllers #3192

Are you sure you want to change the base?

✨ Implement warm replica support for controllers #3192

Conversation

godwinpang commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Shutdown

Testing

Uh oh!

k8s-ci-robot commented Apr 9, 2025

Uh oh!

sbueringer commented Apr 9, 2025

Uh oh!

godwinpang commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

godwinpang commented Apr 14, 2025

Uh oh!

godwinpang commented Apr 14, 2025

Uh oh!

godwinpang commented Apr 14, 2025

Uh oh!

godwinpang commented May 2, 2025

Uh oh!

godwinpang commented May 2, 2025

Uh oh!

godwinpang commented May 2, 2025

Uh oh!

sbueringer commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer commented May 22, 2025

Uh oh!

godwinpang commented May 25, 2025

Uh oh!

godwinpang commented Jun 4, 2025

Uh oh!

sbueringer commented Jun 5, 2025

Uh oh!

alvaroaleman commented Jun 19, 2025

Uh oh!

godwinpang commented Jun 24, 2025

Uh oh!

alvaroaleman left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

sbueringer Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godwinpang Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 28, 2025

Choose a reason for hiding this comment

godwinpang commented Apr 9, 2025 •

edited

Loading

sbueringer commented May 22, 2025 •

edited

Loading

sbueringer Jun 28, 2025 •

edited

Loading

godwinpang Jun 30, 2025 •

edited

Loading