Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. #34624

jayunit100 · 2016-10-12T16:47:42Z

Part 1 of pr for scheduler modularization specifically supporting CheckServiceAffinity improvements #33763.

These are some innoccous changes which help separate the predicate injection machinery from the cleanup parts of CheckServiceAffinity

@timothysc @wojtek-t

changes to metadata.go are for the follow on pr
changes to prio/pred stuff is all passing unit tests

hoping to grease this through and then the interesting part is in the follow on pr.

This change is

jayunit100 · 2016-10-12T18:35:22Z

Just rebased

k8s-ci-robot · 2016-10-12T19:30:53Z

Jenkins GCI GKE smoke e2e failed for commit f0164a8. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

jayunit100 · 2016-10-12T20:04:38Z

@k8s-bot gci gke e2e test this #33175

timothysc · 2016-10-12T23:30:21Z

plugin/pkg/scheduler/algorithm/predicates/utils.go

+import "k8s.io/kubernetes/pkg/api"
+
+// findLabelsInSet gets as many constraints as possible from the pod itself.
+func findLabelsInSet(labelsToKeep []string, nodeSelector labels.Set) map[string]string {


Is there a reason why these are private, as they seem generally useful?

I felt like this was made as a utility for making the nodeSelection stuff more understandable, and the semantics weren't defined well (i.e. findLabelsInSet vs filterLabelsFromMap vs ...) and it wasnt thought-out enough to generalize. In any case, happy to make it public.... what are your thoughts ? im +0 to public but can do if you still feel like we should.

wojtek-t · 2016-10-13T07:59:18Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

-// The pod is checked for the labels and any missing labels are then checked in the node
-// that hosts the service pods (peers) for the given pod.
+// The checkServiceAffinity predicate matches nodes in such a way to force that
+// ServiceAffinity.labels are homogenous for pods added to a node.
 //


nit: remove this line

The line is to separate the concise definition from the drivel :):). But yeah I can delete.

wojtek-t · 2016-10-13T08:01:33Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+
+// ExampleUtils is a https://blog.golang.org/examples styled unit test.
+func ExamplefindLabelsInSet() {
+


nit: remove this empty line

wojtek-t · 2016-10-13T10:03:24Z

plugin/pkg/scheduler/algorithm/types.go

@@ -39,7 +39,8 @@ type PriorityMapFunction func(pod *api.Pod, meta interface{}, nodeInfo *schedule
 type PriorityReduceFunction func(pod *api.Pod, result schedulerapi.HostPriorityList) error

 // MetdataProducer is a function that computes metadata for a given pod.
-type MetadataProducer func(pod *api.Pod) interface{}
+// In some cases NodeInfo is required in order for certain types of metadata caching, but leaving nil may be ok in many cases also.


I would remove this comment, because it basically says "nodeNameToInfo may be required, but may also not be required" (which basically doesn't say anything)

True, but remember the odd thing is that this interface is used for different factorIes. I guess I can directly say that in the comment rather

wojtek-t · 2016-10-13T10:03:45Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+	// will_see_this
+	// label1=value1,label2=value2,label3=will_see_this
+	// [&TypeMeta{Kind:,APIVersion:,} &TypeMeta{Kind:,APIVersion:,}]
+


nit: remove this empty line

wojtek-t · 2016-10-13T10:03:48Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+	addUnsetLabelsToMap(labelSubset, []string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)
+
+	fmt.Println(labelSubset)
+


nit: remove this empty line

wojtek-t · 2016-10-13T10:09:41Z

plugin/pkg/scheduler/algorithm/predicates/utils.go

+	return filtered
+}
+
+// createSelectorFromLabels is used to define the final selector which we will use to decide wether a pod


I wouldn't mention pod or node here - it is pretty generic function.

hmmm i think the semantics are really only useful to predicates and pods (even though its generalizable, it probably will never be used outside this package)... but i guess...

wojtek-t · 2016-10-13T10:10:34Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+
+		{}, // a third pod which will have no effect on anything.
+	}
+


nit: remove this empty line

wojtek-t · 2016-10-13T10:10:38Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+	fmt.Println(findLabelsInSet([]string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)["label3"])
+
+	addUnsetLabelsToMap(labelSubset, []string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)
+


nit: remove this empty line

wojtek-t · 2016-10-13T10:11:46Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+	// Output:
+	// will_see_this
+	// label1=value1,label2=value2,label3=will_see_this
+	// [&TypeMeta{Kind:,APIVersion:,} &TypeMeta{Kind:,APIVersion:,}]


This doesn't give any information (we don't know which pods were filtered out). So maybe just print the number of pods that are in this namespace?

I considered that, but this is simpler to maintain if we expand the pod meta data for the test ...

So let's print those pods so that we know what pods they are. This doesn't provide much value now.

wojtek-t · 2016-10-13T10:15:19Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+// 	the match.
+// Otherwise:
+// 	Create an "implicit selector" which backfills node label selectors for a pod, such that it will land with
+// 	matching labelled nodes to pre-existing pods.


It's probably due to my english, but I'm lost in this sentence. Can you please clarify?

k8s-ci-robot · 2016-10-13T12:59:56Z

