Add specific errors for pod affinity predicates #51889

guangxuli · 2017-09-04T09:34:21Z

What this PR does / why we need it:

Add specific error for pod affinity predicates

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

fix #51655

Special notes for your reviewer:
none
Release note:
none

k8s-ci-robot · 2017-09-04T09:34:28Z

Hi @guangxuli. 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 /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. I understand the commands that are listed here.

guangxuli · 2017-09-04T10:47:53Z

/cc @bsalamat

guangxuli · 2017-09-04T10:49:13Z

/sig scheduling

xiangpengzhao · 2017-09-04T11:02:05Z

/release-note-none
/ok-to-test

guangxuli · 2017-09-04T12:04:34Z

/retest

guangxuli · 2017-09-04T15:40:48Z

/test pull-kubernetes-e2e-kops-aws

bsalamat · 2017-09-04T18:18:13Z

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

-	if !c.satisfiesExistingPodsAntiAffinity(pod, meta, nodeInfo) {
-		return false, []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch}, nil
+	if result, err := c.satisfiesExistingPodsAntiAffinity(pod, meta, nodeInfo); !result {
+		return false, []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch}, err


We should not return these as errors. We should define three new predicate failure reasons and return them in []algorithm.PredicateFailureReason in addition to ErrPodAffinityNotMatch.

yes, you are right. I make a misunderstanding. I have made another modification.

guangxuli · 2017-09-08T12:33:42Z

@bsalamat ptal, thanks.

xiangpengzhao · 2017-09-08T13:52:53Z

/retest

bsalamat · 2017-09-08T18:06:32Z

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


 	tests := []struct {
 		pod    *v1.Pod
 		pods   []*v1.Pod
-		nodes  []v1.Node
+		nodes  []NodeTest


optional: instead of a new type ("NodeTest"), you could have added another slice that would list the expected failure reasons in the same order as the nodes array.

reasonable,done

bsalamat · 2017-09-08T18:07:13Z

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

@@ -2564,7 +2572,7 @@ func TestInterPodAffinity(t *testing.T) {
 			test: "verify that PodAntiAffinity from existing pod is respected when pod has no AntiAffinity constraints. satisfy PodAntiAffinity symmetry with the existing pod",
 		},
 	}
-	expectedFailureReasons := []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch}
+	//expectedFailureReasons := []algorithm.PredicateFailureReason{ErrPodAffinityNotMatch}


Please delete the line.

bsalamat · 2017-09-08T18:15:06Z

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

