You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/09/09 16:14:12 UTC
[GitHub] [yunikorn-k8shim] 0yukali0 opened a new pull request, #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
0yukali0 opened a new pull request, #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462
### What is this PR for?
1. The iteration is not required for searching userLabelKey.
2. The logger adopting debug level show the current userLabelKey
3. The userLabelKey would be replaced by the default userLabelKey when the userLabelKey returned from the configuration is empty.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1314
### How should this be tested?
Pods' labels contains userLabelKey and the username rule is added to the partition.
```
metadata:
labels:
app: nginx
applicationId: "username-rule-example02"
yunikorn.apache.org/username: developer
name: task1
spec:
schedulerName: yunikorn
```
```
partitions:
-
name: default
placementrules:
- name: user
create: true
- name: tag
value: namespace
create: true
queues:
- name: root
submitacl: '*'
```
### Screenshots (if appropriate)
For example, an user, called developer, owns a pod.
The queue doesn't exist or the queue label is not added in the pod.
The pod would be assigned to root.developer.
<img width="1512" alt="截圖 2022-09-09 下午11 24 30" src="https://user-images.githubusercontent.com/45888688/189388641-d9c974db-9855-4bf9-80ab-56a8879b3e38.png">
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#issuecomment-1242189519
# [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/462?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#462](https://codecov.io/gh/apache/yunikorn-k8shim/pull/462?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b4874c) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/1f050c40396513b0b60eec87c6b10b0647fcfbc7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f050c4) will **decrease** coverage by `0.09%`.
> The diff coverage is `66.66%`.
```diff
@@ Coverage Diff @@
## master #462 +/- ##
==========================================
- Coverage 67.00% 66.90% -0.10%
==========================================
Files 41 41
Lines 6767 6769 +2
==========================================
- Hits 4534 4529 -5
- Misses 2062 2068 +6
- Partials 171 172 +1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/462?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/common/utils/utils.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/462/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy91dGlscy5nbw==) | `56.36% <66.66%> (-2.54%)` | :arrow_down: |
| [pkg/dispatcher/dispatcher.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/462/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2Rpc3BhdGNoZXIvZGlzcGF0Y2hlci5nbw==) | `74.82% <0.00%> (-1.40%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r980741918
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
}
- value := constants.DefaultUser
+ // User name to be defined in labels
+ if username, ok := pod.Labels[userLabelKey]; ok {
Review Comment:
A label value can be empty. That would result in `ok` to be set to true but username to be "" (or empty string) in that case we should assign the default user also.
That is not covered by the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#issuecomment-1258977221
re-running the tests see if it is consistently breaking or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r972610278
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
}
- value := constants.DefaultUser
+ // User name to be defined in labels
+ if username, ok := pod.Labels[userLabelKey]; ok {
Review Comment:
Yes, the default user name will be return if yunikorn can't get the username from the pod.
I added the modification in the current commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#issuecomment-1263322324
Hi @wilfred-s
I will check this case and update the commit tonight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r967770912
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
Review Comment:
The default is documented and we should not need to print it every time we try to retrieve a user, simple like this: "userLabelKey not set using default value"
Need to also handle this as part of a unit test, codecov complains now
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
}
- value := constants.DefaultUser
+ // User name to be defined in labels
+ if username, ok := pod.Labels[userLabelKey]; ok {
Review Comment:
Can the label be found and still return an empty username?
We need to handle that case and still set the default user
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r972610278
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
}
- value := constants.DefaultUser
+ // User name to be defined in labels
+ if username, ok := pod.Labels[userLabelKey]; ok {
Review Comment:
Yes, the default user name will be returned if yunikorn can't get the username from the pod.
I added the modification in the current commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#issuecomment-1248919809
Hi @wilfred-s, Is the time-out event for waiting yunikorn pod related to GetUserFromPod in e2e test when the Kubernetes version is 1.25?
Should I re-run the pre-commit check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r984388509
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
}
- value := constants.DefaultUser
+ // User name to be defined in labels
+ if username, ok := pod.Labels[userLabelKey]; ok {
Review Comment:
Ok, I am going to add relevant modifications for this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] 0yukali0 commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
0yukali0 commented on code in PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#discussion_r972611695
##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
// find user name from pod label
func GetUserFromPod(pod *v1.Pod) string {
userLabelKey := conf.GetSchedulerConf().UserLabelKey
- // User name to be defined in labels
- for name, value := range pod.Labels {
- if name == userLabelKey {
- log.Logger().Info("Found user name from pod labels.",
- zap.String("userLabel", userLabelKey), zap.String("user", value))
- return value
- }
+ // UserLabelKey should not be empty
+ if len(userLabelKey) == 0 {
+ userLabelKey = constants.DefaultUserLabel
+ log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+ zap.String("userLabel", userLabelKey))
Review Comment:
Hi @wilfred-s, I had added the relevant thing to the latest commit.
The empty userLabelKey case was added to the unit test.
The log info was removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #462:
URL: https://github.com/apache/yunikorn-k8shim/pull/462#issuecomment-1261885584
Test failed, not sure if it is the same one. Plugin with K8s 1.25 has been the problem, maybe @craigcondit knows more.
@0yukali0 can you have a look at my comment around the empty value that point is not resolved yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org