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

Keep backward compatibility for 'node.Spec.Unschedulable'. #68984

Merged
merged 1 commit into from Sep 27, 2018

Conversation

@k82cn
Member

k82cn commented Sep 23, 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):
Fixes #68899

Release note:

Fixes issue [#68899](https://github.com/kubernetes/kubernetes/issues/68899) where pods might schedule on an unschedulable node.
@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 23, 2018

Member

/kind bug
/priority critical-urgent
/sig scheduling
/milestone 1.12

Member

k82cn commented Sep 23, 2018

/kind bug
/priority critical-urgent
/sig scheduling
/milestone 1.12

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 23, 2018

Contributor

@k82cn: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

In response to this:

/kind bug
/priority critical-urgent
/sig scheduling
/milestone 1.12

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.

Contributor

k8s-ci-robot commented Sep 23, 2018

@k82cn: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

In response to this:

/kind bug
/priority critical-urgent
/sig scheduling
/milestone 1.12

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.

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 23, 2018

Member

/milestone v1.12

Member

k82cn commented Sep 23, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Sep 23, 2018

Keep backward compatibility for 'node.Spec.Unschedulable'.
Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Sep 23, 2018

@tpepper

Given all of the conversation in #68899 and the nature of the problem here, I think this needs a better commit message still and might even deserve a release note under "Known Issues" or SIGs Scheduling/Node to at least remind operators what's changed here and the taints and tolerations they need to be considering across updates in this version range.

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 23, 2018

Member

I think this needs a better commit message still and might even deserve a release note

There's info in the description; no behaviour changed, so no release note here.

Member

k82cn commented Sep 23, 2018

I think this needs a better commit message still and might even deserve a release note

There's info in the description; no behaviour changed, so no release note here.

@tpepper

This comment has been minimized.

Show comment
Hide comment
@tpepper

tpepper Sep 23, 2018

Contributor

So you're saying there's definitely absolutely no way this issue is user visible with respect to cluster operator's existing configs or in their cluster users' pod specs? If so, that feels like an argument this is not release blocking and is just an artifact of a stale test case. Yet you're only touching pkg/scheduler in this PR and not the test case. This doesn't match up.

Plus we do have a bunch of "fixes issue where..." type release notes. It seems to me this would be totally appropriate: "Fixes issue #68899 where pod might schedule on an incorrectly tainted node. Future note: node.Spec.Unschedulable will be deprecated in 1.13 as per issue #XXXXXX."

Ie: you should open an issue now for milestone 1.13 for this deprecation.

Contributor

tpepper commented Sep 23, 2018

So you're saying there's definitely absolutely no way this issue is user visible with respect to cluster operator's existing configs or in their cluster users' pod specs? If so, that feels like an argument this is not release blocking and is just an artifact of a stale test case. Yet you're only touching pkg/scheduler in this PR and not the test case. This doesn't match up.

Plus we do have a bunch of "fixes issue where..." type release notes. It seems to me this would be totally appropriate: "Fixes issue #68899 where pod might schedule on an incorrectly tainted node. Future note: node.Spec.Unschedulable will be deprecated in 1.13 as per issue #XXXXXX."

Ie: you should open an issue now for milestone 1.13 for this deprecation.

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 23, 2018

Member

Plus we do have a bunch of "fixes issue where..." type release notes.

Such kind of release node is ok to me.

Member

k82cn commented Sep 23, 2018

Plus we do have a bunch of "fixes issue where..." type release notes.

Such kind of release node is ok to me.

@krmayankk

This comment has been minimized.

Show comment
Hide comment
@krmayankk

krmayankk Sep 24, 2018

Contributor

will kubectl cordon be implemented using unschedulable taint instead of node.Spec.Unschedulable ?

Contributor

krmayankk commented Sep 24, 2018

will kubectl cordon be implemented using unschedulable taint instead of node.Spec.Unschedulable ?

@tpepper

This comment has been minimized.

Show comment
Hide comment
@tpepper

tpepper Sep 24, 2018

Contributor

Plus we do have a bunch of "fixes issue where..." type release notes.

Such kind of release node is ok to me.

OK then we need confirmation this follows deprecation policy which I'm not sure is actually true, the relnote wording specified below, a decision on whether to put this is the Node or Scheduling notes section, and the deprecation issue opened against 1.13.

Possible relnote:

Fixes issue [#68899](https://github.com/kubernetes/kubernetes/issues/68899) where pods might schedule on an incorrectly tainted node. Deprecation notice: The alpha feature "Critical Pods" has been replaced and `node.Spec.Unschedulable` will be removed in 1.13 as per issue #XXXXXX.
Contributor

tpepper commented Sep 24, 2018

Plus we do have a bunch of "fixes issue where..." type release notes.

Such kind of release node is ok to me.

OK then we need confirmation this follows deprecation policy which I'm not sure is actually true, the relnote wording specified below, a decision on whether to put this is the Node or Scheduling notes section, and the deprecation issue opened against 1.13.

Possible relnote:

Fixes issue [#68899](https://github.com/kubernetes/kubernetes/issues/68899) where pods might schedule on an incorrectly tainted node. Deprecation notice: The alpha feature "Critical Pods" has been replaced and `node.Spec.Unschedulable` will be removed in 1.13 as per issue #XXXXXX.
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Sep 24, 2018

Member

The api field will definitely not be removed from the v1 api. It's not entirely clear to me what behavior can be deprecated in v1. If a client stops setting it and just tries to set the taint, the controller will remove the taint automatically, right?

If kubectl wanted to begin simultaneously setting the unscheduleable field and taint in cordon (and removing them both in uncordon), that could maybe be ok, but would need to think about that a little more.

Member

liggitt commented Sep 24, 2018

The api field will definitely not be removed from the v1 api. It's not entirely clear to me what behavior can be deprecated in v1. If a client stops setting it and just tries to set the taint, the controller will remove the taint automatically, right?

If kubectl wanted to begin simultaneously setting the unscheduleable field and taint in cordon (and removing them both in uncordon), that could maybe be ok, but would need to think about that a little more.

@tpepper

This comment has been minimized.

Show comment
Hide comment
@tpepper

tpepper Sep 24, 2018

Contributor

I really want to see this PR finished off today so we get final confirmation on long running tests over the coming days head of 1.12. This is an important compatibility issue to resolve thoroughly.

Contributor

tpepper commented Sep 24, 2018

I really want to see this PR finished off today so we get final confirmation on long running tests over the coming days head of 1.12. This is an important compatibility issue to resolve thoroughly.

@tpepper

This comment has been minimized.

Show comment
Hide comment
@tpepper

tpepper Sep 24, 2018

Contributor

...and this is the only PR I'm holding the 1.12 release on at this point.

Contributor

tpepper commented Sep 24, 2018

...and this is the only PR I'm holding the 1.12 release on at this point.

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Sep 24, 2018

Member

looks like anyone can LGTM this at this point.
(it is approved, has labels and milestone)

are further changes required here?

Member

neolit123 commented Sep 24, 2018

looks like anyone can LGTM this at this point.
(it is approved, has labels and milestone)

are further changes required here?

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Sep 24, 2018

Member

/hold
just in case.

Member

neolit123 commented Sep 24, 2018

/hold
just in case.

@tpepper

This comment has been minimized.

Show comment
Hide comment
@tpepper

tpepper Sep 24, 2018

Contributor

Chatting with @bsalamat it seems I've misread the intent here. Clarifying based on that:

  • release note suggestion: Fixes issue [#68899](https://github.com/kubernetes/kubernetes/issues/68899) where pods might schedule on an incorrectly tainted node.
  • Bobby will open an issue to track a future deprecation.
  • Deprecation announcement would be targeted for 1.13, with actual removal then being at least a year out given this is v1 stable content.
Contributor

tpepper commented Sep 24, 2018

Chatting with @bsalamat it seems I've misread the intent here. Clarifying based on that:

  • release note suggestion: Fixes issue [#68899](https://github.com/kubernetes/kubernetes/issues/68899) where pods might schedule on an incorrectly tainted node.
  • Bobby will open an issue to track a future deprecation.
  • Deprecation announcement would be targeted for 1.13, with actual removal then being at least a year out given this is v1 stable content.
@bsalamat

This comment has been minimized.

Show comment
Hide comment
@bsalamat

bsalamat Sep 24, 2018

Contributor

Thanks, @tpepper. Looks good to me, however, we can deprecate Node.Spec.Unschedulable only when our replacement (TaintNodeByCondition) is GA'ed. If that happens in 1.13 (which I doubt), we can deprecate Node.Spec.Unschedulable in 1.13. If not, we must wait for TaintNodeByCondition to be promoted to GA and then deprecate Node.Spec.Unschedulable.

Contributor

bsalamat commented Sep 24, 2018

Thanks, @tpepper. Looks good to me, however, we can deprecate Node.Spec.Unschedulable only when our replacement (TaintNodeByCondition) is GA'ed. If that happens in 1.13 (which I doubt), we can deprecate Node.Spec.Unschedulable in 1.13. If not, we must wait for TaintNodeByCondition to be promoted to GA and then deprecate Node.Spec.Unschedulable.

@bsalamat

/lgtm

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 24, 2018

Contributor

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

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

Contributor

k8s-ci-robot commented Sep 24, 2018

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

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

@neolit123

This comment has been minimized.

Show comment
Hide comment
@neolit123

neolit123 Sep 24, 2018

Member

/hold cancel

Member

neolit123 commented Sep 24, 2018

/hold cancel

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 25, 2018

Member

will kubectl cordon be implemented using unschedulable taint instead of node.Spec.Unschedulable ?

Eventually, Yes; but we need to follow deprecation policy to do that.

If kubectl wanted to begin simultaneously setting the unscheduleable field and taint in cordon (and removing them both in uncordon),
that could maybe be ok, but would need to think about that a little more.

Agree, we need to think about it more; we can follow up this in #69010 .

Member

k82cn commented Sep 25, 2018

will kubectl cordon be implemented using unschedulable taint instead of node.Spec.Unschedulable ?

Eventually, Yes; but we need to follow deprecation policy to do that.

If kubectl wanted to begin simultaneously setting the unscheduleable field and taint in cordon (and removing them both in uncordon),
that could maybe be ok, but would need to think about that a little more.

Agree, we need to think about it more; we can follow up this in #69010 .

for _, test := range testCases {
nodeInfo := schedulercache.NewNodeInfo()
nodeInfo.SetNode(test.node)
fit, _, err := CheckNodeUnschedulablePredicate(test.pod, nil, nodeInfo)

This comment has been minimized.

@tianshapjq

tianshapjq Sep 25, 2018

Contributor

seems like the CheckNodeUnschedulablePredicate() has no callers at all, can you confirm about that?
if so, do we still need to update this func?

@tianshapjq

tianshapjq Sep 25, 2018

Contributor

seems like the CheckNodeUnschedulablePredicate() has no callers at all, can you confirm about that?
if so, do we still need to update this func?

This comment has been minimized.

@k82cn

k82cn Sep 25, 2018

Member

Added it by RegisterMandatoryFitPredicate in this PR.

@k82cn

k82cn Sep 25, 2018

Member

Added it by RegisterMandatoryFitPredicate in this PR.

@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Sep 26, 2018

Contributor

It's a bit unclear to me what "deprecation of the field" means here. We cannot remove the field from the api, so will it become a no-op a year from now? I don't think kubernete's deprecation policy explicitly states what to do in this case.

Contributor

yujuhong commented Sep 26, 2018

It's a bit unclear to me what "deprecation of the field" means here. We cannot remove the field from the api, so will it become a no-op a year from now? I don't think kubernete's deprecation policy explicitly states what to do in this case.

@k82cn

This comment has been minimized.

Show comment
Hide comment
@k82cn

k82cn Sep 26, 2018

Member

so will it become a no-op a year from now? I don't think kubernete's deprecation policy explicitly states what to do in this case.

According to deprecation policy, "API elements may only be removed by incrementing the version of the API group.". For now, we only need to highlight spec.Unschedulable will be removed in future; but no behaviour changes right now, or someone has better idea for that :)

Member

k82cn commented Sep 26, 2018

so will it become a no-op a year from now? I don't think kubernete's deprecation policy explicitly states what to do in this case.

According to deprecation policy, "API elements may only be removed by incrementing the version of the API group.". For now, we only need to highlight spec.Unschedulable will be removed in future; but no behaviour changes right now, or someone has better idea for that :)

@k8s-ci-robot k8s-ci-robot merged commit 762ddf7 into kubernetes:master Sep 27, 2018

18 checks passed

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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment