Moved node condition filter into a predicates. #50362

k82cn · 2017-08-09T08:49:53Z

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50360

Release note:

A new predicates, named 'CheckNodeCondition', was added to replace node condition filter. 'NetworkUnavailable', 'OutOfDisk' and 'NotReady' maybe reported as a reason when failed to schedule pods.

k82cn · 2017-08-09T08:52:49Z

/cc @gmarek

k82cn · 2017-08-09T08:54:05Z

If there's a customer configuration, do we need to add 'CheckNodeCondition' by default to keep backward compatibility?

yastij · 2017-08-09T09:21:09Z

@k82cn - I think we should, especially that scheduler should behave the same way as before if flags are disabled.

yastij · 2017-08-09T09:23:36Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+		// - NodeReady condition status is ConditionTrue,
+		// - NodeOutOfDisk condition status is ConditionFalse,
+		// - NodeNetworkUnavailable condition status is ConditionFalse.
+		if cond.Type == v1.NodeReady && cond.Status != v1.ConditionTrue {


~~I find cond.Type == v1.NodeReady && cond.Status == v1.ConditionFalse easier to read WDYT @k82cn ? (same goes for the checks below)~~

we can only schedule pod to ready node, neither False nor Unknown.

yup indeed forgot the unknown one

k82cn · 2017-08-09T14:53:26Z

/retest

gmarek · 2017-08-09T15:11:07Z

plugin/pkg/scheduler/factory/plugins.go

@@ -99,6 +100,16 @@ func RegisterFitPredicate(name string, predicate algorithm.FitPredicate) string
 	return RegisterFitPredicateFactory(name, func(PluginFactoryArgs) algorithm.FitPredicate { return predicate })
 }

+// RegisterRequiredFitPredicate registers a fit predicate with the algorithm registry,
+// it's always included in configuration. Returns the name with which the predicate was registered.
+func RegisterRequiredFitPredicate(name string, predicate algorithm.FitPredicate) string {


I don't like this - it should be a normal predicate, exactly as fit resources and stuff. Is someone want to break their cluster they're free to do so.

this is for customer configuration; if the scheduler is configured by customer's conf, they will not include node condition check until updating scheduler conf.

@gmarek - Is it too risky to have it ?

I am not sure about this either. The way this method is written implies that this is a new first class way of adding "required predicates". I would remove this method and requiredFitPredicateMap and would add this predicate to the predicates map hard-codedly in getFitPredicateFunctions with a TODO that this hard-coded predicate is there for backward compatibility and should be removed in a couple of releases.

I'm okay with these solutions as long as this ensures backward compatibility, @gmarek WDYT ?

If someone want to break their cluster they're free to do so.

I think no one want to break their cluster, but they may not know the admission in kubelet. We may also mark the predicates that used by kubelet as required, so maybe a function is better :).

I agree with @bsalamat. Also somehow marking predicates used by kubelet sounds like a good thing to do.

@bsalamat , does kubelet case make sense to you?

Yes, it does, but we should probably choose a different name for this function and the associated map to make it clear why these predicates are treated differently.

Updated to RegistEssentialFitPredicates :).

bsalamat · 2017-08-09T17:19:12Z

plugin/pkg/scheduler/factory/factory.go

@@ -771,7 +771,7 @@ func (f *ConfigFactory) getPluginArgs() (*PluginFactoryArgs, error) {
 		ReplicaSetLister:  f.replicaSetLister,
 		StatefulSetLister: f.statefulSetLister,
 		// All fit predicates only need to consider schedulable nodes.


Please remove this comment.

bsalamat · 2017-08-09T17:20:10Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+		}
+	}
+
+	// Ignore nodes that are marked unschedulable


Please remove this comment.

bsalamat · 2017-08-09T17:34:27Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

@@ -1301,6 +1301,38 @@ func CheckNodeDiskPressurePredicate(pod *v1.Pod, meta interface{}, nodeInfo *sch
 	return true, nil, nil
 }

+// CheckNodeConditionPredicate checks if a pod can be scheduled on a node
+// reporting out of disk, network unavailable and not ready condition.


optional: Please add to the comment that this predicate doesn't use pod and checks node condition ignoring the pod.

bsalamat · 2017-08-09T18:14:45Z

