Move hardPodAffinitySymmetricWeight to scheduler policy config #44159

wanghaoran1988 · 2017-04-06T14:40:36Z

What this PR does / why we need it:
Move hardPodAffinitySymmetricWeight to scheduler policy config
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #43845

Special notes for your reviewer:
If you like this, will add test later
Release note:

Move hardPodAffinitySymmetricWeight from KubeSchedulerConfiguration to scheduler Policy config

k8s-ci-robot · 2017-04-06T14:40:46Z

Hi @wanghaoran1988. 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-reviewable · 2017-04-06T14:40:56Z

This change is

wanghaoran1988 · 2017-04-06T14:41:35Z

/cc @k82cn

k82cn · 2017-04-07T02:06:36Z

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

+	// RequiredDuringScheduling affinity is not symmetric, but there is an implicit PreferredDuringScheduling affinity rule
+	// corresponding to every RequiredDuringScheduling affinity rule.
+	// HardPodAffinitySymmetricWeight represents the weight of implicit PreferredDuringScheduling affinity rule, in the range 0-100.
+	HardPodAffinitySymmetricWeight int


prefer to *int; the user may set it to 0.

@k82cn If you don't set it, it will be 0.

If I set it to 0 and also set hard-pod-affinity-symmetric-weight, does it have the same behaviour with setting it to 1?

@k82cn yes, 1 is the default value for hard-pod-affinity-symmetric-weight from command line, but if the HardPodAffinitySymmetricWeight from policy config is not 0, will use this .

The concern is that: the behaviour is different when I set HardPodAffinitySymmetricWeight in policy file: 0 vs. not-zero.

Wait! Are we changing the existing behavior of HardPodAffinitySymmetricWeight or just its location? If the answer is the latter, then we shouldn't change the type.

It's a kind of moving its location: a new parameters in policy, it can override the value from cli. prefer to *int, as we can distinguish "set to 0" or "not set".

The value in CLI also can not set to 0 to disable it; so I'm ok with that to keep current behaviours. Please add a comments or TODO on this.

k82cn · 2017-04-07T02:07:20Z

plugin/pkg/scheduler/factory/factory.go

@@ -338,6 +338,11 @@ func (f *ConfigFactory) CreateFromConfig(policy schedulerapi.Policy) (*scheduler
 			}
 		}
 	}
+	// issue: https://github.com/kubernetes/kubernetes/issues/43845
+	// for backward compatibility if user set the value in policy file, then we use it
+	if policy.HardPodAffinitySymmetricWeight != 0 {


after changing it to *int, check it against nil.

k82cn · 2017-04-07T02:11:02Z

plugin/pkg/scheduler/api/types.go

+	// RequiredDuringScheduling affinity is not symmetric, but there is an implicit PreferredDuringScheduling affinity rule
+	// corresponding to every RequiredDuringScheduling affinity rule.
+	// HardPodAffinitySymmetricWeight represents the weight of implicit PreferredDuringScheduling affinity rule, in the range 0-100.
+	HardPodAffinitySymmetricWeight int


As I mentioned in issue, another option is to move it to PredicatePolicy (per RequiredDuringScheduling). But I don-t think it's required in this PR, so please add a TODO in the comments.

@k82cn If we need to move it elsewhere, we should do it in this PR IMO. This is user facing and we don't want to deprecate it again and move it.

Did not have requirements on HardPodAffinitySymmetricWeight per RequiredDuringScheduling, so prefer to keep current behaviour :). And it's complex to achieve that.

bsalamat · 2017-04-07T05:49:24Z

plugin/pkg/scheduler/api/types.go

+	// RequiredDuringScheduling affinity is not symmetric, but there is an implicit PreferredDuringScheduling affinity rule
+	// corresponding to every RequiredDuringScheduling affinity rule.
+	// HardPodAffinitySymmetricWeight represents the weight of implicit PreferredDuringScheduling affinity rule, in the range 0-100.
+	HardPodAffinitySymmetricWeight int


@k82cn If we need to move it elsewhere, we should do it in this PR IMO. This is user facing and we don't want to deprecate it again and move it.

