You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/08/11 13:40:26 UTC
[GitHub] [incubator-yunikorn-core] kingamarton opened a new pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
kingamarton opened a new pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297
### What is this PR for?
To bypass the “placement” phase, the simplest approach is to register a node with infinite resources, so the pod resource always fits into this node.
### What type of PR is it?
* [ ] - Bug Fix
* [X] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-791
### 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] [incubator-yunikorn-core] kingamarton commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-900202092
I have addressed your comments @wilfred-s . Thank you for the hints. I am writing now some new tests for the neewly introduced parts.
--
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] [incubator-yunikorn-core] kingamarton commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r691114541
##########
File path: pkg/scheduler/context.go
##########
@@ -844,3 +888,16 @@ func (cc *ClusterContext) GetNode(nodeID, partitionName string) *objects.Node {
}
return partition.GetNode(nodeID)
}
+
+func (cc *ClusterContext) checkForUnlimitedNodes() bool {
Review comment:
We need to check for unlimited nodes cluster wide, no? If we moove it per partition, we might have an unlimited node in some other partition, than the onee we are checking. Is this acceptable?
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
It is in the loop. I can moove itt after we check the partition, however we need some checks what did not depend on the partition, such as check if reservation is enabled, and check if theerare other nodes in teh `NewSchedulableNodes` list. I think we need this check as well, because if we check only the already registered nodes, the processing of the nodes will depend on the processing order and we might have different behaviours with the same imput list, what is not good.
I will moove the already reeegistered node check after we have the partition, and I will make it a per partition check, and I will keep the other checks at the beginning, since we don't need the partition for those checks and we can fail it earlier.
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
I can moove itt after we check the partition, however we need some checks what did not depend on the partition, such as check if reservation is enabled, and check if theere are other nodes in teh `NewSchedulableNodes` list. I think we need this check as well, because if we check only the already registered nodes, the processing of the nodes will depend on the processing order and we might have different behaviours with the same imput list, what is not good.
I will moove the already reeegistered node check after we have the partition, and I will make it a per partition check, and I will keep the other checks at the beginning, since we don't need the partition for those checks and we can fail it earlier.
--
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] [incubator-yunikorn-core] kingamarton edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-900202092
I have addressed your comments @wilfred-s . Thank you for the hints. I am writing now some new tests for the newly introduced parts.
--
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] [incubator-yunikorn-core] kingamarton commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r691131049
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
It is in the loop. I can moove itt after we check the partition, however we need some checks what did not depend on the partition, such as check if reservation is enabled, and check if theerare other nodes in teh `NewSchedulableNodes` list. I think we need this check as well, because if we check only the already registered nodes, the processing of the nodes will depend on the processing order and we might have different behaviours with the same imput list, what is not good.
I will moove the already reeegistered node check after we have the partition, and I will make it a per partition check, and I will keep the other checks at the beginning, since we don't need the partition for those checks and we can fail it earlier.
--
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] [incubator-yunikorn-core] yangwwei commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-898659170
Nice, if that works then it will be perfect. Sounds good to me. @kingamarton could you pls try to make the changes and see if there is any gap to do so? If that also makes sense to you. Thanks
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92de376) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.98%`.
> The diff coverage is `66.02%`.
> :exclamation: Current head 92de376 differs from pull request most recent head 3f8e983. Consider uploading reports for the commit 3f8e983 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.45% +1.98%
==========================================
Files 60 61 +1
Lines 5220 5732 +512
==========================================
+ Hits 3313 3752 +439
- Misses 1747 1787 +40
- Partials 160 193 +33
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.68% <0.00%> (-0.58%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `60.79% <55.00%> (-0.79%)` | :arrow_down: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?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=) | `79.91% <60.00%> (-1.91%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...3f8e983](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] yangwwei commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r690803242
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
+ unlimitedNodeRegistered := cc.checkForUnlimitedNodes()
+ if unlimitedNodeRegistered {
+ for _, node := range request.NewSchedulableNodes {
+ rejectedNodes = append(rejectedNodes, &si.RejectedNode{
+ NodeID: node.NodeID,
+ Reason: "There is an unlimited node registered, registering regular nodes is forbidden",
+ })
+ }
+ cc.rmEventHandler.HandleEvent(
+ &rmevent.RMNodeUpdateEvent{
+ RmID: request.RmID,
+ AcceptedNodes: acceptedNodes,
+ RejectedNodes: rejectedNodes,
+ })
+ return
+ }
for _, node := range request.NewSchedulableNodes {
sn := objects.NewNode(node)
+ // if the node is unlimited, check the following things:
+ // 1. reservation is enabled or not. If yes, reject the node
+ // 2. wre there any other nodes in the list, if yes reject the node
Review comment:
typo "wre" -> "were"
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4272d80) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.51%`.
> The diff coverage is `63.50%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 64.97% +1.51%
==========================================
Files 60 61 +1
Lines 5220 5745 +525
==========================================
+ Hits 3313 3733 +420
- Misses 1747 1822 +75
- Partials 160 190 +30
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.25% <0.00%> (-1.02%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [19 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...4272d80](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92de376) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.98%`.
> The diff coverage is `66.02%`.
> :exclamation: Current head 92de376 differs from pull request most recent head 4272d80. Consider uploading reports for the commit 4272d80 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.45% +1.98%
==========================================
Files 60 61 +1
Lines 5220 5732 +512
==========================================
+ Hits 3313 3752 +439
- Misses 1747 1787 +40
- Partials 160 193 +33
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.68% <0.00%> (-0.58%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `60.79% <55.00%> (-0.79%)` | :arrow_down: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?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=) | `79.91% <60.00%> (-1.91%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...4272d80](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s closed pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297
--
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] [incubator-yunikorn-core] wilfred-s commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-897432781
yes that is exactly what I mean
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59f0d38) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.47%`.
> The diff coverage is `67.61%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.94% +2.47%
==========================================
Files 60 61 +1
Lines 5220 5738 +518
==========================================
+ Hits 3313 3784 +471
- Misses 1747 1762 +15
- Partials 160 192 +32
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `16.10% <23.33%> (+9.84%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [20 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...59f0d38](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r690899105
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
This needs to be moved into the loop: nodes are linked to partitions. I _could_ have an unlimited node on one partition but not on the other:
- create a new node object
- get the partition: fail if partition not found (existing behaviour)
- check for the unlimited and limited combination:
- if new node is unlimited registered partition nodes must be 0
- if new node is limited registered nodes = 0 or larger than 1 OK otherwise check the single node and make sure it is not unlimited
- reject / accept nodes are processed as per the normal flow from this point
##########
File path: pkg/common/resources/resources.go
##########
@@ -403,6 +403,13 @@ func FitIn(larger, smaller *Resource) bool {
return larger.fitIn(smaller, false)
}
+// Check if smaller fits in larger
+// Types not defined in the larger resource are considered 0 values for Quantity
+// A nil resource is treated as max resource
+func FitInUndef(larger, smaller *Resource) bool {
Review comment:
This is the same as calling `larger.FitInMaxUndef(smaller)` I don't think we need a new function for that.
##########
File path: pkg/scheduler/context.go
##########
@@ -844,3 +888,16 @@ func (cc *ClusterContext) GetNode(nodeID, partitionName string) *objects.Node {
}
return partition.GetNode(nodeID)
}
+
+func (cc *ClusterContext) checkForUnlimitedNodes() bool {
Review comment:
This check should be per partition. A node is linked to one partition. The call for this check must be after the partition retrieval above, see previous comment
--
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] [incubator-yunikorn-core] kingamarton commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-899517345
@wilfred-s , related your remark about not allowing more than one node to be registered if we have a node with infinite resources:
- what should happen with the already registered nodes? It might happen then when we register the infinite node, there are already some nodes registered. In this case we should reject the unlimited node registration, or we should delete the other nodes?
--
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] [incubator-yunikorn-core] wilfred-s commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-900001647
> * what should happen with the already registered nodes? It might happen then when we register the infinite node, there are already some nodes registered. In this case we should reject the unlimited node registration, or we should delete the other nodes?
I think we need to reject the new node. You can only have an unlimited node registration as the first node in the cluster. After that it could cause all kinds of issues.
I do take into account also that we cannot have reservations on either. We have thus some configs to check and fail an unlimited node registration in a number of cases
> The nil value in the resource calculation is handled as zero resource, so the check if the allocation fits in the node will fail: https://github.com/apache/incubator-yunikorn-core/blob/master/pkg/common/resources/resources.go#L417-L419
Since the resource calculation is used in so many places, I think it is safer to implement the checks in multiple places than check the nil value meaning.
The problem with implementing a check in a number of places is that it is easy to change the logic and break or bypass the checks. If we can get to a simple construct like what we did for the queue quota check with stating undefined is interpreted as not limited i.e. maximum value then we cannot break things.
We only have two places in the node where we check the available resources that is in `preAllocateCheck` and `AddAllocation`. In the current change you have 8 points to check for an unlimited node. Even special casing the call to `FitIn` with a nil check before the call would bring it down from 8 to 2. Those two spots are easily documented and simple to maintain.
--
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] [incubator-yunikorn-core] kingamarton edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-900202092
I have addressed your comments @wilfred-s . Thank you for the hints. I am writing now some new tests for the newly introduced parts.
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4272d80) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.51%`.
> The diff coverage is `63.50%`.
> :exclamation: Current head 4272d80 differs from pull request most recent head fbbb879. Consider uploading reports for the commit fbbb879 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 64.97% +1.51%
==========================================
Files 60 61 +1
Lines 5220 5745 +525
==========================================
+ Hits 3313 3733 +420
- Misses 1747 1822 +75
- Partials 160 190 +30
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.25% <0.00%> (-1.02%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [19 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...fbbb879](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r690899105
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
This needs to be moved into the loop: nodes are linked to partitions. I _could_ have an unlimited node on one partition but not on the other:
- create a new node object
- get the partition: fail if partition not found (existing behaviour)
- check for the unlimited and limited combination:
- if new node is unlimited registered partition nodes must be 0
- if new node is limited registered nodes = 0 or larger than 1 OK otherwise check the single node and make sure it is not unlimited
- reject / accept nodes are processed as per the normal flow from this point
##########
File path: pkg/common/resources/resources.go
##########
@@ -403,6 +403,13 @@ func FitIn(larger, smaller *Resource) bool {
return larger.fitIn(smaller, false)
}
+// Check if smaller fits in larger
+// Types not defined in the larger resource are considered 0 values for Quantity
+// A nil resource is treated as max resource
+func FitInUndef(larger, smaller *Resource) bool {
Review comment:
This is the same as calling `larger.FitInMaxUndef(smaller)` I don't think we need a new function for that.
##########
File path: pkg/scheduler/context.go
##########
@@ -844,3 +888,16 @@ func (cc *ClusterContext) GetNode(nodeID, partitionName string) *objects.Node {
}
return partition.GetNode(nodeID)
}
+
+func (cc *ClusterContext) checkForUnlimitedNodes() bool {
Review comment:
This check should be per partition. A node is linked to one partition. The call for this check must be after the partition retrieval above, see previous comment
--
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] [incubator-yunikorn-core] kingamarton commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r691131049
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
Review comment:
I can moove itt after we check the partition, however we need some checks what did not depend on the partition, such as check if reservation is enabled, and check if theere are other nodes in teh `NewSchedulableNodes` list. I think we need this check as well, because if we check only the already registered nodes, the processing of the nodes will depend on the processing order and we might have different behaviours with the same imput list, what is not good.
I will moove the already reeegistered node check after we have the partition, and I will make it a per partition check, and I will keep the other checks at the beginning, since we don't need the partition for those checks and we can fail it earlier.
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92de376) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.98%`.
> The diff coverage is `66.02%`.
> :exclamation: Current head 92de376 differs from pull request most recent head 30118ab. Consider uploading reports for the commit 30118ab to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.45% +1.98%
==========================================
Files 60 61 +1
Lines 5220 5732 +512
==========================================
+ Hits 3313 3752 +439
- Misses 1747 1787 +40
- Partials 160 193 +33
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.68% <0.00%> (-0.58%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `60.79% <55.00%> (-0.79%)` | :arrow_down: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?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=) | `79.91% <60.00%> (-1.91%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...30118ab](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] yangwwei commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-897350589
thank you @kingamarton for working on this, and thank you @wilfred-s for the review.
regarding the comments
> Can we not set the availableResource to nil and leave the normal processing flow unchanged?
@wilfred-s do you mean to set node available resources to nil? and that will represent infinite resources?
--
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] [incubator-yunikorn-core] kingamarton commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-899526564
@wilfred-s
> In the solution chosen you special case each action. This bypasses updating allocated resources completely. That means lots of small changes everywhere. Resource calculations should all be nil safe (if not that is a bug on that calculation). Can we not set the `availableResource` to nil and leave the normal processing flow unchanged? That would remove a lot of the maintenance burden for later and the node will still show "normal" values when displayed in the web UI.
The nil value in the resource calculation is handled as zero resource, so the check if the allocation fits in the node will fail: https://github.com/apache/incubator-yunikorn-core/blob/master/pkg/common/resources/resources.go#L417-L419
Since the resource calculation is used in so many places, I think it is safer to implement the checks in multiple places than check the nil value meaning.
--
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] [incubator-yunikorn-core] yangwwei commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r690803242
##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
acceptedNodes := make([]*si.AcceptedNode, 0)
rejectedNodes := make([]*si.RejectedNode, 0)
+ // if we have an unlimited node, reject all new nodes
+ // if we have an unlimited node between the new nodes, register only that node
+ unlimitedNodeRegistered := cc.checkForUnlimitedNodes()
+ if unlimitedNodeRegistered {
+ for _, node := range request.NewSchedulableNodes {
+ rejectedNodes = append(rejectedNodes, &si.RejectedNode{
+ NodeID: node.NodeID,
+ Reason: "There is an unlimited node registered, registering regular nodes is forbidden",
+ })
+ }
+ cc.rmEventHandler.HandleEvent(
+ &rmevent.RMNodeUpdateEvent{
+ RmID: request.RmID,
+ AcceptedNodes: acceptedNodes,
+ RejectedNodes: rejectedNodes,
+ })
+ return
+ }
for _, node := range request.NewSchedulableNodes {
sn := objects.NewNode(node)
+ // if the node is unlimited, check the following things:
+ // 1. reservation is enabled or not. If yes, reject the node
+ // 2. wre there any other nodes in the list, if yes reject the node
Review comment:
typo "wre" -> "were"
--
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3478994) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.52%`.
> The diff coverage is `67.75%`.
> :exclamation: Current head 3478994 differs from pull request most recent head 59f0d38. Consider uploading reports for the commit 59f0d38 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.98% +2.52%
==========================================
Files 60 61 +1
Lines 5220 5745 +525
==========================================
+ Hits 3313 3791 +478
- Misses 1747 1762 +15
- Partials 160 192 +32
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `17.94% <30.00%> (+11.67%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [20 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...59f0d38](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4272d80) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.51%`.
> The diff coverage is `63.50%`.
> :exclamation: Current head 4272d80 differs from pull request most recent head 3478994. Consider uploading reports for the commit 3478994 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 64.97% +1.51%
==========================================
Files 60 61 +1
Lines 5220 5745 +525
==========================================
+ Hits 3313 3733 +420
- Misses 1747 1822 +75
- Partials 160 190 +30
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.25% <0.00%> (-1.02%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [19 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...3478994](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3478994) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `2.52%`.
> The diff coverage is `67.75%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.98% +2.52%
==========================================
Files 60 61 +1
Lines 5220 5745 +525
==========================================
+ Hits 3313 3791 +478
- Misses 1747 1762 +15
- Partials 160 192 +32
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `17.94% <30.00%> (+11.67%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `64.14% <65.27%> (+10.00%)` | :arrow_up: |
| [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.44% <65.44%> (+8.53%)` | :arrow_up: |
| ... and [20 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...3478994](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] wilfred-s commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-897269967
I have a couple of remarks:
* We must not allow more than one node to be registered if we have a node with infinite resources. Node sorting would become a problem. This needs to be enforced in the code.
* I doubt that we can have reservations turned on. The whole node would be reserved and thus things would stop. Reservations would need to be turned off completely.
In the solution chosen you special case each action. This bypasses updating allocated resources completely. That means lots of small changes everywhere. Resource calculations should all be nil safe (if not that is a bug on that calculation). Can we not set the `availableResource` to nil and leave the normal processing flow unchanged? That would remove a lot of the maintenance burden for later and the node will still show "normal" values when displayed in the web UI.
--
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] [incubator-yunikorn-core] codecov[bot] commented on pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#issuecomment-896838583
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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 [#297](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92de376) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5a1c19e) will **increase** coverage by `1.98%`.
> The diff coverage is `66.02%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
+ Coverage 63.46% 65.45% +1.98%
==========================================
Files 60 61 +1
Lines 5220 5732 +512
==========================================
+ Hits 3313 3752 +439
- Misses 1747 1787 +40
- Partials 160 193 +33
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?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/common/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi91dGlscy5nbw==) | `77.50% <0.00%> (ø)` | |
| [pkg/metrics/init.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvaW5pdC5nbw==) | `62.96% <ø> (ø)` | |
| [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.68% <0.00%> (-0.58%)` | :arrow_down: |
| [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
| [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `32.25% <ø> (+19.35%)` | :arrow_up: |
| [pkg/metrics/metrics\_collector.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3MvbWV0cmljc19jb2xsZWN0b3IuZ28=) | `64.51% <45.00%> (-9.40%)` | :arrow_down: |
| [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
| [pkg/metrics/scheduler.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL21ldHJpY3Mvc2NoZWR1bGVyLmdv) | `60.79% <55.00%> (-0.79%)` | :arrow_down: |
| [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?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=) | `79.91% <60.00%> (-1.91%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [eaa6afb...92de376](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/297?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-yunikorn-core] kingamarton commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r691114541
##########
File path: pkg/scheduler/context.go
##########
@@ -844,3 +888,16 @@ func (cc *ClusterContext) GetNode(nodeID, partitionName string) *objects.Node {
}
return partition.GetNode(nodeID)
}
+
+func (cc *ClusterContext) checkForUnlimitedNodes() bool {
Review comment:
We need to check for unlimited nodes cluster wide, no? If we moove it per partition, we might have an unlimited node in some other partition, than the onee we are checking. Is this acceptable?
--
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