New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resources prefixed with *kubernetes.io/ should remain unscheduled if they are not exposed on the node. #61860

Merged
merged 1 commit into from Apr 3, 2018

Conversation

@mindprince
Member

mindprince commented Mar 28, 2018

Currently, resources prefixed with *kubernetes.io/ get scheduled to any
node whether it's exposing that resource or not.

On the other hand, resources prefixed with someother.domain/ don't get
scheduled to a node until that node is exposing that resource (or if the
resource is ignored because of scheduler extender).

This commit brings the behavior of *kubernetes.io/ prefixed resources in
line with other extended resources and they will remain unscheduled
until some node exposes these resources.

Fixes #50658

Pods requesting resources prefixed with `*kubernetes.io` will remain unscheduled if there are no nodes exposing that resource.

/sig scheduling
/assign jiayingz vishh bsalamat ConnorDoyle k82cn

@bsalamat

minor comment, otherwise LGTM.

@@ -47,13 +47,18 @@ func IsExtendedResourceName(name v1.ResourceName) bool {
return true
}
// IsDefaultnamespaceContainingResource returns true if the resource name is in the
// *kubernetes.io/ namespace.
func IsDefaultNamespaceContainingResource(name v1.ResourceName) bool {

This comment has been minimized.

@bsalamat

bsalamat Mar 28, 2018

Contributor

The name of this function is confusing and the fact that we have used "Namespace" which is a well known concept in Kubernetes causes even more confusion. I would call it something like "IsNativeResource".

This comment has been minimized.

@mindprince

mindprince Mar 28, 2018

Member

I based it on the function below: IsDefaultNamespaceResource90. Also, the prefix variable is called ResourceDefaultNamespacePrefix.

How would you name these two functions? IsNativeResource() seems to make more sense for the function below.

Since the newly added function is very straightforward and seem to not have uses outside of this feel, I don't mind inlining it either.

This comment has been minimized.

@bsalamat

bsalamat Mar 28, 2018

Contributor

I know that this whole "Namespace" thing in the function names existed.
You are right that we should probably use "IsNativeResource" for the function below. This one can be called "IsNativeResourceWithPrefix" or "IsPrefixedNativeResource" or something along these.

This comment has been minimized.

@mindprince

mindprince Mar 29, 2018

Member

Updated. PTAL

@vishh

This comment has been minimized.

Member

vishh commented Mar 28, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 28, 2018

Resources prefixed with *kubernetes.io/ should remain unscheduled if …
…they are not exposed on the node.

Currently, resources prefixed with *kubernetes.io/ get scheduled to any
node whether it's exposing that resource or not.

On the other hand, resources prefixed with someother.domain/ don't get
scheduled to a node until that node is exposing that resource (or if the
resource is ignored because of scheduler extender).

This commit brings the behavior of *kubernetes.io/ prefixed resources in
line with other extended resources and they will remain unscheduled
until some node exposes these resources.

This also includes renaming IsDefaultNamespaceResource() to
IsNativeResource().

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 29, 2018

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Mar 29, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 29, 2018

@mindprince

This comment has been minimized.

Member

mindprince commented Mar 29, 2018

/assign liggitt

@mindprince

This comment has been minimized.

Member

mindprince commented Mar 29, 2018

/retest

@mindprince

This comment has been minimized.

Member

mindprince commented Mar 30, 2018

/assign @thockin

@thockin

This comment has been minimized.

Member

thockin commented Apr 2, 2018

Do we have docs on when a resource would be "bare" vs prefixed with kubernetes ?

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, mindprince, thockin, vishh

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:

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

@mindprince

This comment has been minimized.

Member

mindprince commented Apr 2, 2018

Do we have docs on when a resource would be "bare" vs prefixed with kubernetes ?

The in-tree resources (cpu, memory, ephemeral-storage, hugepages-*) are always supposed to be bare. The docs just use bare resources always when talking about these and don't mention anything about prefixing with kubernetes.io.

When talking about extended resources the docs say:

https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/

Extended Resources are fully-qualified resource names outside the kubernetes.io domain.

https://kubernetes.io/docs/tasks/configure-pod-container/extended-resource/

Extended resources are fully qualified with any domain outside of *.kubernetes.io/.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 3, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 1f69c34 into kubernetes:master Apr 3, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mindprince authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment