You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "wilfred-s (via GitHub)" <gi...@apache.org> on 2023/08/09 09:53:45 UTC
[GitHub] [yunikorn-k8shim] wilfred-s opened a new pull request, #651: [YUNIKORN-1908] Support GPU in placeholders
wilfred-s opened a new pull request, #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651
### What is this PR for?
Extended resources and huge pages can not over commit. The pod validator checks for a Limit setting in the pod spec. The pod fails validation without it. Placeholder creation must make sure that the pods pass the validator.
Set limits for all extended resources and hugepages-* as they cannot be over committed.
Clean up of duplicated task group definition in the test.
### What type of PR is it?
* [X] - Bug Fix
### What is the Jira issue?
* https://issues.apache.org/jira/browse/YUNIKORN-1908
### How should this be tested?
Unit test added
e2e test for placeholders should be extended but we do not have GPU nodes in kind, follow up jira logged to
--
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 #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#discussion_r1290862948
##########
pkg/common/utils/gang_utils.go:
##########
@@ -32,6 +32,11 @@ import (
"github.com/apache/yunikorn-k8shim/pkg/log"
)
+const (
+ k8sDomain = "kubernetes.io"
+ hugepagesPrefix = "hugepages-"
Review Comment:
done
--
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 #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #651: [YUNIKORN-1908] Support GPU in placeholders
URL: https://github.com/apache/yunikorn-k8shim/pull/651
--
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] pbacsko commented on a diff in pull request #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#discussion_r1288301086
##########
pkg/cache/placeholder_test.go:
##########
@@ -33,26 +33,71 @@ import (
siCommon "github.com/apache/yunikorn-scheduler-interface/lib/go/common"
)
+const (
+ gpu = "nvidia.com/gpu"
+ ephemeral = "ephemeral-storage"
+ hugepages = "hugepages-1Gi"
Review Comment:
Nit: these can referenced from core/v1: `v1.ResourceEphemeralStorage` / `v1.ResourceHugePagesPrefix`
--
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 #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#issuecomment-1671035684
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#651](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (cf92d53) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/06f4e58a07d286bc352c5027d90193e65ee695df?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (06f4e58) will **increase** coverage by `0.07%`.
> The diff coverage is `89.47%`.
```diff
@@ Coverage Diff @@
## master #651 +/- ##
==========================================
+ Coverage 71.33% 71.40% +0.07%
==========================================
Files 51 51
Lines 8104 8117 +13
==========================================
+ Hits 5781 5796 +15
+ Misses 2127 2124 -3
- Partials 196 197 +1
```
| [Files Changed](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/common/utils/gang\_utils.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `96.92% <86.66%> (+7.83%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `93.15% <100.00%> (+0.29%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/651/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] wilfred-s commented on a diff in pull request #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#discussion_r1290844019
##########
pkg/cache/placeholder_test.go:
##########
@@ -33,26 +33,71 @@ import (
siCommon "github.com/apache/yunikorn-scheduler-interface/lib/go/common"
)
+const (
+ gpu = "nvidia.com/gpu"
+ ephemeral = "ephemeral-storage"
+ hugepages = "hugepages-1Gi"
Review Comment:
No, in the check we only look at the prefix `hugepages-` anything with that prefix is filtered out
--
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] pbacsko commented on a diff in pull request #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#discussion_r1288303123
##########
pkg/cache/placeholder_test.go:
##########
@@ -33,26 +33,71 @@ import (
siCommon "github.com/apache/yunikorn-scheduler-interface/lib/go/common"
)
+const (
+ gpu = "nvidia.com/gpu"
+ ephemeral = "ephemeral-storage"
+ hugepages = "hugepages-1Gi"
Review Comment:
BTW, "hugepages-xxx" can have multiple values, do we only care about `1Gi`?
--
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] pbacsko commented on a diff in pull request #651: [YUNIKORN-1908] Support GPU in placeholders
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #651:
URL: https://github.com/apache/yunikorn-k8shim/pull/651#discussion_r1288302285
##########
pkg/common/utils/gang_utils.go:
##########
@@ -32,6 +32,11 @@ import (
"github.com/apache/yunikorn-k8shim/pkg/log"
)
+const (
+ k8sDomain = "kubernetes.io"
+ hugepagesPrefix = "hugepages-"
Review Comment:
Nit: these can be referenced by (core/v1) `v1.ResourceHugePagesPrefix`
--
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