You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "craigcondit (via GitHub)" <gi...@apache.org> on 2023/04/25 18:49:46 UTC
[GitHub] [yunikorn-k8shim] craigcondit opened a new pull request, #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
craigcondit opened a new pull request, #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579
### What is this PR for?
The admission controller allows pods in configured namespaces to be either
bypassed entirely, or passed through to YuniKorn but not labeled for
processing (useful in the plugin deployment to pass through to the embedded
default scheduler). However, if YuniKorn detects an applicationId or Spark
spark-app-selector, it will attempt to schedule that Pod via YuniKorn
regardless. This may not succeed if there is not a valid queue available
which matches placement rules.
To prevent this, the admission controller will now annotate pods which are
in non-labeled namespaces to mark them as ignored by YuniKorn, and the
scheduler will honor this annotation. This ensures that a namespace is
either processed completely by YuniKorn, or not at all
### What type of PR is it?
* [x] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1707
### How should this be tested?
Unit tests updated.
### Screenshots (if appropriate)
### 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] zhuqi-lucas commented on a diff in pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177902309
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Good suggestion, the other cases use this way.
```
// filter pods by scheduler name and state
func (os *Manager) filterPods(obj interface{}) bool {
switch obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if utils.GeneralPodFilter(pod) {
// only application ID is required
if _, err := utils.GetApplicationIDFromPod(pod); err == nil {
return true
}
}
return false
default:
return false
}
}
```
```
// filter pods by scheduler name and state
func (ctx *Context) filterPods(obj interface{}) bool {
switch obj := obj.(type) {
case *v1.Pod:
if utils.GeneralPodFilter(obj) {
_, err := utils.GetApplicationIDFromPod(obj)
return err == nil
}
return false
default:
return false
}
}
```
--
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 #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#issuecomment-1522277571
## [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/579?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 [#579](https://codecov.io/gh/apache/yunikorn-k8shim/pull/579?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (260c1c3) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/226b50c6e785dc2c5db26f94181c4f189f48e84b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (226b50c) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #579 +/- ##
==========================================
+ Coverage 69.94% 70.01% +0.07%
==========================================
Files 47 47
Lines 7935 7955 +20
==========================================
+ Hits 5550 5570 +20
Misses 2178 2178
Partials 207 207
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/579?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/admission/admission\_controller.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/579?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FkbWlzc2lvbi9hZG1pc3Npb25fY29udHJvbGxlci5nbw==) | `66.20% <100.00%> (+0.89%)` | :arrow_up: |
| [pkg/common/utils/utils.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/579?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==) | `72.89% <100.00%> (+0.71%)` | :arrow_up: |
: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 #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177526687
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
We should add this in the `utils.GeneralPodFilter(obj)`.
Any pod flows through that filter first. It removes the pod from the internal YuniKorn handling. Pods that do not pass that filter do not end up in the cache either (see FilteringResourceEventHandler).
The plugins (Filter, PreFilter and PostBind) should also use the `GeneralPodFilter`. If the general pod filter passes an application ID must always be there.
It gives us a more consistent way of handling pods. It also removes pods from the cache that we want to bypass YuniKorn.
--
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] zhuqi-lucas commented on a diff in pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177902309
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Good suggestion, and GeneralPodFilter is ahead of GetApplicationIDFromPod.
```
// filter pods by scheduler name and state
func (os *Manager) filterPods(obj interface{}) bool {
switch obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if utils.GeneralPodFilter(pod) {
// only application ID is required
if _, err := utils.GetApplicationIDFromPod(pod); err == nil {
return true
}
}
return false
default:
return false
}
}
```
--
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 #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
URL: https://github.com/apache/yunikorn-k8shim/pull/579
--
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] zhuqi-lucas commented on a diff in pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177902309
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Good suggestion, the Manager class uses this way.
```
// filter pods by scheduler name and state
func (os *Manager) filterPods(obj interface{}) bool {
switch obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if utils.GeneralPodFilter(pod) {
// only application ID is required
if _, err := utils.GetApplicationIDFromPod(pod); err == nil {
return true
}
}
return false
default:
return false
}
}
```
--
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] zhuqi-lucas commented on a diff in pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177902309
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Good suggestion, and GeneralPodFilter is ahead of GetApplicationIDFromPod.
// filter pods by scheduler name and state
func (os *Manager) filterPods(obj interface{}) bool {
switch obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if utils.GeneralPodFilter(pod) {
// only application ID is required
if _, err := utils.GetApplicationIDFromPod(pod); err == nil {
return true
}
}
return false
default:
return false
}
}
--
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 #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177991849
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Merged the two functions in the latest 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] zhuqi-lucas commented on a diff in pull request #579: [YUNIKORN-1707] Ignore pods in non-labeled namespaces
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #579:
URL: https://github.com/apache/yunikorn-k8shim/pull/579#discussion_r1177902309
##########
pkg/common/utils/utils.go:
##########
@@ -109,6 +109,16 @@ func GetQueueNameFromPod(pod *v1.Pod) string {
}
func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
+ // if pod was tagged with ignore-application, return
+ if value := GetPodAnnotationValue(pod, constants.AnnotationIgnoreApplication); value != "" {
+ ignore, err := strconv.ParseBool(value)
+ if err != nil {
+ log.Logger().Warn("Failed to parse annotation "+constants.AnnotationIgnoreApplication, zap.Error(err))
+ } else if ignore {
+ return "", nil
+ }
+ }
+
Review Comment:
Good suggestion, the other cases use GeneralPodFilter ahead GetApplicationIDFromPod.
```
// filter pods by scheduler name and state
func (os *Manager) filterPods(obj interface{}) bool {
switch obj.(type) {
case *v1.Pod:
pod := obj.(*v1.Pod)
if utils.GeneralPodFilter(pod) {
// only application ID is required
if _, err := utils.GetApplicationIDFromPod(pod); err == nil {
return true
}
}
return false
default:
return false
}
}
```
```
// filter pods by scheduler name and state
func (ctx *Context) filterPods(obj interface{}) bool {
switch obj := obj.(type) {
case *v1.Pod:
if utils.GeneralPodFilter(obj) {
_, err := utils.GetApplicationIDFromPod(obj)
return err == nil
}
return false
default:
return false
}
}
```
--
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