Skip to content

Commit 4f8d48a

Browse files
mitdesaicraigcondit
authored andcommitted
[YUNIKORN-2711] Shim: Skip setting the queue name to default queue
Closes: #874 Signed-off-by: Craig Condit <ccondit@apache.org>
1 parent 30b076c commit 4f8d48a

File tree

11 files changed

+25
-127
lines changed

11 files changed

+25
-127
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ module github.com/apache/yunikorn-k8shim
2121
go 1.21
2222

2323
require (
24-
github.com/apache/yunikorn-core v1.5.1-1
24+
github.com/apache/yunikorn-core v0.0.0-20240711174509-5454d7d32663
2525
github.com/apache/yunikorn-scheduler-interface v1.5.1-1
2626
github.com/google/go-cmp v0.6.0
2727
github.com/google/uuid v1.6.0

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ github.com/NYTimes/gziphandler v1.1.1 h1:ZUDjpQae29j0ryrS0u/B8HZfJBtBQHjqw2rQ2cq
99
github.com/NYTimes/gziphandler v1.1.1/go.mod h1:n/CVRwUEOgIxrgPvAQhUUr9oeUtvrhMomdKFjzJNB0c=
1010
github.com/antlr/antlr4/runtime/Go/antlr/v4 v4.0.0-20230305170008-8188dc5388df h1:7RFfzj4SSt6nnvCPbCqijJi1nWCd+TqAT3bYCStRC18=
1111
github.com/antlr/antlr4/runtime/Go/antlr/v4 v4.0.0-20230305170008-8188dc5388df/go.mod h1:pSwJ0fSY5KhvocuWSx4fz3BA8OrA1bQn+K1Eli3BRwM=
12-
github.com/apache/yunikorn-core v1.5.1-1 h1:BmPAoMd+ovH/NAG+VA2Nkb51fVGz2DuO5xoSqNn6lZ8=
13-
github.com/apache/yunikorn-core v1.5.1-1/go.mod h1:49Kd4+44XRAWVdXzhtphnH6ctf5NthrHfRElSZNIiJo=
12+
github.com/apache/yunikorn-core v0.0.0-20240711174509-5454d7d32663 h1:1ENu93SCfvLcIMpRVANLg+L4hHItTevv8E97F4jw3lU=
13+
github.com/apache/yunikorn-core v0.0.0-20240711174509-5454d7d32663/go.mod h1:49Kd4+44XRAWVdXzhtphnH6ctf5NthrHfRElSZNIiJo=
1414
github.com/apache/yunikorn-scheduler-interface v1.5.1-1 h1:8NXsCFSrQ6yBodZQYcJWfyQW7CoYn3MlODYUDdiLmpc=
1515
github.com/apache/yunikorn-scheduler-interface v1.5.1-1/go.mod h1:gk3BtbzoUH7T5lhNoLxp/8g9pw25S1/d7NbiypV/30Q=
1616
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio=

pkg/admission/admission_controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ func (c *AdmissionController) updateLabels(namespace string, pod *v1.Pod, patch
422422
zap.String("namespace", namespace),
423423
zap.Any("labels", pod.Labels))
424424

425-
result := updatePodLabel(pod, namespace, c.conf.GetGenerateUniqueAppIds(), c.conf.GetDefaultQueueName())
425+
result := updatePodLabel(pod, namespace, c.conf.GetGenerateUniqueAppIds())
426426

427427
patch = append(patch, common.PatchOperation{
428428
Op: "add",

pkg/admission/admission_controller_test.go

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,8 @@ func TestUpdateLabels(t *testing.T) {
8181
assert.Equal(t, patch[0].Op, "add")
8282
assert.Equal(t, patch[0].Path, "/metadata/labels")
8383
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
84-
assert.Equal(t, len(updatedMap), 4)
84+
assert.Equal(t, len(updatedMap), 3)
8585
assert.Equal(t, updatedMap["random"], "random")
86-
assert.Equal(t, updatedMap["queue"], "root.default")
8786
assert.Equal(t, updatedMap["disableStateAware"], "true")
8887
assert.Equal(t, strings.HasPrefix(updatedMap["applicationId"], constants.AutoGenAppPrefix), true)
8988
} else {
@@ -118,9 +117,8 @@ func TestUpdateLabels(t *testing.T) {
118117
assert.Equal(t, patch[0].Op, "add")
119118
assert.Equal(t, patch[0].Path, "/metadata/labels")
120119
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
121-
assert.Equal(t, len(updatedMap), 3)
120+
assert.Equal(t, len(updatedMap), 2)
122121
assert.Equal(t, updatedMap["random"], "random")
123-
assert.Equal(t, updatedMap["queue"], "root.default")
124122
assert.Equal(t, updatedMap["applicationId"], "app-0001")
125123
} else {
126124
t.Fatal("patch info content is not as expected")
@@ -188,8 +186,7 @@ func TestUpdateLabels(t *testing.T) {
188186
assert.Equal(t, patch[0].Op, "add")
189187
assert.Equal(t, patch[0].Path, "/metadata/labels")
190188
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
191-
assert.Equal(t, len(updatedMap), 3)
192-
assert.Equal(t, updatedMap["queue"], "root.default")
189+
assert.Equal(t, len(updatedMap), 2)
193190
assert.Equal(t, updatedMap["disableStateAware"], "true")
194191
assert.Equal(t, strings.HasPrefix(updatedMap["applicationId"], constants.AutoGenAppPrefix), true)
195192
} else {
@@ -217,8 +214,7 @@ func TestUpdateLabels(t *testing.T) {
217214
assert.Equal(t, patch[0].Op, "add")
218215
assert.Equal(t, patch[0].Path, "/metadata/labels")
219216
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
220-
assert.Equal(t, len(updatedMap), 3)
221-
assert.Equal(t, updatedMap["queue"], "root.default")
217+
assert.Equal(t, len(updatedMap), 2)
222218
assert.Equal(t, updatedMap["disableStateAware"], "true")
223219
assert.Equal(t, strings.HasPrefix(updatedMap["applicationId"], constants.AutoGenAppPrefix), true)
224220
} else {
@@ -244,8 +240,7 @@ func TestUpdateLabels(t *testing.T) {
244240
assert.Equal(t, patch[0].Op, "add")
245241
assert.Equal(t, patch[0].Path, "/metadata/labels")
246242
if updatedMap, ok := patch[0].Value.(map[string]string); ok {
247-
assert.Equal(t, len(updatedMap), 3)
248-
assert.Equal(t, updatedMap["queue"], "root.default")
243+
assert.Equal(t, len(updatedMap), 2)
249244
assert.Equal(t, updatedMap["disableStateAware"], "true")
250245
assert.Equal(t, strings.HasPrefix(updatedMap["applicationId"], constants.AutoGenAppPrefix), true)
251246
} else {
@@ -456,7 +451,6 @@ func TestMutate(t *testing.T) {
456451
assert.Equal(t, schedulerName(t, resp.Patch), "yunikorn", "yunikorn not set as scheduler for pod")
457452
assert.Equal(t, labels(t, resp.Patch)["applicationId"], "yunikorn-default-autogen", "wrong applicationId label")
458453
assert.Equal(t, labels(t, resp.Patch)["disableStateAware"], "true", "missing disableStateAware label")
459-
assert.Equal(t, labels(t, resp.Patch)["queue"], "root.default", "incorrect queue name")
460454

461455
// pod without applicationID
462456
pod = v1.Pod{ObjectMeta: metav1.ObjectMeta{

pkg/admission/conf/am_conf.go

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ const (
5252
AMFilteringLabelNamespaces = FilteringPrefix + "labelNamespaces"
5353
AMFilteringNoLabelNamespaces = FilteringPrefix + "noLabelNamespaces"
5454
AMFilteringGenerateUniqueAppIds = FilteringPrefix + "generateUniqueAppId"
55-
AMFilteringDefaultQueueName = FilteringPrefix + "defaultQueue"
5655

5756
// access control configuration
5857
AMAccessControlBypassAuth = AccessControlPrefix + "bypassAuth"
@@ -73,7 +72,6 @@ const (
7372
DefaultFilteringLabelNamespaces = ""
7473
DefaultFilteringNoLabelNamespaces = ""
7574
DefaultFilteringGenerateUniqueAppIds = false
76-
DefaultFilteringQueueName = constants.ApplicationDefaultQueue
7775

7876
// access control defaults
7977
DefaultAccessControlBypassAuth = false
@@ -102,7 +100,6 @@ type AdmissionControllerConf struct {
102100
systemUsers []*regexp.Regexp
103101
externalUsers []*regexp.Regexp
104102
externalGroups []*regexp.Regexp
105-
defaultQueueName string
106103
configMaps []*v1.ConfigMap
107104

108105
lock locking.RWMutex
@@ -217,12 +214,6 @@ func (acc *AdmissionControllerConf) GetExternalGroups() []*regexp.Regexp {
217214
return acc.externalGroups
218215
}
219216

220-
func (acc *AdmissionControllerConf) GetDefaultQueueName() string {
221-
acc.lock.RLock()
222-
defer acc.lock.RUnlock()
223-
return acc.defaultQueueName
224-
}
225-
226217
type configMapUpdateHandler struct {
227218
conf *AdmissionControllerConf
228219
}
@@ -326,14 +317,6 @@ func (acc *AdmissionControllerConf) updateConfigMaps(configMaps []*v1.ConfigMap,
326317
acc.externalUsers = parseConfigRegexps(configs, AMAccessControlExternalUsers, DefaultAccessControlExternalUsers)
327318
acc.externalGroups = parseConfigRegexps(configs, AMAccessControlExternalGroups, DefaultAccessControlExternalGroups)
328319

329-
// labeling
330-
acc.defaultQueueName = parseConfigString(configs, AMFilteringDefaultQueueName, DefaultFilteringQueueName)
331-
if acc.defaultQueueName != "" && !strings.HasPrefix(acc.defaultQueueName, constants.RootQueue) {
332-
log.Log(log.AdmissionConf).Warn("invalid default queue. defaultQueue must be fully qualified. Resetting to "+DefaultFilteringQueueName,
333-
zap.String(AMFilteringDefaultQueueName, acc.defaultQueueName))
334-
acc.defaultQueueName = DefaultFilteringQueueName
335-
}
336-
337320
// logging
338321
log.UpdateLoggingConfig(configs)
339322

pkg/admission/conf/am_conf_test.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ func TestConfigMapVars(t *testing.T) {
4343
AMAccessControlExternalUsers: "^yunikorn$",
4444
AMAccessControlExternalGroups: "^devs$",
4545
AMAccessControlTrustControllers: "false",
46-
AMFilteringDefaultQueueName: "root.default.queue",
4746
}}})
4847
assert.Equal(t, conf.GetPolicyGroup(), "testPolicyGroup")
4948
assert.Equal(t, conf.GetAmServiceName(), "testYunikornService")
@@ -58,7 +57,6 @@ func TestConfigMapVars(t *testing.T) {
5857
assert.Equal(t, conf.GetExternalUsers()[0].String(), "^yunikorn$")
5958
assert.Equal(t, conf.GetExternalGroups()[0].String(), "^devs$")
6059
assert.Equal(t, conf.GetTrustControllers(), false)
61-
assert.Equal(t, conf.GetDefaultQueueName(), "root.default.queue")
6260

6361
// test missing settings
6462
conf = NewAdmissionControllerConf([]*v1.ConfigMap{nil, nil})
@@ -75,7 +73,6 @@ func TestConfigMapVars(t *testing.T) {
7573
assert.Equal(t, 0, len(conf.GetExternalUsers()))
7674
assert.Equal(t, 0, len(conf.GetExternalGroups()))
7775
assert.Equal(t, conf.GetTrustControllers(), DefaultAccessControlTrustControllers)
78-
assert.Equal(t, conf.GetDefaultQueueName(), DefaultFilteringQueueName)
7976

8077
// test faulty settings for boolean values
8178
conf = NewAdmissionControllerConf([]*v1.ConfigMap{nil, {Data: map[string]string{
@@ -93,12 +90,6 @@ func TestConfigMapVars(t *testing.T) {
9390
}}})
9491
assert.Equal(t, len(conf.GetProcessNamespaces()), 0)
9592

96-
// test invalid defaultQueue name
97-
conf = NewAdmissionControllerConf([]*v1.ConfigMap{nil, {Data: map[string]string{
98-
AMFilteringDefaultQueueName: "default.queue",
99-
}}})
100-
assert.Equal(t, conf.GetDefaultQueueName(), DefaultFilteringQueueName)
101-
10293
// test disable / enable of config hot refresh
10394
conf = NewAdmissionControllerConf([]*v1.ConfigMap{nil, nil})
10495

pkg/admission/util.go

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
"github.com/apache/yunikorn-k8shim/pkg/log"
3030
)
3131

32-
func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool, defaultQueueName string) map[string]string {
32+
func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool) map[string]string {
3333
existingLabels := pod.Labels
3434
result := make(map[string]string)
3535
for k, v := range existingLabels {
@@ -54,16 +54,6 @@ func updatePodLabel(pod *v1.Pod, namespace string, generateUniqueAppIds bool, de
5454
}
5555
}
5656

57-
// if existing label exist, it takes priority over everything else
58-
if _, ok := existingLabels[constants.LabelQueueName]; !ok {
59-
// if defaultQueueName is "", skip adding default queue name to the pod labels
60-
if defaultQueueName != "" {
61-
// for undefined configuration, am_conf will add 'root.default' to retain existing behavior
62-
// if a custom name is configured for default queue, it will be used instead of root.default
63-
result[constants.LabelQueueName] = defaultQueueName
64-
}
65-
}
66-
6757
return result
6858
}
6959

pkg/admission/util_test.go

Lines changed: 13 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,9 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
103103
// verify when appId/queue are not given,
104104
pod := createTestingPodWithMeta()
105105

106-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
107-
assert.Equal(t, len(result), 4)
106+
if result := updatePodLabel(pod, "default", false); result != nil {
107+
assert.Equal(t, len(result), 3)
108108
assert.Equal(t, result["random"], "random")
109-
assert.Equal(t, result["queue"], "root.default")
110109
assert.Equal(t, result["disableStateAware"], "true")
111110
assert.Equal(t, strings.HasPrefix(result["applicationId"], constants.AutoGenAppPrefix), true)
112111
} else {
@@ -117,10 +116,9 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
117116
// we won't modify it
118117
pod = createTestingPodWithAppId()
119118

120-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
121-
assert.Equal(t, len(result), 3)
119+
if result := updatePodLabel(pod, "default", false); result != nil {
120+
assert.Equal(t, len(result), 2)
122121
assert.Equal(t, result["random"], "random")
123-
assert.Equal(t, result["queue"], "root.default")
124122
assert.Equal(t, result["applicationId"], "app-0001")
125123
} else {
126124
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
@@ -129,23 +127,22 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
129127
// verify if queue is given in the labels,
130128
// we won't modify it
131129
pod = createTestingPodWithQueue()
132-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
130+
if result := updatePodLabel(pod, "default", false); result != nil {
133131
assert.Equal(t, len(result), 4)
134132
assert.Equal(t, result["random"], "random")
135-
assert.Equal(t, result["queue"], "root.abc")
136133
assert.Equal(t, result["disableStateAware"], "true")
134+
assert.Equal(t, result["queue"], "root.abc")
137135
assert.Equal(t, strings.HasPrefix(result["applicationId"], constants.AutoGenAppPrefix), true)
138136
} else {
139-
t.Fatal("UpdatePodLabelForAdmissionControllert is not as expected")
137+
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
140138
}
141139

142140
// namespace might be empty
143141
// labels might be empty
144142
pod = createTestingPodNoNamespaceAndLabels()
145143

146-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
147-
assert.Equal(t, len(result), 3)
148-
assert.Equal(t, result["queue"], "root.default")
144+
if result := updatePodLabel(pod, "default", false); result != nil {
145+
assert.Equal(t, len(result), 2)
149146
assert.Equal(t, result["disableStateAware"], "true")
150147
assert.Equal(t, strings.HasPrefix(result["applicationId"], constants.AutoGenAppPrefix), true)
151148
} else {
@@ -154,76 +151,20 @@ func TestUpdatePodLabelForAdmissionController(t *testing.T) {
154151

155152
// pod name might be empty, it can comes from generatedName
156153
pod = createTestingPodWithGenerateName()
157-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
158-
assert.Equal(t, len(result), 3)
159-
assert.Equal(t, result["queue"], "root.default")
154+
if result := updatePodLabel(pod, "default", false); result != nil {
155+
assert.Equal(t, len(result), 2)
160156
assert.Equal(t, result["disableStateAware"], "true")
161157
assert.Equal(t, strings.HasPrefix(result["applicationId"], constants.AutoGenAppPrefix), true)
162158
} else {
163159
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
164160
}
165161

166162
pod = createMinimalTestingPod()
167-
if result := updatePodLabel(pod, "default", false, "root.default"); result != nil {
168-
assert.Equal(t, len(result), 3)
169-
assert.Equal(t, result["queue"], "root.default")
163+
if result := updatePodLabel(pod, "default", false); result != nil {
164+
assert.Equal(t, len(result), 2)
170165
assert.Equal(t, result["disableStateAware"], "true")
171166
assert.Equal(t, strings.HasPrefix(result["applicationId"], constants.AutoGenAppPrefix), true)
172167
} else {
173168
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
174169
}
175170
}
176-
177-
func TestDefaultQueueName(t *testing.T) {
178-
defaultConf := createConfig()
179-
pod := createTestingPodWithMeta()
180-
if result := updatePodLabel(pod, defaultConf.GetNamespace(), defaultConf.GetGenerateUniqueAppIds(), defaultConf.GetDefaultQueueName()); result != nil {
181-
assert.Equal(t, len(result), 4)
182-
assert.Equal(t, result["random"], "random")
183-
assert.Equal(t, result["applicationId"], "yunikorn-default-autogen")
184-
assert.Equal(t, result["disableStateAware"], "true")
185-
assert.Equal(t, result["queue"], "root.default")
186-
} else {
187-
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
188-
}
189-
190-
queueNameEmptyConf := createConfigWithOverrides(map[string]string{
191-
conf.AMFilteringDefaultQueueName: "",
192-
})
193-
if result := updatePodLabel(pod, queueNameEmptyConf.GetNamespace(), queueNameEmptyConf.GetGenerateUniqueAppIds(), queueNameEmptyConf.GetDefaultQueueName()); result != nil {
194-
assert.Equal(t, len(result), 3)
195-
assert.Equal(t, result["random"], "random")
196-
assert.Equal(t, result["applicationId"], "yunikorn-default-autogen")
197-
assert.Equal(t, result["disableStateAware"], "true")
198-
assert.Equal(t, result["queue"], "")
199-
} else {
200-
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
201-
}
202-
203-
customQueueNameConf := createConfigWithOverrides(map[string]string{
204-
conf.AMFilteringDefaultQueueName: "yunikorn",
205-
})
206-
if result := updatePodLabel(pod, customQueueNameConf.GetNamespace(), customQueueNameConf.GetGenerateUniqueAppIds(), customQueueNameConf.GetDefaultQueueName()); result != nil {
207-
assert.Equal(t, len(result), 4)
208-
assert.Equal(t, result["random"], "random")
209-
assert.Equal(t, result["applicationId"], "yunikorn-default-autogen")
210-
assert.Equal(t, result["disableStateAware"], "true")
211-
assert.Assert(t, result["queue"] != "yunikorn")
212-
} else {
213-
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
214-
}
215-
216-
customValidQueueNameConf := createConfigWithOverrides(map[string]string{
217-
conf.AMFilteringDefaultQueueName: "root.yunikorn",
218-
})
219-
if result := updatePodLabel(pod, customValidQueueNameConf.GetNamespace(),
220-
customValidQueueNameConf.GetGenerateUniqueAppIds(), customValidQueueNameConf.GetDefaultQueueName()); result != nil {
221-
assert.Equal(t, len(result), 4)
222-
assert.Equal(t, result["random"], "random")
223-
assert.Equal(t, result["applicationId"], "yunikorn-default-autogen")
224-
assert.Equal(t, result["disableStateAware"], "true")
225-
assert.Equal(t, result["queue"], "root.yunikorn")
226-
} else {
227-
t.Fatal("UpdatePodLabelForAdmissionController is not as expected")
228-
}
229-
}

pkg/common/constants/constants.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ const RootQueue = "root"
4242
const AnnotationQueueName = DomainYuniKorn + "queue"
4343
const AnnotationParentQueue = DomainYuniKorn + "parentqueue"
4444
const LabelDisableStateAware = "disableStateAware"
45-
const ApplicationDefaultQueue = "root.default"
4645
const DefaultPartition = "default"
4746
const AppTagNamespace = "namespace"
4847
const AppTagNamespaceParentQueue = "namespace.parentqueue"

pkg/common/utils/utils.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func IsAssignedPod(pod *v1.Pod) bool {
104104
}
105105

106106
func GetQueueNameFromPod(pod *v1.Pod) string {
107-
queueName := constants.ApplicationDefaultQueue
107+
queueName := ""
108108
if an := GetPodLabelValue(pod, constants.LabelQueueName); an != "" {
109109
queueName = an
110110
} else if qu := GetPodAnnotationValue(pod, constants.AnnotationQueueName); qu != "" {

0 commit comments

Comments
 (0)