Revert "Remove rescheduler and corresponding tests from master" #64592

ravisantoshgudimetla · 2018-06-01T02:27:14Z

After discussing with @bsalamat on how DS controllers(ref: #63223 (comment)) cannot create pods if the cluster is at capacity and they have to rely on rescheduler for making some space, we thought it is better to

Bring rescheduler back.
Make rescheduler priority aware.
If cluster is full and if only DS controller is not able to create pods, let rescheduler be run and let it evict some pods which have less priority.
The DS controller pods will be scheduled now.

So, I am reverting this PR now. Step 2, 3 above are going to be in rescheduler.

/cc @bsalamat @aveshagarwal @k82cn

Please let me know your thoughts on this.

Revert #64364 to resurrect rescheduler. More info https://github.com/kubernetes/kubernetes/issues/64725 :)

wgliang · 2018-06-01T02:42:30Z

test/e2e/scheduling/rescheduler.go

@@ -0,0 +1,133 @@
+/*
+Copyright 2016 The Kubernetes Authors.


that's a revert PR, let keep it as is :)

OK, make sense!

wgliang · 2018-06-01T02:43:16Z

test/e2e/scheduling/rescheduler.go

+	testutils "k8s.io/kubernetes/test/utils"
+	imageutils "k8s.io/kubernetes/test/utils/image"
+
+	. "github.com/onsi/ginkgo"


above k8s.io*

k82cn · 2018-06-01T03:25:10Z

Bobby's concern is a good point :).
I went through the original PR and was curious why the e2e was passed. So I re-check the code, and found that critical pod (ExperimentalCriticalPodAnnotation) is still alpha feature :).

Anyway , let me also check whether it is enabled specially in test-infra :). If not enabled, I think that's safe for us to remove it.

BTW, did you grep yaml files to see whether we need replace critical pod with priority?

ravisantoshgudimetla · 2018-06-01T03:35:06Z

I went through the original PR and was curious why the e2e was passed. So I re-check the code, and found that critical pod (ExperimentalCriticalPodAnnotation) is still alpha feature :).

That is correct.

BTW, did you grep yaml files to see whether we need replace critical pod with priority?

I believe you are talking about critical pod annotation, as of now, by definition a critical pod is one which has either critical pod annotation or system-node-critical, system-cluster-critical priorities.

k82cn · 2018-06-01T05:23:56Z

I believe you are talking about critical pod annotation

Yes, that's great if we already defined.

bsalamat

/lgtm

bsalamat · 2018-06-01T17:42:52Z

Yes, that's great if we already defined.

As Revi said, we have already changed our code to recognize system-node-critical and system-cluster-critical priorities as "critical". The annotation still works as well (the code is not removed). We have also changed critical pod manifests and have added appropriate priority class names in addition to the critical annotation.

ravisantoshgudimetla · 2018-06-01T19:32:25Z

/retest

bsalamat · 2018-06-02T19:57:13Z

@ravisantoshgudimetla Please add a release note (NONE?)

k82cn · 2018-06-03T09:00:37Z

Re-notes, prefer to "Revert #64364 to resurrect rescheduler."

LGTM, thanks very much :)

ravisantoshgudimetla · 2018-06-04T00:00:00Z

/retest

ravisantoshgudimetla · 2018-06-04T00:03:17Z

ping @ixdy @eparis @Random-Liu for review/approval.

/cc @sjenning @derekwaynecarr

bsalamat · 2018-06-04T19:14:07Z

ping
We need to get this approved ASAP. Thanks!

eparis · 2018-06-04T20:27:28Z

/approve

dims · 2018-06-04T20:34:16Z

/assign @mikedanese @yujuhong

ixdy · 2018-06-04T20:35:08Z

/approve

k8s-github-robot · 2018-06-04T20:35:23Z

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

@bsalamat @mikedanese @ravisantoshgudimetla @yujuhong

Pull Request Labels

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

yujuhong

Why was the rescheduler deprecated/removed and what conditions changed such that the rescheduler is needed again?

yujuhong · 2018-06-04T20:58:25Z

pkg/kubelet/types/pod_update.go

@@ -141,7 +141,7 @@ func (sp SyncPodType) String() string {
 }

 // IsCriticalPod returns true if the pod bears the critical pod annotation key or if pod's priority is greater than
-// or equal to SystemCriticalPriority. Both the default scheduler and the kubelet use this function
+// or equal to SystemCriticalPriority. Both the rescheduler(deprecated in 1.10) and the kubelet use this function


Comment should be fixed since rescheduler is no longer deprecated.

So, I think the statement is still valid just that we will delete rescheduler in next release. This is a stop gap solution.

What's the long term fix (and if we rely on a deprecated feature, is it really deprecated?)

Please find my comments below(#64592 (comment)). While I am not familiar with deprecation policies and this may not sound like exact reason, last release had rescheduler deprecated too but we relied on it for DS pods.

yujuhong · 2018-06-04T20:59:46Z

test/e2e/scheduling/rescheduler.go

+	var totalMillicores int
+
+	BeforeEach(func() {
+		framework.Skipf("Rescheduler is being deprecated soon in Kubernetes 1.10")


The test is always skipped?!

I will add a correct test in another PR.

Why adding the test back if we always skip it?

ravisantoshgudimetla · 2018-06-04T21:12:22Z

Thanks @yujuhong for review.

Why was the rescheduler deprecated/removed

Default scheduler is supposed to do preemptions for all the pods there by eliminating need for rescheduler.

what conditions changed such that the rescheduler is needed again?

For pods generated by DS controller, the preemption is not happening as DS controller schedules its own pods, so we have a PR to change the behaviour of DS controller so that its pods are scheduled by default scheduler(#63223) but this feature is still in alpha making DS controller to schedule its own pods instead of default scheduler. We were initially hoping to make this beta but since the code-path was not sufficiently tested scheduling/apps sigs have decided to make it alpha. With this in picture, we had to bring back rescheduler. More explaination in here: (#63223 (comment))

yujuhong · 2018-06-04T21:22:24Z

@ravisantoshgudimetla thanks for the explanation! Could you create an issue with the explanation and plan to remove the rescheduler?

Assuming the tests will be removed/updated, the PR lgtm.
/lgtm

k8s-ci-robot · 2018-06-04T21:22:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, eparis, ixdy, ravisantoshgudimetla, yujuhong

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:

~~build/OWNERS~~ [ixdy]
~~cluster/OWNERS~~ [eparis]
~~pkg/kubelet/OWNERS~~ [yujuhong]
~~test/e2e/scheduling/OWNERS~~ [bsalamat,eparis,ixdy]

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

yujuhong · 2018-06-04T21:23:31Z

Could you create an issue with the explanation and plan to remove the rescheduler?

Forgot to mention, could you add a link to the issue in your release note?

ravisantoshgudimetla · 2018-06-04T21:48:27Z

Thanks @yujuhong for the review and approval.

Forgot to mention, could you add a link to the issue in your release note?

Done - #64725

k8s-github-robot · 2018-06-04T23:56:10Z

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

k8s-ci-robot · 2018-06-05T10:46:17Z

@ravisantoshgudimetla: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	`872addf`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

zparnold · 2018-06-13T15:02:20Z

Hello there! @ravisantoshgudimetla I'm Zach Arnold working on Docs for the 1.11 release. This PR was identified as one needing some documentation in the https://github.com/kubernetes/website repo around your contributions (thanks by the way!) When you have some time, could you please modify/add/remove the relevant content that needs changing in our documentation repo? Thanks! Please let me or my colleague Misty know (@zparnold/@misty on K8s Slack) if you need any assistance with the documentation.

ravisantoshgudimetla · 2018-06-26T14:20:27Z

@zparnold Apologies for not getting back earlier. I already have updated documents with these changes to rescheduler.

zparnold · 2018-06-27T15:08:19Z

Thanks!

…

On Tue, Jun 26, 2018 at 7:21 AM RaviSantosh Gudimetla < ***@***.***> wrote: @zparnold <https://github.com/zparnold> Apologies for not getting back earlier. I already have updated documents with these changes to rescheduler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#64592 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE81SKuM1pHW8bznQ9YFMGBeiGUTO7tOks5uAkOBgaJpZM4UWAIk> .

Revert "Remove rescheduler and corresponding tests from master"

872addf

k8s-ci-robot requested review from aveshagarwal, bsalamat and k82cn June 1, 2018 02:27

wgliang reviewed Jun 1, 2018

View reviewed changes

bsalamat approved these changes Jun 1, 2018

View reviewed changes

k8s-ci-robot assigned bsalamat Jun 1, 2018

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

ravisantoshgudimetla mentioned this pull request Jun 1, 2018

Add a readme and deprecation note for rescheduler kubernetes-retired/contrib#2905

Closed

bsalamat added this to the v1.11 milestone Jun 2, 2018

bsalamat added status/approved-for-milestone priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jun 2, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 2, 2018

bsalamat added kind/feature Categorizes issue or PR as related to a new feature. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed milestone/incomplete-labels labels Jun 2, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 3, 2018

k8s-ci-robot requested review from derekwaynecarr and sjenning June 4, 2018 00:03

k82cn approved these changes Jun 4, 2018

View reviewed changes

k8s-ci-robot assigned mikedanese and yujuhong Jun 4, 2018

yujuhong reviewed Jun 4, 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 Jun 4, 2018

k8s-github-robot merged commit 898831a into kubernetes:master Jun 4, 2018

ravisantoshgudimetla deleted the revert-64364-remove-rescheduler branch June 26, 2018 14:20

Revert "Remove rescheduler and corresponding tests from master" #64592

Revert "Remove rescheduler and corresponding tests from master" #64592

Conversation

ravisantoshgudimetla commented Jun 1, 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

k82cn commented Jun 1, 2018

ravisantoshgudimetla commented Jun 1, 2018 • edited Loading

k82cn commented Jun 1, 2018

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat commented Jun 1, 2018

ravisantoshgudimetla commented Jun 1, 2018

bsalamat commented Jun 2, 2018

k82cn commented Jun 3, 2018

ravisantoshgudimetla commented Jun 4, 2018

ravisantoshgudimetla commented Jun 4, 2018

bsalamat commented Jun 4, 2018

eparis commented Jun 4, 2018

dims commented Jun 4, 2018

ixdy commented Jun 4, 2018

k8s-github-robot commented Jun 4, 2018

yujuhong 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

ravisantoshgudimetla Jun 4, 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

ravisantoshgudimetla commented Jun 4, 2018

yujuhong commented Jun 4, 2018

k8s-ci-robot commented Jun 4, 2018

yujuhong commented Jun 4, 2018

ravisantoshgudimetla commented Jun 4, 2018

k8s-github-robot commented Jun 4, 2018

k8s-ci-robot commented Jun 5, 2018

zparnold commented Jun 13, 2018

ravisantoshgudimetla commented Jun 26, 2018

zparnold commented Jun 27, 2018 via email

ravisantoshgudimetla commented Jun 1, 2018 •

edited

Loading

ravisantoshgudimetla commented Jun 1, 2018 •

edited

Loading

ravisantoshgudimetla Jun 4, 2018 •

edited

Loading