Implement alpha version of PreferAvoidPods by jiangyaoguo · Pull Request #20699 · kubernetes/kubernetes

jiangyaoguo · 2016-02-05T08:25:01Z

This is part of #18853

This change is

k8s-github-robot · 2016-02-05T10:13:33Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

k8s-github-robot · 2016-02-05T15:27:37Z

PR needs rebase

k8s-github-robot · 2016-02-06T21:02:44Z

PR needs rebase

davidopp · 2016-02-07T18:07:40Z

cc/ @smarterclayton @mikedanese @bgrant0607

davidopp · 2016-02-07T18:12:05Z

pkg/api/v1/types.go

Slightly different wording that might be a bit clearer: "If the object to be deleted is a pod managed by a controller, setting to true requests that the replacement pod that the controller creates (if the controller decides to create such a pod) be scheduled to a different node than the one where it is currently running. Otherwise has no effect."

smarterclayton · 2016-02-07T18:20:30Z

I thought we said in the other thread this needs to be on a new API object and that we have to be able to convert both DeleteOptions and PodDeleteOptions to the internal type?

davidopp · 2016-02-07T19:55:26Z

Here are the two comments I think you're referring to:

#18853 (comment)
#18853 (comment)

I interpreted these comments to mean that anything beyond what @jiangyaoguo was a "nice to have."

Do we foresee other Pod-specific delete options? It doesn't seem so bad to just put in a comment "has no effect when applied to non-Pod" unless there are going to be multiple other scenarios like this.

cc/ @lavalamp

smarterclayton · 2016-02-07T21:16:32Z

We can always change the schema in the future to take away the flag since
it would have no effect, but we wouldn't be able to be stricter in
validating incoming requests, because the field would be accepted by
version 1.2 (even if ignored). So it paints us into a corner a little bit
more than today.

On Sun, Feb 7, 2016 at 2:55 PM, David Oppenheimer notifications@github.com
wrote:

Here are the two comments I think you're referring to:

#18853 (comment)
#18853 (comment)
#18853 (comment)
#18853 (comment)

I interpreted these comments to mean that anything beyond what
@jiangyaoguo https://github.com/jiangyaoguo was a "nice to have."

Do we foresee other Pod-specific delete options? It doesn't seem so bad to
just put in a comment "has no effect when applied to non-Pod" unless there
are going to be multiple other scenarios like this.

cc/ @lavalamp https://github.com/lavalamp

—
Reply to this email directly or view it on GitHub
#20699 (comment)
.

k8s-github-robot · 2016-02-08T04:14:38Z

@jiangyaoguo
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

k8s-github-robot · 2016-02-08T16:55:40Z

PR needs rebase

lavalamp · 2016-02-09T01:41:12Z

I would really prefer to make a specific PodDeletionOptions object. How urgent is this?

davidopp · 2016-02-10T09:07:01Z

Quick notes from conversation with @lavalamp today:

The fact that AvoidPreviousNode was requested in the api.DeleteOptions needs to be persisted somewhere by the API server, presumably an annotation on the pod that it then deletes. An alternative would be to just have the client who requests the delete add the annotation before doing the delete, and not bother with adding this option to DeleteOptions at all.
More controversial alternatives
- have a new API resource that records the information; it would be available to all controllers (though it's not clear that other controllers than the one that created the pod need the information), and could eventually generalize to a RateLimiter-type object that records pod-machine pairs to avoid
- similar to previous sub-bullet but put the information in the Node object rather than a new object

I'm pretty opposed to the "more controversial alternatives." It seems overly complicated, at least in the sense that associating the "avoid previous machine" information with the pod that is being deleted avoids any concern about garbage collecting the information, as compared to storing it in some global structure (as in either of the two "more controversial alternatives").

I'm on the fence about the first bullet.

Urgency: This is not super-urgent; it blocks deployment of rescheduler, but we haven't even started implementing rescheduler.

davidopp · 2016-06-03T20:04:13Z

pkg/api/helpers_test.go

add a comment why you expect this to fail

davidopp · 2016-06-03T20:28:03Z

Thanks! Just a few comments, which are all trivial. Once you fix them I will LGTM.

wojtek-t · 2016-06-03T20:36:07Z

I don't understand title of this PR - this doesn't touch NodeStatus at all.

davidopp · 2016-06-04T05:16:36Z

Thanks for noticing, the title is out-of-date (we decided to use annotation for first version). I will update it now.

jiangyaoguo · 2016-06-11T16:14:10Z

@davidopp updated.

davidopp · 2016-06-11T19:03:56Z

pkg/api/v1/types.go

s/desribes/describes/

jiangyaoguo · 2016-06-15T08:29:29Z

@davidopp updated.

jiangyaoguo · 2016-07-05T02:26:49Z

@davidopp rebased.

davidopp · 2016-07-09T22:13:33Z

LGTM

Very sorry for the delay in reviewing. Please rebase one more time, and then I will add LGTM label.

Thanks for your work on this!

davidopp · 2016-07-12T06:51:04Z

Please squash the commits to one commit.

davidopp · 2016-07-13T08:41:39Z

@jiangyaoguo your new commit has a compiler error:

# k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/priorities
plugin/pkg/scheduler/algorithm/priorities/priorities.go:307: nodes.Items undefined (type []*"k8s.io/kubernetes/pkg/api".Node has no field or method Items)
plugin/pkg/scheduler/algorithm/priorities/priorities.go:314: nodes.Items undefined (type []*"k8s.io/kubernetes/pkg/api".Node has no field or method Items)

I didn't look carefully, but it might be that you need to update your code to incorporate the change made in #28781

jiangyaoguo · 2016-07-13T08:57:59Z

@davidopp I encounter some network problem now. Will update soon.

…fic node. 1. define PreferAvoidPods annotation 2. add PreferAvoidPodsPriority 3. validate AvoidPods in node annotations

k8s-bot · 2016-07-13T09:45:33Z

GCE e2e build/test passed for commit 4e91166.

jiangyaoguo · 2016-07-13T11:24:55Z

@davidopp

wojtek-t · 2016-07-13T11:36:30Z

plugin/pkg/scheduler/algorithm/priorities/priorities.go

+	return nodePreferAvoid.CalculateNodePreferAvoidPodsPriority
+}
+
+func (npa *NodePreferAvoidPod) CalculateNodePreferAvoidPodsPriority(pod *api.Pod, nodeNameToInfo map[string]*schedulercache.NodeInfo, nodeLister algorithm.NodeLister) (schedulerapi.HostPriorityList, error) {


@davidopp @jiangyaoguo - do we know the preformance impact of it? Since you are switching it on by default, it would be good to know...

No, we haven't tested performance impact. What would you think of the following:

for now, at the beginning of the function put a loop over all nodes, that checks to see if

api.GetAvoidPodsFromNodeAnnotations(node.Annotations)

has a value for any of the nodes. If it is not set for any nodes, the function immediately returns a HostPiorityList with all nodes having same priority. This way the significant performance impact only happens if you actually use the feature, and we can warn users about it.

hopefully ownerReference will be implemented for ReplicaSet and ReplicationController by the time of code freeze for 1.4 (RC is already in the final stages of review, will merge very soon), and then we can use it instead of using rcs, err := npa.controllerLister.GetPodControllers(pod) (and similar one for ReplicaSets).

Does that sound reasonable?

So let's do this way:

let's merge as it is now (basically in selector spreading we are still calling all the functions that we have here, so it shouldn't be worse)

Once this is merged, I will do a pass over it and fix some issues that I already fixed for other priorities/predicates

once ControllerRef is ready, I will switch to that (together with selector_spreading)

[Generally, the asymptotical complexity of it is the same as selector spreading, We should probably solve both of them in the same way in the future.]

SGTM. Thanks!

davidopp · 2016-07-14T06:48:35Z

LGTM

k8s-github-robot · 2016-07-14T09:53:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-07-14T09:55:01Z

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

k8s-bot · 2016-07-14T10:30:14Z

GCE e2e build/test passed for commit 4e91166.

k8s-github-robot · 2016-07-14T10:30:25Z

Automatic merge from submit-queue

k8s-bot · 2016-07-14T10:31:26Z

GCE e2e build/test passed for commit 4e91166.

vishh · 2017-06-21T18:56:59Z

@davidopp Is this feature documented anywhere?

