✨ 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

…ther runnables.

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

sbueringer · 2025-05-22T10:47:21Z

pkg/internal/controller/controller.go

+		// which won't be garbage collected if we hold a reference to it.
+		c.startWatches = nil
+
+		// Mark event sources as started after resetting the startWatches slice to no-op a Watch()


Watch won't be a no-op. It will start the source immediately instead of storing it in startWatches

sbueringer · 2025-05-22T10:56:06Z

pkg/internal/controller/controller.go

@@ -221,60 +299,71 @@ func (c *Controller[request]) Start(ctx context.Context) error {
 // startEventSources launches all the sources registered with this controller and waits
 // for them to sync. It returns an error if any of the sources fail to start or sync.
 func (c *Controller[request]) startEventSources(ctx context.Context) error {


I think the problem is bigger. Does Warmup start the sources with a nil queue? (if it is called before Start)

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

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

Change shutdown order to shutdown warmup runnables in parallel with o…

ba51d28

…ther runnables.

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 22, 2025

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

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

✨ 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!

sbueringer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer May 22, 2025

Choose a reason for hiding this comment

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!

Uh oh!

godwinpang commented Apr 9, 2025 •

edited

Loading

sbueringer commented May 22, 2025 •

edited

Loading