plugin/pkg/scheduler/factory/plugins.go

@@ -99,6 +100,16 @@ func RegisterFitPredicate(name string, predicate algorithm.FitPredicate) string
 	return RegisterFitPredicateFactory(name, func(PluginFactoryArgs) algorithm.FitPredicate { return predicate })
 }

+// RegisterRequiredFitPredicate registers a fit predicate with the algorithm registry,
+// it's always included in configuration. Returns the name with which the predicate was registered.
+func RegisterRequiredFitPredicate(name string, predicate algorithm.FitPredicate) string {


I am not sure about this either. The way this method is written implies that this is a new first class way of adding "required predicates". I would remove this method and requiredFitPredicateMap and would add this predicate to the predicates map hard-codedly in getFitPredicateFunctions with a TODO that this hard-coded predicate is there for backward compatibility and should be removed in a couple of releases.

k82cn · 2017-08-11T09:48:58Z

/retest

yastij · 2017-08-11T10:14:39Z

@k82cn flake #50262

yastij · 2017-08-11T12:50:32Z

/retest

yastij · 2017-08-11T13:40:47Z

@gmarek LGTY ?

k82cn · 2017-08-11T15:43:33Z

@gmarek , @bsalamat , is the latest PR OK for you ?

bsalamat · 2017-08-11T17:24:36Z

@k82cn Please do not squash your commits during the review process: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#6-squashing-and-commit-titles

k82cn · 2017-08-12T00:43:28Z

/retest

bsalamat · 2017-08-12T00:53:13Z

/lgtm

Thanks, @k82cn!

k8s-github-robot · 2017-08-12T00:53:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, k82cn

Associated issue: 50360

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

Needs approval from an approver in each of these OWNERS Files:

~~plugin/pkg/scheduler/OWNERS~~ [k82cn]
~~test/integration/scheduler/OWNERS~~ [k82cn]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-08-12T20:35:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-12T21:18:41Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-12T22:10:31Z

Automatic merge from submit-queue

rohitagarwal003 · 2017-08-14T21:35:00Z

Hey @k82cn,

One of the GPU tests started failing after this PR:
https://k8s-testgrid.appspot.com/wg-resource-management#gke-gpu&show-stale-tests=

First failed run: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gke-gpu/647?log#log
Last successful run: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gke-gpu/646?log#log

Can you please take a look?

bsalamat · 2017-08-16T00:11:12Z

@k82cn Could you please take a look at the broken test?

k82cn · 2017-08-16T00:13:18Z

sure :).

k82cn · 2017-08-16T00:38:25Z

One interesting thing is that: seems calico is deployed by deployment; the network is unavailable before it's deployed. By default, we'll use DaemonSet for calico; the DaemonSet will tolerate network unavailable to start calico daemons :). Let me check what happened for this case.

I0812 22:53:52.595] Aug 12 22:53:52.595: INFO: At 2017-08-12 22:42:51 +0000 UTC - event for calico-typha: {deployment-controller } ScalingReplicaSet: Scaled up replica set calico-typha-224432132 to 1

calico-typha-224432132-4smcp               Pending       [{Type:PodScheduled Status:False LastProbeTime:0001-01-01 00:00:00 +0000 UTC LastTransitionTime:2017-08-12 22:42:51 +0000 UTC Reason:Unschedulable Message:No nodes are available that match all of the following predicates: NodeNetworkUnavailable (3), NodeNotReady (3).}]

k82cn · 2017-08-16T12:55:44Z

@mindprince , @bsalamat , do you know how to check deploy script for gpu e2e test? I'd like to know why we deploy calico by Deployment.

bsalamat · 2017-08-16T17:03:34Z

@k82cn While we could look into the script, I am a bit worried that this change may break similar deployments. Do you know why we see this change of behavior? My understanding is that this PR should not change behavior of the system.

k82cn · 2017-08-16T23:18:59Z

@bsalamat , yes, this PR did not change behavior of system; I'd like to check whether other change than this PR for gpu e2e test, my understanding is that we can not deploy calico by Deployment.

rohitagarwal003 · 2017-08-16T23:38:55Z

This was the only change between the last successful run and the first failure.

k82cn · 2017-08-17T00:18:47Z

@mindprince , I means the change to deploy script. I'm not sure where to check it.

