⚠️ [Warm Replicas] 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 a NeedWarmUp option for controllers to optionally start as warmed replicas.

Note for reviewers: This draft PR is feature complete with tests but the main purpose is to make sure that things are on the right track. Some of the naming / comments are a bit inconsistent, I will address them in a followup cleanup tomorrow.

Builds upon #3190.

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: godwinpang
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 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

alvaroaleman · 2025-04-09T14:45:12Z

pkg/manager/internal.go

@@ -439,6 +439,11 @@ func (cm *controllerManager) Start(ctx context.Context) (err error) {
 		return fmt.Errorf("failed to start other runnables: %w", err)
 	}

+	// Start and wait for sources to start.
+	if err := cm.runnables.Warmup.Start(cm.internalCtx); err != nil {


I think this should come right after the caches and the comment shouldn't make assumptions about what the Warmup internally does.

The other issue: This needs to block until the Warmup has terminated, otherwise we may end up starting the controller before the sources are started, as the check we added only checks if we started to start the sources, not if we finished doing so

Will change to coming right after the caches, but don't completely understand the significance of having it after vs. before the non-leader election runnables; mind explaining a bit?

This needs to block until the Warmup has terminated, otherwise we may end up starting the controller before the sources are started, as the check we added only checks if we started to start the sources, not if we finished doing so

Is this a big problem? Replicas will only start the controller after they win leader election so I don't see an issue in the leader election failover case; are you saying that in non-leader election cases the behavior of warmup should be that it completely blocks controller startup?

Will change to coming right after the caches, but don't completely understand the significance of having it after vs. before the non-leader election runnables; mind explaining a bit?

Just so it starts asap

Is this a big problem? Replicas will only start the controller after they win leader election so I don't see an issue in the leader election failover case; are you saying that in non-leader election cases the behavior of warmup should be that it completely blocks controller startup?

Yes. The expectation in the controller is that the sources are fully started before the rest of the controller starts. If we don't ensure this actually finished before we start the controller, we violate that expectation.

Additionally, there may be other cases where ppl implement this interface and it seems surprising to me that we wouldn't let the warmup finish before we start.

If we wanted to be really fancy, we could do this per-runnable (i.E. not block all runnables on Warmup having finished, just the ones whose Warmup didn't finish) but that is probably more involved.

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

godwinpang · 2025-05-02T06:37:56Z

/retest

godwinpang · 2025-05-02T09:48:19Z

/retest

godwinpang · 2025-05-02T17:59:35Z

/retest

alvaroaleman · 2025-05-07T13:56:46Z

pkg/controller/controller.go

@@ -93,6 +93,13 @@ type TypedOptions[request comparable] struct {
 	//
 	// Note: This flag is disabled by default until a future version. It's currently in beta.
 	UsePriorityQueue *bool
+
+	// NeedWarmup specifies whether the controller should start its sources when the manager is not
+	// the leader. When set to true, the controller will not restart its sources when/if it


Please explain what reasons there might be to use this and the restart bit is incorrect, it would be start. Please also use the same explanation in pkg/config/controller

alvaroaleman · 2025-05-07T13:59:13Z

pkg/internal/controller/controller.go

+// Warmup implements the manager.WarmupRunnable interface.
+func (c *Controller[request]) Warmup(ctx context.Context) error {
+	if c.NeedWarmup == nil || !*c.NeedWarmup {
+		c.didEventSourcesFinishSync.Store(ptr.To(true))


It didn't finish sync, why are we storing true in here?

alvaroaleman · 2025-05-07T14:01:45Z

pkg/internal/controller/controller_test.go

@@ -1014,6 +1014,82 @@ var _ = Describe("controller", func() {
 			})
 		})
 	})
+
+	Describe("Warmup", func() {


Please add a test where Warmup is called but NeedsWarmup is disabled that validates that nothing happens in Warmup and that the sources do get started when Start is called

alvaroaleman · 2025-05-07T14:02:28Z

pkg/manager/manager.go

+
+	// WaitForWarmupComplete is a blocking function that waits for the warmup to be completed. It
+	// returns false if it could not successfully finish warmup.
+	WaitForWarmupComplete(context.Context) bool


I don't understand, why can't we instead wait for Warmup to return for this check?

pkg/manager/manager.go

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

Keep test helper structs private.

43118a3

godwinpang force-pushed the warm-replica-impl branch from 7cc29dc to 43118a3 Compare May 2, 2025 09:45

godwinpang marked this pull request as ready for review May 2, 2025 18:18

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2025

k8s-ci-robot requested review from FillZpp and JoelSpeed May 2, 2025 18:18

alvaroaleman reviewed May 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ [Warm Replicas] Implement warm replica support for controllers. #3192

⚠️ [Warm Replicas] Implement warm replica support for controllers. #3192

godwinpang commented Apr 9, 2025

k8s-ci-robot commented Apr 9, 2025

k8s-ci-robot commented Apr 9, 2025

sbueringer commented Apr 9, 2025

godwinpang commented Apr 9, 2025

alvaroaleman Apr 9, 2025

godwinpang Apr 14, 2025

godwinpang Apr 14, 2025 •

edited

Loading

alvaroaleman Apr 25, 2025

godwinpang commented Apr 14, 2025

godwinpang commented Apr 14, 2025

godwinpang commented Apr 14, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

alvaroaleman May 7, 2025

alvaroaleman May 7, 2025

alvaroaleman May 7, 2025

alvaroaleman May 7, 2025

⚠️ [Warm Replicas] Implement warm replica support for controllers. #3192

Are you sure you want to change the base?

⚠️ [Warm Replicas] Implement warm replica support for controllers. #3192

Conversation

godwinpang commented Apr 9, 2025

k8s-ci-robot commented Apr 9, 2025

k8s-ci-robot commented Apr 9, 2025

sbueringer commented Apr 9, 2025

godwinpang commented Apr 9, 2025

alvaroaleman Apr 9, 2025

Choose a reason for hiding this comment

godwinpang Apr 14, 2025

Choose a reason for hiding this comment

godwinpang Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

alvaroaleman Apr 25, 2025

Choose a reason for hiding this comment

godwinpang commented Apr 14, 2025

godwinpang commented Apr 14, 2025

godwinpang commented Apr 14, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

godwinpang commented May 2, 2025

alvaroaleman May 7, 2025

Choose a reason for hiding this comment

alvaroaleman May 7, 2025

Choose a reason for hiding this comment

alvaroaleman May 7, 2025

Choose a reason for hiding this comment

alvaroaleman May 7, 2025

Choose a reason for hiding this comment

godwinpang Apr 14, 2025 •

edited

Loading