Do not schedule pod to the node under PID pressure. #60007

k82cn · 2018-02-17T02:47:28Z

Signed-off-by: Da K. Ma klaus1982.cn@gmail.com

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #54313

Release note:

Added CheckNodePIDPressurePredicate to checks if a pod can be scheduled on
a node reporting pid pressure condition.

k82cn · 2018-02-17T02:58:07Z

This PR adds a predicate for pid pressure; and the Taint/Toleration way is handled in #60008 as TaintNodeByConditions is still alpha now.

k82cn · 2018-02-25T02:07:24Z

@kubernetes/sig-scheduling-pr-reviews

ravisantoshgudimetla · 2018-02-25T19:57:30Z

pkg/scheduler/algorithm/predicates/predicates.go

+// CheckNodePIDPressurePredicate checks if a pod can be scheduled on a node
+// reporting pid pressure condition.
+func CheckNodePIDPressurePredicate(pod *v1.Pod, meta algorithm.PredicateMetadata, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {
+	// check if node is under pid pressure


Would it better to have a featuregating checked here? Something like utilfeature.DefaultFeatureGate.Enabled(features.TaintNodesByCondition)

Good point :) , we need to remove/disable this predicate if TaintNodesByCondition enabled.

xref #60398

wanghaoran1988 · 2018-02-26T02:14:31Z

pkg/scheduler/algorithmprovider/defaults/defaults.go

@@ -160,6 +160,9 @@ func defaultPredicates() sets.String {
 		// Fit is determined by node disk pressure condition.
 		factory.RegisterFitPredicate(predicates.CheckNodeDiskPressurePred, predicates.CheckNodeDiskPressurePredicate),

+		// Fit is determined by node pid pressure condition.
+		factory.RegisterFitPredicate(predicates.CheckNodeDiskPressurePred, predicates.CheckNodePIDPressurePredicate),


s/CheckNodeDiskPressurePred/CheckNodePIDPressurePred

k82cn · 2018-02-26T08:35:02Z

/retest

k82cn · 2018-03-01T03:21:48Z

@kubernetes/sig-scheduling-pr-reviews

bsalamat

Can you add a unit or integration test please?

bsalamat · 2018-03-02T02:31:25Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -1504,6 +1506,16 @@ func CheckNodeDiskPressurePredicate(pod *v1.Pod, meta algorithm.PredicateMetadat
 	return true, nil, nil
 }

+// CheckNodePIDPressurePredicate checks if a pod can be scheduled on a node
+// reporting pid pressure condition.
+func CheckNodePIDPressurePredicate(pod *v1.Pod, meta algorithm.PredicateMetadata, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {


Why didn't you add this check to CheckNodeConditionPredicate instead of this function?

make sense :)

agree with @bsalamat, we might want to check it there as it is nodeCondition.

Also to me, we might want to use taint instead, and let the toleration mechanism do the work, WDYT @k82cn ?

@bsalamat CheckNodeMemoryPressurePred, CheckNodeDiskPressurePred is also node condition, why we seprate these to two predicates?

@wanghaoran1988 @k82cn I see a benefit to having separate predicates. Users can enable or disable individual predicates in scheduler policy config. So, a user can disable CheckNodeMemoryPressurePred and/or CheckNodeDiskPressurePred in their policy config while keeping CheckNodeConditionPredicate enabled. For the same reason it may make sense to keep CheckNodePIDPressurePredicate as a separate predicate as well.

yes. separate predicates give user more options :).

Also to me, we might want to use taint instead, and let the toleration mechanism do the work,

Handled by #60008 .

yastij · 2018-03-02T17:11:44Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -86,6 +86,8 @@ const (
 	CheckNodeMemoryPressurePred = "CheckNodeMemoryPressure"
 	// CheckNodeDiskPressurePred defines the name of predicate CheckNodeDiskPressure.
 	CheckNodeDiskPressurePred = "CheckNodeDiskPressure"
+	// CheckNodePIDPressurePred defines the name of predicate CheckNodePIDPressure.
+	CheckNodePIDPressurePred = "CheckNodePIDPressure"


If we keep this as a predicate, we might want to add it to the predicatesOrdering list, see predicates.go

cc @bsalamat @resouer

yastij · 2018-03-02T17:14:40Z

pkg/scheduler/algorithm/predicates/predicates.go

@@ -1504,6 +1506,16 @@ func CheckNodeDiskPressurePredicate(pod *v1.Pod, meta algorithm.PredicateMetadat
 	return true, nil, nil
 }

+// CheckNodePIDPressurePredicate checks if a pod can be scheduled on a node
+// reporting pid pressure condition.
+func CheckNodePIDPressurePredicate(pod *v1.Pod, meta algorithm.PredicateMetadata, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {


agree with @bsalamat, we might want to check it there as it is nodeCondition.

Also to me, we might want to use taint instead, and let the toleration mechanism do the work, WDYT @k82cn ?

resouer

Just left comment per equiv class cache need. Thanks.

resouer · 2018-03-03T19:43:16Z

pkg/scheduler/algorithmprovider/defaults/defaults.go

@@ -160,6 +160,9 @@ func defaultPredicates() sets.String {
 		// Fit is determined by node disk pressure condition.
 		factory.RegisterFitPredicate(predicates.CheckNodeDiskPressurePred, predicates.CheckNodeDiskPressurePredicate),

+		// Fit is determined by node pid pressure condition.
+		factory.RegisterFitPredicate(predicates.CheckNodePIDPressurePred, predicates.CheckNodePIDPressurePredicate),
+


Should handle equiv cache invalidation properly when node condition changes. See: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/factory/factory.go#L822-L827

k8s-ci-robot · 2018-03-19T10:23:31Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

k82cn · 2018-04-19T12:42:48Z

Added integration test for the predicates, PTAL :)

bsalamat · 2018-04-19T23:44:08Z

test/integration/scheduler/predicates_test.go

+
+// TestNodePIDPressue verifies that scheduler's CheckNodePIDPressurePredicate predicate
+// functions works correctly.
+func TestNodePIDPressue(t *testing.T) {


s/TestNodePIDPressue/TestNodePIDPressure/
Pleaes fix the comment as well.

bsalamat · 2018-04-19T23:48:52Z

test/integration/scheduler/predicates_test.go

+			pod.Namespace, pod.Name, targetNode)
+	}
+
+	err = cs.CoreV1().Pods(context.ns.Name).Delete(testPod.Name, metav1.NewDeleteOptions(0))


Deleting the pods is not necessary in this test. You can leave it to the cleanupTest to take care of pod cleanup.

The cleanupTest only cleanup nodes :)

bsalamat · 2018-04-19T23:50:52Z

test/integration/scheduler/predicates_test.go

+	targetNode := nodes[1].Name
+
+	// Creats test pod.
+	testPod := &v1.Pod{


In order to ensure that the pod is not randomly assigned to the second node, I would add nodeSelector to the spec to choose the first node and would check that the pod is marked unschedulable.

Good point :)

k82cn · 2018-04-20T16:30:58Z

/retest

bsalamat · 2018-04-21T00:21:50Z

test/integration/scheduler/predicates_test.go

+		t.Fatalf("Test Failed: error: %v, while creating pod", err)
+	}
+
+	err = wait.Poll(pollInterval, wait.ForeverTestTimeout, podUnschedulable(cs, testPod.Namespace, testPod.Name))


You can use waitForPodUnschedulable instead.

bsalamat · 2018-04-21T00:24:03Z

test/integration/scheduler/predicates_test.go

+		t.Errorf("Test Failed: error, %v, while waiting for scheduled", err)
+	}
+
+	err = cs.CoreV1().Pods(context.ns.Name).Delete(testPod.Name, metav1.NewDeleteOptions(0))


optional: you don't need to delete the pod. cleanupTest deletes it anyway.

checked cleanupTest: it only deleted Nodes, DeleteTestingNamespace is empty. Did I miss anything?

ah, you are right. You can use cleanupPods to delete pods though.

bsalamat

Should we update the node controller (in a separate PR) to update Node taints when node is under PID pressure?

One nit and then LGTM.

bsalamat · 2018-04-23T23:31:44Z

test/integration/scheduler/predicates_test.go

+		t.Errorf("Test Failed: error, %v, while waiting for scheduled", err)
+	}
+
+	err = cs.CoreV1().Pods(context.ns.Name).Delete(testPod.Name, metav1.NewDeleteOptions(0))


