tabletmanager: remove v22 backwards-compatibility for vterrors migration (#18986)

Signed-off-by: Tim Vaillancourt <[email protected]>

Author: timvaillancourt

go/vt/vttablet/tabletserver/throttle/base/metric_result.go

@@ -18,10 +18,6 @@ package base
 
 import (
 	"errors"
-	"net"
-
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/vterrors"
@@ -59,30 +55,17 @@ var ErrAppDenied = errors.New("app denied")
 // ErrInvalidCheckType is an internal error indicating an unknown check type
 var ErrInvalidCheckType = errors.New("unknown throttler check type")
 
-// IsDialTCPError sees if the given error indicates a TCP issue
-func IsDialTCPError(err error) bool {
+// IsTabletRPCError sees if the given error indicates an issue performing an RPC call
+// to the tabletmanager service of a tablet. This is used to parse errors returned by
+// the CheckThrottler RPC of grpctmclient.
+func IsTabletRPCError(err error) bool {
 	if err == nil {
 		return false
 	}
 
-	// match vterror/vtrpc-style errors (v23+).
-	// v22 (and below) tablets will return vtrpcpb.Code_UNKNOWN
-	switch vterrors.Code(err) {
-	case vtrpcpb.Code_UNAVAILABLE, vtrpcpb.Code_DEADLINE_EXCEEDED:
-		return true
-	}
-
-	// match google grpc errors (v22 and below)
-	// TODO: remove after v24+
-	if s, ok := status.FromError(err); ok {
-		return s.Code() == codes.Unavailable || s.Code() == codes.DeadlineExceeded
-	}
-
-	switch err := err.(type) {
-	case *net.OpError:
-		return err.Op == "dial" && err.Net == "tcp"
-	}
-	return false
+	// The tmclient returns vterrors-style errors. Any
+	// error code other than "OK" indicates a problem.
+	return vterrors.Code(err) != vtrpcpb.Code_OK
 }
 
 type noHostsMetricResult struct{}

go/vt/vttablet/tabletserver/throttle/base/tablet_results.go

@@ -33,7 +33,7 @@ func AggregateTabletMetricResults(
 	metricName MetricName,
 	tabletResultsMap TabletResultMap,
 	ignoreHostsCount int,
-	IgnoreDialTCPErrors bool,
+	ignoreTabletRPCErrors bool,
 	ignoreHostsThreshold float64,
 ) (worstMetric MetricResult) {
 	// probes is known not to change. It can be *replaced*, but not changed.
@@ -49,7 +49,7 @@ func AggregateTabletMetricResults(
 		}
 		value, err := tabletMetricResult.Get()
 		if err != nil {
-			if IgnoreDialTCPErrors && IsDialTCPError(err) {
+			if ignoreTabletRPCErrors && IsTabletRPCError(err) {
 				continue
 			}
 			if ignoreHostsCount > 0 {

go/vt/vttablet/tabletserver/throttle/config/mysql_config.go

@@ -47,9 +47,10 @@ package config
 
 // MySQLConfigurationSettings has the general configuration for all MySQL clusters
 type MySQLConfigurationSettings struct {
-	CacheMillis         int      // optional, if defined then probe result will be cached, and future probes may use cached value
-	Port                int      // Specify if different than 3306; applies to all clusters
-	IgnoreDialTCPErrors bool     // Skip hosts where a metric cannot be retrieved due to TCP dial errors
-	IgnoreHostsCount    int      // Number of hosts that can be skipped/ignored even on error or on exceeding thresholds
-	IgnoreHosts         []string // If non empty, substrings to indicate hosts to be ignored/skipped
+	CacheMillis           int      // optional, if defined then probe result will be cached, and future probes may use cached value
+	Port                  int      // Specify if different than 3306; applies to all clusters
+	IgnoreTabletRPCErrors bool     // Skip hosts where a metric cannot be retrieved due to tablet RPC errors
+	IgnoreDialTCPErrors   bool     // TODO: deprecate/replace with IgnoreTabletRPCErrors.
+	IgnoreHostsCount      int      // Number of hosts that can be skipped/ignored even on error or on exceeding thresholds
+	IgnoreHosts           []string // If non empty, substrings to indicate hosts to be ignored/skipped
 }

go/vt/vttablet/tabletserver/throttle/throttler.go

@@ -77,7 +77,6 @@ import (
 
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
-	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
 const (
@@ -332,7 +331,7 @@ func (throttler *Throttler) initConfig() {
 
 	throttler.configSettings = &config.ConfigurationSettings{
 		MySQLStore: config.MySQLConfigurationSettings{
-			IgnoreDialTCPErrors: true,
+			IgnoreTabletRPCErrors: true,
 		},
 	}
 }
@@ -904,15 +903,10 @@ func (throttler *Throttler) generateTabletProbeFunction(scope base.Scope, probe
 		metrics := make(base.ThrottleMetrics)
 
 		req := &tabletmanagerdatapb.CheckThrottlerRequest{} // We leave AppName empty; it will default to VitessName anyway, and we can save some proto space
-		resp, gRPCErr := tmClient.CheckThrottler(ctx, probe.Tablet, req)
-		if gRPCErr != nil {
-			if vtErrCode := vterrors.Code(gRPCErr); vtErrCode != vtrpcpb.Code_UNKNOWN {
-				gRPCErr = vterrors.Errorf(vtErrCode, "gRPC error accessing tablet %v. Err=%s", probe.Alias, gRPCErr.Error())
-			} else {
-				// TODO: remove after v24+ when all errors are vterrors/vtrpc-based
-				gRPCErr = fmt.Errorf("gRPC error accessing tablet %v. Err=%w", probe.Alias, gRPCErr)
-			}
-			return metricsWithError(gRPCErr)
+		resp, err := tmClient.CheckThrottler(ctx, probe.Tablet, req)
+		if err != nil {
+			err = vterrors.Wrapf(err, "gRPC error accessing tablet %v. Err=%s", probe.Alias, err.Error())
+			return metricsWithError(err)
 		}
 		throttleMetric.Value = resp.Value
 		if resp.ResponseCode == tabletmanagerdatapb.CheckThrottlerResponseCode_INTERNAL_ERROR {
@@ -1173,9 +1167,9 @@ func (throttler *Throttler) aggregateMetrics() error {
 	aggregateTabletsMetrics := func(scope base.Scope, metricName base.MetricName, tabletResultsMap base.TabletResultMap) {
 		ignoreHostsCount := throttler.inventory.IgnoreHostsCount
 		ignoreHostsThreshold := throttler.inventory.IgnoreHostsThreshold
-		ignoreDialTCPErrors := throttler.configSettings.MySQLStore.IgnoreDialTCPErrors
+		ignoreTabletRPCErrors := (throttler.configSettings.MySQLStore.IgnoreTabletRPCErrors || throttler.configSettings.MySQLStore.IgnoreDialTCPErrors)
 
-		aggregatedMetric := base.AggregateTabletMetricResults(metricName, tabletResultsMap, ignoreHostsCount, ignoreDialTCPErrors, ignoreHostsThreshold)
+		aggregatedMetric := base.AggregateTabletMetricResults(metricName, tabletResultsMap, ignoreHostsCount, ignoreTabletRPCErrors, ignoreHostsThreshold)
 		aggregatedMetricName := metricName.AggregatedName(scope)
 		throttler.aggregatedMetrics.Set(aggregatedMetricName, aggregatedMetric, cache.DefaultExpiration)
 		if metricName == metricNameUsedAsDefault {

go/vt/vttablet/tabletserver/throttle/throttler_test.go

@@ -18,7 +18,6 @@ package throttle
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"math"
 	"sync"
@@ -31,14 +30,10 @@ import (
 	"github.com/stretchr/testify/require"
 	"golang.org/x/exp/maps"
 
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/credentials/insecure"
-
 	"vitess.io/vitess/go/protoutil"
-
-	"vitess.io/vitess/go/vt/grpcclient"
 	"vitess.io/vitess/go/vt/topo"
 	"vitess.io/vitess/go/vt/vtenv"
+	"vitess.io/vitess/go/vt/vterrors"
 	"vitess.io/vitess/go/vt/vttablet/grpctmclient"
 	"vitess.io/vitess/go/vt/vttablet/tabletserver/connpool"
 	"vitess.io/vitess/go/vt/vttablet/tabletserver/tabletenv"
@@ -49,6 +44,7 @@ import (
 
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
+	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
 var (
@@ -844,19 +840,45 @@ func TestApplyThrottlerConfigAppCheckedMetrics(t *testing.T) {
 	})
 }
 
-func TestIsDialTCPError(t *testing.T) {
-	// Verify that IsDialTCPError actually recognizes grpc dial errors
-	cc, err := grpcclient.DialContext(t.Context(), ":0", true, grpc.WithTransportCredentials(insecure.NewCredentials()))
-	require.NoError(t, err)
-	defer cc.Close()
+func TestIsTabletRPCError(t *testing.T) {
+	c := grpctmclient.NewClient()
+
+	// simulate an RPC cancellation using .CheckThrottler().
+	t.Run("CANCELLED", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(t.Context())
+		cancel() // cancel before check
+
+		_, err := c.CheckThrottler(ctx, &topodatapb.Tablet{
+			Hostname: "this.should.fail",
+			PortMap: map[string]int32{
+				"grpc": 12345,
+			},
+		}, &tabletmanagerdatapb.CheckThrottlerRequest{})
+		require.Equal(t, vtrpcpb.Code_CANCELED, vterrors.Code(err))
+		require.True(t, base.IsTabletRPCError(err))
+	})
 
-	err = cc.Invoke(context.Background(), "/Fail", nil, nil)
+	// simulate an RPC failure (dial error) using .CheckThrottler() to a host we cannot resolve.
+	t.Run("DEADLINE_EXCEEDED", func(t *testing.T) {
+		ctx, cancel := context.WithTimeout(t.Context(), time.Second)
+		defer cancel()
 
-	require.True(t, base.IsDialTCPError(err))
-	require.True(t, base.IsDialTCPError(fmt.Errorf("wrapped: %w", err)))
+		_, err := c.CheckThrottler(ctx, &topodatapb.Tablet{
+			Hostname: "this.should.fail",
+			PortMap: map[string]int32{
+				"grpc": 12345,
+			},
+		}, &tabletmanagerdatapb.CheckThrottlerRequest{})
+		require.Equal(t, vtrpcpb.Code_DEADLINE_EXCEEDED, vterrors.Code(err))
+		require.True(t, base.IsTabletRPCError(err))
+		require.ErrorContains(t, err, "rpc error: code = DeadlineExceeded desc = latest balancer error: connection error")
+	})
 
-	nonDialErr := errors.New("rpc error: code = NotFound desc = method not found")
-	require.False(t, base.IsDialTCPError(nonDialErr))
+	// simulate hypothetical NOT_FOUND RPC failure.
+	t.Run("NOT_FOUND", func(t *testing.T) {
+		nonDialErr := vterrors.New(vtrpcpb.Code_NOT_FOUND, "rpc error: code = NotFound desc = method not found")
+		require.True(t, base.IsTabletRPCError(nonDialErr))
+	})
 }
 
 func TestProbeWithUnavailableHost(t *testing.T) {
@@ -893,7 +915,7 @@ func TestProbeWithUnavailableHost(t *testing.T) {
 	probeFunc := throttler.generateTabletProbeFunction(base.ShardScope, probe)
 
 	metrics := probeFunc(t.Context(), tmClient)
-	require.True(t, base.IsDialTCPError(metrics["custom"].Err))
+	require.True(t, base.IsTabletRPCError(metrics["custom"].Err))
 
 	tabletResultsMap := base.TabletResultMap{
 		"cell1-100": base.MetricResultMap{
@@ -939,7 +961,7 @@ func TestProbeWithEmptyHostAndPort(t *testing.T) {
 	probeFunc := throttler.generateTabletProbeFunction(base.ShardScope, probe)
 
 	metrics := probeFunc(t.Context(), tmClient)
-	require.True(t, base.IsDialTCPError(metrics["custom"].Err))
+	require.True(t, base.IsTabletRPCError(metrics["custom"].Err))
 
 	tabletResultsMap := base.TabletResultMap{
 		"cell1-100": base.MetricResultMap{