Add pvc as part of equivalence hash #56577

resouer · 2017-11-29T14:47:19Z

What this PR does / why we need it:

Should add PVC as part of equivalence hash so that StatefulSet and Operator will always run the volume predicate, while the ReplicaSet can still re-use cached ones.

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

Special notes for your reviewer:

Release note:

Add pvc as part of equivalence hash

foxish · 2017-11-29T15:56:08Z

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

+			continue
+		}
+		pvcName := volume.PersistentVolumeClaim.ClaimName
+		result.Insert(pod.GetNamespace() + "-" + pvcName)


Can we use the PVC UID? I understand this is probably equivalent, but using the UID might make it more obvious. Is there a reason to avoid using the UID? If @bsalamat is ok with this, then maybe we can add a comment explaining it.

A user could delete a PVC and create a new one with the same name, but different UID. The PVC contents could be different, so we would not want to use the cached results for the old PVC.

But actually, how do entries in the equivalence cache get removed?

I agree that UID will be more reliable to ensure uniqueness.

@msau42 Yes when PVC is updated, the volume predicate cache will be invalidated so it still works.

I was just trying to avoid fetching UID it from scheduler cache since we may need to change get ecache function's interface a bit. Not sure if it's allowed during code freeze ...

Anyway let me update the patch to a more reliable way.

Oh I see what you mean, the Pod object only contains the PVC name, so you have to lookup the PVC object from the informer cache to get the UID

Right, so now I've already refactored it to a factory like NewEquivalencePodGenerator(args.PVCInfo), will update the PR soon after test passes.

msau42

Also, the NoVolumeZoneConflict predicate needs to be invalidated on PVC delete

resouer · 2017-12-01T10:12:03Z

@msau42 Thanks, just updated. Also cleaned up and fixed the related MaxPDVolumeCountPredicate

msau42 · 2017-12-01T14:16:08Z

plugin/pkg/scheduler/factory/factory.go

-	if pvc.Spec.VolumeName != "" {
-		c.equivalencePodCache.InvalidateCachedPredicateItemOfAllNodes(maxPDVolumeCountPredicateSet)
-	}
+	// TODO(harry): make all predicate key to be global const, e.g. predicates.NoVolumeZoneConflict


Can you add a comment that we need to do this here because the ecache uses PVC uid to hash the pod

I guess for that same reason, VolumeBind predicate also needs to be invalidated

ack, just updated.

msau42 · 2017-12-01T14:23:35Z

plugin/pkg/scheduler/factory/factory.go