rohitagarwal003 · 2017-08-17T00:21:50Z

There were no changes in test-infra (it stayed at kubernetes/test-infra@d3713c353) between the last successful run and the first failed run.

k82cn · 2017-08-17T00:35:26Z

Good, let me check it :).

Automatic merge from submit-queue (batch tested with PRs 49342, 50581, 50777) Update RegisterMandatoryFitPredicate to avoid double register. **What this PR does / why we need it**: In #50362 , we introduced `RegisterMandatoryFitPredicate` to make some predicates always included by scheduler. This PRs is to improve it by avoiding double register: `RegisterFitPredicate` and `RegisterMandatoryFitPredicate` **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #50360 **Release note**: ```release-note None ```

k82cn · 2017-08-18T05:23:02Z

#50884 is used for the flaky e2e test.

Automatic merge from submit-queue Revert kubernetes#50362. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: part of kubernetes#50884 **Release note**: ```release-note None ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017

k8s-github-robot assigned thockin and dchen1107 Aug 9, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 9, 2017

k82cn force-pushed the k8s_50360 branch from eaf2cc3 to bab615b Compare August 9, 2017 08:52

k8s-ci-robot requested a review from gmarek August 9, 2017 08:52

yastij reviewed Aug 9, 2017

View reviewed changes

k82cn force-pushed the k8s_50360 branch 3 times, most recently from 165b4be to 814cc56 Compare August 9, 2017 13:58

thockin assigned bsalamat and unassigned thockin Aug 9, 2017

gmarek reviewed Aug 9, 2017

View reviewed changes

bsalamat reviewed Aug 9, 2017

View reviewed changes

k82cn force-pushed the k8s_50360 branch from 814cc56 to 8283190 Compare August 11, 2017 07:29

Moved node condition filter into a predicates.

e9738c0

k82cn force-pushed the k8s_50360 branch from 8283190 to e9738c0 Compare August 11, 2017 08:51

k82cn mentioned this pull request Aug 11, 2017

Mark essentail predicates as un-configurable. #50516

Closed

k82cn force-pushed the k8s_50360 branch from 2618c7b to 78e0783 Compare August 11, 2017 23:28

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2017

k8s-github-robot merged commit 2820b45 into kubernetes:master Aug 12, 2017

k82cn deleted the k8s_50360 branch August 12, 2017 23:01

k82cn mentioned this pull request Aug 14, 2017

Update RegisterMandatoryFitPredicate to avoid double register. #50581

Merged

yujuhong mentioned this pull request Aug 16, 2017

gce-alpha-features flaky/broken #50723

Closed

k82cn mentioned this pull request Aug 18, 2017

[Flaky] GPU test failed #50884

Closed

k82cn pushed a commit to k82cn/kubernetes that referenced this pull request Aug 19, 2017

Revert kubernetes#50362.

df3a699

janetkuo mentioned this pull request Aug 30, 2017

Mark essential predicates as un-configurable #50990

Closed

Moved node condition filter into a predicates. #50362

Moved node condition filter into a predicates. #50362

Conversation

k82cn commented Aug 9, 2017

k82cn commented Aug 9, 2017

k82cn commented Aug 9, 2017

yastij commented Aug 9, 2017

yastij Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Aug 11, 2017

yastij commented Aug 11, 2017

yastij commented Aug 11, 2017

yastij commented Aug 11, 2017

k82cn commented Aug 11, 2017

bsalamat commented Aug 11, 2017

k82cn commented Aug 12, 2017

bsalamat commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

fejta-bot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

k8s-github-robot commented Aug 12, 2017

rohitagarwal003 commented Aug 14, 2017

bsalamat commented Aug 16, 2017

k82cn commented Aug 16, 2017

k82cn commented Aug 16, 2017 • edited Loading

k82cn commented Aug 16, 2017

bsalamat commented Aug 16, 2017

k82cn commented Aug 16, 2017

rohitagarwal003 commented Aug 16, 2017

k82cn commented Aug 17, 2017

rohitagarwal003 commented Aug 17, 2017

k82cn commented Aug 17, 2017

k82cn commented Aug 18, 2017

yastij Aug 9, 2017 •

edited

Loading

k82cn commented Aug 16, 2017 •

edited

Loading