@ahg-g: GitHub didn't allow me to assign the following users: adtac.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

/assign

/cc @denkensk

I'm thinking of if we can amplify the scope: which is to say, not only enable/disable moving (all) pods, but also provides mechanics to let plugins notify the scheduler framework which particular pods should be moved.

This would benefits in @denkensk 's coscheduling implementation to move the preceding (N-1) pods back to activeQ when the Nth pod comes - which satisfies the "minAvaialbe" requirement. Moreover, this would help in getting rid of flushing
pods back to activeQ #87850.

The suggestion is not to enable/disable moving all pods in all events, this is only true for the Service events. For the other two, node and volumes, we will test each pod and move the ones that pass the condition.

I agree that our proposal of integrating this with filters/permit plugins is more generic, but it adds a lot of complexity and potential for bugs, so we need to be careful with that I think.

One approach could be to start simple, validate and establish a reference point in terms of perf improvements, and then migrate to something more generic. The more generic approach will not be more performant than what is proposed in this issue.

I agree that our proposal of integrating this with filters/permit plugins is more generic, but it adds a lot of complexity and potential for bugs, so we need to be careful with that I think.

Yes, filters/permit runs in the critical path, and move Pods isn't a lock-free operation. What I want to say is during the design/implementation of this proposal, keep it in mind that plugins may need to actively "suggest" moving particular Pods back to activeQ.

One approach could be to start simple, validate and establish a reference point in terms of perf improvements, and then migrate to something more generic.

Agree.

Yes, filters/permit runs in the critical path, and move Pods isn't a lock-free operation. What I want to say is during the design/implementation of this proposal, keep it in mind that plugins may need to actively "suggest" moving particular Pods back to activeQ.

Right, we already designed the interface for such a potential eventuality: https://docs.google.com/document/d/1Dw1qPi4eryllSv0F419sKVbGXiPvSv6N_mJd6AVSg74/edit.

just fixing double "moving" in title:
/retitle Avoid moving pods out of unschedulable status unconditionally

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale

/assign
/unassign @adtac

/cc

Actually, to get rid of the reaction to service changes... all plugins would have to opt-out, so nvm.

Yes, to achieve that, every plugin has to implement EventsToRegister()...

BTW: one way is work it around is to add some temporary logic on framework side - skip movingPods upon Service events if ServiceAffinity is not enabled. And remove this temporary logic when all plugins opt-in EventsToRegister() in the next release.

That wouldn't qualify for codefreeze though.

That wouldn't qualify for codefreeze though.

Nvm then :)

We should simply not register for service updates at all if ServiceAffinity is not configured, which is the default.

We should simply not register for service updates at all if ServiceAffinity is not configured, which is the default.

Probably not a great idea to totally disable the registration, as an out-of-tree plugin may choose to respond to Service events.

We need to find a way for external plugins to register events because we will remove this registration once the plugin is deprecated.

We need to find a way for external plugins to register events because we will remove this registration once the plugin is deprecated.

Yes, also should take CRD into consideration. Probably dynamic informer.

I still think we should disabled Service event handlers when the plugin is not registered. If an external plugin is using it, then they could enable the service affinity filter plugin with an empty affinity labels or patch event handlers until we figure out a way for external plugins to register extra events.

btw, do you know of any external plugins that actually rely on service events?

I still think we should disabled Service event handlers when the plugin is not registered. If an external plugin is using it, then they could enable the service affinity filter plugin with an empty affinity labels or patch event handlers until we figure out a way for external plugins to register extra events.

That's fair, and I can work on this, adding some comments caveating this is a temporary workaround.

btw, do you know of any external plugins that actually rely on service events?

I'm not aware so far.

tbh, I would say plugins depending on services is an antipattern, given the existence of pod affinity.

Probably some CRDs might make sense for specialized plugins.

@Huang-Wei we should add metrics to track the efficiency of the solution. The metric would track the number of pods that were not put back into the queue broken down by plugin and even type for example.

we should add metrics to track the efficiency of the solution. The metric would track the number of pods that were not put back into the queue broken down by plugin and even type for example.

SG. I compiled a list to include on-going and to-do items here #100347.

I would like take "podtopologyspread" plugin.

@yuzhiquan Are you still working on this?

@Huang-Wei did we have PRs for number 2 and number 3?

I am asking about the two points from the four mentioned in the description of this issue: #94009 (comment)

I am asking about the two points from the four mentioned in the description of this issue: #94009 (comment)

Oops, misunderstanding, ignore me.

@Huang-Wei did we have PRs for number 2 and number 3?

• number 2: implement EnqueueExtensions interface in serviceaffinity #100357 mitigated the problem. In short, implement EnqueueExtensions interface in serviceaffinity #100357 includes two parts: one part is to remove Service event from default list so that only Pods failed by a plugin (e.g., ServiceAffinity) which explicitly are interested in Service events can be moved; the other part is to implement EventsToRegister() in ServcieAfffinity plugin. But that doesn't prevent the internal processing cycles like going through the unschedulabe pods and verify whether each is failed by that plugin, so it's a mitigation.
  PR sched: dynamic event handlers registration #101394 will fix this problem thoroughly.
• number 3: sched: support PreEnqueueChecks prior to moving Pods #100049

number 3: #100049

ah, yeah, I knew we worked on that in the previous release, lol. Thanks.