Fix an error in NodeUnschedulable plugin comment #93706

houminz · 2020-08-05T03:44:26Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
It corrected an error in the comment of NodeUnschedulable Plugin struct.
"a plugin that priorities nodes according to the node annotation scheduler.alpha.kubernetes.io/preferAvoidPods"
->" a plugin that checks if a pod can be scheduled on a node with Unschedulable spec."

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:
No

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

No

k8s-ci-robot · 2020-08-05T03:44:34Z

Hi @SimpCosm. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

houminz · 2020-08-05T03:48:13Z

/assign @hex108 @damemi

hex108

Thanks!

hex108 · 2020-08-05T08:28:04Z

pkg/scheduler/framework/plugins/nodeunschedulable/node_unschedulable.go

@@ -25,8 +25,7 @@ import (
 	framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1"
 )

-// NodeUnschedulable is a plugin that priorities nodes according to the node annotation
-// "scheduler.alpha.kubernetes.io/preferAvoidPods".
+// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a node with Unschedulable spec.


Suggested change

// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a node with Unschedulable spec.

// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a unschedulable node according to its tolerations.

Hi, thanks for your suggestion, I have updated the commit

hex108 · 2020-08-05T08:31:53Z

/remove-kind bug
/kind cleanup
/release-note-none

hex108 · 2020-08-05T10:42:15Z

/ok-to-test

Please squash the commits into one commit.

adtac · 2020-08-05T11:42:29Z

pkg/scheduler/framework/plugins/nodeunschedulable/node_unschedulable.go

@@ -25,8 +25,8 @@ import (
 	framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1"
 )

-// NodeUnschedulable is a plugin that priorities nodes according to the node annotation
-// "scheduler.alpha.kubernetes.io/preferAvoidPods".
+// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a


nit: a -> an :)

Thanks for your reminding, I would change that after we agree on the comment

adtac · 2020-08-05T11:48:23Z

pkg/scheduler/framework/plugins/nodeunschedulable/node_unschedulable.go

-// NodeUnschedulable is a plugin that priorities nodes according to the node annotation
-// "scheduler.alpha.kubernetes.io/preferAvoidPods".
+// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a
+// unschedulable node according to its tolerations.


I think "NodeUnschedulable is a plugin that filters pods from being scheduled on nodes with the node.kubernetes.io/unschedulable taint set to NoSchedule." would be a better description as it describes which specific taint would affect this plugin and what the effect is. What do the others think?

Yes, according to the code, if the node with the node.kubernetes.io/unschedulable taint set to NoSchedule and the pod does not tolerate the unschedulable taint, the pod would not be scheduled to the node.

By the way, according to the comment @k82cn

TODO (k82cn): deprecates node.Spec.Unschedulable in 1.13.

node.Spec.Unschedulable would have been deprecated in 1.13. I am wondering if we still need to check node.Spec.Unschedulable here.

