Address code review comments III

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

Author: yacovm

RELEASES.md

@@ -2,11 +2,11 @@
 
 ## [v1.12.3] (pending)
 
-- Added a health check that alerts if a primary network validator has no nodes connected to it. Runs a configurable time after bootstrapping or 20 minutes by default.
+- Extended the network health check by also alerting if a primary network validator has no nodes connected to it. Runs a configurable time after startup or 10 minutes by default.
 
 ### Configs
--  How long after bootstrapping the aforementioned health check runs can be configued via:
-`--no-ingress-validator-connection-timeout`
+-  How long after startup the aforementioned health check runs can be configured via:
+`--network-no-ingress-connections-grace-period`
 
 
 ## [v1.12.2](https://github.com/ava-labs/avalanchego/releases/tag/v1.12.2)

config/config.go

@@ -327,13 +327,14 @@ func getNetworkConfig(
 		},
 
 		HealthConfig: network.HealthConfig{
-			Enabled:                      sybilProtectionEnabled,
-			MaxTimeSinceMsgSent:          v.GetDuration(NetworkHealthMaxTimeSinceMsgSentKey),
-			MaxTimeSinceMsgReceived:      v.GetDuration(NetworkHealthMaxTimeSinceMsgReceivedKey),
-			MaxPortionSendQueueBytesFull: v.GetFloat64(NetworkHealthMaxPortionSendQueueFillKey),
-			MinConnectedPeers:            v.GetUint(NetworkHealthMinPeersKey),
-			MaxSendFailRate:              v.GetFloat64(NetworkHealthMaxSendFailRateKey),
-			SendFailRateHalflife:         halflife,
+			Enabled:                                 sybilProtectionEnabled,
+			MaxTimeSinceMsgSent:                     v.GetDuration(NetworkHealthMaxTimeSinceMsgSentKey),
+			MaxTimeSinceMsgReceived:                 v.GetDuration(NetworkHealthMaxTimeSinceMsgReceivedKey),
+			MaxPortionSendQueueBytesFull:            v.GetFloat64(NetworkHealthMaxPortionSendQueueFillKey),
+			MinConnectedPeers:                       v.GetUint(NetworkHealthMinPeersKey),
+			MaxSendFailRate:                         v.GetFloat64(NetworkHealthMaxSendFailRateKey),
+			SendFailRateHalflife:                    halflife,
+			NoIngressValidatorConnectionGracePeriod: v.GetDuration(NetworkNoIngressValidatorConnectionsGracePeriodKey),
 		},
 
 		ProxyEnabled:           v.GetBool(NetworkTCPProxyEnabledKey),
@@ -1437,7 +1438,7 @@ func GetNodeConfig(v *viper.Viper) (node.Config, error) {
 	nodeConfig.SystemTrackerProcessingHalflife = v.GetDuration(SystemTrackerProcessingHalflifeKey)
 	nodeConfig.SystemTrackerCPUHalflife = v.GetDuration(SystemTrackerCPUHalflifeKey)
 	nodeConfig.SystemTrackerDiskHalflife = v.GetDuration(SystemTrackerDiskHalflifeKey)
-	nodeConfig.NoIngressValidatorConnectionTimeout = v.GetDuration(NoIngressValidatorConnectionTimeoutKey)
+	nodeConfig.NoIngressValidatorConnectionTimeout = v.GetDuration(NetworkNoIngressValidatorConnectionsGracePeriodKey)
 
 	nodeConfig.RequiredAvailableDiskSpace, nodeConfig.WarningThresholdAvailableDiskSpace, err = getDiskSpaceConfig(v)
 	if err != nil {

config/flags.go

@@ -169,7 +169,7 @@ func addNodeFlags(fs *pflag.FlagSet) {
 	fs.Duration(NetworkReadHandshakeTimeoutKey, constants.DefaultNetworkReadHandshakeTimeout, "Timeout value for reading handshake messages")
 	fs.Duration(NetworkPingTimeoutKey, constants.DefaultPingPongTimeout, "Timeout value for Ping-Pong with a peer")
 	fs.Duration(NetworkPingFrequencyKey, constants.DefaultPingFrequency, "Frequency of pinging other peers")
-	fs.Duration(NoIngressValidatorConnectionTimeoutKey, constants.DefaultNoIngressValidatorConnectionTimeout, "Time after which nodes are expected to be connected to us if we are a primary network validator, otherwise a health check fails")
+	fs.Duration(NetworkNoIngressValidatorConnectionsGracePeriodKey, constants.DefaultNoIngressValidatorConnectionGracePeriod, "Time after which nodes are expected to be connected to us if we are a primary network validator, otherwise a health check fails")
 	fs.String(NetworkCompressionTypeKey, constants.DefaultNetworkCompressionType.String(), fmt.Sprintf("Compression type for outbound messages. Must be one of [%s, %s]", compression.TypeZstd, compression.TypeNone))
 
 	fs.Duration(NetworkMaxClockDifferenceKey, constants.DefaultNetworkMaxClockDifference, "Max allowed clock difference value between this node and peers")

config/keys.go

@@ -120,6 +120,7 @@ const (
 	NetworkInboundThrottlerMaxConnsPerSecKey           = "network-inbound-connection-throttling-max-conns-per-sec"
 	NetworkOutboundConnectionThrottlingRpsKey          = "network-outbound-connection-throttling-rps"
 	NetworkOutboundConnectionTimeoutKey                = "network-outbound-connection-timeout"
+	NetworkNoIngressValidatorConnectionsGracePeriodKey = "network-no-ingress-connections-grace-period"
 	BenchlistFailThresholdKey                          = "benchlist-fail-threshold"
 	BenchlistDurationKey                               = "benchlist-duration"
 	BenchlistMinFailingDurationKey                     = "benchlist-min-failing-duration"
@@ -191,7 +192,6 @@ const (
 	SystemTrackerDiskHalflifeKey                       = "system-tracker-disk-halflife"
 	SystemTrackerRequiredAvailableDiskSpaceKey         = "system-tracker-disk-required-available-space"
 	SystemTrackerWarningThresholdAvailableDiskSpaceKey = "system-tracker-disk-warning-threshold-available-space"
-	NoIngressValidatorConnectionTimeoutKey             = "no-ingress-validator-connection-timeout"
 	DiskVdrAllocKey                                    = "throttler-inbound-disk-validator-alloc"
 	DiskMaxNonVdrUsageKey                              = "throttler-inbound-disk-max-non-validator-usage"
 	DiskMaxNonVdrNodeUsageKey                          = "throttler-inbound-disk-max-non-validator-node-usage"

network/config.go

@@ -26,6 +26,10 @@ type HealthConfig struct {
 	// Marks if the health check should be enabled
 	Enabled bool `json:"-"`
 
+	// NoIngressValidatorConnectionGracePeriod denotes the time after which the health check fails
+	// for primary network validators with no ingress connections.
+	NoIngressValidatorConnectionGracePeriod time.Duration
+
 	// MinConnectedPeers is the minimum number of peers that the network should
 	// be connected to be considered healthy.
 	MinConnectedPeers uint `json:"minConnectedPeers"`

network/network.go

@@ -42,10 +42,11 @@ import (
 )
 
 const (
-	ConnectedPeersKey           = "connectedPeers"
-	TimeSinceLastMsgReceivedKey = "timeSinceLastMsgReceived"
-	TimeSinceLastMsgSentKey     = "timeSinceLastMsgSent"
-	SendFailRateKey             = "sendFailRate"
+	PrimaryNetworkValidatorHealthKey = "primary network validator health"
+	ConnectedPeersKey                = "connectedPeers"
+	TimeSinceLastMsgReceivedKey      = "timeSinceLastMsgReceived"
+	TimeSinceLastMsgSentKey          = "timeSinceLastMsgSent"
+	SendFailRateKey                  = "sendFailRate"
 )
 
 var (
@@ -156,6 +157,9 @@ type network struct {
 	connectedPeers  peer.Set
 	closing         bool
 
+	ingressConnAlerter noIngressConnAlert
+	startupTime        time.Time
+
 	// router is notified about all peer [Connected] and [Disconnected] events
 	// as well as all non-handshake peer messages.
 	//
@@ -259,38 +263,34 @@ func NewNetwork(
 		ipTracker.ManuallyTrack(nodeID)
 	}
 
-	// ingressConnCount tracks connections to all remote peers,
-	// so for safety we set it as pointer, in case the config object is copied.
-	var ingressConnCount atomic.Uint64
-
 	peerConfig := &peer.Config{
-		ReadBufferSize:         config.PeerReadBufferSize,
-		WriteBufferSize:        config.PeerWriteBufferSize,
-		Metrics:                peerMetrics,
-		MessageCreator:         msgCreator,
-		IngressConnectionCount: &ingressConnCount,
-		Log:                    log,
-		InboundMsgThrottler:    inboundMsgThrottler,
-		Network:                nil, // This is set below.
-		Router:                 router,
-		VersionCompatibility:   version.GetCompatibility(minCompatibleTime),
-		MyNodeID:               config.MyNodeID,
-		MySubnets:              config.TrackedSubnets,
-		Beacons:                config.Beacons,
-		Validators:             config.Validators,
-		NetworkID:              config.NetworkID,
-		PingFrequency:          config.PingFrequency,
-		PongTimeout:            config.PingPongTimeout,
-		MaxClockDifference:     config.MaxClockDifference,
-		SupportedACPs:          config.SupportedACPs.List(),
-		ObjectedACPs:           config.ObjectedACPs.List(),
-		ResourceTracker:        config.ResourceTracker,
-		UptimeCalculator:       config.UptimeCalculator,
-		IPSigner:               peer.NewIPSigner(config.MyIPPort, config.TLSKey, config.BLSKey),
+		ReadBufferSize:       config.PeerReadBufferSize,
+		WriteBufferSize:      config.PeerWriteBufferSize,
+		Metrics:              peerMetrics,
+		MessageCreator:       msgCreator,
+		Log:                  log,
+		InboundMsgThrottler:  inboundMsgThrottler,
+		Network:              nil, // This is set below.
+		Router:               router,
+		VersionCompatibility: version.GetCompatibility(minCompatibleTime),
+		MyNodeID:             config.MyNodeID,
+		MySubnets:            config.TrackedSubnets,
+		Beacons:              config.Beacons,
+		Validators:           config.Validators,
+		NetworkID:            config.NetworkID,
+		PingFrequency:        config.PingFrequency,
+		PongTimeout:          config.PingPongTimeout,
+		MaxClockDifference:   config.MaxClockDifference,
+		SupportedACPs:        config.SupportedACPs.List(),
+		ObjectedACPs:         config.ObjectedACPs.List(),
+		ResourceTracker:      config.ResourceTracker,
+		UptimeCalculator:     config.UptimeCalculator,
+		IPSigner:             peer.NewIPSigner(config.MyIPPort, config.TLSKey, config.BLSKey),
 	}
 
 	onCloseCtx, cancel := context.WithCancel(context.Background())
 	n := &network{
+		startupTime:          time.Now(),
 		config:               config,
 		peerConfig:           peerConfig,
 		metrics:              metrics,
@@ -318,6 +318,11 @@ func NewNetwork(
 		router:          router,
 	}
 	n.peerConfig.Network = n
+	n.ingressConnAlerter = noIngressConnAlert{
+		ingressConnections: n,
+		validators:         config.Validators,
+		selfID:             config.MyNodeID,
+	}
 	return n, nil
 }
 
@@ -408,6 +413,15 @@ func (n *network) HealthCheck(context.Context) (interface{}, error) {
 	details[SendFailRateKey] = sendFailRate
 	n.metrics.sendFailRate.Set(sendFailRate)
 
+	reachablePrimaryNetworkValidator := true
+	// Make sure if we're a primary network validator, we have ingress connections
+	if time.Since(n.startupTime) > n.config.NoIngressValidatorConnectionGracePeriod {
+		connectedPrimaryValidatorInfo, isConnectedPrimaryValidatorErr := n.ingressConnAlerter.checkHealth()
+		reachablePrimaryNetworkValidator = isConnectedPrimaryValidatorErr == nil
+		details[PrimaryNetworkValidatorHealthKey] = connectedPrimaryValidatorInfo
+	}
+	healthy = healthy && reachablePrimaryNetworkValidator
+
 	// emit metrics about the lifetime of peer connections
 	n.metrics.updatePeerConnectionLifetimeMetrics()
 
@@ -434,6 +448,11 @@ func (n *network) HealthCheck(context.Context) (interface{}, error) {
 	if !isMsgFailRate {
 		errorReasons = append(errorReasons, fmt.Sprintf("messages failure send rate %g > %g", sendFailRate, n.config.HealthConfig.MaxSendFailRate))
 	}
+
+	if !reachablePrimaryNetworkValidator {
+		errorReasons = append(errorReasons, "primary network validator is unreachable")
+	}
+
 	return details, fmt.Errorf("network layer is unhealthy reason: %s", strings.Join(errorReasons, ", "))
 }
 

network/network_test.go

@@ -325,6 +325,11 @@ func TestIngressConnCount(t *testing.T) {
 
 	wg.Done()
 
+	for _, net := range networks {
+		net.config.NoIngressValidatorConnectionGracePeriod = 0
+		net.config.HealthConfig.Enabled = true
+	}
+
 	require.Eventually(func() bool {
 		result := true
 		for _, net := range networks {
@@ -334,15 +339,20 @@ func TestIngressConnCount(t *testing.T) {
 	}, time.Minute, time.Millisecond*10)
 
 	ingressConnections := make([]int, 0, len(networks))
+	healthCheckErrors := make([]error, 0, len(networks))
 
 	for _, net := range networks {
 		ingressConnections = append(ingressConnections, net.IngressConnCount())
+		_, err := net.HealthCheck(context.Background())
+		healthCheckErrors = append(healthCheckErrors, err)
 	}
 
 	// First node has all nodes connected to it.
 	// Second node has only the third node connected to it.
 	// Third node has no node connected to it, as it connects to the first and second node.
 	require.Equal([]int{2, 1, 0}, ingressConnections)
+	require.Equal([]error{nil, nil}, healthCheckErrors[:2])
+	require.EqualError(healthCheckErrors[2], "network layer is unhealthy reason: primary network validator is unreachable")
 }
 
 func TestSend(t *testing.T) {

network/no_ingress_conn_alert.go

@@ -0,0 +1,48 @@
+// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package network
+
+import (
+	"errors"
+
+	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/snow/validators"
+	"github.com/ava-labs/avalanchego/utils/constants"
+)
+
+// ErrNoIngressConnections denotes that no node is connected to this validator.
+var ErrNoIngressConnections = errors.New("no ingress connections")
+
+type ingressConnectionCounter interface {
+	IngressConnCount() int
+}
+
+type validatorRetriever interface {
+	GetValidator(subnetID ids.ID, nodeID ids.NodeID) (*validators.Validator, bool)
+}
+
+type noIngressConnAlert struct {
+	selfID             ids.NodeID
+	ingressConnections ingressConnectionCounter
+	validators         validatorRetriever
+}
+
+func ingressConnResult(n int, areWeValidator bool) map[string]interface{} {
+	return map[string]interface{}{"ingressConnectionCount": n, "primary network validator": areWeValidator}
+}
+
+func (nica *noIngressConnAlert) checkHealth() (interface{}, error) {
+	connCount := nica.ingressConnections.IngressConnCount()
+	_, areWeValidator := nica.validators.GetValidator(constants.PrimaryNetworkID, nica.selfID)
+
+	if connCount > 0 {
+		return ingressConnResult(connCount, areWeValidator), nil
+	}
+
+	if !areWeValidator {
+		return ingressConnResult(connCount, areWeValidator), nil
+	}
+
+	return ingressConnResult(connCount, areWeValidator), ErrNoIngressConnections
+}

network/no_ingress_conn_alert_test.go

@@ -0,0 +1,71 @@
+// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package network
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/snow/validators"
+)
+
+type fakeValidatorRetriever struct {
+	result bool
+}
+
+func (m *fakeValidatorRetriever) GetValidator(ids.ID, ids.NodeID) (*validators.Validator, bool) {
+	return nil, m.result
+}
+
+type fakeIngressConnectionCounter struct {
+	res int
+}
+
+func (m *fakeIngressConnectionCounter) IngressConnCount() int {
+	return m.res
+}
+
+func TestNoIngressConnAlertHealthCheck(t *testing.T) {
+	for _, testCase := range []struct {
+		name                   string
+		getValidatorResult     bool
+		ingressConnCountResult int
+		expectedErr            error
+		expectedResult         interface{}
+	}{
+		{
+			name:           "not a validator of a primary network",
+			expectedResult: map[string]interface{}{"ingressConnectionCount": 0, "primary network validator": false},
+		},
+		{
+			name:               "a validator of the primary network",
+			getValidatorResult: true,
+			expectedResult: map[string]interface{}{
+				"ingressConnectionCount": 0, "primary network validator": true,
+			},
+			expectedErr: ErrNoIngressConnections,
+		},
+		{
+			name:                   "a validator with ingress connections",
+			expectedResult:         map[string]interface{}{"ingressConnectionCount": 42, "primary network validator": true},
+			expectedErr:            nil,
+			ingressConnCountResult: 42,
+			getValidatorResult:     true,
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			nica := &noIngressConnAlert{
+				selfID:             ids.EmptyNodeID,
+				validators:         &fakeValidatorRetriever{result: testCase.getValidatorResult},
+				ingressConnections: &fakeIngressConnectionCounter{res: testCase.ingressConnCountResult},
+			}
+
+			result, err := nica.checkHealth()
+			require.Equal(t, testCase.expectedErr, err)
+			require.Equal(t, testCase.expectedResult, result)
+		})
+	}
+}

network/peer/config.go

@@ -61,6 +61,5 @@ type Config struct {
 	IPSigner *IPSigner
 
 	// IngressConnectionCount counts the ingress (to us) connections.
-	// Needs to be a pointer because it's shared across all peer connections.
-	IngressConnectionCount *atomic.Uint64
+	IngressConnectionCount atomic.Uint64
 }