-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clusterloader2: check labels additionally to decide master node #1196
Clusterloader2: check labels additionally to decide master node #1196
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @xial-thu! |
Hi @xial-thu. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @wojtek-t |
"I signed it" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
@@ -86,7 +86,8 @@ func (s *schedulerLatencyMeasurement) Execute(config *measurement.MeasurementCon | |||
|
|||
var masterRegistered = false | |||
for _, node := range nodes.Items { | |||
if util.LegacyIsMasterNode(node.Name) { | |||
node := node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove it - it's unnecessary copy that isn't needed if the LegacyIsMasterNode isn't mutating the object (which it obviously shouldn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -103,12 +103,26 @@ func NewResourceUsageGatherer(c clientset.Interface, host string, port int, prov | |||
return nil, fmt.Errorf("listing pods error: %v", err) | |||
} | |||
} | |||
|
|||
var nodeList *corev1.NodeList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove it and simply change "=" to ":=" in the list call below
@@ -103,12 +103,26 @@ func NewResourceUsageGatherer(c clientset.Interface, host string, port int, prov | |||
return nil, fmt.Errorf("listing pods error: %v", err) | |||
} | |||
} | |||
|
|||
var nodeList *corev1.NodeList | |||
var masterNodesMap map[string]bool = make(map[string]bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move after the list (if the list call fails you have unnecessary memory allocation).
Also, change to simply:
masterNodesMap := mak(map[string]bool)
or even better reuse string set:
https://github.com/kubernetes/perf-tests/blob/master/clusterloader2/vendor/k8s.io/apimachinery/pkg/util/sets/string.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lessons are learned
return nil, fmt.Errorf("listing nodes error: %v", err) | ||
} | ||
for _, node := range nodeList.Items { | ||
node := node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid copy if not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
for _, node := range nodeList.Items { | ||
if options.Nodes == AllNodes || pkgutil.LegacyIsMasterNode(node.Name) || dnsNodes[node.Name] { | ||
node := node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid copy if not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not done
clusterloader2/pkg/util/cluster.go
Outdated
@@ -165,11 +167,19 @@ func GetMasterIPs(c clientset.Interface, addressType corev1.NodeAddressType) ([] | |||
// This code will not be allowed to update to use the node-role label, since | |||
// node-roles may not be used for feature enablement. | |||
// DEPRECATED: this will be removed in Kubernetes 1.19 | |||
func LegacyIsMasterNode(nodeName string) bool { | |||
func LegacyIsMasterNode(node *corev1.Node) bool { | |||
// Check labels as an additional signal, fix #1191 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove bug number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
func LegacyIsMasterNode(node *corev1.Node) bool { | ||
// Check labels as an additional signal, fix #1191 | ||
for key := range node.GetLabels() { | ||
if key == keyMasterNodeLabel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the value too - it can be set to false too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how to define its value? "false", "False", "No"... It can be set to anything
I would compare ToLower(value) with "false"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go the other way. If it's set "node-role.kubernetes.io/master=skfjds", then I wouldn't treat it as master.
Let's change explicitly to:
strings.ToLower(val) == "true"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more minor comments.
|
||
for _, node := range nodeList.Items { | ||
if options.Nodes == AllNodes || pkgutil.LegacyIsMasterNode(node.Name) || dnsNodes[node.Name] { | ||
node := node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not done
func LegacyIsMasterNode(node *corev1.Node) bool { | ||
// Check labels as an additional signal, fix #1191 | ||
for key := range node.GetLabels() { | ||
if key == keyMasterNodeLabel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go the other way. If it's set "node-role.kubernetes.io/master=skfjds", then I wouldn't treat it as master.
Let's change explicitly to:
strings.ToLower(val) == "true"
Some utils like kubeadm will label master node as "node-role.kubernetes.io/master=''" so it would be better to check labels as additional signal. If that label is not set, it does not affect the existing logic.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wojtek-t, xial-thu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Some utils like kubeadm will label master node as
"node-role.kubernetes.io/master=''" so it would be better to check
labels as additional signal. If that label is not set, it does not
affect the existing logic.
fix #1191