@@ -384,16 +384,7 @@ func (c *configFactory) onPvDelete(obj interface{}) {
 }

 func (c *configFactory) invalidatePredicatesForPv(pv *v1.PersistentVolume) {
-	invalidPredicates := sets.NewString("MaxPDVolumeCountPredicate")


I think this needs to stay. You could have a PVC that points to a PV, but the PV object doesn't exist. So when the PV object gets added, we can recount.

fixed, and also updated other comments to clarify the invalidation op reasosn.

msau42 · 2017-12-01T14:25:57Z

plugin/pkg/scheduler/core/equivalence_cache.go

@@ -188,12 +188,29 @@ func (ec *EquivalenceCache) InvalidateCachedPredicateItemForPodAdd(pod *v1.Pod,

 	// GeneralPredicates: will always be affected by adding a new pod
 	invalidPredicates := sets.NewString("GeneralPredicates")
+
+	// MaxPDVolumeCountPredicate: we check the volumes of pod to make decision.
+	for _, vol := range pod.Spec.Volumes {


Why does this need to be added here?

I noticed MaxPDVolumeCountChecker actually relies on volumes count of specify Pod to do predicate. So when a pod with PV is add/delete on a node. The MaxPDVolumeCountChecker e-cache of this node should be re-calculated for subsequent pods.

Hope I am understanding right :)

the volume count checker counts the volumes on pods that are assigned on nodes. I guess for the case where you create the Pod with NodeName already set, then yes, the predicate should be invalidated.

msau42 · 2017-12-01T16:49:08Z

lgtm from volume perspective.

Have you tried testing your change with a StatefulSet with PVCs to make sure it is fixed?

bsalamat · 2017-12-01T23:27:50Z

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

@@ -1492,3 +1492,14 @@ func (c *VolumeBindingChecker) predicate(pod *v1.Pod, meta algorithm.PredicateMe
 	glog.V(5).Info("All PVCs found matches for pod %v/%v, node %q", pod.Namespace, pod.Name, node.Name)
 	return true, nil, nil
 }
+
+type FakePersistentVolumeClaimInfo []v1.PersistentVolumeClaim


Why is this block moved to this file? It does not belong here.

This will be used by equivalence_cache_test.go, maybe create a test helper pkg?

bsalamat · 2017-12-01T23:31:58Z

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

 	// For now we only consider pods:
 	// 1. OwnerReferences is Controller
 	// 2. with same OwnerReferences
+	// 3. with same PVC claim
 	// to be equivalent
 	if len(pod.OwnerReferences) != 0 {


Now that you are making changes, please drop this if statement. The for loop takes care of this.

bsalamat · 2017-12-01T23:33:11Z

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

 	// For now we only consider pods:
 	// 1. OwnerReferences is Controller
 	// 2. with same OwnerReferences
+	// 3. with same PVC claim
 	// to be equivalent
 	if len(pod.OwnerReferences) != 0 {
 		for _, ref := range pod.OwnerReferences {
 			if *ref.Controller {


can't ref.Controller ever be nil? We should probably check.

Good catch ... just fixed

bsalamat · 2017-12-01T23:47:42Z

plugin/pkg/scheduler/factory/factory.go

 			// PVC volume binding has changed
 			invalidPredicates.Insert(predicates.CheckVolumeBinding)
 		}
+		// The binded volume type may change


s/binded/bound/ here an other places in your PR

resouer · 2017-12-02T10:52:40Z

@msau42 Yes, I've tested the patch with a StatefulSet with ReadWriteOnce PVC:

  volumeClaimTemplates:
  - metadata:
      name: www
    spec:
      accessModes: [ "ReadWriteOnce" ]
      resources:
        requests:
          storage: 1Gi

And when set export FEATURE_GATES=EnableEquivalenceClassCache=true,VolumeScheduling=true, the second Pod is expected to be failed to schedule.

cluster/kubectl.sh get pods
NAME      READY     STATUS    RESTARTS   AGE
web-0     1/1       Running   0          4s
web-1     0/1       Pending   0          1s

Events:
  Type     Reason            Age               From               Message
  ----     ------            ----              ----               -------
  Warning  FailedScheduling  0s (x6 over 15s)  default-scheduler  pod has unbound PersistentVolumeClaims

resouer · 2017-12-02T10:56:45Z

The eclass feature do have e2e tests but normally skipped due to it's Serial. While I strongly agree #56104 would be great way to guard this.

Use factory to generat get equivalence pod func

msau42 · 2017-12-02T15:46:20Z

The second pod fails to schedule with or without your patch? The original reported 1.8 bug was that the second pod did get scheduled, but in the wrong zone. You would need a multizone cluster to test this.

Or the other way is to enable my alpha features in 1.9 for local volumes and to run my local e2e test. Or I can run it with your patch.

Also in general, for e2e alpha tests, you have to explicitly add it to the regex for the alpha suite. See kubernetes/test-infra#5679

msau42 · 2017-12-04T19:20:54Z

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

-			if *ref.Controller {
-				// a pod can only belongs to one controller
+	for _, ref := range pod.OwnerReferences {
+		if ref.Controller != nil && *ref.Controller {


Why do you also need to *ref.Controller?

@msau42 ref.Controller is a *bool. Its value can be false.

Ah thanks, makes sense

msau42 · 2017-12-04T20:59:13Z

lgtm from volume side

bsalamat · 2017-12-04T21:42:40Z

plugin/pkg/scheduler/factory/factory.go

+	matchInterPodAffinitySet      = sets.NewString("MatchInterPodAffinity")
+	generalPredicatesSets         = sets.NewString("GeneralPredicates")
+	noDiskConflictSet             = sets.NewString("NoDiskConflict")
+	maxPDVolumeCountPredicateKeys = []string{"MaxGCEPDVolumeCount", "MaxAzureDiskVolumeCount", "MaxEBSVolumeCount"}


I thought I had asked this question before, but looks like I haven't:
why do we need to break MaxPDVolumeCountPredicate to three keys? Is it only for optimization purposes? If so, I doubt if it has a major effect on performance. It will probably be rare to have more than one of these volume types on a single node.

Actually the registered predicates are three separated names instead of "MaxPDVolumeCount".

Otherwise we may need to check the type of volume before invalidating, while just as you pointed out, it's rare to have more than one of them as PV, so I chose to invalidate them all in most cases unless we can check the PV type very easily.

I see. Thanks for pointing it out.

bsalamat · 2017-12-05T16:08:41Z

/lgtm

k8s-github-robot · 2017-12-05T16:09:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, resouer

Associated issue: 56265

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~~ [bsalamat]

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

msau42 · 2017-12-05T21:45:02Z

/kind bug

msau42 · 2017-12-05T21:45:27Z

/priority critical-urgent

k8s-github-robot · 2017-12-05T21:46:27Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@bsalamat @msau42 @resouer

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2017-12-05T21:58:06Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-12-05T22:31:09Z

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

k8s-ci-robot · 2017-12-05T22:32:19Z

@resouer: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`b3bb74e`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

…7-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #56577 Cherry pick of #56577 on release-1.8. #56577: Fix PV counter predicate in eclass fixes: #54370

resouer added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Nov 29, 2017

resouer assigned bsalamat and msau42 Nov 29, 2017

resouer requested review from foxish, bsalamat and msau42 November 29, 2017 14:47

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 29, 2017

foxish reviewed Nov 29, 2017

View reviewed changes

resouer force-pushed the fix-eclass-pvc branch 2 times, most recently from b746d6d to 2932f4c Compare November 30, 2017 10:28

msau42 reviewed Nov 30, 2017

View reviewed changes

resouer force-pushed the fix-eclass-pvc branch from 2932f4c to cdb1295 Compare December 1, 2017 10:10

resouer force-pushed the fix-eclass-pvc branch from cdb1295 to dcfd861 Compare December 1, 2017 10:15

msau42 reviewed Dec 1, 2017

View reviewed changes

resouer force-pushed the fix-eclass-pvc branch from dcfd861 to 8fbc5f2 Compare December 1, 2017 16:22

bsalamat reviewed Dec 1, 2017

View reviewed changes

resouer force-pushed the fix-eclass-pvc branch from 8fbc5f2 to 055b3a5 Compare December 2, 2017 10:48

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 2, 2017

Harry Zhang added 3 commits December 2, 2017 22:24

Fix PV counter predicate in eclass

af243f4

Add pvc as part of equivalence hash

e4055c0

Use factory to generat get equivalence pod func

Update generated bazel

b3bb74e

resouer force-pushed the fix-eclass-pvc branch from 055b3a5 to b3bb74e Compare December 2, 2017 14:24

msau42 reviewed Dec 4, 2017

View reviewed changes

bsalamat reviewed Dec 4, 2017

View reviewed changes

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

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

saad-ali added this to the v1.9 milestone Dec 5, 2017

saad-ali added status/approved-for-milestone labels Dec 5, 2017

k8s-github-robot added the milestone/incomplete-labels label Dec 5, 2017

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 5, 2017

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 5, 2017

k8s-github-robot removed the milestone/incomplete-labels label Dec 5, 2017

k8s-github-robot merged commit 305d644 into kubernetes:master Dec 5, 2017

msau42 mentioned this pull request Dec 5, 2017

Pod scheduling not respecting PVC zone (NoVolumeZoneConflict) #54370

Closed

jberkus mentioned this pull request Dec 6, 2017

[1.9] Issue Burndown kubernetes/sig-release#38

Closed

resouer mentioned this pull request Dec 8, 2017

Automated cherry pick of #56577 #56966

Merged

msau42 mentioned this pull request Dec 12, 2017

Re-enable local e2e statefulset test kubernetes/test-infra#5901

Merged

resouer deleted the fix-eclass-pvc branch December 12, 2017 17:35

This was referenced Jan 11, 2018

Move equivalence class based scheduling to beta kubernetes/enhancements#537

Closed

Move equivalence class based scheduling to beta #58222

Closed

resouer mentioned this pull request Jan 22, 2018

Update design doc with latest implementation kubernetes/community#1641

Closed

This pull request was closed.

Add pvc as part of equivalence hash #56577

Add pvc as part of equivalence hash #56577

Conversation

resouer commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 left a comment

Choose a reason for hiding this comment

resouer commented Dec 1, 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

resouer Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Dec 1, 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

resouer commented Dec 2, 2017

resouer commented Dec 2, 2017 • edited Loading

msau42 commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Dec 5, 2017

k8s-github-robot commented Dec 5, 2017

msau42 commented Dec 5, 2017

msau42 commented Dec 5, 2017

k8s-github-robot commented Dec 5, 2017

k8s-github-robot commented Dec 5, 2017

k8s-github-robot commented Dec 5, 2017

k8s-ci-robot commented Dec 5, 2017 • edited Loading

resouer commented Nov 29, 2017 •

edited

Loading

resouer Nov 30, 2017 •

edited

Loading

resouer commented Dec 1, 2017 •

edited

Loading

resouer Dec 1, 2017 •

edited

Loading

resouer Dec 1, 2017 •

edited

Loading

resouer commented Dec 2, 2017 •

edited

Loading

k8s-ci-robot commented Dec 5, 2017 •

edited

Loading