GeneralPredicate as framework plugin config #84054
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
sig/scheduling
|
/assign @liu-cong |
|
/cc @Huang-Wei |
| @@ -651,7 +651,7 @@ func (g *genericScheduler) podFitsOnNode( | |||
| if err != nil { | |||
| return false, []predicates.PredicateFailureReason{}, nil, err | |||
| } | |||
| } else if !podsAdded || len(failedPredicates) != 0 { | |||
| } else if !podsAdded || len(failedPredicates) != 0 || !status.IsSuccess() { | |||
There was a problem hiding this comment.
I don't fully understand why we run predicates twice here. But looking at the code, when this branch is reached, the status can only be Success or Unschedulable. So this change is to break if status is Unschedulable?
There was a problem hiding this comment.
The comment at the top tries to explain this, do you have questions about that?
There was a problem hiding this comment.
Now I understand it. Discussed offline, we are going to keep it for now and I am gonna do a followup cleanup.
| @@ -105,6 +105,15 @@ func NewDefaultConfigProducerRegistry() *ConfigProducerRegistry { | |||
| PredicateToConfigProducer: make(map[string]ConfigProducer), | |||
| PriorityToConfigProducer: make(map[string]ConfigProducer), | |||
| } | |||
| registry.RegisterPredicate(predicates.GeneralPred, | |||
| func(_ ConfigProducerArgs) (plugins config.Plugins, pluginConfig []config.PluginConfig) { | |||
| // GeneralPredicate is a combination of predicates. | |||
There was a problem hiding this comment.
Is it cleaner to call RegisterPredicate each time for each predicate instead of combining them in one "general predicate"? Or what's special about "GeneralPred" compared to others?
There was a problem hiding this comment.
Unfortunately "GeneralPredicate" is registered as a "composite" standalone predicate. By composite I mean that its implementation basically calls a group of other predicates. I think it was a mistake making it a predicate on of its own, but we are stuck with it since it is now a value that we deem valid in the schedule config file.
There was a problem hiding this comment.
As @ahg-g said, we were not doing well in the "GeneralPredicate" thing - it shouldn't be considered as, or registered as a serious Predicate, instead, it should serve as a helper to invoke a series of real Predicate, and called by consumers like controller manager and kubelet.
To make our code readable and the semantics of Predicates/FilterPlugins clear, I hope we can get rid of the "GeneralPredicate" thing in 2 aspects:
- Rename it to be not related with "Predicate" - just serve as a "composite" to invoke other Predicate/Plugin
- Discard the support to expose it as a regular Predicate in Policy API.
WDYT?
There was a problem hiding this comment.
I am happy to rename/discard it, but it is part of the Policy v1 API. If someone has a custom policy configuration that included "GeneralPredicate" as one of the priorities, their scheduler will fail to start.
However, if we do remove it as a predicate in the policy API, it is easy for users to migrate, they just need to modify their policy file to include the corresponding predicates, and it is backward compatible (i.e., specifying the list of corresponding predicate will work for new and older versions of the scheduler). I am fine with this if we get an API review approval for it, but that should be in a separate PR.
There was a problem hiding this comment.
if we do remove it as a predicate in the policy API, it is easy for users to migrate, they just need to modify their policy file to include the corresponding predicates, and it is backward compatible
Yup, it sounds good to me.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind feature
What this PR does / why we need it:
GeneralPred as Filter configuration that combines PodFitsResources, PodFitsHost, PodFitsHostPorts and PodMatchNodeSelector.
Which issue(s) this PR fixes:
Fixes #84039
Does this PR introduce a user-facing change?:
/priority important-soon