Node Affinity and Node Anti-Affinity enhancement related changes. #98

shivramsrivastava · 2018-05-24T13:18:34Z

Refer #51

Pending:

Node Affinity E2E test cases.

k8s-ci-robot · 2018-05-24T13:18:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shivramsrivastava

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:

~~OWNERS~~ [shivramsrivastava]

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

k8s-ci-robot · 2018-05-24T13:18:53Z

Hi @shivramsrivastava. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /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.

I understand the commands that are listed here.

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.

m1093782566 · 2018-05-24T13:48:39Z

/ok-to-test

shivramsrivastava · 2018-05-25T04:53:50Z

/assign @m1093782566

m1093782566 · 2018-05-25T05:58:58Z

@shivramsrivastava

A quick question. I find you have changed some existing *.pb.go. I am wondering why since you just added a new protobuf file. What command did you use to generate *.pb.go?

protoc --go_out=. *.proto

?

nikita15p · 2018-05-25T06:12:55Z

@m1093782566
protoc --go_out=,import_path=$GOPATH/src/github.com/kubernetes-sigs/poseidon/pkg/firmament,plugins=grpc:. *.proto is used for generating *.pb.go

m1093782566 · 2018-05-25T06:17:02Z

@nikita15p

Thanks for your reply, I am fine with it. I just want a clarification :)

m1093782566 · 2018-05-25T06:21:21Z

pkg/k8sclient/podwatcher.go

+			if pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil {
+				err := copier.Copy(&nodeSelTerm, pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms)
+				if err != nil {
+					glog.Info("NodeSelectorTerm Data could not be copied")


glog.Errorf("NodeSelectorTerm %v could not be copied, err: %v", pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms, err)

m1093782566 · 2018-05-25T06:23:07Z

pkg/k8sclient/podwatcher.go

+
+			err := copier.Copy(&prefSchTerm, pod.Spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution)
+			if err != nil {
+				glog.Info("PreferredSchedulingTerm Data could not be copied")


same as above. Also, there are some similar cases below.

m1093782566 · 2018-05-25T06:23:36Z

pkg/k8sclient/podwatcher.go

+	var prefSchTerm []PreferredSchedulingTerm
+	if pod.Spec.Affinity != nil {
+		if pod.Spec.Affinity.NodeAffinity != nil {
+


nit: unnecessary empty line.

m1093782566 · 2018-05-25T06:24:34Z

pkg/firmament/task_desc.proto

@@ -101,4 +102,6 @@ message TaskDescriptor {
  repeated Label labels = 32;
  // Resource label selectors
  repeated LabelSelector label_selectors = 33;
+  //Affinity


Does it include both node and pod affinity? If so, please just make it clear.

Yes, the kubernetes also follow same naming convention ie Affinity being the superset of Node Affinity, Pod Affinity and Pod AntiAffinity.

m1093782566 · 2018-05-25T06:25:52Z

pkg/firmament/pod_anti_affinity.proto

+
+message PodAffinityTermAntiAff {
+
+    LabelSelectorAntiAff labelSelector = 1;


nit: extra whitespaces?

m1093782566 · 2018-05-25T06:25:59Z

pkg/firmament/pod_anti_affinity.proto

+
+message LabelSelectorAntiAff{
+
+    MatchLabelsAntiAff matchLabels = 1;


nit: extra whitespaces?

m1093782566 · 2018-05-25T06:32:04Z

pkg/k8sclient/podwatcher.go

+	if pod.Spec.Affinity != nil {
+		if pod.Spec.Affinity.NodeAffinity != nil {
+			if pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil {
+				err := copier.Copy(&nodeSelTerm, pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms)


Please comment the usage of copier.Copy, does it deep copy all the common fields from podSpec to nodeSelTerm?

Yes it does.

m1093782566 · 2018-05-25T06:37:48Z

pkg/k8sclient/podwatcher.go

+	nodeAffinity := len(pod.Affinity.NodeAffinity.HardScheduling.NodeSelectorTerms) > 0 || len(pod.Affinity.NodeAffinity.SoftScheduling) > 0
+	podAffinity := len(pod.Affinity.PodAffinity.HardScheduling) > 0 || len(pod.Affinity.PodAffinity.SoftScheduling) > 0
+	podAntiAffinity := len(pod.Affinity.PodAntiAffinity.HardScheduling) > 0 || len(pod.Affinity.PodAntiAffinity.SoftScheduling) > 0
+	localAffinity := new(firmament.Affinity)


Please comment the localAffinity in firmament because as far as I know there are concepts of node local(topology) in Kubernetes and what's the different between them?

localAffinity is just the name of local pointer variable pointing to the type firmament.Affinity used for the Affinity. It is not related to the concepts of local(topology) in Kubernetes.

m1093782566 · 2018-05-25T07:15:20Z

LGTM overall except some nits.

m1093782566 · 2018-05-25T10:26:32Z

/lgtm

Node Affinity and Node Anti-Affinity enhancement related changes.

07dbeb6

shivramsrivastava added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2018

k8s-ci-robot requested review from m1093782566 and shashidharatd May 24, 2018 13:18

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 24, 2018

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2018

Adding few E2E test cases for Nodeaffinity related enhancement

faf9992

shivramsrivastava changed the title ~~[WIP] Node Affinity and Node Anti-Affinity enhancement related changes.~~ Node Affinity and Node Anti-Affinity enhancement related changes. May 25, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2018

k8s-ci-robot assigned m1093782566 May 25, 2018

m1093782566 reviewed May 25, 2018

View reviewed changes

Formatted the code as per review comments

29b2fa4

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2018

k8s-ci-robot merged commit 096d0af into kubernetes-retired:master May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node Affinity and Node Anti-Affinity enhancement related changes. #98

Node Affinity and Node Anti-Affinity enhancement related changes. #98

shivramsrivastava commented May 24, 2018

k8s-ci-robot commented May 24, 2018

k8s-ci-robot commented May 24, 2018

m1093782566 commented May 24, 2018

shivramsrivastava commented May 25, 2018

m1093782566 commented May 25, 2018

nikita15p commented May 25, 2018 •

edited by m1093782566

Loading

m1093782566 commented May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018 •

edited

Loading

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 May 25, 2018

nikita15p May 25, 2018

m1093782566 commented May 25, 2018

m1093782566 commented May 25, 2018


		message PodAffinityTermAntiAff {

		LabelSelectorAntiAff labelSelector = 1;


		message LabelSelectorAntiAff{

		MatchLabelsAntiAff matchLabels = 1;

Node Affinity and Node Anti-Affinity enhancement related changes. #98

Node Affinity and Node Anti-Affinity enhancement related changes. #98

Conversation

shivramsrivastava commented May 24, 2018

k8s-ci-robot commented May 24, 2018

k8s-ci-robot commented May 24, 2018

m1093782566 commented May 24, 2018

shivramsrivastava commented May 25, 2018

m1093782566 commented May 25, 2018

nikita15p commented May 25, 2018 • edited by m1093782566 Loading

m1093782566 commented May 25, 2018

Choose a reason for hiding this comment

nikita15p May 25, 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

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

m1093782566 commented May 25, 2018

m1093782566 commented May 25, 2018

nikita15p commented May 25, 2018 •

edited by m1093782566

Loading

nikita15p May 25, 2018 •

edited

Loading