if nodeInfo.Node().Spec.Unschedulable && !podToleratesUnschedulable {

node.Spec.Unschedulable is still used in places like kubectl cordon:

kubernetes/staging/src/k8s.io/kubectl/pkg/drain/cordon.go

Line 83 in aa06322

c.node.Spec.Unschedulable = c.desired

Hi @damemi , thanks for your reply. For this PR, I have updated the comment as suggested by @adtac . Do we need to deprecate node.Spec.Unschedulable in another PR ? Maybe I can help with that.

"NodeUnschedulable is a plugin that filters pods from being scheduled on nodes with the node.kubernetes.io/unschedulable taint set to NoSchedule."

This is not accurate, I think it should be:

"NodeUnschedulable plugin filters nodes that set node.Spec.Unschedulable=true unless the pod tolerates {key=node.kubernetes.io/unschedulable, effect:NoSchedule} taint."

Hi @damemi , thanks for your reply. For this PR, I have updated the comment as suggested by @adtac . Do we need to deprecate node.Spec.Unschedulable in another PR ? Maybe I can help with that.

I don't know the history, but I think this is why the filter is here, allow the pod to schedule on the pod which is tagged with node.kubernetes.io/unschedulable doesn't make sense to me, unless the case @damemi mentioned, if node.Spec.Unschedulable is deprecated we need to figure out alternatives.

As mentioned in #69010 , node.Spec.Unschedulable was supposed to be deprecated, but would not be removed until the API v2. To keep backward compatibility #68984 , we would filter the nodes that set node.Spec.Unschedulable=true unless the pod tolerates {key=node.kubernetes.io/unschedulable, effect:NoSchedule} taint.

Maybe we can fix the comment error here and when node.Spec.Unschedulable is deprecated, we could keep working on that.

houminz · 2020-08-10T07:15:33Z

/retest

ahg-g · 2020-08-27T15:28:48Z

/lgtm
/approve
/kind cleanup
/priority backlog
/release-note-none

k8s-ci-robot · 2020-08-27T15:28:50Z

@ahg-g: The label(s) kind/ cannot be applied, because the repository doesn't have them

In response to this:

/lgtm
/approve
/kind cleanup
/priority backlog
/release-note-none

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.

k8s-ci-robot · 2020-08-27T15:29:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, SimpCosm

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~~ [ahg-g]

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2020

k8s-ci-robot requested review from damemi and hex108 August 5, 2020 03:45

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 5, 2020

k8s-ci-robot assigned damemi and hex108 Aug 5, 2020

hex108 reviewed Aug 5, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 5, 2020

houminz force-pushed the fix/scheduler-plugin-comment branch from 38f82e0 to 9a44fa3 Compare August 5, 2020 11:10

adtac reviewed Aug 5, 2020

View reviewed changes

houminz force-pushed the fix/scheduler-plugin-comment branch from 9a44fa3 to 3e0e67d Compare August 6, 2020 01:49

Fix an error in NodeUnschedulable plugin comment

868dd41

houminz force-pushed the fix/scheduler-plugin-comment branch from 3e0e67d to 868dd41 Compare August 10, 2020 03:21

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 27, 2020

k8s-ci-robot assigned ahg-g Aug 27, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2020

justaugustus added this to the v1.20 milestone Aug 27, 2020

k8s-ci-robot merged commit a516041 into kubernetes:master Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix an error in NodeUnschedulable plugin comment #93706

Fix an error in NodeUnschedulable plugin comment #93706

houminz commented Aug 5, 2020

k8s-ci-robot commented Aug 5, 2020

houminz commented Aug 5, 2020

hex108 left a comment

hex108 Aug 5, 2020

houminz Aug 5, 2020

hex108 commented Aug 5, 2020

hex108 commented Aug 5, 2020

adtac Aug 5, 2020

houminz Aug 5, 2020

adtac Aug 5, 2020 •

edited

houminz Aug 5, 2020 •

edited

damemi Aug 5, 2020

houminz Aug 6, 2020

ahg-g Aug 6, 2020

chendave Aug 9, 2020

houminz Aug 10, 2020

houminz commented Aug 10, 2020

ahg-g commented Aug 27, 2020

k8s-ci-robot commented Aug 27, 2020

k8s-ci-robot commented Aug 27, 2020

	// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a node with Unschedulable spec.
	// NodeUnschedulable is a plugin that checks if a pod can be scheduled on a unschedulable node according to its tolerations.

Fix an error in NodeUnschedulable plugin comment #93706

Fix an error in NodeUnschedulable plugin comment #93706

Conversation

houminz commented Aug 5, 2020

k8s-ci-robot commented Aug 5, 2020

houminz commented Aug 5, 2020

hex108 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hex108 commented Aug 5, 2020

hex108 commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adtac Aug 5, 2020 • edited

Choose a reason for hiding this comment

houminz Aug 5, 2020 • edited

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

houminz commented Aug 10, 2020

ahg-g commented Aug 27, 2020

k8s-ci-robot commented Aug 27, 2020

k8s-ci-robot commented Aug 27, 2020

adtac Aug 5, 2020 •

edited

houminz Aug 5, 2020 •

edited