Task 2: Schedule DaemonSet Pods by default scheduler. #59862

k82cn · 2018-02-14T09:15:36Z

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 #59194
kubernetes/enhancements#548

Release note:

When ScheduleDaemonSetPods is enabled, the DaemonSet controller will delegate Pods scheduling to default scheduler.

k82cn · 2018-02-14T09:15:53Z

/cc @kow3ns @bsalamat

k82cn · 2018-02-16T13:52:11Z

@bsalamat @kow3ns , would you help to review whether this solution is OK: in this PR, I just 'hack' nodeShouldRunDaemonPod to return 'correct' value, e.g. wantToRun==shouldSchedule==true. And I'd like to simply it to one return value when graduating to beta.

bsalamat

Thanks, @k82cn!
Your change looks correct to me and given that we want to keep the old behavior and enable the new behavior with a flag, your change is OK, but in the future when we remove the scheduling logic of the DS controller, we can refactor and simplify the logic.

bsalamat · 2018-02-19T02:14:02Z

pkg/controller/daemon/daemon_controller.go

 // Predicates checks if a DaemonSet's pod can be scheduled on a node using GeneralPredicates
 // and PodToleratesNodeTaints predicate
 func Predicates(pod *v1.Pod, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {
 	var predicateFails []algorithm.PredicateFailureReason
-	critical := utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) && kubelettypes.IsCriticalPod(pod)
+
+	// If NoDaemonSetScheduler is enable, only check nodeSelector and nodeAffinity.


s/enable/enabled/

bsalamat · 2018-02-19T02:19:28Z

pkg/controller/daemon/daemon_controller.go

@@ -1327,11 +1342,48 @@ func NewPod(ds *extensions.DaemonSet, nodeName string) *v1.Pod {
 	return newPod
 }

+// nodeSelectPredicates are the predicates that used to select node candidates for DaemonSet
+func nodeSelectPredicates(pod *v1.Pod, meta algorithm.PredicateMetadata, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {


I would suggest renaming to nodeSelectionPredicates.

bsalamat · 2018-02-19T02:22:56Z

pkg/controller/daemon/daemon_controller.go

@@ -1327,11 +1342,48 @@ func NewPod(ds *extensions.DaemonSet, nodeName string) *v1.Pod {
 	return newPod
 }

+// nodeSelectPredicates are the predicates that used to select node candidates for DaemonSet


Please change to:
nodeSelectionPredicates runs a set of predicates that select candidate nodes for the DaemonSet.

bsalamat · 2018-02-19T02:24:41Z

pkg/controller/daemon/daemon_controller.go

+
+	// If NoDaemonSetScheduler is enable, only check nodeSelector and nodeAffinity.
+	if utilfeature.DefaultFeatureGate.Enabled(features.NoDaemonSetScheduler) {
+		// EssentialPredicates includes PodFitsHost, PodFitsHostPorts and PodMatchNodeSelector


We don't need to run PodFitsHostPorts and actually your function does not run it. Please remove it from the comment.

janetkuo · 2018-02-27T00:00:33Z

Please add a release note.

janetkuo

~~Is #60163 planned to be solved in this PR?~~ Never mind I saw you opened #60386

janetkuo · 2018-02-27T00:02:30Z

pkg/controller/daemon/daemon_controller.go

+					// Clearup hostname, default scheduler will handle it.
+					hostname = ""
+					podTemplate = template.DeepCopy()
+					podTemplate.Spec.Affinity = util.AppendDaemonSetPodNodeAffinity(


Please combine this and #59798 into one PR (ok to be separate commits)

sure; let me combine them :)

janetkuo · 2018-02-27T00:08:11Z

pkg/controller/daemon/daemon_controller.go

 // Predicates checks if a DaemonSet's pod can be scheduled on a node using GeneralPredicates
 // and PodToleratesNodeTaints predicate
 func Predicates(pod *v1.Pod, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {
 	var predicateFails []algorithm.PredicateFailureReason
-	critical := utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) && kubelettypes.IsCriticalPod(pod)
+
+	// If NoDaemonSetScheduler is enabled, only check nodeSelector and nodeAffinity.


So we can't remove all predicates in DS controller even when switching to default scheduler?

As daemonset can use NodeAffinity and NodeSelector to define a subset of cluster to stat the Daemon Pods, we re-use those predicates.

The advantage is that we can choose which predicate we'd like to re-use according to DaemonSet's requirement instead of keep it align with default scheduler when a new feature introduced.

yastij · 2018-02-28T09:28:32Z

@k82cn @bsalamat - is this one making it for 1.10 ?

janetkuo · 2018-03-08T18:22:27Z

/lgtm

janetkuo · 2018-03-09T00:50:45Z

/assign @smarterclayton
Would you approve the change? Thanks!

mfojtik · 2018-03-09T12:22:24Z

pkg/controller/daemon/daemon_controller.go

+
+				if utilfeature.DefaultFeatureGate.Enabled(features.ScheduleDaemonSetPods) {
+					podTemplate = template.DeepCopy()
+					podTemplate.Spec.Affinity = util.ReplaceDaemonSetPodHostnameNodeAffinity(


will this deal with existing daemonset pods during migration? or the daemonset have to be recreated/rescaled?

This only affects Daemon pod creation. Existing pods won't be affected. Existing DaemonSet might be affected when, for example, their pods are killed/dead or new nodes are added.

smarterclayton · 2018-03-09T15:01:44Z

Feature name approved

/approve

k8s-ci-robot · 2018-03-09T15:01:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, k82cn, smarterclayton

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/OWNERS~~ [smarterclayton]

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

k82cn · 2018-03-10T01:57:56Z

/kind feature
/priority critial-urgent

k82cn · 2018-03-10T01:58:28Z

/priority critical-urgent

k82cn · 2018-03-10T01:58:52Z

and ping for milestone approve :)

k8s-github-robot · 2018-03-11T12:36:54Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@janetkuo @k82cn @smarterclayton

Pull Request Labels

sig/apps sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

k8s-github-robot · 2018-03-11T12:37:54Z

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

k8s-github-robot · 2018-03-11T13:19:26Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 14, 2018

k82cn changed the title ~~Task 3: Schedule DaemonSet Pods by default scheduler.~~ WIP: Task 3: Schedule DaemonSet Pods by default scheduler. Feb 14, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2018

k8s-ci-robot requested review from bsalamat, kow3ns, janetkuo and tnozicka February 14, 2018 09:15

k82cn force-pushed the k8s_59194_3 branch from 0b29bfd to c89c9a7 Compare February 16, 2018 00:59

bsalamat reviewed Feb 19, 2018

View reviewed changes

k82cn force-pushed the k8s_59194_3 branch from c89c9a7 to ec3d001 Compare February 23, 2018 01:15

janetkuo self-assigned this Feb 23, 2018

k82cn changed the title ~~WIP: Task 3: Schedule DaemonSet Pods by default scheduler.~~ Task 3: Schedule DaemonSet Pods by default scheduler. Feb 25, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2018

janetkuo reviewed Feb 27, 2018

View reviewed changes

k82cn mentioned this pull request Feb 28, 2018

Helper func to add NodeAffinity to DaemonSet Pod. #59798

Closed

k82cn force-pushed the k8s_59194_3 branch from ec3d001 to 8af079b Compare February 28, 2018 09:09

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 28, 2018

k82cn changed the title ~~Task 3: Schedule DaemonSet Pods by default scheduler.~~ Task 2: Schedule DaemonSet Pods by default scheduler. Feb 28, 2018

k82cn changed the title ~~Task 2: Schedule DaemonSet Pods by default scheduler.~~ Task 3: Schedule DaemonSet Pods by default scheduler. Feb 28, 2018

k82cn force-pushed the k8s_59194_3 branch 2 times, most recently from 8f13c7e to 3b9650b Compare February 28, 2018 12:20

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

k8s-ci-robot assigned smarterclayton Mar 9, 2018

kubernetes deleted a comment from k8s-ci-robot Mar 9, 2018

mfojtik reviewed Mar 9, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2018

janetkuo added this to the v1.10 milestone Mar 9, 2018

k8s-github-robot added the milestone/incomplete-labels label Mar 9, 2018

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 10, 2018

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 10, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels Mar 10, 2018

k82cn added status/approved-for-milestone and removed milestone/needs-approval labels Mar 11, 2018

k8s-github-robot merged commit f7aafae into kubernetes:master Mar 11, 2018

k82cn deleted the k8s_59194_3 branch March 11, 2018 13:25

krzyzacy mentioned this pull request Mar 12, 2018

[test failed] gci-gce-alpha-features #61050

Closed

bsalamat mentioned this pull request Mar 16, 2018

Move Priority and Preemption to Beta #57471

Closed

13 tasks

k82cn mentioned this pull request Apr 19, 2018

Schedule DaemonSet Pods by default scheduler #59194

Closed

7 tasks

dims mentioned this pull request Jun 20, 2018

Before Suite running and failing occasionally on gci-gce-alpha-features job in master-blocking and release-1.11-blocking tests #65192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task 2: Schedule DaemonSet Pods by default scheduler. #59862

Task 2: Schedule DaemonSet Pods by default scheduler. #59862

k82cn commented Feb 14, 2018 •

edited by janetkuo

Loading

k82cn commented Feb 14, 2018

k82cn commented Feb 16, 2018

bsalamat left a comment

bsalamat Feb 19, 2018

bsalamat Feb 19, 2018

bsalamat Feb 19, 2018

bsalamat Feb 19, 2018

janetkuo commented Feb 27, 2018

janetkuo left a comment •

edited

Loading

janetkuo Feb 27, 2018

k82cn Feb 27, 2018

janetkuo Feb 27, 2018

k82cn Feb 27, 2018

yastij commented Feb 28, 2018

janetkuo commented Mar 8, 2018

janetkuo commented Mar 9, 2018

mfojtik Mar 9, 2018

janetkuo Mar 9, 2018

smarterclayton commented Mar 9, 2018

k8s-ci-robot commented Mar 9, 2018

k82cn commented Mar 10, 2018

k82cn commented Mar 10, 2018

k82cn commented Mar 10, 2018

k8s-github-robot commented Mar 11, 2018

k8s-github-robot commented Mar 11, 2018

k8s-github-robot commented Mar 11, 2018

Task 2: Schedule DaemonSet Pods by default scheduler. #59862

Task 2: Schedule DaemonSet Pods by default scheduler. #59862

Conversation

k82cn commented Feb 14, 2018 • edited by janetkuo Loading

k82cn commented Feb 14, 2018

k82cn commented Feb 16, 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

janetkuo commented Feb 27, 2018

janetkuo left a comment • 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

yastij commented Feb 28, 2018

janetkuo commented Mar 8, 2018

janetkuo commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 9, 2018

k8s-ci-robot commented Mar 9, 2018

k82cn commented Mar 10, 2018

k82cn commented Mar 10, 2018

k82cn commented Mar 10, 2018

k8s-github-robot commented Mar 11, 2018

k8s-github-robot commented Mar 11, 2018

k8s-github-robot commented Mar 11, 2018

k82cn commented Feb 14, 2018 •

edited by janetkuo

Loading

janetkuo left a comment •

edited

Loading