Consolidate extended resources and hugepages in Scheduler #52137

aveshagarwal · 2017-09-08T01:39:25Z

@bsalamat @derekwaynecarr @sjenning @kubernetes/sig-scheduling-pr-reviews

sjenning · 2017-09-08T03:00:22Z

vishh · 2017-09-08T03:14:55Z

pkg/api/helper/helpers.go

-// default namespace, or it has the opaque integer resource prefix.
-func IsExtendedResourceName(name api.ResourceName) bool {
+// IsGenericResourceName returns true if the resource name is not in the
+// default namespace, or it has the opaque integer or hugepages resource prefix.


I don't like this refactoring.
Extended resources != Integer resources in the long run.
I suspect what you are looking for is IsIntegerResource which I'd recommend composing

vishh · 2017-09-08T03:15:58Z

pkg/api/v1/helper/helpers.go

@@ -29,11 +29,11 @@ import (
 	"k8s.io/kubernetes/pkg/api/helper"
 )

-// IsExtendedResourceName returns true if the resource name is not in the
+// IsGenericResourceName returns true if the resource name is not in the


What is a generic resource? Instead have an integer resource list for first class resources and use that.

+1, integer resource predicate can match all extended resources (for now) and the integral first-class resources including hugepages.

bsalamat · 2017-09-08T04:47:23Z

@ConnorDoyle was thinking about a more generic resource type that could possibly cover all the existing resource types and let us add new ones without the need to change scheduler's code. That's obviously a larger change and requires a design doc first.

This PR is OK and fixes the duplication of code introduced when adding HugePages in the scheduler. The only issue is that I am not sure if "GenericResource" is a good name.

derekwaynecarr · 2017-09-12T15:03:55Z

I suggested this name, I am not married to the name, but I am not sure its worth a lot of investment to bikeshed around it versus remove the code duplication. Calling extended resources + native resources that require no special handling felt generic to me.

vishh · 2017-09-12T18:39:51Z

+1 for avoiding code duplication. My only ask is to keep the separation between first class and extended resources explicit. It's OK to have an additional wrapper that helps avoid code duplication.

aveshagarwal · 2017-09-13T17:24:37Z

My only ask is to keep the separation between first class and extended resources explicit.

Just to clarify which one is true: we do not want to combine HugePage with Extended resources? Or we want to combine Extended resources with HugePages but want to go with some other name than GenericResources? Or we want to combine Extended resources with HugePages but want to continue with the name Extended Resources?

Because if the first one, that means, we would have to continue with HugePage field like this: https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/scheduler/schedulercache/node_info.go#L75 and I only see marginal improvements even w.r.t code duplication.

bsalamat · 2017-09-13T19:53:58Z

As far as scheduler is concerned, I'd prefer combining ExtendedResources with HugePages and call them either ExtendedResources or ScalarResources or IntegerResources. I am not sure what are the implications of this approach on the node side though. Derek and Vish should comment on that.

ConnorDoyle · 2017-09-13T21:02:29Z

I am wary of combining the concept of ExtendedResources with a subset of first-class resources in the code. This is mainly because Extended Resources are a documented concept and contributors who have read those docs will have trouble if they are conflated in the code.

How about this plan of action (forgive if restating ground already covered):

Retain the existing IsExtendedResourceName() predicate.
Add an IsIntegerResourceName() predicate, which returns IsExtendedResourceName() || IsHugePageResourceName() for now.
Test IsIntegerResourceName() in the scheduler predicates in place of IsExtendedResourceName().
Update the various uses of the ExtendedResources map in scheduler cache types to IntegerResources.

In the future, more unification could be possible to moving some more of the first-class integral resources under the IsIntegerResourceName() predicate and removing the special-case code paths for those resource names. I suggest leaving this as a follow-up after this PR.

bsalamat · 2017-09-13T21:09:57Z

@ConnorDoyle SGTM

aveshagarwal · 2017-09-14T15:39:27Z

@ConnorDoyle Currently IsIntegerResourceName includes standard integer resources and extended resources as shown here: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/helper/helpers.go#L264

So just using IsIntegerResourceName could cause conflicts in Scheduler for resources (like Pods). So I am suggesting to add IsNonStandardIntegerResources as follows:

func IsIntegerResourceName(str string) bool {
        return integerResources.Has(str) || IsNonStandardIntegerResources(api.ResourceName(str))
}

func IsNonStandardIntegerResources(str) bool {
IsExtendedResourceName(api.ResourceName(str)) || IsHugePageResourceName(api.ResourceName(str))
}

So that this way IsNonStandardIntegerResources could be used in Scheduler without causing any unintentional issues.

ConnorDoyle · 2017-09-14T15:54:06Z

OK. It seems weird to call hugepages "nonstandard" once they achieve GA but please don't block on this naming nit.

aveshagarwal · 2017-09-14T16:26:43Z

OK. It seems weird to call hugepages "nonstandard" once they achieve GA but please don't block on this naming nit.

Are you guys @bsalamat @derekwaynecarr @vishh ok with this? Or do we still want to go with other names(something like @bsalamat's suggestion of ScalarResources looks good too)? Let me know as I'd prefer to avoid further iterations on this? Or if I dont get any response, I'll go ahead with IsNonStandardIntegerResources.

bsalamat · 2017-09-14T18:03:02Z

@aveshagarwal It is unfortunate that cluster objects (pods, configmaps, etc.) are called "resource" in K8s. If we plan to expand the concept and add other resources, like memory, to it in the future, I would suggest using ScalarResource. Otherwise, NonStandardResource is fine for lack of a better name.

vishh · 2017-09-14T21:37:08Z

ScalarResources or IntegerResources sounds better than NonStandardResources personally.

…

On Thu, Sep 14, 2017 at 11:04 AM, Bobby (Babak) Salamat < ***@***.***> wrote: @aveshagarwal <https://github.com/aveshagarwal> It is unfortunate that cluster objects (pods, configmaps, etc.) are called "resource" in K8s. If we plan to expand the concept and add other resources, like memory, to it in the future, I would suggest using ScalarResource. Otherwise, NonStandardResource is fine for lack of a better name. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKOilVUN84vyYEDI_krpSfqby-82Eks5siWq5gaJpZM4PQlt-> .

aveshagarwal · 2017-09-15T14:49:47Z

I have updated this PR to consolidate extended and hugepages resources as scalar resources. PTAL.

@sjenning @derekwaynecarr can you check the bug fix commit to make sure its correct?

aveshagarwal · 2017-09-15T14:53:32Z

/test pull-kubernetes-e2e-gce-bazel

aveshagarwal · 2017-09-15T16:23:15Z

/test pull-kubernetes-e2e-gce-bazel
/test pull-kubernetes-e2e-gce-etcd3

aveshagarwal · 2017-09-15T19:55:11Z

/test pull-kubernetes-e2e-gce-bazel
/test pull-kubernetes-e2e-gce-etcd3

aveshagarwal · 2017-09-15T20:38:29Z

/release-note-none

This is only visible in scheduler, and is to avoid code duplication introduced due to separate handling of hugepages from extended resources.

bsalamat · 2017-09-15T22:02:54Z

plugin/pkg/scheduler/schedulercache/node_info.go

-	ExtendedResources map[v1.ResourceName]int64
-	HugePages         map[v1.ResourceName]int64
+	AllowedPodNumber int
+	// OIRs and HugePages


I would just write ScalarResources to cover other scalar resources we add in the future.

bsalamat · 2017-09-15T22:05:47Z

plugin/pkg/scheduler/schedulercache/node_info.go

-	if r.ExtendedResources == nil {
-		r.ExtendedResources = map[v1.ResourceName]int64{}
+func (r *Resource) SetScalar(name v1.ResourceName, quantity int64) {
+	// Lazily allocate opaque integer or hugepages resource map.


Similarly, I would just say: // Lazily allocate scalar resource map.

bsalamat · 2017-09-15T22:07:43Z

pkg/api/helper/helpers.go

@@ -265,6 +265,11 @@ func IsIntegerResourceName(str string) bool {
 	return integerResources.Has(str) || IsExtendedResourceName(api.ResourceName(str))
 }

+// Extended and Hugepages resources


s/Hugepages/HugePage/

bsalamat · 2017-09-15T22:09:51Z

pkg/api/helper/helpers.go

@@ -265,6 +265,11 @@ func IsIntegerResourceName(str string) bool {
 	return integerResources.Has(str) || IsExtendedResourceName(api.ResourceName(str))
 }

+// Extended and Hugepages resources
+func IsScalarResourceName(name api.ResourceName) bool {


Do we need this function here?

In my understanding, helper/helpers.go and v1/helper/helpers.go have been kept in sync previously, Isn't that case any more?

They don't look exactly in sync, but if that's the convention, it is fine.

aveshagarwal · 2017-09-18T13:33:34Z

@bsalamat updated PR, PTAL.

bsalamat · 2017-09-18T22:13:08Z

/lgtm
/approve

aveshagarwal · 2017-09-19T13:46:28Z

@lavalamp could you approve this PR? It has go lgtm and approval from @bsalamat.

ConnorDoyle

/lgtm

feiskyer · 2017-09-20T13:41:34Z

/unassign

aveshagarwal · 2017-09-21T13:20:23Z

@smarterclayton can you approve this?

smarterclayton · 2017-09-21T13:57:38Z

/approve
/retest

k8s-github-robot · 2017-09-21T13:57:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorDoyle, aveshagarwal, bsalamat, smarterclayton

Associated issue: 51732

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

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

~~pkg/api/OWNERS~~ [smarterclayton]
~~plugin/pkg/scheduler/OWNERS~~ [bsalamat,smarterclayton]

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-23T14:15:24Z

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

Review the full test history for this PR.

fejta-bot · 2017-09-23T17:24:23Z

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

Review the full test history for this PR.

fejta-bot · 2017-09-23T20:33:24Z

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

Review the full test history for this PR.

k8s-github-robot · 2017-09-24T02:49:53Z

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

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 8, 2017

k8s-github-robot assigned vishh and feiskyer Sep 8, 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 8, 2017

aveshagarwal changed the title ~~Consolidate extended resources (OIRs) and hugepages into genericresources.~~ Consolidate extended resources (OIRs) and hugepages into generic resources. Sep 8, 2017

vishh reviewed Sep 8, 2017

View reviewed changes

aveshagarwal changed the title ~~Consolidate extended resources (OIRs) and hugepages into generic resources.~~ Consolidate extended resources (OIRs) and hugepages in Scheduler Sep 14, 2017

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

Bugfix to check overcommit for hugepages.

3e8a828

aveshagarwal force-pushed the master-scheduler-resources-consolidation branch from 10dcb74 to a898673 Compare September 15, 2017 14:46

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

aveshagarwal changed the title ~~Consolidate extended resources (OIRs) and hugepages in Scheduler~~ Consolidate extended resources and hugepages in Scheduler Sep 15, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 15, 2017

bsalamat reviewed Sep 15, 2017

View reviewed changes

Consolidate extended resources and hugepages in Scheduler.

ae05a6d

aveshagarwal force-pushed the master-scheduler-resources-consolidation branch from a898673 to ae05a6d Compare September 18, 2017 13:32

ConnorDoyle approved these changes Sep 19, 2017

View reviewed changes

k8s-ci-robot assigned ConnorDoyle Sep 19, 2017

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

k8s-ci-robot unassigned feiskyer Sep 20, 2017

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

k8s-github-robot merged commit 6e9012c into kubernetes:master Sep 24, 2017

aveshagarwal deleted the master-scheduler-resources-consolidation branch August 3, 2018 14:23

Consolidate extended resources and hugepages in Scheduler #52137

Consolidate extended resources and hugepages in Scheduler #52137

Conversation

aveshagarwal commented Sep 8, 2017

sjenning 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

bsalamat commented Sep 8, 2017

derekwaynecarr commented Sep 12, 2017

vishh commented Sep 12, 2017

aveshagarwal commented Sep 13, 2017

bsalamat commented Sep 13, 2017

ConnorDoyle commented Sep 13, 2017 • edited Loading

bsalamat commented Sep 13, 2017

aveshagarwal commented Sep 14, 2017

ConnorDoyle commented Sep 14, 2017

aveshagarwal commented Sep 14, 2017

bsalamat commented Sep 14, 2017

vishh commented Sep 14, 2017 via email

aveshagarwal commented Sep 15, 2017

aveshagarwal commented Sep 15, 2017

aveshagarwal commented Sep 15, 2017

aveshagarwal commented Sep 15, 2017

aveshagarwal commented Sep 15, 2017

bsalamat Sep 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aveshagarwal commented Sep 18, 2017

bsalamat commented Sep 18, 2017

aveshagarwal commented Sep 19, 2017

ConnorDoyle left a comment

Choose a reason for hiding this comment

feiskyer commented Sep 20, 2017

aveshagarwal commented Sep 21, 2017

smarterclayton commented Sep 21, 2017

k8s-github-robot commented Sep 21, 2017

fejta-bot commented Sep 23, 2017

fejta-bot commented Sep 23, 2017

fejta-bot commented Sep 23, 2017

k8s-github-robot commented Sep 24, 2017

ConnorDoyle commented Sep 13, 2017 •

edited

Loading

bsalamat Sep 15, 2017 •

edited

Loading