[release 18.0]: ReadBinlogFilesTimestamps backwards compatibility (#…

…14526)

Signed-off-by: Shlomi Noach <[email protected]>

go/test/endtoend/mysqlctld/mysqlctld_test.go

@@ -28,6 +28,7 @@ import (
 
 	"vitess.io/vitess/go/constants/sidecar"
 	"vitess.io/vitess/go/vt/mysqlctl/mysqlctlclient"
+	"vitess.io/vitess/go/vt/proto/mysqlctl"
 
 	"vitess.io/vitess/go/test/endtoend/cluster"
 )
@@ -169,3 +170,10 @@ func TestVersionString(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, version)
 }
+
+func TestReadBinlogFilesTimestamps(t *testing.T) {
+	client, err := mysqlctlclient.New("unix", primaryTablet.MysqlctldProcess.SocketFile)
+	require.NoError(t, err)
+	_, err = client.ReadBinlogFilesTimestamps(context.Background(), &mysqlctl.ReadBinlogFilesTimestampsRequest{})
+	require.ErrorContains(t, err, "empty binlog list in ReadBinlogFilesTimestampsRequest")
+}

go/vt/mysqlctl/backup_test.go

@@ -29,6 +29,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"vitess.io/vitess/go/test/utils"
@@ -42,6 +43,12 @@ import (
 	"vitess.io/vitess/go/vt/mysqlctl/backupstorage"
 )
 
+func TestFormatRFC3339(t *testing.T) {
+	var tm time.Time
+	res := FormatRFC3339(tm)
+	assert.Equal(t, "0001-01-01T00:00:00Z", res)
+}
+
 // TestBackupExecutesBackupWithScopedParams tests that Backup passes
 // a Scope()-ed stats to backupengine ExecuteBackup.
 func TestBackupExecutesBackupWithScopedParams(t *testing.T) {

go/vt/mysqlctl/builtinbackupengine.go

@@ -29,6 +29,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -341,19 +342,35 @@ func (be *BuiltinBackupEngine) executeIncrementalBackup(ctx context.Context, par
 		}
 		req.BinlogFileNames = append(req.BinlogFileNames, fullPath)
 	}
+	skipBinlogTimestampsDueToIncompatibility := false
 	resp, err := params.Mysqld.ReadBinlogFilesTimestamps(ctx, req)
 	if err != nil {
-		return false, vterrors.Wrapf(err, "reading timestamps from binlog files %v", binaryLogsToBackup)
-	}
-	if resp.FirstTimestampBinlog == "" || resp.LastTimestampBinlog == "" {
-		return false, vterrors.Errorf(vtrpc.Code_ABORTED, "empty binlog name in response. Request=%v, Response=%v", req, resp)
+		if strings.Contains(err.Error(), "rpc error: code = Unimplemented") {
+			// Backwards compatibility fix. This is v18, potentially calling a v17 server, which does not yet
+			// implement ReadBinlogFilesTimestamps() gRPC.
+			skipBinlogTimestampsDueToIncompatibility = true
+		} else {
+			return false, vterrors.Wrapf(err, "reading timestamps from binlog files %v", binaryLogsToBackup)
+		}
 	}
-	log.Infof("ReadBinlogFilesTimestampsResponse: %+v", resp)
-	incrDetails := &IncrementalBackupDetails{
-		FirstTimestamp:       FormatRFC3339(protoutil.TimeFromProto(resp.FirstTimestamp).UTC()),
-		FirstTimestampBinlog: filepath.Base(resp.FirstTimestampBinlog),
-		LastTimestamp:        FormatRFC3339(protoutil.TimeFromProto(resp.LastTimestamp).UTC()),
-		LastTimestampBinlog:  filepath.Base(resp.LastTimestampBinlog),
+
+	var incrDetails *IncrementalBackupDetails
+	if skipBinlogTimestampsDueToIncompatibility {
+		incrDetails = &IncrementalBackupDetails{
+			FirstTimestamp: FormatRFC3339(time.Time{}), // zero timestamp
+			LastTimestamp:  FormatRFC3339(time.Time{}), // zero timestamp
+		}
+	} else {
+		if resp.FirstTimestampBinlog == "" || resp.LastTimestampBinlog == "" {
+			return false, vterrors.Errorf(vtrpc.Code_ABORTED, "empty binlog name in response. Request=%v, Response=%v", req, resp)
+		}
+		log.Infof("ReadBinlogFilesTimestampsResponse: %+v", resp)
+		incrDetails = &IncrementalBackupDetails{
+			FirstTimestamp:       FormatRFC3339(protoutil.TimeFromProto(resp.FirstTimestamp).UTC()),
+			FirstTimestampBinlog: filepath.Base(resp.FirstTimestampBinlog),
+			LastTimestamp:        FormatRFC3339(protoutil.TimeFromProto(resp.LastTimestamp).UTC()),
+			LastTimestampBinlog:  filepath.Base(resp.LastTimestampBinlog),
+		}
 	}
 	// It's worthwhile we explain the difference between params.IncrementalFromPos and incrementalBackupFromPosition.
 	// params.IncrementalFromPos is supplied by the user. They want an incremental backup that covers that position.

go/vt/mysqlctl/grpcmysqlctlserver/server.go

@@ -56,6 +56,11 @@ func (s *server) ApplyBinlogFile(ctx context.Context, request *mysqlctlpb.ApplyB
 	return &mysqlctlpb.ApplyBinlogFileResponse{}, s.mysqld.ApplyBinlogFile(ctx, request)
 }
 
+// ReadBinlogFilesTimestamps implements the server side of the MysqlctlClient interface.
+func (s *server) ReadBinlogFilesTimestamps(ctx context.Context, request *mysqlctlpb.ReadBinlogFilesTimestampsRequest) (*mysqlctlpb.ReadBinlogFilesTimestampsResponse, error) {
+	return s.mysqld.ReadBinlogFilesTimestamps(ctx, request)
+}
+
 // ReinitConfig implements the server side of the MysqlctlClient interface.
 func (s *server) ReinitConfig(ctx context.Context, request *mysqlctlpb.ReinitConfigRequest) (*mysqlctlpb.ReinitConfigResponse, error) {
 	return &mysqlctlpb.ReinitConfigResponse{}, s.mysqld.ReinitConfig(ctx, s.cnf)