Embedded load-balancer behavior is flakey and hard to understand

[brandond]

The loadbalancer server list is a bit of a mess. its behavior has been tinkered with a lot over the last year, but it's still hard to reason about. This has caused a spate of issues:

• Etcd load-balancer on control-plane-only server may select removed etcd cluster member as default server #9882
• Stopping service on ControlPlane node causes other nodes to go NotReady #9910
• Health checks may remove all available servers from load-balancer under high load #10240
• Loadbalancer may panic due to race condition when selecting a new server #10317
• Secondary etcd-only nodes do not reconnect to apiserver after outage if joined against an etcd-only node #11311
• etcd-only and agent nodes do not properly fail back during apiserver outage #11349

From a code perspective, the loadbalancer state is directly accessed by a number of functions that all poke at various index vars, current and default server name vars, a list of server addresses, another RANDOM list of server addresses, and a map of addresses to structs that hold state:

k3s/pkg/agent/loadbalancer/loadbalancer.go

Lines 43 to 53 in cd4dded

	serviceName string
	configFile string
	localAddress string
	localServerURL string
	defaultServerAddress string
	ServerURL string
	ServerAddresses []string
	randomServers []string
	servers map[string]*server
	currentServerAddress string
	nextServerIndex int

The DialContext function is called whenever a new connection comes in, and holds a read lock while iterating (possibly twice) over the random server list, and servers may be added or removed at any time. The code is VERY hard to read and understand, given the number of variables involved:

k3s/pkg/agent/loadbalancer/loadbalancer.go

Lines 162 to 208 in cd4dded

	var allChecksFailed bool
	startIndex := lb.nextServerIndex
	for {
	targetServer := lb.currentServerAddress
	server := lb.servers[targetServer]
	if server == nil || targetServer == "" {
	logrus.Debugf("Nil server for load balancer %s: %s", lb.serviceName, targetServer)
	} else if allChecksFailed || server.healthCheck() {
	dialTime := time.Now()
	conn, err := server.dialContext(ctx, network, targetServer)
	if err == nil {
	return conn, nil
	}
	logrus.Debugf("Dial error from load balancer %s after %s: %s", lb.serviceName, time.Now().Sub(dialTime), err)
	// Don't close connections to the failed server if we're retrying with health checks ignored.
	// We don't want to disrupt active connections if it is unlikely they will have anywhere to go.
	if !allChecksFailed {
	defer server.closeAll()
	}
	} else {
	logrus.Debugf("Dial health check failed for %s", targetServer)
	}
	newServer, err := lb.nextServer(targetServer)
	if err != nil {
	return nil, err
	}
	if targetServer != newServer {
	logrus.Debugf("Failed over to new server for load balancer %s: %s -> %s", lb.serviceName, targetServer, newServer)
	}
	if ctx.Err() != nil {
	return nil, ctx.Err()
	}
	maxIndex := len(lb.randomServers)
	if startIndex > maxIndex {
	startIndex = maxIndex
	}
	if lb.nextServerIndex == startIndex {
	if allChecksFailed {
	return nil, errors.New("all servers failed")
	}
	logrus.Debugf("Health checks for all servers in load balancer %s have failed: retrying with health checks ignored", lb.serviceName)
	allChecksFailed = true
	}
	}

We should simplify the load-balancer behavior so that it functions more reliably, and its functionality is easier to understand and explain.