diff --git a/pkg/admission/admission_controller.go b/pkg/admission/admission_controller.go index 3b058da6b..35c3fecb0 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 88bf8d6a8..e5c66189e 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 fe994da72..5276b5b61 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 fa28c824c..2181bd5a1 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 7a4c415d4..bdee70a8e 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"