Jenkins verification failed for commit dce89117a0f296012168ca82a53c315973f0c184. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

wojtek-t

One minor outstanding comment. other than that lgtm

wojtek-t · 2016-10-13T13:10:55Z

Also please fix "Jenkins verification"

…ation for predicate injection support, Update metadata struct

jayunit100 · 2016-10-13T14:44:58Z

plugin/pkg/scheduler/algorithm/predicates/utils_test.go

+				Namespace: "ns1",
+			},
+		},
+


blank line here intentional : to separate the third pod from the other two for readability

jayunit100 · 2016-10-13T15:46:30Z

i believe all comments addressed now...

jayunit100 · 2016-10-13T15:47:33Z

i think jenkins verify looks ok to me ?

wojtek-t · 2016-10-13T15:55:16Z

lgtm

k8s-github-robot · 2016-10-14T04:51:35Z

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

k8s-github-robot · 2016-10-14T05:30:49Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 12, 2016

k8s-github-robot assigned davidopp Oct 12, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 12, 2016

jayunit100 mentioned this pull request Oct 12, 2016

Predicate cacheing and cleanup #33763

Merged

wojtek-t assigned wojtek-t and unassigned davidopp Oct 12, 2016

jayunit100 force-pushed the scheduler-checksvc-1 branch from e250694 to afd8864 Compare October 12, 2016 18:16

timothysc added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 12, 2016

jayunit100 force-pushed the scheduler-checksvc-1 branch 2 times, most recently from 1ce8291 to f0164a8 Compare October 12, 2016 18:34

timothysc reviewed Oct 12, 2016

View reviewed changes

wojtek-t reviewed Oct 13, 2016

View reviewed changes

jayunit100 force-pushed the scheduler-checksvc-1 branch from f0164a8 to dce8911 Compare October 13, 2016 12:25

wojtek-t reviewed Oct 13, 2016

View reviewed changes

k8s-github-robot mentioned this pull request Oct 13, 2016

[k8s.io] Cluster level logging using GCL should check that logs from containers are ingested in GCL {Kubernetes e2e suite} #34713

Closed

Part 1 of pr kubernetes#33763: cleanup CheckServiceAffinity in prepar…

182e89b

…ation for predicate injection support, Update metadata struct

jayunit100 force-pushed the scheduler-checksvc-1 branch from dce8911 to 182e89b Compare October 13, 2016 14:39

jayunit100 commented Oct 13, 2016

View reviewed changes

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2016

timothysc added this to the v1.5 milestone Oct 13, 2016

timothysc added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Oct 13, 2016

k8s-github-robot merged commit f42337f into kubernetes:master Oct 14, 2016

k8s-github-robot mentioned this pull request Oct 14, 2016

[k8s.io] Kubectl client [k8s.io] Simple pod should support exec {Kubernetes e2e suite} #34797

Closed

jayunit100 changed the title ~~Minor cleanup to scheduler.~~ Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. Oct 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. #34624

Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. #34624

jayunit100 commented Oct 12, 2016 •

edited

jayunit100 commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

jayunit100 commented Oct 12, 2016

timothysc Oct 12, 2016

jayunit100 Oct 13, 2016 •

edited

wojtek-t Oct 13, 2016

jayunit100 Oct 13, 2016

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

jayunit100 Oct 13, 2016

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

jayunit100 Oct 13, 2016 •

edited

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

jayunit100 Oct 13, 2016

wojtek-t Oct 13, 2016

wojtek-t Oct 13, 2016

k8s-ci-robot commented Oct 13, 2016

wojtek-t left a comment

wojtek-t commented Oct 13, 2016

jayunit100 Oct 13, 2016

jayunit100 commented Oct 13, 2016

jayunit100 commented Oct 13, 2016

wojtek-t commented Oct 13, 2016

k8s-github-robot commented Oct 14, 2016

k8s-github-robot commented Oct 14, 2016


		// ExampleUtils is a https://blog.golang.org/examples styled unit test.
		func ExamplefindLabelsInSet() {

		addUnsetLabelsToMap(labelSubset, []string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)

		fmt.Println(labelSubset)

		fmt.Println(findLabelsInSet([]string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)["label3"])

		addUnsetLabelsToMap(labelSubset, []string{"label1", "label2", "label3"}, nsPods[0].ObjectMeta.Labels)

Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. #34624

Refactoring + Cleanup to scheduler +utils for modularizing checkServiceAffinity. #34624

Conversation

jayunit100 commented Oct 12, 2016 • edited

jayunit100 commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

jayunit100 commented Oct 12, 2016

Choose a reason for hiding this comment

jayunit100 Oct 13, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayunit100 Oct 13, 2016 • 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

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 13, 2016

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t commented Oct 13, 2016

Choose a reason for hiding this comment

jayunit100 commented Oct 13, 2016

jayunit100 commented Oct 13, 2016

wojtek-t commented Oct 13, 2016

k8s-github-robot commented Oct 14, 2016

k8s-github-robot commented Oct 14, 2016

jayunit100 commented Oct 12, 2016 •

edited

jayunit100 Oct 13, 2016 •

edited

jayunit100 Oct 13, 2016 •

edited