You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2023/09/02 08:26:18 UTC
[GitHub] [yunikorn-k8shim] FrankYang0529 opened a new pull request, #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
FrankYang0529 opened a new pull request, #666:
URL: https://github.com/apache/yunikorn-k8shim/pull/666
### What is this PR for?
Placeholder pods can't be created in a job with non-zero value PriorityClass. It got following error in the yunikorn-k8shim:
```
2023-09-02T07:11:57.330Z ERROR shim.cache.placeholder cache/placeholder_manager.go:99 failed to create placeholder pod {"error": "pods \"tg-group-low-app-low-0\" is forbidden: the integer value of priority (-100) must not be provided in pod spec; priority admission controller computed 0 from the given PriorityClass name"}
```
### What type of PR is it?
* [X] - Bug Fix
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1953
### How should this be tested?
Covered by unit tests.
E2E test will be covered by https://github.com/apache/yunikorn-k8shim/pull/659.
--
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] FrankYang0529 commented on a diff in pull request #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #666:
URL: https://github.com/apache/yunikorn-k8shim/pull/666#discussion_r1315779362
##########
pkg/cache/placeholder_manager_test.go:
##########
@@ -110,11 +112,12 @@ func createAndCheckPlaceholderCreate(mockedAPIProvider *client.MockedAPIProvider
err := placeholderMgr.createAppPlaceholders(app)
assert.NilError(t, err, "create app placeholders should be successful")
assert.Equal(t, len(createdPods), 30)
- var priority *int32
for _, tg := range []string{"tg-test-group-1-app01-", "tg-test-group-2-app01-"} {
for i := 2; i <= 9; i++ {
podName := tg + strconv.Itoa(i)
- assert.Equal(t, priority, createdPods[podName].Spec.Priority, "Pod should not have been created")
+ var priority *int32
Review Comment:
Updated it. Thank you.
--
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 #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #666:
URL: https://github.com/apache/yunikorn-k8shim/pull/666#issuecomment-1703765658
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/666?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#666](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/666?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1de960b) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/8b26c373b4b5ba1c1462cb779a3f467ee0ae6c1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8b26c37) will **decrease** coverage by `0.03%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #666 +/- ##
==========================================
- Coverage 71.87% 71.85% -0.03%
==========================================
Files 51 51
Lines 8079 8079
==========================================
- Hits 5807 5805 -2
- Misses 2074 2076 +2
Partials 198 198
```
| [Files Changed](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `93.58% <100.00%> (ø)` | |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/666/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] manirajv06 commented on a diff in pull request #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 commented on code in PR #666:
URL: https://github.com/apache/yunikorn-k8shim/pull/666#discussion_r1315738504
##########
pkg/cache/placeholder_manager_test.go:
##########
@@ -110,11 +112,12 @@ func createAndCheckPlaceholderCreate(mockedAPIProvider *client.MockedAPIProvider
err := placeholderMgr.createAppPlaceholders(app)
assert.NilError(t, err, "create app placeholders should be successful")
assert.Equal(t, len(createdPods), 30)
- var priority *int32
for _, tg := range []string{"tg-test-group-1-app01-", "tg-test-group-2-app01-"} {
for i := 2; i <= 9; i++ {
podName := tg + strconv.Itoa(i)
- assert.Equal(t, priority, createdPods[podName].Spec.Priority, "Pod should not have been created")
+ var priority *int32
Review Comment:
let's keep this declaration before line no. 115.
--
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] manirajv06 closed pull request #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 closed pull request #666: [YUNIKORN-1953] replace priority with priorityclass in placeholder
URL: https://github.com/apache/yunikorn-k8shim/pull/666
--
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