You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "mitdesai (via GitHub)" <gi...@apache.org> on 2023/05/23 16:39:03 UTC
[GitHub] [yunikorn-k8shim] mitdesai opened a new pull request, #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
mitdesai opened a new pull request, #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597
### What is this PR for?
This PR add a configuration to skip adding default queue name label to the pods.
This option is needed when complex placement rules are enabled. Especially when _provided_ rule is used in conjunction with other placement rules and provided rule is higher in the hierarchy. If default queue label is always getting added for pods that do not have a queue label, all the apps will be scheduled via _provided_ rule and the other rules after that will never be executed.
### What type of PR is it?
* [ ] - Bug Fix
* [X] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN-1650
### How should this be tested?
By default, the configuration is turned off
1. Out of the box, no changes are required if you do not intent to change the current behavior.
2. For skipping to add the default queue name, add this configuration in the admission controller configs: `admissionController.label.defaultQueue=""`. The queue name label should only get added to the pod if a value for queue name was specified during the app submission
3. For changing the default queue name to something other than `root.default`, add this configuration to the admission controller configs: `admissionController.label.defaultQueue="root.mydefault"`. Note: The value for the default queue has to be a fully qualified queue name.
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [X] - 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] craigcondit commented on a diff in pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597#discussion_r1202744088
##########
pkg/admission/conf/am_conf.go:
##########
@@ -60,6 +61,9 @@ const (
AMAccessControlSystemUsers = AccessControlPrefix + "systemUsers"
AMAccessControlExternalUsers = AccessControlPrefix + "externalUsers"
AMAccessControlExternalGroups = AccessControlPrefix + "externalGroups"
+
+ // label configurations
+ AMLabelDefaultQueue = LabelPrefix + "defaultQueue"
Review Comment:
Minor nit: Let's put these under filtering (ala `generateUniqueAppId`).
--
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] craigcondit closed pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
URL: https://github.com/apache/yunikorn-k8shim/pull/597
--
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] mitdesai commented on pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "mitdesai (via GitHub)" <gi...@apache.org>.
mitdesai commented on PR #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597#issuecomment-1560239895
Thanks @craigcondit !
--
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 #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597#issuecomment-1559866132
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#597](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e10d34c) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/95157f9fdc357fb758e774c4c7effb815bea51f5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (95157f9) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #597 +/- ##
==========================================
+ Coverage 70.19% 70.24% +0.05%
==========================================
Files 47 47
Lines 7951 7962 +11
==========================================
+ Hits 5581 5593 +12
Misses 2164 2164
+ Partials 206 205 -1
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/admission/admission\_controller.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2FkbWlzc2lvbi9hZG1pc3Npb25fY29udHJvbGxlci5nbw==) | `66.53% <100.00%> (+0.59%)` | :arrow_up: |
| [pkg/admission/conf/am\_conf.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2FkbWlzc2lvbi9jb25mL2FtX2NvbmYuZ28=) | `72.91% <100.00%> (+0.81%)` | :arrow_up: |
| [pkg/admission/util.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2FkbWlzc2lvbi91dGlsLmdv) | `96.15% <100.00%> (+0.32%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/597/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] mitdesai commented on a diff in pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "mitdesai (via GitHub)" <gi...@apache.org>.
mitdesai commented on code in PR #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597#discussion_r1202762072
##########
pkg/admission/conf/am_conf.go:
##########
@@ -60,6 +61,9 @@ const (
AMAccessControlSystemUsers = AccessControlPrefix + "systemUsers"
AMAccessControlExternalUsers = AccessControlPrefix + "externalUsers"
AMAccessControlExternalGroups = AccessControlPrefix + "externalGroups"
+
+ // label configurations
+ AMLabelDefaultQueue = LabelPrefix + "defaultQueue"
Review Comment:
Thanks for the review @craigcondit.
I think that looks better. Let me update the PR.
--
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] mitdesai commented on a diff in pull request #597: [YUNIKORN-1650] Add toggle for adding default queue in admission controller
Posted by "mitdesai (via GitHub)" <gi...@apache.org>.
mitdesai commented on code in PR #597:
URL: https://github.com/apache/yunikorn-k8shim/pull/597#discussion_r1202915081
##########
pkg/admission/conf/am_conf.go:
##########
@@ -60,6 +61,9 @@ const (
AMAccessControlSystemUsers = AccessControlPrefix + "systemUsers"
AMAccessControlExternalUsers = AccessControlPrefix + "externalUsers"
AMAccessControlExternalGroups = AccessControlPrefix + "externalGroups"
+
+ // label configurations
+ AMLabelDefaultQueue = LabelPrefix + "defaultQueue"
Review Comment:
@craigcondit, I've updated the PR. I'll resolve this conversation.
--
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