bsalamat · 2017-04-07T05:58:33Z

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

+	// RequiredDuringScheduling affinity is not symmetric, but there is an implicit PreferredDuringScheduling affinity rule
+	// corresponding to every RequiredDuringScheduling affinity rule.
+	// HardPodAffinitySymmetricWeight represents the weight of implicit PreferredDuringScheduling affinity rule, in the range 0-100.
+	HardPodAffinitySymmetricWeight int


Wait! Are we changing the existing behavior of HardPodAffinitySymmetricWeight or just its location? If the answer is the latter, then we shouldn't change the type.

bsalamat · 2017-04-07T06:02:08Z

plugin/pkg/scheduler/factory/factory.go

@@ -338,6 +338,11 @@ func (f *ConfigFactory) CreateFromConfig(policy schedulerapi.Policy) (*scheduler
 			}
 		}
 	}
+	// issue: https://github.com/kubernetes/kubernetes/issues/43845
+	// for backward compatibility if user set the value in policy file, then we use it


Policy is the new location of HardPodAffinitySymmetricWeight, so backward compatibility is not addressed here, unless I am missing something.

@bsalamat Backward scenario is user don't set the value in policy file, it will use the value from command line when create the scheduler. here the logic is if we set the value in policy file, we should use the value from new location.

@wanghaoran1988 Backward compatibility applies if the user does not set it in the policy file and keep using the old way of providing the value, i.e., as a command line argument. The scenario that you described is the new way of providing the value. Nothing is wrong with your logic, just the comment is misleading.

bsalamat · 2017-04-07T23:34:52Z

We should warn users if they provide hardPodAffinitySymmetricWeight on the CLI and tell them about the new location. Also, we should add tests to verify that hardPodAffinitySymmetricWeight is effective when provided in a config file.

wanghaoran1988 · 2017-04-08T03:02:38Z

@bsalamat The MarkDeprecated will tell use the new location when they run the command. Yes, I will add test as I first comment said.

wanghaoran1988 · 2017-04-09T13:38:48Z

@k82cn @bsalamat Comments addressed, PTAL :)

k82cn · 2017-04-10T01:53:37Z

@k8s-bot ok to test

wanghaoran1988 · 2017-04-10T07:40:23Z

@k8s-bot verify test this

bsalamat · 2017-04-10T18:28:05Z

plugin/pkg/scheduler/factory/factory.go

@@ -338,6 +338,12 @@ func (f *ConfigFactory) CreateFromConfig(policy schedulerapi.Policy) (*scheduler
 			}
 		}
 	}
+	// issue: https://github.com/kubernetes/kubernetes/issues/43845


I would rewrite the whole comment to something like:
Providing HardPodAffinitySymmetricWeight in the policy config is the new and preferred way of providing the value. Give it higher precedence when it is provided.

bsalamat · 2017-04-10T18:29:40Z

plugin/pkg/scheduler/factory/factory_test.go

@@ -119,6 +119,68 @@ func TestCreateFromConfig(t *testing.T) {
 	}

 	factory.CreateFromConfig(policy)
+	hpa := factory.GetHardPodAffinitySymmetricWeight()
+	if hpa != v1.DefaultHardPodAffinitySymmetricWeight {
+		t.Errorf("Wrong hardPodAffinitySymmetricWeight, ecpected: %d, get %d", v1.DefaultHardPodAffinitySymmetricWeight, hpa)


s/get %d/got: %d

bsalamat · 2017-04-10T18:34:58Z

plugin/pkg/scheduler/factory/factory_test.go

+	factory.CreateFromConfig(policy)
+	hpa := factory.GetHardPodAffinitySymmetricWeight()
+	if hpa != 10 {
+		t.Errorf("Wrong hardPodAffinitySymmetricWeight, ecpected: %d, get %d", 10, hpa)


wanghaoran1988 · 2017-04-12T16:07:36Z

@k8s-bot non-cri e2e test this

wanghaoran1988 · 2017-04-13T02:52:58Z

@bsalamat @k82cn Comments addressed, PTAL

k82cn · 2017-04-13T03:06:37Z

lgtm

wanghaoran1988 · 2017-04-26T14:29:06Z

@timothysc @davidopp Any more comments on this?

wanghaoran1988 · 2017-05-09T08:04:51Z

@gyliu513 Yeah, exactly, just want to make sure @davidopp like this change, maybe he have other concerns.

bsalamat · 2017-05-09T17:39:36Z

@wanghaoran1988 I agree with @gyliu513 about updating the scheduler config policy example file.

gyliu513 · 2017-05-10T03:14:01Z

@wanghaoran1988 Found another issue that you may want to fix when reviewing @bsalamat's PR #44805 : There is a integration test cases for policy config map https://github.com/kubernetes/kubernetes/blob/master/test/integration/scheduler/scheduler_test.go#L78-L153 , I think that you may want to create a new case named as TestSchedulerCreationFromConfigMapWithHardPodAffinitySymmetricWeight or sth more meaningful, for this issue I think that it can be addressed in another follow up PR.

wanghaoran1988 · 2017-05-10T05:12:02Z

@gyliu513 How about update the integration test instead of create a new one?

gyliu513 · 2017-05-10T06:54:33Z

@gyliu513 How about update the integration test instead of create a new one?

+1, @wanghaoran1988

gyliu513 · 2017-05-10T07:11:58Z

test/integration/scheduler/scheduler_test.go

@@ -107,7 +107,8 @@ func TestSchedulerCreationFromConfigMap(t *testing.T) {
 			"priorities" : [
 				{"name" : "PriorityOne", "weight" : 1},
 				{"name" : "PriorityTwo", "weight" : 5}
-			]
+			],
+			"hardPodAffinitySymmetricWeight" : 10
 			}`,


You also need to verify the config is applied as others, such as schedPrioritizers etc.

wanghaoran1988 · 2017-05-10T11:09:08Z

@gyliu513 I decide not update the integration test in this pr, if you want to verify the value, you need export a lot variable or functions, an the unit test already covered this, if you guy still think we need that, I can do it in a follow up pr.

gyliu513 · 2017-05-10T13:56:38Z

@wanghaoran1988 I'm OK to handle this in another follow up PR.

wanghaoran1988 · 2017-05-20T14:27:43Z

@k82cn @bsalamat @davidopp @timothysc I addressed the comments, but not sure where to go from here other than to keep fixing rebase issues.

k82cn · 2017-05-22T00:15:17Z

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

@@ -32,6 +32,10 @@ type Policy struct {
 	Priorities []PriorityPolicy `json:"priorities"`
 	// Holds the information to communicate with the extender(s)
 	ExtenderConfigs []ExtenderConfig `json:"extenders"`
+	// RequiredDuringScheduling affinity is not symmetric, but there is an implicit PreferredDuringScheduling affinity rule
+	// corresponding to every RequiredDuringScheduling affinity rule.
+	// HardPodAffinitySymmetricWeight represents the weight of implicit PreferredDuringScheduling affinity rule, in the range 1-100.


in the range 1-100 ? Any check for it?

See: https://github.com/kubernetes/kubernetes/pull/44159/files#diff-77bdee21550bc698bfa654de11c64324R393

wow, great, thanks.

gyliu513 · 2017-05-22T00:35:12Z

examples/scheduler-policy-config-with-extender.json

@@ -14,7 +14,7 @@
 	{"name" : "ServiceSpreadingPriority", "weight" : 1},
 	{"name" : "EqualPriority", "weight" : 1}
 	],
-"extenders":[ 
+"extenders":[


Not yours, but I think we should add a blank space after :.

"extenders": [

gyliu513 · 2017-05-26T02:47:11Z

@wanghaoran1988 any update on this? I think this can catch 1.7 once the comments are addressed.

wanghaoran1988 · 2017-05-26T06:07:31Z

@gyliu513 Updated

k82cn · 2017-05-26T06:46:45Z

/lgtm

@wanghaoran1988 , thanks very much for your work :).

gyliu513

/lgtm

bsalamat · 2017-05-26T17:16:35Z

/lgtm

Thanks, @wanghaoran1988!

k8s-github-robot · 2017-05-26T17:21:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsalamat, k82cn, wanghaoran1988
We suggest the following additional approvers: davidopp, zmerlynn

Assign the PR to them by writing /assign @davidopp @zmerlynn in a comment when ready.

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-30T17:15:48Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 6, 2017

k8s-github-robot assigned davidopp Apr 6, 2017

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 6, 2017

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

k82cn reviewed Apr 7, 2017

View reviewed changes

davidopp assigned bsalamat and unassigned davidopp Apr 7, 2017

bsalamat reviewed Apr 7, 2017

View reviewed changes

wanghaoran1988 force-pushed the fix_43845 branch from adde2c0 to 19d4a5a Compare April 9, 2017 13:38

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 9, 2017

k82cn approved these changes Apr 10, 2017

View reviewed changes

bsalamat reviewed Apr 10, 2017

View reviewed changes

wanghaoran1988 force-pushed the fix_43845 branch 2 times, most recently from ffd1f73 to 0a15f6f Compare April 12, 2017 15:14

wanghaoran1988 force-pushed the fix_43845 branch from 0a15f6f to 5c886ae Compare May 10, 2017 06:50

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2017

gyliu513 reviewed May 10, 2017

View reviewed changes

wanghaoran1988 force-pushed the fix_43845 branch 2 times, most recently from 8f90706 to 5c886ae Compare May 10, 2017 11:05

k82cn reviewed May 22, 2017

View reviewed changes

gyliu513 reviewed May 22, 2017

View reviewed changes

move hardPodAffinitySymmetricWeight to scheduler policy config

31d2f45

wanghaoran1988 force-pushed the fix_43845 branch from 5c886ae to 31d2f45 Compare May 26, 2017 06:04

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

gyliu513 reviewed May 26, 2017

View reviewed changes

timothysc self-assigned this May 30, 2017

timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2017

k8s-github-robot merged commit 657c01c into kubernetes:master May 30, 2017

wanghaoran1988 deleted the fix_43845 branch November 22, 2017 05:59

tfussell mentioned this pull request Jul 30, 2020

Remove old scheduler configs giantswarm/k8scloudconfig#780

Merged

1 task

Move hardPodAffinitySymmetricWeight to scheduler policy config #44159

Move hardPodAffinitySymmetricWeight to scheduler policy config #44159

Conversation

wanghaoran1988 commented Apr 6, 2017

k8s-ci-robot commented Apr 6, 2017

k8s-reviewable commented Apr 6, 2017

wanghaoran1988 commented Apr 6, 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

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

wanghaoran1988 Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Apr 7, 2017

wanghaoran1988 commented Apr 8, 2017 • edited Loading

wanghaoran1988 commented Apr 9, 2017

k82cn commented Apr 10, 2017

wanghaoran1988 commented Apr 10, 2017

bsalamat Apr 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

wanghaoran1988 commented Apr 12, 2017

wanghaoran1988 commented Apr 13, 2017

k82cn commented Apr 13, 2017

wanghaoran1988 commented Apr 26, 2017

wanghaoran1988 commented May 9, 2017 • edited Loading

bsalamat commented May 9, 2017

gyliu513 commented May 10, 2017

wanghaoran1988 commented May 10, 2017

gyliu513 commented May 10, 2017

Choose a reason for hiding this comment

wanghaoran1988 commented May 10, 2017

gyliu513 commented May 10, 2017

wanghaoran1988 commented May 20, 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

gyliu513 commented May 26, 2017

wanghaoran1988 commented May 26, 2017

k82cn commented May 26, 2017

gyliu513 left a comment

Choose a reason for hiding this comment

bsalamat commented May 26, 2017

k8s-github-robot commented May 26, 2017

k8s-github-robot commented May 30, 2017

wanghaoran1988 Apr 7, 2017 •

edited

Loading

wanghaoran1988 commented Apr 8, 2017 •

edited

Loading

bsalamat Apr 10, 2017 •

edited

Loading

wanghaoran1988 commented May 9, 2017 •

edited

Loading