@@ -1115,10 +1115,10 @@ func (c *PodAffinityChecker) getMatchingAntiAffinityTerms(pod *v1.Pod, allPods [

 // Checks if scheduling the pod onto this node would break any anti-affinity
 // rules indicated by the existing pods.
-func (c *PodAffinityChecker) satisfiesExistingPodsAntiAffinity(pod *v1.Pod, meta interface{}, nodeInfo *schedulercache.NodeInfo) bool {
+func (c *PodAffinityChecker) satisfiesExistingPodsAntiAffinity(pod *v1.Pod, meta interface{}, nodeInfo *schedulercache.NodeInfo) (bool, algorithm.PredicateFailureReason) {


I see why you are returning algorithm.PredicateFailureReason in this function, but the failure reason is always ErrExistingPodsAntiAffinityRulesNotMatch, unless there is an error. It would be better to return an error instead of algorithm.PredicateFailureReason and then in InterPodAffinityMatches function you check the error. If it is nil, you add ErrExistingPodsAntiAffinityRulesNotMatch to the failure reasons. If it is not nil, you return the error. You should do this for satisfiesPodsAffinityAntiAffinity as well. This would be more compatible with the existing behavior and would also address a problem in the current code that errors occurred in satisfiesPodsAffinityAntiAffinity and satisfiesExistingPodsAntiAffinity are not trickled up.

If we don't return algorithm.PredicateFailureReason in function satisfiesPodsAffinityAntiAffinity , we have to check the specific error just as your comment, two cases:
a. check the error: PodAffinityRulesNotMatch
b. check the error: PodAntiAffinityRulesNotMatch
if error is a or b, we add ErrPodsAffinityRulesNotMatch or ErrPodsAntiAffinityRulesNotMatch to algorithm.PredicateFailureReason in addition to ErrPodAffinityNotMatc and return nil. If got other errors we should return the error directly, right?

guangxuli · 2017-09-09T12:58:47Z

@bsalamat ptal.

guangxuli · 2017-09-12T03:56:36Z

@bsalamat I have added the specific returning error. IMO, i think there is no need to check the err again which done in original test case. It is enough just to check predicates failure reason.

bsalamat

Looks Good. Thanks! Just a minor comment.

bsalamat · 2017-09-12T22:12:13Z

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

@@ -1156,25 +1158,28 @@ func (c *PodAffinityChecker) satisfiesExistingPodsAntiAffinity(pod *v1.Pod, meta
 		// present in nodeInfo. Pods on other nodes pass the filter.
 		filteredPods, err := c.podLister.FilteredList(nodeInfo.Filter, labels.Everything())
 		if err != nil {
-			glog.Errorf("Failed to get all pods, %+v", err)
-			return false
+			format := "Failed to get all pods, %+v"


I think it would be cleaner (and a bit more efficient) if we did:

errMessage := fmt.sprintf("Failed to get all pods, %+v", err)
glog.Error(errMessage)
return ErrExistingPodsAntiAffinityRulesNotMatch, errors.New(errMessage)

If you agree, please change it for the cases below as well.

yes, that's more cleaner than what i have done. :) thanks. Will do.

guangxuli · 2017-09-13T15:51:27Z

@bsalamat All comments have addressed. PTAL.

bsalamat · 2017-09-13T19:04:29Z

Thanks, @guangxuli!

/lgtm

guangxuli · 2017-09-14T00:38:07Z

/assign @k82cn

guangxuli · 2017-09-14T00:38:34Z

/test pull-kubernetes-e2e-gce-bazel
/test pull-kubernetes-e2e-kops-aws

guangxuli · 2017-09-14T06:49:32Z

/test pull-kubernetes-e2e-gce-bazel

guangxuli · 2017-09-14T13:44:46Z

/test pull-kubernetes-e2e-gce-bazel

guangxuli · 2017-09-14T13:47:53Z

@k82cn ptal.

guangxuli · 2017-09-15T01:23:21Z

/test pull-kubernetes-e2e-gce-bazel

k82cn · 2017-09-15T01:47:07Z

sure, I'll review it today :).

k82cn · 2017-09-15T08:57:02Z

/approve

guangxuli · 2017-09-15T09:15:33Z

/test pull-kubernetes-e2e-gce-bazel

k8s-github-robot · 2017-09-15T09:15:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, guangxuli, k82cn

Associated issue: 51655

The full list of commands accepted by this bot can be found here.

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

~~plugin/pkg/scheduler/OWNERS~~ [k82cn]

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

fejta-bot · 2017-09-15T17:15:02Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

guangxuli · 2017-09-19T07:41:06Z

/retest

k8s-github-robot · 2017-09-23T08:14:33Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2017-09-23T20:40:49Z

Automatic merge from submit-queue (batch tested with PRs 52168, 48939, 51889, 52051, 50396). If you want to cherry-pick this change to another branch, please follow the instructions here..

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 4, 2017

k8s-github-robot assigned jayunit100 and k82cn Sep 4, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 4, 2017

k8s-ci-robot requested a review from bsalamat September 4, 2017 10:47

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Sep 4, 2017

guangxuli force-pushed the pod_affinity_error branch from 834627a to b6f92fa Compare September 4, 2017 12:03

bsalamat reviewed Sep 4, 2017

View reviewed changes

guangxuli force-pushed the pod_affinity_error branch from b6f92fa to 5142e51 Compare September 8, 2017 12:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 8, 2017

bsalamat reviewed Sep 8, 2017

View reviewed changes

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

guangxuli force-pushed the pod_affinity_error branch 2 times, most recently from 9643627 to e928999 Compare September 9, 2017 08:23

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

guangxuli force-pushed the pod_affinity_error branch from 3e3f999 to a9dabb9 Compare September 12, 2017 03:50

guangxuli force-pushed the pod_affinity_error branch from a9dabb9 to 63cd9ad Compare September 12, 2017 04:47

bsalamat reviewed Sep 12, 2017

View reviewed changes

guangxuli force-pushed the pod_affinity_error branch from 63cd9ad to 358c2e9 Compare September 13, 2017 15:42

Add specific errors for pod affinity predicates

a724a0f

guangxuli force-pushed the pod_affinity_error branch from 358c2e9 to a724a0f Compare September 13, 2017 15:48

k8s-ci-robot assigned bsalamat Sep 13, 2017

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

guangxuli mentioned this pull request Sep 14, 2017

[e2e test] pull-kubernetes-e2e-gce-bazel failure #52492

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017

k8s-github-robot merged commit 507e21e into kubernetes:master Sep 23, 2017

guangxuli deleted the pod_affinity_error branch October 20, 2017 05:38

irfanurrehman mentioned this pull request Oct 30, 2017

[e2e test]: pull-kubernetes-federation-e2e-gce test always fail kubernetes-retired/federation#14

Closed

Add specific errors for pod affinity predicates #51889

Add specific errors for pod affinity predicates #51889

Conversation

guangxuli commented Sep 4, 2017

k8s-ci-robot commented Sep 4, 2017

guangxuli commented Sep 4, 2017

guangxuli commented Sep 4, 2017

xiangpengzhao commented Sep 4, 2017

guangxuli commented Sep 4, 2017

guangxuli commented Sep 4, 2017

bsalamat Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guangxuli commented Sep 8, 2017

xiangpengzhao commented Sep 8, 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

guangxuli commented Sep 9, 2017

guangxuli commented Sep 12, 2017

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guangxuli commented Sep 13, 2017

bsalamat commented Sep 13, 2017

guangxuli commented Sep 14, 2017

guangxuli commented Sep 14, 2017

guangxuli commented Sep 14, 2017

guangxuli commented Sep 14, 2017

guangxuli commented Sep 14, 2017

guangxuli commented Sep 15, 2017

k82cn commented Sep 15, 2017

k82cn commented Sep 15, 2017

guangxuli commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

fejta-bot commented Sep 15, 2017

guangxuli commented Sep 19, 2017

k8s-github-robot commented Sep 23, 2017

k8s-github-robot commented Sep 23, 2017

bsalamat Sep 4, 2017 •

edited

Loading