ah, you are right. You can use cleanupPods to delete pods though.

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

k82cn · 2018-04-26T02:11:27Z

Should we update the node controller (in a separate PR) to update Node taints when node is under PID pressure?

Handled at #60008 :)

bsalamat

/lgtm

Thanks, @k82cn!

k8s-ci-robot · 2018-04-26T05:44:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/scheduler/OWNERS~~ [bsalamat,k82cn]
~~test/integration/scheduler/OWNERS~~ [bsalamat,k82cn]

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

k8s-github-robot · 2018-04-26T07:43:09Z

Automatic merge from submit-queue (batch tested with PRs 59367, 60007). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from resouer and wojtek-t February 17, 2018 02:47

k82cn mentioned this pull request Feb 17, 2018

Node still marks as ready when running out of processes #54313

Closed

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Feb 25, 2018

ravisantoshgudimetla reviewed Feb 25, 2018

View reviewed changes

wanghaoran1988 reviewed Feb 26, 2018

View reviewed changes

k82cn force-pushed the k8s_54313_1 branch 2 times, most recently from def3513 to 2c80306 Compare February 26, 2018 03:06

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2018

k82cn force-pushed the k8s_54313_1 branch from 2c80306 to 3e5bc65 Compare February 26, 2018 05:25

bsalamat reviewed Mar 2, 2018

