Optimization of on-wire information sent to scheduler extender interfaces that are stateful #41119

sarat-k · 2017-02-08T03:11:44Z

The changes are to address the issue described in #40991

Added support to minimize sending verbose node information to scheduler extender by sending only node names and expecting extenders to cache the rest of the node information

cc @ravigadde

k8s-ci-robot · 2017-02-08T03:11:45Z

Hi @sarat-k. 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 @k8s-bot 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.

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.

k8s-ci-robot · 2017-02-08T03:11:46Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

k8s-reviewable · 2017-02-08T03:11:51Z

This change is

k8s-github-robot · 2017-02-08T03:11:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: sarat-k

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @wojtek-t @madhusudancs @brendandburns
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

sarat-k · 2017-02-08T03:27:14Z

I signed the CLA.

ravigadde

Added some comments.

cc @davidopp for scheduler annotation changes

ravigadde · 2017-02-08T18:07:06Z

plugin/pkg/scheduler/api/types.go

@@ -136,16 +140,27 @@ type ExtenderArgs struct {
 	// Pod being scheduled
 	Pod v1.Pod
 	// List of candidate nodes where the pod can be scheduled


Can you update this comment too?

ravigadde · 2017-02-08T18:07:52Z

plugin/pkg/scheduler/api/types.go

 }

 // FailedNodesMap represents the filtered out nodes, with node names and failure messages
 type FailedNodesMap map[string]string

+// Annotations represents the annotations
+type Annotations map[string]string
+
 // ExtenderFilterResult represents the results of a filter call to an extender
 type ExtenderFilterResult struct {
 	// Filtered set of nodes where the pod can be scheduled


ravigadde · 2017-02-08T18:08:23Z

plugin/pkg/scheduler/api/v1/types.go

 }

 // ExtenderArgs represents the arguments needed by the extender to filter/prioritize
 // nodes for a pod.
 type ExtenderArgs struct {
 	// Pod being scheduled
 	Pod apiv1.Pod `json:"pod"`
-	// List of candidate nodes where the pod can be scheduled


Added the comment

ravigadde · 2017-02-08T18:08:48Z

plugin/pkg/scheduler/api/v1/types.go

 }

 // FailedNodesMap represents the filtered out nodes, with node names and failure messages
 type FailedNodesMap map[string]string

+// Annotations represents the annotations
+type Annotations map[string]string
+
 // ExtenderFilterResult represents the results of a filter call to an extender
 type ExtenderFilterResult struct {
 	// Filtered set of nodes where the pod can be scheduled


Added comment here too

ravigadde · 2017-02-08T18:12:02Z

plugin/pkg/scheduler/extender.go

-		Pod:   *pod,
-		Nodes: v1.NodeList{Items: nodeItems},
+		Pod:       *pod,
+		Nodes:     &v1.NodeList{Items: nodeItems},


Nodes should be nil when node cache is enabled, rather than an empty list.

ravigadde · 2017-02-08T18:15:03Z

plugin/pkg/scheduler/extender.go

-		Pod:   *pod,
-		Nodes: v1.NodeList{Items: nodeItems},
+		Pod:       *pod,
+		Nodes:     &v1.NodeList{Items: nodeItems},


Same here: send nil when cache is enabled

ravigadde · 2017-02-08T18:16:27Z

plugin/pkg/scheduler/extender.go

+	filterVerb       string
+	prioritizeVerb   string
+	weight           int
+	apiVersion       string


Should we get rid of the apiVersion?

yes .. cleaned this up in the latest commit.

ravigadde · 2017-02-08T18:19:31Z

plugin/pkg/scheduler/extender_test.go

@@ -109,15 +109,15 @@ type FakeExtender struct {
 	weight       int
 }

-func (f *FakeExtender) Filter(pod *v1.Pod, nodes []*v1.Node) ([]*v1.Node, schedulerapi.FailedNodesMap, error) {
+func (f *FakeExtender) Filter(pod *v1.Pod, nodes []*v1.Node, nodeNameToInfo map[string]*schedulercache.NodeInfo) ([]*v1.Node, schedulerapi.FailedNodesMap, *schedulerapi.Annotations, error) {
 	filtered := []*v1.Node{}
 	failedNodesMap := schedulerapi.FailedNodesMap{}
 	for _, node := range nodes {
 		fits := true
 		for _, predicate := range f.predicates {
 			fit, err := predicate(pod, node)


Can you add an annotation test for extender?

added one in the latest commit. please check if that is what you are suggesting.

ravigadde · 2017-02-08T18:27:06Z

test/integration/scheduler/extender_test.go

+	}
+}
+
+func (e *Extender) Prioritize(args *schedulerapi.ExtenderArgs) (schedulerapi.HostPriorityList, error) {


Why is the signature change needed?

Not intentional .. fixed it.

ravigadde · 2017-02-08T18:31:08Z

plugin/pkg/scheduler/scheduler.go

@@ -148,6 +148,7 @@ func (s *Scheduler) Run() {

 func (s *Scheduler) scheduleOne() {
 	var podAnnotations schedulerapi.Annotations
+


resouer · 2017-02-10T08:14:12Z

@k8s-bot ok to test

timothysc · 2017-02-10T14:49:48Z

@ravigadde is this ready for final review?

ravigadde · 2017-02-10T17:22:49Z

plugin/pkg/scheduler/extender.go

@@ -104,6 +102,7 @@ func (h *HTTPExtender) Filter(pod *v1.Pod, nodes []*v1.Node, nodeNameToInfo map[
 		nodeNames      *[]string
 		nodeResult     []*v1.Node
 		podAnnotations schedulerapi.Annotations
+		args           *schedulerapi.ExtenderArgs


Instead of nodeItems, have a nodeList *v1.NodeList here

ravigadde · 2017-02-10T17:23:23Z

plugin/pkg/scheduler/extender.go

@@ -123,25 +122,33 @@ func (h *HTTPExtender) Filter(pod *v1.Pod, nodes []*v1.Node, nodeNameToInfo map[
 		}


Populate nodeList here with nodeItems.

ravigadde · 2017-02-10T17:23:49Z

plugin/pkg/scheduler/extender.go

-		Pod:       *pod,
-		Nodes:     &v1.NodeList{Items: nodeItems},
-		NodeNames: nodeNames,
+	if h.nodeCacheCapable {


Assign nodeList to Nodes. You dont need the if check here.

ravigadde · 2017-02-10T17:24:25Z

plugin/pkg/scheduler/extender.go

 		return nil, nil, nil, err
 	}
 	if result.Error != "" {
 		return nil, nil, nil, fmt.Errorf(result.Error)
 	}

-	if h.nodeCacheCapable {
+	if h.nodeCacheCapable && (result.NodeNames != nil) {


Lose the ()

ravigadde · 2017-02-10T17:24:52Z

plugin/pkg/scheduler/extender.go

@@ -165,6 +172,7 @@ func (h *HTTPExtender) Prioritize(pod *v1.Pod, nodes []*v1.Node) (*schedulerapi.
 		result    schedulerapi.HostPriorityList
 		nodeItems []v1.Node
 		nodeNames *[]string
+		args      *schedulerapi.ExtenderArgs


Same comments as above

Done .. latest commit addressed all the changes suggested

ravigadde · 2017-02-10T17:26:54Z

@sarat-k LGTM other than the minor comments. Please make the changes (and may be squash) for @timstclair to review.

timothysc

So several big questions that leaves other parts of the PR in question that we need answers on 1st.

timothysc · 2017-02-13T14:36:14Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// List of candidate nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


So you're changing the api here. I don't know what type of policy enforcement we have around this, but there would need to be some global notification for anyone currently using it. I know @ravigadde introduced this mechanism, but do we have details on who else may use this?

/cc @davidopp

@timothysc these changes are backward compatible. Do you see anything that breaks backward compatibility?

Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == false"

I think the backward compatibility concern might be that you changed the field from required to optional (Nodes to Nodes *) (At least I assume that's the implication of changing from Nodes to Nodes * -- I guess we're not using the JSON annotations here that say "omitempty" and such.)

timothysc · 2017-02-13T14:36:33Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// Filtered set of nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


same comment.

Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == false"

I think the backward compatibility concern might be that you changed the field from required to optional (Nodes to Nodes *)

timothysc · 2017-02-13T14:39:00Z

plugin/pkg/scheduler/api/types.go

+	// only if extender is capable of maintaining its own node cache
+	NodeNames *[]string
+	// Annotations are the pod annotations returned by the extender, if any
+	PodAnnotations *Annotations


We are ridding most of these, are there other annotations that you are using, or parsing here?

Oh, I didn't know that. It should be OK to not have this in the extender imo.

timothysc · 2017-02-13T14:52:01Z

plugin/pkg/scheduler/extender.go

+		for _, node := range nodes {
+			nodeNameSlice = append(nodeNameSlice, node.Name)
+		}
+		nodeNames = &nodeNameSlice


So slightly confused, does the extender maintain it's own watch then? and now you're only passing in nodename?

Yes to both.

@timothysc @ravigadde synced up with Ravi offline ... backed out pod annotations related changes and pushed the updated changes. Thanks for the review.

timothysc · 2017-02-13T14:56:08Z

plugin/pkg/scheduler/extender.go

-	return nodeResult, result.FailedNodes, nil
+
+	if result.PodAnnotations != nil {
+		podAnnotations = *result.PodAnnotations


same comment re: we are ridding annotations in 1.6...

Are there other things you are passing using this mechanism?

timothysc · 2017-02-13T22:41:48Z

plugin/pkg/scheduler/api/types.go

+	// Filtered set of nodes where the pod can be scheduled; to be populated
+	// only if extender is capable of maintaining its own node cache
+	NodeNames *[]string
+	// Annotations are the pod annotations returned by the extender, if any


needs.

to be populated only if extender is capable of maintaining its own node cache

timothysc

k, I don't see anything major.

@sarat-k could you add a release note block on your initial PR comment please. This will get picked up by the bots based on the tagging.

@davidopp did you want to review?

timothysc · 2017-02-14T15:39:13Z

plugin/pkg/scheduler/extender.go

@@ -160,7 +200,7 @@ func (h *HTTPExtender) send(action string, args interface{}, result interface{})
 		return err
 	}

-	url := h.extenderURL + "/" + h.apiVersion + "/" + action
+	url := h.extenderURL + "/" + action


Is there a reason you removed the apiVersion from the endpoint?

apiVersion is always empty string and so the URL is coming out to be something like http://127.0.0.1:9999/api/scheduler//schedule with double-slash in between where apiVersion was supposed to be. Due to this double-slash, multiple 301 redirects are seen (sent by go router mostly) and extender doesn't even see these scheduling requests, eventually requests fail.

sarat-k · 2017-02-21T18:24:49Z

@timothysc @davidopp

The test failure is a known issue, same as the one reported in #41419.

Can you please let me know if there is anything more that needs to be done for this PR or if it can be merged ?

Thanks,
Sarat

davidopp · 2017-02-22T06:02:45Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// List of candidate nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == false"

davidopp · 2017-02-22T06:02:49Z

plugin/pkg/scheduler/api/types.go

+	// List of candidate nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList
+	// List of candidate node names where the pod can be scheduled; to be


Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == true"

davidopp · 2017-02-22T06:04:35Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// List of candidate nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


I think the backward compatibility concern might be that you changed the field from required to optional (Nodes to Nodes *) (At least I assume that's the implication of changing from Nodes to Nodes * -- I guess we're not using the JSON annotations here that say "omitempty" and such.)

davidopp · 2017-02-22T06:04:51Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// Filtered set of nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == false"

davidopp · 2017-02-22T06:05:05Z

plugin/pkg/scheduler/api/types.go

-	Nodes v1.NodeList
+	// Filtered set of nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList


I think the backward compatibility concern might be that you changed the field from required to optional (Nodes to Nodes *)

davidopp · 2017-02-22T06:05:26Z

plugin/pkg/scheduler/api/types.go

+	// Filtered set of nodes where the pod can be scheduled; to be populated
+	// only if extender is not capable of maintaining its own node cache
+	Nodes *v1.NodeList
+	// Filtered set of nodes where the pod can be scheduled; to be populated


Instead of "to be populated only if extender is not capable of maintaining its own node cache" say "to be populated only if ExtenderConfig.NodeCacheCapable == true"

davidopp · 2017-02-22T06:05:46Z

plugin/pkg/scheduler/api/v1/types.go

@@ -128,24 +128,36 @@ type ExtenderConfig struct {
 	// HTTPTimeout specifies the timeout duration for a call to the extender. Filter timeout fails the scheduling of the pod. Prioritize
 	// timeout is ignored, k8s/other extenders priorities are used to select the node.
 	HTTPTimeout time.Duration `json:"httpTimeout,omitempty"`
+	// NodeCacheCapable specifies that the extender is capable of caching node information,


See all the aforementioned comments for the other types.go file

@timothysc @davidopp @ravigadde
I fixed all the comments as suggested and pushed the changes. Also, added omitempty tag for node list in ExtenderArgs. When the node list is empty, older implementations will just see empty list of nodes and they should be able to handle that.

timothysc · 2017-02-22T17:47:27Z

I have no more comments on this PR. I see @davidopp has a couple.

@k8s-bot unit test this

node cache don't need to get all the information about every candidate node. For huge clusters, sending full node information of all nodes in the cluster on the wire every time a pod is scheduled is expensive. If the scheduler is capable of caching node information along with its capabilities, sending node name alone is sufficient. These changes provide that optimization in a backward compatible way - removed the inadvertent signature change of Prioritize() function - added defensive checks as suggested - added annotation specific test case - updated the comments in the scheduler types - got rid of apiVersion thats unused - using *v1.NodeList as suggested - backing out pod annotation update related changes made in the 1st commit - Adjusted the comments in types.go and v1/types.go as suggested in the code review

davidopp · 2017-02-23T23:37:13Z

Seems OK

sarat-k · 2017-02-24T20:41:27Z

@timothysc Can you please help with merging this ?

timothysc · 2017-02-24T22:11:25Z

I don't know why this is stuck in the submit queue... I can't merge directly.
/cc @kubernetes/sig-contributor-experience-bugs

apelisse · 2017-02-24T22:46:01Z

@timothysc @sarat-k That looks good to me, it's in the submit-queue. It will be picked-up eventually.

k8s-github-robot · 2017-02-26T22:02:53Z

Automatic merge from submit-queue (batch tested with PRs 41701, 41818, 41897, 41119, 41562)

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 8, 2017

k8s-github-robot assigned rmmh Feb 8, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 8, 2017

ravigadde suggested changes Feb 8, 2017

View reviewed changes

rmmh removed their assignment Feb 8, 2017

k8s-github-robot assigned timothysc Feb 8, 2017

sarat-k force-pushed the master branch from 9147a7e to 612abb0 Compare February 9, 2017 06:04

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 9, 2017

sarat-k mentioned this pull request Feb 9, 2017

scheduler extender interface enhancement to send minimal node info on the wire #40991

Closed

ravigadde reviewed Feb 10, 2017

View reviewed changes

sarat-k force-pushed the master branch from 612abb0 to 6e41b59 Compare February 11, 2017 01:19

timothysc suggested changes Feb 13, 2017

View reviewed changes

timothysc reviewed Feb 13, 2017

View reviewed changes

sarat-k force-pushed the master branch from 6e41b59 to 6990b07 Compare February 14, 2017 02:18

timothysc reviewed Feb 14, 2017

View reviewed changes

timothysc assigned davidopp Feb 14, 2017

timothysc added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 14, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 14, 2017

timothysc added this to the v1.6 milestone Feb 15, 2017

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

sarat-k force-pushed the master branch from 6990b07 to 2b818d6 Compare February 18, 2017 01:05

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

davidopp reviewed Feb 22, 2017

View reviewed changes

timothysc approved these changes Feb 22, 2017

View reviewed changes

sarat-k force-pushed the master branch from 2b818d6 to e65b444 Compare February 22, 2017 20:47

sarat-k force-pushed the master branch from e65b444 to dda62ec Compare February 23, 2017 18:26

timothysc added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 23, 2017

ravigadde mentioned this pull request Feb 24, 2017

Added Bind method to SchedulerExtender interface #41447

Closed

k8s-github-robot merged commit 11b9a2d into kubernetes:master Feb 26, 2017

		@@ -148,6 +148,7 @@ func (s *Scheduler) Run() {

		func (s *Scheduler) scheduleOne() {
		var podAnnotations schedulerapi.Annotations

		@@ -123,25 +122,33 @@ func (h HTTPExtender) Filter(pod v1.Pod, nodes []*v1.Node, nodeNameToInfo map[
		}

Optimization of on-wire information sent to scheduler extender interfaces that are stateful #41119

Optimization of on-wire information sent to scheduler extender interfaces that are stateful #41119

Conversation

sarat-k commented Feb 8, 2017 • edited Loading

k8s-ci-robot commented Feb 8, 2017

k8s-ci-robot commented Feb 8, 2017

k8s-reviewable commented Feb 8, 2017

k8s-github-robot commented Feb 8, 2017

sarat-k commented Feb 8, 2017

ravigadde 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

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

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

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

resouer commented Feb 10, 2017

timothysc commented Feb 10, 2017 • 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

Choose a reason for hiding this comment

ravigadde Feb 10, 2017 • 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

ravigadde commented Feb 10, 2017

timothysc 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

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

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

timothysc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarat-k commented Feb 21, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Feb 22, 2017

davidopp commented Feb 23, 2017

sarat-k commented Feb 24, 2017

timothysc commented Feb 24, 2017

apelisse commented Feb 24, 2017

k8s-github-robot commented Feb 26, 2017

sarat-k commented Feb 8, 2017 •

edited

Loading

timothysc commented Feb 10, 2017 •

edited

Loading

ravigadde Feb 10, 2017 •

edited

Loading

timothysc left a comment •

edited

Loading