Skip to content
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.

Sign up for GitHub

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

Yunikorn 2631 2 #51

Closed
wants to merge 3 commits into from
+72 −27
Closed

Yunikorn 2631 2 #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88f9adf
[YUNIKORN-2631] Support canonical labels for queue/applicationId in A…
chenyulin0719 May 17, 2024
8089fba
code review: reorder constans
chenyulin0719 May 19, 2024
6a31d07
Merge branch 'master' into YUNIKORN-2631
chenyulin0719 May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/admission/admission_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
32 changes: 22 additions & 10 deletions pkg/admission/admission_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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{},
Expand All @@ -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")
Expand All @@ -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{},
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down
19 changes: 16 additions & 3 deletions pkg/admission/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,43 @@ func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool, de
result[k] = v
}

sparkAppID := utils.GetPodLabelValue(pod, constants.SparkLabelAppID)
canonicalAppID := utils.GetPodLabelValue(pod, constants.CanonicalLabelApplicationID)
labelAppID := utils.GetPodLabelValue(pod, constants.LabelApplicationID)
sparkAppID := utils.GetPodLabelValue(pod, constants.SparkLabelAppID)
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:
// application ID convention: ${NAMESPACE}-${POD_UID}
// 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
Expand Down
44 changes: 31 additions & 13 deletions pkg/admission/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -157,17 +163,21 @@ 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")
}

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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down
2 changes: 2 additions & 0 deletions pkg/common/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading