craigcondit commented on code in PR #572:
URL: https://github.com/apache/yunikorn-k8shim/pull/572#discussion_r1172922725
##########
test/e2e/simple_preemptor/simple_preemptor_test.go:
##########
@@ -79,13 +81,29 @@ var _ = ginkgo.BeforeSuite(func() {
// Extract node allocatable resources
for _, node := range nodes.Items {
- if node.Name == Worker1 {
+ // skip master if it's marked as such
+ node := node
+ if k8s.IsMasterNode(&node) || !k8s.IsComputeNode(&node) {
+ continue
+ }
+ if Worker1 == "" {
Review Comment:
This code seems brittle. Can we just iterate the nodes and take the first
two matching candidates without all the special-casing?
##########
test/e2e/simple_preemptor/simple_preemptor_test.go:
##########
@@ -79,13 +81,29 @@ var _ = ginkgo.BeforeSuite(func() {
// Extract node allocatable resources
for _, node := range nodes.Items {
- if node.Name == Worker1 {
+ // skip master if it's marked as such
+ node := node
+ if k8s.IsMasterNode(&node) || !k8s.IsComputeNode(&node) {
Review Comment:
Isn't any node which is not a master node by definition a compute node? Why
the additional check?
##########
test/e2e/framework/helpers/k8s/k8s_utils.go:
##########
@@ -1292,10 +1292,22 @@ func IsMasterNode(node *v1.Node) bool {
return true
}
}
-
return false
}
+func IsComputeNode(node *v1.Node) bool {
+ roleNodeLabelExists := false
+ for labelKey, labelValue := range node.Labels {
+ if labelKey == common.RoleNodeLabel {
+ roleNodeLabelExists = true
+ if _, ok := common.ComputeNodeLabels[labelValue]; ok {
+ return true
+ }
+ }
+ }
+ return !roleNodeLabelExists
Review Comment:
I think you can remove the variable and simply return `false` here. The
`return true` earlier should take care of the positive case.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
