New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 11, 2018

Conversation

@k82cn
Member

k82cn commented Feb 14, 2018

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 k82cn changed the title from Task 3: Schedule DaemonSet Pods by default scheduler. to WIP: Task 3: Schedule DaemonSet Pods by default scheduler. Feb 14, 2018

@k82cn

This comment has been minimized.

Member

k82cn commented Feb 14, 2018

@k82cn

This comment has been minimized.

Member

k82cn commented Feb 16, 2018

@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.

// 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.

This comment has been minimized.

@bsalamat

bsalamat Feb 19, 2018

Contributor

s/enable/enabled/

@@ -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) {

This comment has been minimized.

@bsalamat

bsalamat Feb 19, 2018

Contributor

I would suggest renaming to nodeSelectionPredicates.

@@ -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

This comment has been minimized.

@bsalamat

bsalamat Feb 19, 2018

Contributor

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

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

This comment has been minimized.

@bsalamat

bsalamat Feb 19, 2018

Contributor

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

@janetkuo janetkuo self-assigned this Feb 23, 2018

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

@janetkuo

This comment has been minimized.

Member

janetkuo commented Feb 27, 2018

Please add a release note.

@janetkuo

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

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

This comment has been minimized.

@janetkuo

janetkuo Feb 27, 2018

Member

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

This comment has been minimized.

@k82cn

k82cn Feb 27, 2018

Member

sure; let me combine them :)

// 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.

This comment has been minimized.

@janetkuo

janetkuo Feb 27, 2018

Member

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

This comment has been minimized.

@k82cn

k82cn Feb 27, 2018

Member

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.

@kow3ns kow3ns added this to Backlog in Workloads via automation Feb 27, 2018

@kow3ns kow3ns moved this from Backlog to In Progress in Workloads Feb 27, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 28, 2018

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

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

@yastij

This comment has been minimized.

Member

yastij commented Feb 28, 2018

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

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

@k82cn

This comment has been minimized.

Member

k82cn commented Feb 28, 2018

It should be in 1.10 :)

@k82cn

This comment has been minimized.

Member

k82cn commented Feb 28, 2018

/retest

@k82cn

This comment has been minimized.

Member

k82cn commented Mar 1, 2018

@janetkuo , comments addressed, PTAL; btw, I think this also should be in 1.10, would you help to /approve-for-milestone ?

@janetkuo

This comment has been minimized.

Member

janetkuo commented Mar 2, 2018

I think this also should be in 1.10, would you help to /approve-for-milestone ?

This feature needs to be accepted by 1.10 release team before I approve it for milestone, please see my comment: kubernetes/enhancements#548 (comment)

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 8, 2018

@janetkuo

This comment has been minimized.

Member

janetkuo commented Mar 9, 2018

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

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

This comment has been minimized.

@mfojtik

mfojtik Mar 9, 2018

Contributor

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

This comment has been minimized.

@janetkuo

janetkuo Mar 9, 2018

Member

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

This comment has been minimized.

Contributor

smarterclayton commented Mar 9, 2018

Feature name approved

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 9, 2018

[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:

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

@k82cn

This comment has been minimized.

Member

k82cn commented Mar 10, 2018

/kind feature
/priority critial-urgent

@k82cn

This comment has been minimized.

Member

k82cn commented Mar 10, 2018

/priority critical-urgent

@k82cn

This comment has been minimized.

Member

k82cn commented Mar 10, 2018

and ping for milestone approve :)

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 11, 2018

[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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 11, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 11, 2018

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

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

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation k82cn authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

Workloads automation moved this from In Progress to Done Mar 11, 2018

@k82cn k82cn deleted the k82cn:k8s_59194_3 branch Mar 11, 2018

@bsalamat bsalamat referenced this pull request Mar 16, 2018

Closed

Move Priority and Preemption to Beta #57471

12 of 13 tasks complete

@k82cn k82cn referenced this pull request Apr 19, 2018

Closed

Schedule DaemonSet Pods by default scheduler #59194

7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment