Skip to content
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

Merged
merged 1 commit into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clusterloader2/pkg/measurement/common/scheduler_latency.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (s *schedulerLatencyMeasurement) Execute(config *measurement.MeasurementCon

var masterRegistered = false
for _, node := range nodes.Items {
if util.LegacyIsMasterNode(node.Name) {
if util.LegacyIsMasterNode(&node) {
masterRegistered = true
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
clientset "k8s.io/client-go/kubernetes"
"k8s.io/klog"
"k8s.io/perf-tests/clusterloader2/pkg/measurement/util"
Expand Down Expand Up @@ -103,12 +104,25 @@ func NewResourceUsageGatherer(c clientset.Interface, host string, port int, prov
return nil, fmt.Errorf("listing pods error: %v", err)
}
}

nodeList, err := c.CoreV1().Nodes().List(metav1.ListOptions{})
if err != nil {
return nil, fmt.Errorf("listing nodes error: %v", err)
}

masterNodes := sets.NewString()
for _, node := range nodeList.Items {
if pkgutil.LegacyIsMasterNode(&node) {
masterNodes.Insert(node.Name)
}
}

dnsNodes := make(map[string]bool)
for _, pod := range pods.Items {
if (options.Nodes == MasterNodes) && !pkgutil.LegacyIsMasterNode(pod.Spec.NodeName) {
if (options.Nodes == MasterNodes) && !masterNodes.Has(pod.Spec.NodeName) {
continue
}
if (options.Nodes == MasterAndDNSNodes) && !pkgutil.LegacyIsMasterNode(pod.Spec.NodeName) && pod.Labels["k8s-app"] != "kube-dns" {
if (options.Nodes == MasterAndDNSNodes) && !masterNodes.Has(pod.Spec.NodeName) && pod.Labels["k8s-app"] != "kube-dns" {
continue
}
for _, container := range pod.Status.InitContainerStatuses {
Expand All @@ -121,16 +135,12 @@ func NewResourceUsageGatherer(c clientset.Interface, host string, port int, prov
dnsNodes[pod.Spec.NodeName] = true
}
}
nodeList, err := c.CoreV1().Nodes().List(metav1.ListOptions{})
if err != nil {
return nil, fmt.Errorf("listing nodes error: %v", err)
}

for _, node := range nodeList.Items {
if options.Nodes == AllNodes || pkgutil.LegacyIsMasterNode(node.Name) || dnsNodes[node.Name] {
if options.Nodes == AllNodes || masterNodes.Has(node.Name) || dnsNodes[node.Name] {
g.workerWg.Add(1)
resourceDataGatheringPeriod := options.ResourceDataGatheringPeriod
if pkgutil.LegacyIsMasterNode(node.Name) {
if masterNodes.Has(node.Name) {
resourceDataGatheringPeriod = options.MasterResourceDataGatheringPeriod
}
g.workers = append(g.workers, resourceGatherWorker{
Expand Down
2 changes: 1 addition & 1 deletion clusterloader2/pkg/prometheus/prometheus.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ func (pc *PrometheusController) runNodeExporter() error {
numMasters := 0
for _, node := range nodes {
node := node
if util.LegacyIsMasterNode(node.Name) {
if util.LegacyIsMasterNode(&node) {
numMasters++
g.Go(func() error {
f, err := os.Open(os.ExpandEnv(nodeExporterPod))
Expand Down
15 changes: 12 additions & 3 deletions clusterloader2/pkg/util/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"k8s.io/perf-tests/clusterloader2/pkg/framework/client"
)

const keyMasterNodeLabel = "node-role.kubernetes.io/master"

// GetSchedulableUntainedNodesNumber returns number of nodes in the cluster.
func GetSchedulableUntainedNodesNumber(c clientset.Interface) (int, error) {
nodes, err := GetSchedulableUntainedNodes(c)
Expand Down Expand Up @@ -129,7 +131,7 @@ func GetMasterName(c clientset.Interface) (string, error) {
return "", err
}
for i := range nodeList {
if LegacyIsMasterNode(nodeList[i].Name) {
if LegacyIsMasterNode(&nodeList[i]) {
return nodeList[i].Name, nil
}
}
Expand All @@ -144,7 +146,7 @@ func GetMasterIPs(c clientset.Interface, addressType corev1.NodeAddressType) ([]
}
var ips []string
for i := range nodeList {
if LegacyIsMasterNode(nodeList[i].Name) {
if LegacyIsMasterNode(&nodeList[i]) {
for _, address := range nodeList[i].Status.Addresses {
if address.Type == addressType && address.Address != "" {
ips = append(ips, address.Address)
Expand All @@ -165,11 +167,18 @@ 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 {
for key, val := range node.GetLabels() {
if key == keyMasterNodeLabel {
Copy link
Member

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.

Copy link
Contributor Author

@xial-thu xial-thu Apr 29, 2020

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"

Copy link
Member

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"

return strings.ToLower(val) == "true"
}
}

// We are trying to capture "master(-...)?$" regexp.
// However, using regexp.MatchString() results even in more than 35%
// of all space allocations in ControllerManager spent in this function.
// That's why we are trying to be a bit smarter.
nodeName := node.GetName()
if strings.HasSuffix(nodeName, "master") {
return true
}
Expand Down