You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@yunikorn.apache.org by ch...@apache.org on 2024/06/08 05:53:24 UTC
(yunikorn-k8shim) branch master updated: [YUNIKORN-2631] Support canonical labels for queue/applicationId in Admission Controller (#843)
This is an automated email from the ASF dual-hosted git repository.
chenyulin0719 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-k8shim.git
The following commit(s) were added to refs/heads/master by this push:
new 223e1f3b [YUNIKORN-2631] Support canonical labels for queue/applicationId in Admission Controller (#843)
223e1f3b is described below
commit 223e1f3b69262a379a2f19efd2294cd7597d8f77
Author: Yu-Lin Chen <ch...@apache.org>
AuthorDate: Sat Jun 8 05:50:55 2024 +0000
[YUNIKORN-2631] Support canonical labels for queue/applicationId in Admission Controller (#843)
Add canonical representations when Admission Controller try to patch queue/applicationID to pod
Closes: #843
Signed-off-by: Yu-Lin Chen <ch...@apache.org>
---
pkg/admission/admission_controller.go | 2 +-
pkg/admission/admission_controller_test.go | 32 +++++++++++++++-------
pkg/admission/util.go | 17 ++++++++++--
pkg/admission/util_test.go | 44 +++++++++++++++++++++---------
pkg/common/constants/constants.go | 2 ++
5 files changed, 71 insertions(+), 26 deletions(-)
diff --git a/pkg/admission/admission_controller.go b/pkg/admission/admission_controller.go
index 3b058da6..35c3fecb 100644
--- a/pkg/admission/admission_controller.go
+++ b/pkg/admission/admission_controller.go
@@ -322,7 +322,7 @@ func (c *AdmissionController) processPodUpdate(req *admissionv1.AdmissionRequest
func (c *AdmissionController) shouldProcessAdmissionReview(namespace string, labels map[string]string) bool {
if c.shouldProcessNamespace(namespace) &&
- (labels[constants.LabelApplicationID] != "" || labels[constants.SparkLabelAppID] != "" || c.shouldLabelNamespace(namespace)) {
+ (labels[constants.CanonicalLabelApplicationID] != "" || labels[constants.LabelApplicationID] != "" || labels[constants.SparkLabelAppID] != "" || c.shouldLabelNamespace(namespace)) {
return true
}
diff --git a/pkg/admission/admission_controller_test.go b/pkg/admission/admission_controller_test.go
index 88bf8d6a..e5c66189 100644
--- a/pkg/admission/admission_controller_test.go
+++ b/pkg/admission/admission_controller_test.go
@@ -81,9 +81,11 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 3)
+ assert.Equal(t, len(updatedMap), 5)
assert.Equal(t, updatedMap["random"], "random")
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.default")
+ assert.Equal(t, strings.HasPrefix(updatedMap[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(updatedMap[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("patch info content is not as expected")
@@ -104,8 +106,8 @@ func TestUpdateLabels(t *testing.T) {
UID: "7f5fd6c5d5",
ResourceVersion: "10654",
Labels: map[string]string{
- "random": "random",
- constants.LabelApplicationID: "app-0001",
+ "random": "random",
+ constants.CanonicalLabelApplicationID: "app-0001",
},
},
Spec: v1.PodSpec{},
@@ -117,9 +119,11 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 3)
+ assert.Equal(t, len(updatedMap), 5)
assert.Equal(t, updatedMap["random"], "random")
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.default")
+ assert.Equal(t, updatedMap[constants.CanonicalLabelApplicationID], "app-0001")
assert.Equal(t, updatedMap[constants.LabelApplicationID], "app-0001")
} else {
t.Fatal("patch info content is not as expected")
@@ -140,8 +144,8 @@ func TestUpdateLabels(t *testing.T) {
UID: "7f5fd6c5d5",
ResourceVersion: "10654",
Labels: map[string]string{
- "random": "random",
- constants.LabelQueueName: "root.abc",
+ "random": "random",
+ constants.CanonicalLabelQueueName: "root.abc",
},
},
Spec: v1.PodSpec{},
@@ -154,9 +158,11 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 3)
+ assert.Equal(t, len(updatedMap), 5)
assert.Equal(t, updatedMap["random"], "random")
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.abc")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.abc")
+ assert.Equal(t, strings.HasPrefix(updatedMap[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(updatedMap[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("patch info content is not as expected")
@@ -186,8 +192,10 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 2)
+ assert.Equal(t, len(updatedMap), 4)
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.default")
+ assert.Equal(t, strings.HasPrefix(updatedMap[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(updatedMap[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("patch info content is not as expected")
@@ -214,8 +222,10 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 2)
+ assert.Equal(t, len(updatedMap), 4)
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.default")
+ assert.Equal(t, strings.HasPrefix(updatedMap[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(updatedMap[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("patch info content is not as expected")
@@ -240,8 +250,10 @@ func TestUpdateLabels(t *testing.T) {
assert.Equal(t, patch[0].Op, "add")
assert.Equal(t, patch[0].Path, "/metadata/labels")
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
- assert.Equal(t, len(updatedMap), 2)
+ assert.Equal(t, len(updatedMap), 4)
+ assert.Equal(t, updatedMap[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, updatedMap[constants.LabelQueueName], "root.default")
+ assert.Equal(t, strings.HasPrefix(updatedMap[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(updatedMap[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("patch info content is not as expected")
diff --git a/pkg/admission/util.go b/pkg/admission/util.go
index fe994da7..5276b5b6 100644
--- a/pkg/admission/util.go
+++ b/pkg/admission/util.go
@@ -35,10 +35,11 @@ func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool, de
result[k] = v
}
+ canonicalAppID := utils.GetPodLabelValue(pod, constants.CanonicalLabelApplicationID)
sparkAppID := utils.GetPodLabelValue(pod, constants.SparkLabelAppID)
labelAppID := utils.GetPodLabelValue(pod, constants.LabelApplicationID)
annotationAppID := utils.GetPodAnnotationValue(pod, constants.AnnotationApplicationID)
- if sparkAppID == "" && labelAppID == "" && annotationAppID == "" {
+ if canonicalAppID == "" && sparkAppID == "" && labelAppID == "" && annotationAppID == "" {
// if app id not exist, generate one
// for each namespace, we group unnamed pods to one single app - if GenerateUniqueAppId is not set
// if GenerateUniqueAppId:
@@ -46,19 +47,31 @@ func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool, de
// else
// application ID convention: ${AUTO_GEN_PREFIX}-${NAMESPACE}-${AUTO_GEN_SUFFIX}
generatedID := utils.GenerateApplicationID(namespace, generateUniqueAppIds, string(pod.UID))
+
+ result[constants.CanonicalLabelApplicationID] = generatedID
+ // Deprecated: After 1.7.0, admission controller will only add canonical label if application ID was not set
result[constants.LabelApplicationID] = generatedID
+ } else if canonicalAppID != "" {
+ // Deprecated: Added in 1.6.0 for backward compatibility, in case the prior shim version can't handle canonical label
+ result[constants.LabelApplicationID] = canonicalAppID
}
+ canonicalQueueName := utils.GetPodLabelValue(pod, constants.CanonicalLabelQueueName)
labelQueueName := utils.GetPodLabelValue(pod, constants.LabelQueueName)
annotationQueueName := utils.GetPodAnnotationValue(pod, constants.AnnotationQueueName)
- if labelQueueName == "" && annotationQueueName == "" {
+ if canonicalQueueName == "" && labelQueueName == "" && annotationQueueName == "" {
// if queueName not exist, generate one
// if defaultQueueName is "", skip adding default queue name to the pod labels
if defaultQueueName != "" {
// for undefined configuration, am_conf will add 'root.default' to retain existing behavior
// if a custom name is configured for default queue, it will be used instead of root.default
+ result[constants.CanonicalLabelQueueName] = defaultQueueName
+ // Deprecated: After 1.7.0, admission controller will only add canonical label if queue was not set
result[constants.LabelQueueName] = defaultQueueName
}
+ } else if canonicalQueueName != "" {
+ // Deprecated: Added in 1.6.0 for backward compatibility, in case the prior shim version can't handle canonical label
+ result[constants.LabelQueueName] = canonicalQueueName
}
return result
diff --git a/pkg/admission/util_test.go b/pkg/admission/util_test.go
index fa28c824..2181bd5a 100644
--- a/pkg/admission/util_test.go
+++ b/pkg/admission/util_test.go
@@ -70,8 +70,8 @@ func createTestingPodWithMeta() *v1.Pod {
func createTestingPodWithLabels(appId string, queue string) *v1.Pod {
pod := createTestingPodWithMeta()
- pod.ObjectMeta.Labels[constants.LabelApplicationID] = appId
- pod.ObjectMeta.Labels[constants.LabelQueueName] = queue
+ pod.ObjectMeta.Labels[constants.CanonicalLabelApplicationID] = appId
+ pod.ObjectMeta.Labels[constants.CanonicalLabelQueueName] = queue
return pod
}
@@ -111,21 +111,25 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
// we generate new appId/queue labels
pod := createTestingPodWithMeta()
if result := updatePodLabel(pod, "default", false, defaultQueueName); result != nil {
- assert.Equal(t, len(result), 3)
+ assert.Equal(t, len(result), 5)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, strings.HasPrefix(result[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(result[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], defaultQueueName)
assert.Equal(t, result[constants.LabelQueueName], defaultQueueName)
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
}
- // verify if applicationId and queue is given in the labels,
- // we won't modify it
+ // verify if appId/queue is given in the canonical labels
+ // we won't modify the value and will add it to non-canonical label for backward compatibility
pod = createTestingPodWithLabels(dummyAppId, dummyQueueName)
if result := updatePodLabel(pod, "default", false, defaultQueueName); result != nil {
- assert.Equal(t, len(result), 3)
+ assert.Equal(t, len(result), 5)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, result[constants.CanonicalLabelApplicationID], dummyAppId)
assert.Equal(t, result[constants.LabelApplicationID], dummyAppId)
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], dummyQueueName)
assert.Equal(t, result[constants.LabelQueueName], dummyQueueName)
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -147,8 +151,10 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
pod = createTestingPodNoNamespaceAndLabels()
if result := updatePodLabel(pod, "default", false, defaultQueueName); result != nil {
- assert.Equal(t, len(result), 2)
+ assert.Equal(t, len(result), 4)
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], defaultQueueName)
assert.Equal(t, result[constants.LabelQueueName], defaultQueueName)
+ assert.Equal(t, strings.HasPrefix(result[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(result[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -157,8 +163,10 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
// pod name might be empty, it can comes from generatedName
pod = createTestingPodWithGenerateName()
if result := updatePodLabel(pod, "default", false, defaultQueueName); result != nil {
- assert.Equal(t, len(result), 2)
+ assert.Equal(t, len(result), 4)
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], defaultQueueName)
assert.Equal(t, result[constants.LabelQueueName], defaultQueueName)
+ assert.Equal(t, strings.HasPrefix(result[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(result[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -166,8 +174,10 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
pod = createMinimalTestingPod()
if result := updatePodLabel(pod, "default", false, defaultQueueName); result != nil {
- assert.Equal(t, len(result), 2)
+ assert.Equal(t, len(result), 4)
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], defaultQueueName)
assert.Equal(t, result[constants.LabelQueueName], defaultQueueName)
+ assert.Equal(t, strings.HasPrefix(result[constants.CanonicalLabelApplicationID], constants.AutoGenAppPrefix), true)
assert.Equal(t, strings.HasPrefix(result[constants.LabelApplicationID], constants.AutoGenAppPrefix), true)
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -178,9 +188,11 @@ func TestDefaultQueueName(t *testing.T) {
defaultConf := createConfig()
pod := createTestingPodWithMeta()
if result := updatePodLabel(pod, defaultConf.GetNamespace(), defaultConf.GetGenerateUniqueAppIds(), defaultConf.GetDefaultQueueName()); result != nil {
- assert.Equal(t, len(result), 3)
+ assert.Equal(t, len(result), 5)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, result[constants.CanonicalLabelApplicationID], "yunikorn-default-autogen")
assert.Equal(t, result[constants.LabelApplicationID], "yunikorn-default-autogen")
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], "root.default")
assert.Equal(t, result[constants.LabelQueueName], "root.default")
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -190,9 +202,11 @@ func TestDefaultQueueName(t *testing.T) {
conf.AMFilteringDefaultQueueName: "",
})
if result := updatePodLabel(pod, queueNameEmptyConf.GetNamespace(), queueNameEmptyConf.GetGenerateUniqueAppIds(), queueNameEmptyConf.GetDefaultQueueName()); result != nil {
- assert.Equal(t, len(result), 2)
+ assert.Equal(t, len(result), 3)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, result[constants.CanonicalLabelApplicationID], "yunikorn-default-autogen")
assert.Equal(t, result[constants.LabelApplicationID], "yunikorn-default-autogen")
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], "")
assert.Equal(t, result[constants.LabelQueueName], "")
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -202,9 +216,11 @@ func TestDefaultQueueName(t *testing.T) {
conf.AMFilteringDefaultQueueName: "yunikorn",
})
if result := updatePodLabel(pod, customQueueNameConf.GetNamespace(), customQueueNameConf.GetGenerateUniqueAppIds(), customQueueNameConf.GetDefaultQueueName()); result != nil {
- assert.Equal(t, len(result), 3)
+ assert.Equal(t, len(result), 5)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, result[constants.CanonicalLabelApplicationID], "yunikorn-default-autogen")
assert.Equal(t, result[constants.LabelApplicationID], "yunikorn-default-autogen")
+ assert.Assert(t, result[constants.CanonicalLabelQueueName] != "yunikorn")
assert.Assert(t, result[constants.LabelQueueName] != "yunikorn")
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -215,9 +231,11 @@ func TestDefaultQueueName(t *testing.T) {
})
if result := updatePodLabel(pod, customValidQueueNameConf.GetNamespace(),
customValidQueueNameConf.GetGenerateUniqueAppIds(), customValidQueueNameConf.GetDefaultQueueName()); result != nil {
- assert.Equal(t, len(result), 3)
+ assert.Equal(t, len(result), 5)
assert.Equal(t, result["random"], "random")
+ assert.Equal(t, result[constants.CanonicalLabelApplicationID], "yunikorn-default-autogen")
assert.Equal(t, result[constants.LabelApplicationID], "yunikorn-default-autogen")
+ assert.Equal(t, result[constants.CanonicalLabelQueueName], "root.yunikorn")
assert.Equal(t, result[constants.LabelQueueName], "root.yunikorn")
} else {
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
diff --git a/pkg/common/constants/constants.go b/pkg/common/constants/constants.go
index 7a4c415d..bdee70a8 100644
--- a/pkg/common/constants/constants.go
+++ b/pkg/common/constants/constants.go
@@ -38,9 +38,11 @@ const DomainYuniKornInternal = siCommon.DomainYuniKornInternal
const LabelApp = "app"
const LabelApplicationID = "applicationId"
const AnnotationApplicationID = DomainYuniKorn + "app-id"
+const CanonicalLabelApplicationID = DomainYuniKorn + "app-id"
const LabelQueueName = "queue"
const RootQueue = "root"
const AnnotationQueueName = DomainYuniKorn + "queue"
+const CanonicalLabelQueueName = DomainYuniKorn + "queue"
const AnnotationParentQueue = DomainYuniKorn + "parentqueue"
const ApplicationDefaultQueue = "root.default"
const DefaultPartition = "default"
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org