DRA: scheduler event handlers via assume cache

[pohly]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Events that make pods scheduleable were triggered by the informer cache, not the assume cache. For "claim was deallocated", this led to a small, unlikely race if a pod got scheduled and stopped so quickly that the informer cache didn't ever see the "claim is allocated" state. The event handler now reacts to changes in the assume cache because that cache is guaranteed to receive the "claim is allocated" state which cause some pod to not get scheduled, because by definition the cache must have listed some other claim as using resources needed for that pod.

Which issue(s) this PR fixes:

Fixes ##123698

Does this PR introduce a user-facing change?

DRA: fix some small, unlikely race condition during pod scheduling

/assign @kerthcet

Do you have time to review?

/cc @towca

This is related to the work that you are doing for the cluster autoscaler.

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please ask for approval from kerthcet. For more information see the Kubernetes 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:

• pkg/scheduler/OWNERS
• test/OWNERS [pohly]

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

[pohly]

/retest

[kerthcet]

Will take a look later, sorry I didn't notice the assignment.

[kerthcet]

The eventQueue is a bit hard to understand, I may need more time on that.

[kerthcet]

Seems we didn't test the DRA in the test because the feature gate is always closed.

[pohly]

Good catch. I added new test cases which exercise the informer creation more completely.

The feature gate check in eventhandlers.go is a bit redundant: the plugin itself never asks for any of these events when it is disabled.

[kerthcet]

Suggested change

	// which gets populated by the shared informer factory.
	// which gets populated by the shared informer factory and the dynamicResources plugin.

[kerthcet]

The Handle interface is getting fatter ands seems not under a good design.

[pohly]

Added, with slightly different spelling.

[kerthcet]

Seems we're maintaining two level event handlers here? I didn't quite get the idea why we need this? I read the comment of eventQueue, but still can't quite understand...

And do we have several handlers, what I learned from the PR is we only build the handler for ResourceClaim.

	for gvk, at := range gvkMap {		
                 case framework.ResourceClaim:
			if utilfeature.DefaultFeatureGate.Enabled(features.DynamicResourceAllocation) {
				// No need to wait for this cache to be
				// populated. If a claim is not yet in the
				// cache, scheduling will get retried once it
				// is.
				resourceClaimCache.AddEventHandler(buildEvtResHandler(at, framework.ResourceClaim, "ResourceClaim"))
			}
        }

[pohly]

The chain of events is this now: claim informer -> assume cache -> plugin.

This ensures that local changes to the assume cache are properly reported to the plugin. This wasn't the case before, leading to the race explained in #123698

[kerthcet]

Thanks for the explanation, I got the idea here, seems we have a MPSC architecture, we have producers like informer, local update, single consumer as the event handler.

Based on that, maybe we'd better not consume the events immediately after producing them, but consuming them in another goroutine independently, that's say we'll move something like emitEvents to another place, decoupled with producer, which makes the code more readable and maintainable IMHO.

Moreover, we can hide the producer-consumer logic at cache layer, but locate them at the storage layer, take the update for example (this is just a demo) :

c.updateStore(objInfo) // rather than call c.store.Update() and push the events together.

func (c *AssumeCache) updateStore(oldObj, newObj) {
    c.store.Update()
    c.eventQueue.Push(oldObj, newObj)
}

My two cents.

[pohly]

What you are proposing sounds very scary to me in terms of correctness and race conditions.

Can we keep the current architecture for event processing and just slightly extend it so that the source of events can be informers as well as local assume cache updates?

[kerthcet]

I mean something like this c7da77c, decouple the consumer logic.

[pohly]

Can you explain why that is useful? What is the advantage of decoupling?

Event handlers are meant to complete quickly. That is already true for informer events. So making the code more complex to support long-running event handlers doesn't seem warranted to me.

[kerthcet]

So the original idea is for simplify, the original code is a bit hard to understand, at least to me, so I don't think we're making it complex.

And I don't think it's slower, we'll consume the events continuously and we'll sent less events, on the contrary, I think we make less races, like 1) before when holding the lock, we'll iterate the handlers and send several related events, but now we only send one event, and 2) we'll call emitEvents at each OP, however, this will drain the whole events, which means we'll acquire unnecessary lock with several calls to emitEvents the same time.

BTW, for each OP, we'll send them to a queue and consume them from the queue in one function, what's your take here vs invoking handlers directly? Are you afraid the handlers will take a long time?

Would like to cc @alculquicondor @jsafrane for advices as well.

[pohly]

Sorry, I am not following. The assume cache should emit the same number of events regardless in which goroutine the event handlers are called, so "now we only send one event" doesn't make sense to me.

What does "OP" stand for?