View reviewed changes

yastij reviewed Mar 2, 2018

View reviewed changes

resouer reviewed Mar 3, 2018

View reviewed changes

k82cn force-pushed the k8s_54313_1 branch from 3e5bc65 to 9e299bc Compare March 19, 2018 10:23

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 19, 2018

k82cn force-pushed the k8s_54313_1 branch from 9e299bc to ac6bb09 Compare March 19, 2018 10:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 19, 2018

k82cn force-pushed the k8s_54313_1 branch from ac6bb09 to 6af24de Compare April 19, 2018 08:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2018

k82cn force-pushed the k8s_54313_1 branch from 6af24de to fea831e Compare April 19, 2018 12:40

bsalamat reviewed Apr 19, 2018

View reviewed changes

k82cn force-pushed the k8s_54313_1 branch 2 times, most recently from 237c9c6 to 6bfab6e Compare April 20, 2018 15:03

bsalamat reviewed Apr 21, 2018

View reviewed changes

k82cn force-pushed the k8s_54313_1 branch from 6bfab6e to cef8dea Compare April 21, 2018 00:51

bsalamat reviewed Apr 23, 2018

View reviewed changes

Do not schedule pod to the node under PID pressure.

2c10d15

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

k82cn force-pushed the k8s_54313_1 branch from cef8dea to 2c10d15 Compare April 26, 2018 02:10

bsalamat reviewed Apr 26, 2018

View reviewed changes

k8s-ci-robot assigned bsalamat Apr 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2018

k8s-github-robot merged commit 30e811c into kubernetes:master Apr 26, 2018

This was referenced Jun 28, 2018

CheckNodePIDPressure is not supported in v1.10 kubernetes-sigs/kubespray#2946

Merged

Taints and Tolerations not respected #63197

Closed

k82cn deleted the k8s_54313_1 branch May 16, 2022 06:29

Do not schedule pod to the node under PID pressure. #60007

Do not schedule pod to the node under PID pressure. #60007

Conversation

k82cn commented Feb 17, 2018 • edited Loading

k82cn commented Feb 17, 2018 • edited Loading

k82cn commented Feb 25, 2018

ravisantoshgudimetla Feb 25, 2018 • edited Loading

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 Feb 26, 2018

k82cn commented Mar 1, 2018

bsalamat left a 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

yastij Mar 2, 2018 • edited Loading

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

resouer left a 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

k8s-ci-robot commented Mar 19, 2018

k82cn commented Apr 19, 2018

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 Apr 20, 2018

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

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Apr 26, 2018

bsalamat left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 26, 2018

k8s-github-robot commented Apr 26, 2018

k82cn commented Feb 17, 2018 •

edited

Loading

k82cn commented Feb 17, 2018 •

edited

Loading

ravisantoshgudimetla Feb 25, 2018 •

edited

Loading

yastij Mar 2, 2018 •

edited

Loading