✨ [Warm Replicas] Implement warm replica support for controllers.

[godwinpang]

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.

Builds upon #3190.

[k8s-ci-robot]

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

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]

/ok-to-test

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[godwinpang]

/retest

[sbueringer]

Review of the prod code. I'll try to get to test code as well

[sbueringer]

I think c.Started doesn't work anymore as expected.

With the current version of the PR it's used for two purposes:

1. Ensure a 2nd call to controller.Start returns an error
2. Ensure we are appending Sources to c.startWatches only until we call Start()

=> 1. still works as expected. 2 leads to problems

Now the following can happen:

1. Warmup is completed (i.e. c.startWatches is nil, c.Started is false)
2. Watch is called => source gets added to c.startWatches
3. Start is called and skips over the sync.Once in startEventSources

=> So the Source added in 2. is never started

I would suggest to

• introduce a new field called c.startedEventSources
• use c.startedEventSources in l.205 instead of c.Started
• set c.startedEventSources to true in startEventSources after we set c.startWatches to nil
• Update godoc comment in l.207 to:

	// Sources weren't started yet, store the sources locally and return.
	// These sources are going to be held until either Warmup() or Start(...) is called.

• Add the following to Warmup after we lock / defer unlock (so further calls to Watch are using this context to start watches instead of nil, Start will overwrite this later)

	// Set the internal context.
	c.ctx = ctx

@alvaroaleman does this sound correct to you?

[alvaroaleman]

Yes, makes sense. Also, lets add a test for this

[godwinpang]

d8650df fixes this but there is another race that this uncovers while testing

L336 starts a goroutine that can potentially outlive the duration of the caller holding the lock. The errGroup blocks on case <-sourceStartCtx.Done(), meaning that when the context is cancelled, startEventSources() doesn't wait for the the goroutine on L336 to complete before returning. This means that the mutex acquired in Warmup can be released before L339 is executed.

The problem variable is c.Queue since it can be read on L339, but also assigned to in Start on L261.

[godwinpang]

Maybe we can do something as follows? Keep the current error handling behavior, but add a channel blocking until watch.Start is called in the defer block

select {
case err := <-sourceStartErrChan:
	return err
case <-sourceStartCtx.Done():
        defer func() {<-sourceStartErrChan}() // <-- ⏰ this is the change
	if didStartSyncingSource.Load() { // We are racing with WaitForSync, wait for it to let it tell us what happened
		return <-sourceStartErrChan
	}
	if ctx.Err() != nil { // Don't return an error if the root context got cancelled
		return nil
	}
	return fmt.Errorf("timed out waiting for source %s to Start. Please ensure that its Start() method is non-blocking", watch)
}

edit: 730b30e was the attempt, but looks like it doesn't work because it fails the case where watch.Start blocks indefinitely.

edit 2:
bca3e2a I added a hasAccessedQueueChan channel to track whether or not c.Queue has been accessed and is safe to release the lock. Ideas for a better name is welcome :)

[sbueringer]

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

[sbueringer]

I think we would have to write c.Queue already in New. This also starts the queue though.
So this would also change the observable behavior of functions like NewUnmanaged / NewTypedUnmanaged.
So I think we can't move it into New.

The easiest might be to move it into startEventSourcesLocked (and rename that func to startEventSourcesAndQueueLocked)

Like this

	c.didStartEventSourcesOnce.Do(func() {
		queue := c.NewQueue(c.Name, c.RateLimiter)
		if priorityQueue, isPriorityQueue := queue.(priorityqueue.PriorityQueue[request]); isPriorityQueue {
			c.Queue = priorityQueue
		} else {
			c.Queue = &priorityQueueWrapper[request]{TypedRateLimitingInterface: queue}
		}
		go func() {
			<-ctx.Done()
			c.Queue.ShutDown()
		}()

[sbueringer]

I think one consequence is that depending on if Warmup or Start is actually running the sync.Once in startEventSourcesLocked the queue is getting shutdown if either the Warmup or one of the other runnable groups is shutdown, but this should be fine?

[alvaroaleman]

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

Ohh, extremely good catch.

The easiest might be to move it into startEventSourcesLocked (and rename that func to startEventSourcesAndQueueLocked)

This makes sense to me.

I think one consequence is that depending on if Warmup or Start is actually running the sync.Once in startEventSourcesLocked the queue is getting shutdown if either the Warmup or one of the other runnable groups is shutdown, but this should be fine?

I think so, yeah

[alvaroaleman]

Can we make sure that our tests validate that the queue we pass to the sources is non-nil? This should've been caught by tests and not by a human ideally

[sbueringer]

Also reviewed tests now

[sbueringer]

@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]

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

[sbueringer]

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

[sbueringer]

@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)