kube-scheduler: Support extender managed extended resources in kube-scheduler #60332

yguo0905 · 2018-02-23T23:08:42Z

What this PR does / why we need it:

This is the continuation of #58851.

This PR adds new extender configurations in scheduler policy config.
- A set of extended resource names can be specified in an extender config. They are the resources that are managed by the extender. The scheduler will only send pods to the extender if the pod requests at least one of the extended resources in the set.
- An IgnoredByScheduler flag can be set along with each of such resources. If this flag is set to true, the scheduler will not check the resource in the PodFitsResources predicate.
This PR also changes the default behavior of the PodFitsResources predicate. Now, by default, PodFitsResources will ignore the extended resources that are not in node status. This is required to support extender managed extended resources (including cluster-level resources) on node. Note that in kube-scheduler we override the default behavior by not ignoring such missing extended resources.

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 #53616 #58850

Special notes for your reviewer:

Release note:

Support extender managed extended resources in kube-scheduler

yguo0905 · 2018-02-23T23:09:16Z

/assign @bsalamat @vishh

yguo0905 · 2018-02-23T23:46:50Z

/retest

yguo0905 · 2018-02-24T00:39:34Z

/retest

vishh · 2018-02-24T00:46:25Z

/lgtm

yguo0905 · 2018-02-24T00:56:49Z

pkg/scheduler/algorithm/predicates/metadata.go

