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/30 12:33:33 UTC
[GitHub] [yunikorn-k8shim] zhuqi-lucas opened a new pull request, #518: [YUNIKORN-1549] Introduce a config to specify what attribute to use as instance type
zhuqi-lucas opened a new pull request, #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518
### What is this PR for?
We need instance type for https://issues.apache.org/jira/browse/YUNIKORN-1385, and we need to make the node label instance type configuration.
### 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/
* 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-k8shim] zhuqi-lucas commented on pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1409695334
And the corresponding change in the core side is:
https://github.com/apache/yunikorn-core/pull/498
cc @wilfred-s @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] zhuqi-lucas commented on pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1408624622
Thank you @craigcondit , good suggestion, let me refactor to core and parse it in the core side.
--
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 pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1408712532
Hi @craigcondit , If i make sense right, we need to make it configuration in shim, the core will read the change if we add the config in shim side?
--
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 pull request #518: [YUNIKORN-1549] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on PR #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1410587193
There is NO change required on the shim side, as all configuration is passed along to the core as of v1.2.0, including unknown / extra variables. Define a constant in the scheduler interface for the value and default, and reference that value in the core.
-1 on 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] wilfred-s commented on pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1411218866
Let me clarify why I am a -1 on the change, this includes the change in the core:
The design of YuniKorn allows multiple shims to be registered to one core. Even if we do not use it currently we should not go and willingly break that. If you make this a configuration value that is processed on the core side every shim must use the same setting. That is wrong.
If you want to allow multiple possible labels to be used you need to hide that in the shim. Setup a well known attribute name to pass on to the core. Only use that well known attribute on the core side. The key for that value would be defined in the `scheduler interface` as a constant. That would at least not break the multiple shims with different settings. How we would fit that into the config is a different question.
Before we even go there you need to fix YUNIKORN-1550, the way the attributes are used to pass a json string is broken.
--
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 pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1409666135
Thank you @wilfred-s for review, if you mean we shouldn't pass all labels to core, we just need to pass which we needed to core, for example, here we pass the instanceType 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-k8shim] zhuqi-lucas commented on pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1427330020
Close it now.
We don't need the change in shim side, update the code change in core side.
--
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 closed pull request #518: [YUNIKORN-1549] Introduce a config to specify what attribute to use as instance type
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas closed pull request #518: [YUNIKORN-1549] Introduce a config to specify what attribute to use as instance type
URL: https://github.com/apache/yunikorn-k8shim/pull/518
--
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 pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1411389644
Thank you @wilfred-s @craigcondit , it makes sense to me now, let me do the YUNIKORN-1550 fix first, so that we don't need to pass the json string!
--
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 pull request #518: [YUNIKORN-1549] Introduce a config to specify what attribute to use as instance type
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit commented on PR #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1408569280
We already have all the node labels passed in for each node. Rather than specifying this configuration on every node creation, wouldn't it make more sense to just use the config on the core side? Then the correct key could be extracted there.
--
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 pull request #518: [YUNIKORN-1549] 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 #518:
URL: https://github.com/apache/yunikorn-k8shim/pull/518#issuecomment-1409651916
-1 on the proposal and implementation
The k8shim should pass all labels that are defined on a node, no filtering, as tags to the core.
That is what is implemented in YUNIKORN-1389. I do think that the way that jira is implemented is broken to begin with.
--
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