googlebot added the cla: yes label Feb 5, 2016

k8s-github-robot assigned thockin Feb 5, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 5, 2016

jiangyaoguo force-pushed the add-AvoidPreviousNode branch from c9bc79d to 449d956 Compare February 5, 2016 09:33

k8s-github-robot added the needs-ok-to-merge label Feb 5, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2016

thockin assigned davidopp and unassigned thockin Feb 5, 2016

jiangyaoguo force-pushed the add-AvoidPreviousNode branch from 449d956 to 25e45b4 Compare February 6, 2016 01:29

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2016

davidopp reviewed Feb 7, 2016
View reviewed changes

jiangyaoguo force-pushed the add-AvoidPreviousNode branch from 25e45b4 to 162c832 Compare February 8, 2016 03:06

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2016

davidopp changed the title ~~Add AvoidPreviousNodeto api.DeleteOptions~~ Add AvoidPreviousNode to api.DeleteOptions Feb 10, 2016

jiangyaoguo force-pushed the add-AvoidPreviousNode branch from 162c832 to 8dd3bbd Compare February 15, 2016 06:27

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2016

davidopp reviewed Jun 3, 2016
View reviewed changes

pkg/api/helpers_test.go Outdated

Copy link
Copy Markdown

Contributor

davidopp Jun 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment why you expect this to fail

davidopp added ok-to-merge and removed needs-ok-to-merge labels Jun 3, 2016

davidopp reviewed Jun 11, 2016
View reviewed changes

pkg/api/v1/types.go Outdated

Copy link
Copy Markdown

Contributor

davidopp Jun 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/desribes/describes/

Use PreferAvoidPods annotation to avoid pods being scheduled to speci…

4e91166

…fic node. 1. define PreferAvoidPods annotation 2. add PreferAvoidPodsPriority 3. validate AvoidPods in node annotations

wojtek-t reviewed Jul 13, 2016
View reviewed changes

davidopp mentioned this pull request Mar 18, 2017

Support reason parameter on pod delete #1462

Closed

davidopp mentioned this pull request Jun 21, 2017

Design proposal for pod preemption in Kubernetes kubernetes/community#745

Merged

Conversation

jiangyaoguo commented Feb 5, 2016 • edited by davidopp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Feb 5, 2016

Uh oh!

k8s-github-robot commented Feb 5, 2016

Uh oh!

k8s-github-robot commented Feb 6, 2016

Uh oh!

davidopp commented Feb 7, 2016

Uh oh!

davidopp Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Feb 7, 2016 via email

Uh oh!

davidopp commented Feb 7, 2016

Uh oh!

smarterclayton commented Feb 7, 2016

Uh oh!

k8s-github-robot commented Feb 8, 2016

Uh oh!

k8s-github-robot commented Feb 8, 2016

Uh oh!

lavalamp commented Feb 9, 2016

Uh oh!

davidopp commented Feb 10, 2016

Uh oh!

davidopp Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

davidopp commented Jun 3, 2016

Uh oh!

wojtek-t commented Jun 3, 2016

Uh oh!

davidopp commented Jun 4, 2016

Uh oh!

jiangyaoguo commented Jun 11, 2016

Uh oh!

davidopp Jun 11, 2016

Choose a reason for hiding this comment

Uh oh!

jiangyaoguo commented Jun 15, 2016

Uh oh!

jiangyaoguo commented Jul 5, 2016

Uh oh!

davidopp commented Jul 9, 2016

Uh oh!

davidopp commented Jul 12, 2016

Uh oh!

davidopp commented Jul 13, 2016

Uh oh!

jiangyaoguo commented Jul 13, 2016

Uh oh!

k8s-bot commented Jul 13, 2016

Uh oh!

jiangyaoguo commented Jul 13, 2016

Uh oh!

wojtek-t Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

davidopp Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

wojtek-t Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

davidopp Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

davidopp commented Jul 14, 2016

Uh oh!

k8s-github-robot commented Jul 14, 2016

Uh oh!

k8s-github-robot commented Jul 14, 2016

Uh oh!

k8s-bot commented Jul 14, 2016

Uh oh!

k8s-github-robot commented Jul 14, 2016

jiangyaoguo commented Feb 5, 2016 •

edited by davidopp

Loading