+// PredicateMetadataProducer that creates predicate metadata with the provided
+// ignoredExtendedResources.
+func RegisterPredicateMetadataProducerWithIgnoredExtendedResources(ignoredExtendedResources sets.String) {
+	RegisterPredicateMetadataProducer("PredicateWithIgnoredExtendedResources", func(pm *predicateMetadata) {


I changed the predicate name from PodFitsResources to PredicateWithIgnoredExtendedResources.

IIUC, the "precomp" function that sets the ignoredExtendedResources will override the one for the same predicate created here:

kubernetes/pkg/scheduler/factory/plugins.go

Line 217 in 01bd3c4

predicates.RegisterPredicateMetadataProducer(policy.Name, precomputationFunction)

Is this the case? And I don't see why predicateMetadataProducers is not a slice here:

kubernetes/pkg/scheduler/algorithm/predicates/metadata.go

Line 65 in 01bd3c4

var predicateMetadataProducers = make(map[string]PredicateMetadataProducer)

bsalamat · 2018-02-25T02:02:37Z

pkg/scheduler/algorithm/predicates/metadata.go

@@ -95,6 +134,7 @@ func (pfactory *PredicateMetadataFactory) GetMetadata(pod *v1.Pod, nodeNameToInf
 		podRequest:                GetResourceRequest(pod),
 		podPorts:                  schedutil.GetContainerPorts(pod),
 		matchingAntiAffinityTerms: matchingTerms,
+		ignoredExtendedResources:  sets.NewString(),


Why is this needed? Can't it be default (nil)?

Not needed. Will remove this.

bsalamat · 2018-02-25T02:07:31Z

pkg/scheduler/algorithm/predicates/predicates.go

+	// Missing extended resources will be ignored by default unless specified
+	// in predicate metadata. Note that kube-scheduler sets this flag to false
+	// in predicate metadata.
+	ignoreMissingExtendedResources := true


The value of this variable is confusing. I think default value of "true" is a backward incompatible change. Besides, the value of predicateMeta.ignoreMissingExtendedResources is false by default. So, I guess it makes sense to make the default value false.

Thanks for looking into this over the weekend!

Yes, this is a backward incompatible change but I haven't come up with a promising solution yet.

In PodFitsResources, we cast algorithm.PredicateMetadata to predicate.predicateMetadata and then use the metadata, including ignoreMissingExtendedResources in this PR.

kubernetes/pkg/scheduler/algorithm/predicates/predicates.go

Lines 716 to 717 in 0394ffb

if predicateMeta, ok := meta.(*predicateMetadata); ok {

podRequest = predicateMeta.podRequest

This is how the predicate is invoked in pod admission on node.

kubernetes/pkg/kubelet/lifecycle/predicate.go

Line 80 in 0394ffb

fit, reasons, err := predicates.GeneralPredicates(pod, nil, nodeInfo)

Here we pass nil as the predicate metadata, which means we use the defaults in pod admission on node. That's why I set ignoreMissingExtendedResources default to true.

Ideally, kubelet should create a predicate metadata with ignoreMissingExtendedResources set to true and pass it to the predicate. But this is not possible because of the type cast in the predicate function.

I think the type cast should be removed, and the metadata should be accessed via the algorithm.PredicateMetadata interface. kube-scheduler/node/clusterautoscalar can provide their own metadata implementation. But this will be a larger change. Maybe I'm missing some thing. WDYT?

I see. So, for now please add a comment and explain that this default value is chosen so that the kubelet code works fine and add the details.
My only concern right now is that this is backward incompatible from the kubelet side. It is however, backward compatible on the scheduler side, because if scheduler's policy config does not exists or misses this field, the default value of the field will be false and overrides the default value provided here.

Done.

Besides scheduler and kubelet, cluster autoscaler and damonset controller also use this predicate. But I don't see any negative impacts from this change. This is also the behavior before 1.8 #53616.

@vishh @jiayingz

I created algorithm.PredicateMetadataProducer in kubelet using predicates.NewPredicateMetadataFactory().

It turned out that NewPredicateMetadataFactory's podLister argument is not used at all so I can just use this function in kubelet.

Now, the default value of predicateMeta.ignoreMissingExtendedResources is false.

bsalamat · 2018-02-25T03:18:06Z

pkg/scheduler/factory/factory_test.go

+		podName   string
+		extenders []algorithm.SchedulerExtender
+
+		expectedBinderType string


Why don't you check the pointer to the extender instead of its string type?

I don't have access to the pointer to the default binder.

Ah, right. Never mind.

bsalamat · 2018-02-25T04:37:57Z

pkg/scheduler/scheduler_test.go

+		}, {
+			sendPod:          podWithID(testBindingPodName, ""),
+			algo:             mockScheduler{testNode.Name, nil},
+			expectBind:       testBinding, // testBinding has a name different than testBindingPodName


I am confused. Why is the name different?

This is confusing and not that useful. I already added full tests in TestGetBinderFunc in pkg/scheduler/factory/factory_test.go so this should have been removed.

yguo0905 · 2018-02-25T18:58:33Z

/hold cancel

bsalamat · 2018-02-26T19:18:38Z

pkg/scheduler/algorithm/predicates/predicates.go

+	// Missing extended resources will be ignored by default unless a predicate
+	// metadata is provided.
+	//
+	// This predicate is used by scheduler, kubelet, cluster autoscaler and


@dchen1107 @mwielgus
This is a backward incompatible change in kubelet and autoscaler, but it applies only to Pods that have extended resources missing on the node. Do you guys approve such a change?

yguo0905 · 2018-02-27T00:24:49Z

pkg/kubelet/kubelet_node_status.go

@@ -606,8 +606,16 @@ func (kl *Kubelet) setNodeStatusMachineInfo(node *v1.Node) {
 		}

 		for _, removedResource := range removedDevicePlugins {
-			glog.V(2).Infof("Remove capacity for %s", removedResource)
-			delete(node.Status.Capacity, v1.ResourceName(removedResource))
+			glog.V(2).Infof("Set capacity for %s to 0 on device removal", removedResource)


@jiayingz, please take a look.

should allocatable also be set to 0 ?

Allocatable will be set to capacity at line 644.

jiayingz · 2018-02-27T00:58:01Z

/lgtm for the device plugin related change.

yguo0905 · 2018-02-27T01:12:47Z

@bsalamat and @vishh. I changed the default behavior for cluster autoscaler back to what it does for now.

I keep ignoreMissingExtendedResources as a flag rather than a list of resources that are required. I think ff we need to support new extended resources in cluster autoscaler, it's very likely that we will add the support in its cloud provider on a case by case basis.

yguo0905 · 2018-02-27T21:09:54Z

Per discussion with @vishh offline, I removed the ignoreMissingExtendedResource flag. Instead, we remove the requests of the extended resources from pod if the resources are missing from node status in kubelet.

vishh · 2018-02-27T22:55:27Z

pkg/kubelet/lifecycle/predicate.go

+
+	// Remove the requests of the extended resources that are missing in the
+	// node info. This is required to support cluster-level resources, which
+	// are extended resources unknown to nodes.


There is an issue where if a pod was manually (or incorrectly) bound to a node where a node level extended resource it requires is not found, then kubelet will not fail admission. We should document this as known issue and fix it with resource APIs.

vishh · 2018-02-27T22:58:05Z

pkg/scheduler/algorithmprovider/defaults/defaults.go

@@ -39,7 +39,7 @@ func init() {
 	// Register functions that extract metadata used by predicates and priorities computations.
 	factory.RegisterPredicateMetadataProducerFactory(
 		func(args factory.PluginFactoryArgs) algorithm.PredicateMetadataProducer {
-			return predicates.NewPredicateMetadataFactory()
+			return predicates.NewPredicateMetadataFactory(args.PodLister)


Is that argument even necessary? @bsalamat can this be removed?

I think this can be removed. If yes, I can do that in a separate PR in 1.11.

vishh · 2018-02-27T22:59:49Z

/lgtm
/approve
@yguo0905 please update public documentation with the repercussions of this PR. It enables cluster level resources, but with a few caveats (lack of support for daemonsets, requires scheduler extender config, etc.)

jiayingz · 2018-02-27T23:03:32Z

pkg/kubelet/lifecycle/predicate_test.go

+			),
+			expectedPod: makeTestPod(
+				v1.ResourceList{},
+				v1.ResourceList{"foo.com/bar": quantity},


Hmm, shouldn't this resource request be removed?

The request is in Limits which is skipped in this logic. I will refine this test once the overall design is LGTM'd.

Oh I see. So this is limit and we leave limit unchanged. For extended resources, we actually require limits to equal to requests and in devicemanager, we actually examine resource Limits during resource allocation. Can we make sure to also remove limits for non-existing extended resources?

Never mind. Whether we have Limit or not doesn't affect GeneralPredicate evaluation.

jiayingz · 2018-02-27T23:27:00Z

/lgtm

bsalamat · 2018-02-27T23:36:36Z

/lgtm

vishh · 2018-02-28T01:11:29Z

Can you squash the commits @yguo0905 ?

/lgtm

Co-authored-by: Yang Guo <ygg@google.com> Co-authored-by: Chun Chen <chenchun.feed@gmail.com>

vishh · 2018-02-28T01:44:08Z

/lgtm

k8s-ci-robot · 2018-02-28T01:44:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, jiayingz, vishh, yguo0905

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:

~~pkg/kubelet/OWNERS~~ [vishh]
~~pkg/scheduler/OWNERS~~ [bsalamat]
~~test/integration/scheduler/OWNERS~~ [bsalamat,vishh]

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

k8s-github-robot · 2018-02-28T09:12:23Z

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

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 Feb 23, 2018

k8s-ci-robot requested review from k82cn and wojtek-t February 23, 2018 23:08

k8s-ci-robot assigned bsalamat and vishh Feb 23, 2018

yguo0905 mentioned this pull request Feb 23, 2018

Add InterestedResources as a pod filter based on pods' requested resources to extender config #58851

Closed

vishh added this to the v1.10 milestone Feb 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2018

yguo0905 commented Feb 24, 2018

View reviewed changes

yguo0905 changed the title ~~[Scheduler] Support extender managed extended resources~~ [WIP] kube-scheduler: Support extender managed extended resources Feb 24, 2018

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

yguo0905 mentioned this pull request Feb 24, 2018

Support cluster level resources in the node #59835

Closed

yguo0905 force-pushed the sched branch from 424cad7 to fbe6654 Compare February 25, 2018 01:04

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2018

yguo0905 force-pushed the sched branch from fbe6654 to ae891d9 Compare February 25, 2018 03:48

bsalamat reviewed Feb 25, 2018

View reviewed changes

yguo0905 force-pushed the sched branch from ae891d9 to 98f7563 Compare February 25, 2018 07:02

yguo0905 changed the title ~~[WIP] kube-scheduler: Support extender managed extended resources~~ kube-scheduler: Support extender managed extended resources in kube-scheduler Feb 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 Feb 25, 2018

vikaschoudhary16 mentioned this pull request Feb 26, 2018

Modify endpoint unregister handling to set all devices unhealthy #60296

Closed

yguo0905 force-pushed the sched branch 2 times, most recently from 06d7769 to 33cb03b Compare February 26, 2018 17:55

bsalamat reviewed Feb 26, 2018

View reviewed changes

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 27, 2018

yguo0905 commented Feb 27, 2018

View reviewed changes

yguo0905 force-pushed the sched branch from 1a55f8d to 770bfa9 Compare February 27, 2018 21:07

vishh reviewed Feb 27, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

jiayingz reviewed Feb 27, 2018

View reviewed changes

k8s-ci-robot assigned jiayingz Feb 27, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2018

yguo0905 force-pushed the sched branch from 770bfa9 to 7918be1 Compare February 27, 2018 23:51

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2018

Support cluster-level extended resources in kubelet and kube-scheduler

8d88050

Co-authored-by: Yang Guo <ygg@google.com> Co-authored-by: Chun Chen <chenchun.feed@gmail.com>

yguo0905 force-pushed the sched branch from 7918be1 to 8d88050 Compare February 28, 2018 01:25

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2018

jdumars added the status/approved-for-milestone label Feb 28, 2018

k8s-github-robot merged commit 1aee9fd into kubernetes:master Feb 28, 2018

jiayingz mentioned this pull request Feb 28, 2018

Device Plugin failure handling in kubelet is racy #60176

Closed

yguo0905 mentioned this pull request Mar 16, 2018

Update Extended Resource doc wrt cluster-level resources kubernetes/website#7759

Merged

	if predicateMeta, ok := meta.(*predicateMetadata); ok {
	podRequest = predicateMeta.podRequest

kube-scheduler: Support extender managed extended resources in kube-scheduler #60332

kube-scheduler: Support extender managed extended resources in kube-scheduler #60332

Conversation

yguo0905 commented Feb 23, 2018 • edited Loading

yguo0905 commented Feb 23, 2018

yguo0905 commented Feb 23, 2018

yguo0905 commented Feb 24, 2018

vishh commented Feb 24, 2018

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

yguo0905 Feb 27, 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

yguo0905 commented Feb 25, 2018

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

jiayingz commented Feb 27, 2018

yguo0905 commented Feb 27, 2018

yguo0905 commented Feb 27, 2018

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

vishh commented Feb 27, 2018

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

jiayingz commented Feb 27, 2018

bsalamat commented Feb 27, 2018

vishh commented Feb 28, 2018

vishh commented Feb 28, 2018

k8s-ci-robot commented Feb 28, 2018

k8s-github-robot commented Feb 28, 2018

yguo0905 commented Feb 23, 2018 •

edited

Loading

yguo0905 Feb 27, 2018 •

edited

Loading