From e6db88b12d5c3c398366171f7f5386a3e7d5ee3c Mon Sep 17 00:00:00 2001 From: Rohit Agarwal Date: Wed, 28 Mar 2018 13:09:23 -0700 Subject: [PATCH] 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(). --- pkg/apis/core/helper/helpers.go | 8 +++--- pkg/apis/core/v1/helper/helpers.go | 20 +++++++++----- pkg/apis/core/v1/helper/helpers_test.go | 4 +-- pkg/apis/core/v1/validation/validation.go | 2 +- pkg/apis/core/validation/validation.go | 2 +- pkg/quota/evaluator/core/pods.go | 2 +- .../algorithm/predicates/predicates_test.go | 26 ++++++++++++++++--- 7 files changed, 45 insertions(+), 19 deletions(-) diff --git a/pkg/apis/core/helper/helpers.go b/pkg/apis/core/helper/helpers.go index 96899f1841..539c611b1b 100644 --- a/pkg/apis/core/helper/helpers.go +++ b/pkg/apis/core/helper/helpers.go @@ -153,7 +153,7 @@ func IsStandardContainerResourceName(str string) bool { // to avoid confusion with the convention in quota // 3. it satisfies the rules in IsQualifiedName() after converted into quota resource name func IsExtendedResourceName(name core.ResourceName) bool { - if IsDefaultNamespaceResource(name) || strings.HasPrefix(string(name), core.DefaultResourceRequestsPrefix) { + if IsNativeResource(name) || strings.HasPrefix(string(name), core.DefaultResourceRequestsPrefix) { return false } // Ensure it satisfies the rules in IsQualifiedName() after converted into quota resource name @@ -164,10 +164,10 @@ func IsExtendedResourceName(name core.ResourceName) bool { return true } -// IsDefaultNamespaceResource returns true if the resource name is in the +// IsNativeResource returns true if the resource name is in the // *kubernetes.io/ namespace. Partially-qualified (unprefixed) names are // implicitly in the kubernetes.io/ namespace. -func IsDefaultNamespaceResource(name core.ResourceName) bool { +func IsNativeResource(name core.ResourceName) bool { return !strings.Contains(string(name), "/") || strings.Contains(string(name), core.ResourceDefaultNamespacePrefix) } @@ -177,7 +177,7 @@ var overcommitBlacklist = sets.NewString(string(core.ResourceNvidiaGPU)) // IsOvercommitAllowed returns true if the resource is in the default // namespace and not blacklisted. func IsOvercommitAllowed(name core.ResourceName) bool { - return IsDefaultNamespaceResource(name) && + return IsNativeResource(name) && !IsHugePageResourceName(name) && !overcommitBlacklist.Has(string(name)) } diff --git a/pkg/apis/core/v1/helper/helpers.go b/pkg/apis/core/v1/helper/helpers.go index bb6d9385f9..e52d38195b 100644 --- a/pkg/apis/core/v1/helper/helpers.go +++ b/pkg/apis/core/v1/helper/helpers.go @@ -36,7 +36,7 @@ import ( // to avoid confusion with the convention in quota // 3. it satisfies the rules in IsQualifiedName() after converted into quota resource name func IsExtendedResourceName(name v1.ResourceName) bool { - if IsDefaultNamespaceResource(name) || strings.HasPrefix(string(name), v1.DefaultResourceRequestsPrefix) { + if IsNativeResource(name) || strings.HasPrefix(string(name), v1.DefaultResourceRequestsPrefix) { return false } // Ensure it satisfies the rules in IsQualifiedName() after converted into quota resource name @@ -47,13 +47,18 @@ func IsExtendedResourceName(name v1.ResourceName) bool { return true } -// IsDefaultNamespaceResource returns true if the resource name is in the +// IsPrefixedNativeResource returns true if the resource name is in the +// *kubernetes.io/ namespace. +func IsPrefixedNativeResource(name v1.ResourceName) bool { + return strings.Contains(string(name), v1.ResourceDefaultNamespacePrefix) +} + +// IsNativeResource returns true if the resource name is in the // *kubernetes.io/ namespace. Partially-qualified (unprefixed) names are // implicitly in the kubernetes.io/ namespace. -func IsDefaultNamespaceResource(name v1.ResourceName) bool { +func IsNativeResource(name v1.ResourceName) bool { return !strings.Contains(string(name), "/") || - strings.Contains(string(name), v1.ResourceDefaultNamespacePrefix) - + IsPrefixedNativeResource(name) } // IsHugePageResourceName returns true if the resource name has the huge page @@ -85,14 +90,15 @@ var overcommitBlacklist = sets.NewString(string(v1.ResourceNvidiaGPU)) // IsOvercommitAllowed returns true if the resource is in the default // namespace and not blacklisted and is not hugepages. func IsOvercommitAllowed(name v1.ResourceName) bool { - return IsDefaultNamespaceResource(name) && + return IsNativeResource(name) && !IsHugePageResourceName(name) && !overcommitBlacklist.Has(string(name)) } // Extended and Hugepages resources func IsScalarResourceName(name v1.ResourceName) bool { - return IsExtendedResourceName(name) || IsHugePageResourceName(name) + return IsExtendedResourceName(name) || IsHugePageResourceName(name) || + IsPrefixedNativeResource(name) } // this function aims to check if the service's ClusterIP is set or not diff --git a/pkg/apis/core/v1/helper/helpers_test.go b/pkg/apis/core/v1/helper/helpers_test.go index be1ecfaf93..d632821b28 100644 --- a/pkg/apis/core/v1/helper/helpers_test.go +++ b/pkg/apis/core/v1/helper/helpers_test.go @@ -28,7 +28,7 @@ import ( "k8s.io/apimachinery/pkg/labels" ) -func TestIsDefaultNamespaceResource(t *testing.T) { +func TestIsNativeResource(t *testing.T) { testCases := []struct { resourceName v1.ResourceName expectVal bool @@ -58,7 +58,7 @@ func TestIsDefaultNamespaceResource(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("resourceName input=%s, expected value=%v", tc.resourceName, tc.expectVal), func(t *testing.T) { t.Parallel() - v := IsDefaultNamespaceResource(tc.resourceName) + v := IsNativeResource(tc.resourceName) if v != tc.expectVal { t.Errorf("Got %v but expected %v", v, tc.expectVal) } diff --git a/pkg/apis/core/v1/validation/validation.go b/pkg/apis/core/v1/validation/validation.go index cba05040c3..c9d67fc1aa 100644 --- a/pkg/apis/core/v1/validation/validation.go +++ b/pkg/apis/core/v1/validation/validation.go @@ -75,7 +75,7 @@ func validateContainerResourceName(value string, fldPath *field.Path) field.Erro if !helper.IsStandardContainerResourceName(value) { return append(allErrs, field.Invalid(fldPath, value, "must be a standard resource for containers")) } - } else if !v1helper.IsDefaultNamespaceResource(v1.ResourceName(value)) { + } else if !v1helper.IsNativeResource(v1.ResourceName(value)) { if !v1helper.IsExtendedResourceName(v1.ResourceName(value)) { return append(allErrs, field.Invalid(fldPath, value, "doesn't follow extended resource name standard")) } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5b54d6fe64..5d82e6d5d0 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4140,7 +4140,7 @@ func validateContainerResourceName(value string, fldPath *field.Path) field.Erro if !helper.IsStandardContainerResourceName(value) { return append(allErrs, field.Invalid(fldPath, value, "must be a standard resource for containers")) } - } else if !helper.IsDefaultNamespaceResource(core.ResourceName(value)) { + } else if !helper.IsNativeResource(core.ResourceName(value)) { if !helper.IsExtendedResourceName(core.ResourceName(value)) { return append(allErrs, field.Invalid(fldPath, value, "doesn't follow extended resource name standard")) } diff --git a/pkg/quota/evaluator/core/pods.go b/pkg/quota/evaluator/core/pods.go index a2d94c96c7..06aa59e393 100644 --- a/pkg/quota/evaluator/core/pods.go +++ b/pkg/quota/evaluator/core/pods.go @@ -81,7 +81,7 @@ func maskResourceWithPrefix(resource api.ResourceName, prefix string) api.Resour func isExtendedResourceNameForQuota(name api.ResourceName) bool { // As overcommit is not supported by extended resources for now, // only quota objects in format of "requests.resourceName" is allowed. - return !helper.IsDefaultNamespaceResource(name) && strings.HasPrefix(string(name), api.DefaultResourceRequestsPrefix) + return !helper.IsNativeResource(name) && strings.HasPrefix(string(name), api.DefaultResourceRequestsPrefix) } // NOTE: it was a mistake, but if a quota tracks cpu or memory related resources, diff --git a/pkg/scheduler/algorithm/predicates/predicates_test.go b/pkg/scheduler/algorithm/predicates/predicates_test.go index 7f91a3b4b6..736c864650 100644 --- a/pkg/scheduler/algorithm/predicates/predicates_test.go +++ b/pkg/scheduler/algorithm/predicates/predicates_test.go @@ -37,9 +37,11 @@ import ( ) var ( - extendedResourceA = v1.ResourceName("example.com/aaa") - extendedResourceB = v1.ResourceName("example.com/bbb") - hugePageResourceA = v1helper.HugePageResourceName(resource.MustParse("2Mi")) + extendedResourceA = v1.ResourceName("example.com/aaa") + extendedResourceB = v1.ResourceName("example.com/bbb") + kubernetesIOResourceA = v1.ResourceName("kubernetes.io/something") + kubernetesIOResourceB = v1.ResourceName("subdomain.kubernetes.io/something") + hugePageResourceA = v1helper.HugePageResourceName(resource.MustParse("2Mi")) ) func makeResources(milliCPU, memory, nvidiaGPUs, pods, extendedA, storage, hugePageA int64) v1.NodeResources { @@ -297,6 +299,24 @@ func TestPodFitsResources(t *testing.T) { test: "extended resource allocatable enforced for unknown resource for init container", reasons: []algorithm.PredicateFailureReason{NewInsufficientResourceError(extendedResourceB, 1, 0, 0)}, }, + { + pod: newResourcePod( + schedulercache.Resource{MilliCPU: 1, Memory: 1, ScalarResources: map[v1.ResourceName]int64{kubernetesIOResourceA: 10}}), + nodeInfo: schedulercache.NewNodeInfo( + newResourcePod(schedulercache.Resource{MilliCPU: 0, Memory: 0})), + fits: false, + test: "kubernetes.io resource capacity enforced", + reasons: []algorithm.PredicateFailureReason{NewInsufficientResourceError(kubernetesIOResourceA, 10, 0, 0)}, + }, + { + pod: newResourceInitPod(newResourcePod(schedulercache.Resource{}), + schedulercache.Resource{MilliCPU: 1, Memory: 1, ScalarResources: map[v1.ResourceName]int64{kubernetesIOResourceB: 10}}), + nodeInfo: schedulercache.NewNodeInfo( + newResourcePod(schedulercache.Resource{MilliCPU: 0, Memory: 0})), + fits: false, + test: "kubernetes.io resource capacity enforced for init container", + reasons: []algorithm.PredicateFailureReason{NewInsufficientResourceError(kubernetesIOResourceB, 10, 0, 0)}, + }, { pod: newResourcePod( schedulercache.Resource{MilliCPU: 1, Memory: 1, ScalarResources: map[v1.ResourceName]int64{hugePageResourceA: 10}}),