You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2023/01/31 03:18:38 UTC
[GitHub] [yunikorn-core] zhuqi-lucas opened a new pull request, #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
zhuqi-lucas opened a new pull request, #498:
URL: https://github.com/apache/yunikorn-core/pull/498
### What is this PR for?
We add a new config in https://github.com/apache/yunikorn-k8shim/pull/518 , the core side will use the extraconfig.
And we need to parse the instanceType in core side.
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - 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-1549
* Put link here, and add [YUNIKORN-*Jira number*] in PR title, eg. `[YUNIKORN-2] Gang scheduling interface parameters`
### How should this be tested?
### 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-core] codecov[bot] commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1429733019
# [Codecov](https://codecov.io/gh/apache/yunikorn-core/pull/498?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 [#498](https://codecov.io/gh/apache/yunikorn-core/pull/498?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab36c31) into [master](https://codecov.io/gh/apache/yunikorn-core/commit/87aff78ca890a4b3feabc3d5b3f91a9862ff1024?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87aff78) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #498 +/- ##
==========================================
+ Coverage 73.42% 73.45% +0.03%
==========================================
Files 69 69
Lines 10449 10451 +2
==========================================
+ Hits 7672 7677 +5
+ Misses 2529 2527 -2
+ Partials 248 247 -1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-core/pull/498?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/scheduler/objects/node.go](https://codecov.io/gh/apache/yunikorn-core/pull/498?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `80.44% <100.00%> (+0.10%)` | :arrow_up: |
| [pkg/scheduler/objects/sorters.go](https://codecov.io/gh/apache/yunikorn-core/pull/498?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3NvcnRlcnMuZ28=) | `97.00% <0.00%> (+1.79%)` | :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-core] craigcondit closed pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
URL: https://github.com/apache/yunikorn-core/pull/498
--
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-core] zhuqi-lucas commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1427331314
Hi @wilfred-s ,@craigcondit , https://github.com/apache/yunikorn-k8shim/pull/519 has been merged, and i have changed the code now.
And i will add the default value settings to yunikorn-site after this merged.
--
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-core] zhuqi-lucas commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1429734405
Thank you @craigcondit for review and clarify, i change the core side code to use the SI value:
'''
InstanceType = "si/instance-type"
'''
And the shim side will fill this value, if i make sense right?
--
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-core] craigcondit commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1428758135
@zhuqi-lucas, I'm now -1 on this approach. As I'm looking at this I see a problem. The core should not have any knowledge of the shim -- it is meant to be platform-agnostic, so we shouldn't have a Kubernetes-specific key or value present here. We already have an `InstanceType` constant in the SI that resolves to `si/instance-type`. The kubernetes-specific code should go in the shim, and be used to determine which node label or annotation should be used to populate the `si/instance-type` attribute. This way, no matter what implementation the shim uses, the core can always rely on a node's instance type (if specified) being present in that attribute.
--
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-core] craigcondit commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1429764608
> Thank you @craigcondit for review and clarify, i change the core side code to use the SI value: InstanceType = "si/instance-type"
>
> And the shim side will fill this value, if i make sense right?
Yes, the shim side code will need to be written yet, but this looks better.
--
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-core] craigcondit commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1410588873
Please add a constant to the SI for the configuration key and default value. There is NO need to change the shim at all - every configuration setting in the configmap is passed along to the core as of v1.2.0.
--
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-core] wilfred-s commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1411251021
-1 see the remarks in the K8shim PR.
First fix the attributes implementation and use a standardised label name.
--
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-core] yzhangal commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "yzhangal (via GitHub)" <gi...@apache.org>.
yzhangal commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1446832700
Thanks @craigcondit for approving the PR. HI @wilfred-s, would you please help reviewing again? thanks. FYI @zhuqi-lucas
--
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-core] zhuqi-lucas commented on pull request #498: [YUNIKORN-1549][core] Introduce a config to specify what attribute to use as instance type
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #498:
URL: https://github.com/apache/yunikorn-core/pull/498#issuecomment-1429791350
Thank you @craigcondit for quick checking, and the shim side change i submitted a new PR:
https://github.com/apache/yunikorn-k8shim/pull/531
--
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