You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2023/02/01 03:31:40 UTC
[GitHub] [yunikorn-k8shim] zhuqi-lucas opened a new pull request, #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
zhuqi-lucas opened a new pull request, #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519
### What is this PR for?
he k8shim in [YUNIKORN-1389](https://issues.apache.org/jira/browse/YUNIKORN-1389) was extended to pass on the K8s node labels to the core via the attributes. The K8s node labels are marshalled into a single json string. The string is added using a new key.
There are multiple issues with that. The overhead of un-marshalling it each time a label needs to be checked for existence is large. It also needs a two step check to find a label. First find the label in the attributes and then un-marshal the string and do another check for the real key & value needed.
We should add the labels from the K8s node as entries in the attributes map. Each label becomes and entry not json marshalling.
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN-1550
* Put link here, and add [YUNIKORN-*Jira number*] in PR title, eg. `[YUNIKORN-2] Gang scheduling interface parameters`
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519#discussion_r1096927110
##########
pkg/cache/nodes.go:
##########
@@ -101,20 +100,13 @@ func (nc *schedulerNodes) addAndReportNode(node *v1.Node, reportNode bool) {
// add node to nodes map
if _, ok := nc.nodesMap[node.Name]; !ok {
- var nodeLabels []byte
- nodeLabels, err := json.Marshal(node.Labels) // A nil pointer encodes as the "null" JSON value.
- if err != nil {
- log.Logger().Error("failed to marshall node labels to json", zap.Error(err))
- nodeLabels = make([]byte, 0)
- }
-
log.Logger().Info("adding node to context",
zap.String("nodeName", node.Name),
- zap.String("nodeLabels", string(nodeLabels)),
+ zap.String("nodeLabels", fmt.Sprintf("%v", node.Labels)),
Review Comment:
Thank you @wilfred-s for review, good suggestion, addressed in latest PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519#discussion_r1096883584
##########
pkg/cache/nodes.go:
##########
@@ -101,20 +100,13 @@ func (nc *schedulerNodes) addAndReportNode(node *v1.Node, reportNode bool) {
// add node to nodes map
if _, ok := nc.nodesMap[node.Name]; !ok {
- var nodeLabels []byte
- nodeLabels, err := json.Marshal(node.Labels) // A nil pointer encodes as the "null" JSON value.
- if err != nil {
- log.Logger().Error("failed to marshall node labels to json", zap.Error(err))
- nodeLabels = make([]byte, 0)
- }
-
log.Logger().Info("adding node to context",
zap.String("nodeName", node.Name),
- zap.String("nodeLabels", string(nodeLabels)),
+ zap.String("nodeLabels", fmt.Sprintf("%v", node.Labels)),
Review Comment:
Leverage zap.Any() it can handle the map with less overhead.
There will be no string allocated etc if the level is not set to INFO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by codecov.
codecov[bot] commented on PR #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519#issuecomment-1411429298
# [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?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 [#519](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7b5e89) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/a9cdd44d9785c883f6bf4e0a926b824368132767?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a9cdd44) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #519 +/- ##
==========================================
+ Coverage 69.41% 69.43% +0.01%
==========================================
Files 45 45
Lines 7714 7712 -2
==========================================
Hits 5355 5355
+ Misses 2162 2161 -1
+ Partials 197 196 -1
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?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/cache/node.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGUuZ28=) | `86.77% <ø> (ø)` | |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL25vZGVzLmdv) | `81.81% <100.00%> (+2.08%)` | :arrow_up: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `97.60% <100.00%> (+0.04%)` | :arrow_up: |
| [pkg/dispatcher/dispatcher.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/519?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2Rpc3BhdGNoZXIvZGlzcGF0Y2hlci5nbw==) | `74.82% <0.00%> (-1.40%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s closed pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s closed pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
URL: https://github.com/apache/yunikorn-k8shim/pull/519
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519#issuecomment-1415818005
Thank you @pbacsko for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on pull request #519: [YUNIKORN-1550] pass k8s node labels as attributes not json
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #519:
URL: https://github.com/apache/yunikorn-k8shim/pull/519#issuecomment-1423602251
Hi @wilfred-s , @pbacsko , @craigcondit if any other suggestions about this PR? 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