Merge pull request #483 from planetscale/support-mysql-upgrade

pkg/controller/vitessshardreplication/reconcile_drain.go

@@ -19,9 +19,15 @@ package vitessshardreplication
 import (
 	"context"
 	"fmt"
+	"regexp"
+	"slices"
+	"strconv"
+	"strings"
 	"time"
 
+	"github.com/sirupsen/logrus"
 	"vitess.io/vitess/go/mysql/replication"
+	"vitess.io/vitess/go/vt/proto/tabletmanagerdata"
 	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 	"vitess.io/vitess/go/vt/topo"
 	"vitess.io/vitess/go/vt/topo/topoproto"
@@ -94,7 +100,7 @@ drainer delete something at a bad time.  However, by deleting only one tablet at
 a time we still ensure that for shards with three or more tablets we still have
 redundancy during the decommissioning.  Maybe later we can do better.
 */
-func (r *ReconcileVitessShard) reconcileDrain(ctx context.Context, vts *planetscalev2.VitessShard, wr *wrangler.Wrangler) (reconcile.Result, error) {
+func (r *ReconcileVitessShard) reconcileDrain(ctx context.Context, vts *planetscalev2.VitessShard, wr *wrangler.Wrangler, log *logrus.Entry) (reconcile.Result, error) {
 	clusterName := vts.Labels[planetscalev2.ClusterLabel]
 	keyspaceName := vts.Labels[planetscalev2.KeyspaceLabel]
 	resultBuilder := &results.Builder{}
@@ -255,8 +261,16 @@ func (r *ReconcileVitessShard) reconcileDrain(ctx context.Context, vts *planetsc
 		}
 	}
 
+	// 4. Check if we need to perform any operations like disabling fast shutdown
+	// for upgrades here.
+	if err := r.disableFastShutdown(ctx, wr, pods, tablets, vts.Spec.Images.Mysqld.Image(), log); err != nil {
+		r.recorder.Eventf(vts, corev1.EventTypeWarning,
+			"MysqldSafeUpgradeFailed", "failed to disable fast shutdown: %v", err)
+		return resultBuilder.Error(err)
+	}
+
 	//
-	// 4. Reparent draining primarys only if marked/will be marked as "Finished".
+	// 5. Reparent draining primarys only if marked/will be marked as "Finished".
 	//
 
 	// If we have acknowledged a drain and haven't already marked the primary as
@@ -458,6 +472,151 @@ func candidatePrimary(ctx context.Context, wr *wrangler.Wrangler, shard *topo.Sh
 	return bestCandidate
 }
 
+func (r *ReconcileVitessShard) disableFastShutdown(
+	ctx context.Context,
+	wr *wrangler.Wrangler,
+	pods map[string]*corev1.Pod,
+	tablets map[string]*topo.TabletInfo,
+	desiredImage string,
+	log *logrus.Entry,
+) error {
+	const disableFastShutdown = "set @@global.innodb_fast_shutdown = 0"
+
+	fetchReq := &tabletmanagerdata.ExecuteFetchAsDbaRequest{
+		Query:          []byte(disableFastShutdown),
+		DbName:         "_vt",
+		MaxRows:        0,
+		DisableBinlogs: false,
+		ReloadSchema:   false,
+	}
+
+	tmc := wr.TabletManagerClient()
+
+	for tabletAlias, pod := range pods {
+		tablet, ok := tablets[tabletAlias]
+		if !ok {
+			continue
+		}
+
+		var current string
+		for _, container := range pod.Spec.Containers {
+			if container.Name == vttablet.MysqldContainerName {
+				current = container.Image
+				break
+			}
+		}
+
+		needsSafe, err := safeMysqldUpgrade(current, desiredImage)
+		if err != nil {
+			return err
+		}
+
+		if !needsSafe {
+			continue
+		}
+		_, err = tmc.ExecuteFetchAsDba(ctx, tablet.Tablet, true /*usePool*/, fetchReq)
+		if err != nil {
+			return fmt.Errorf("failed to disable fast shutdown for tablet %v: %w", tabletAlias, err)
+		}
+		r.recorder.Eventf(pod, corev1.EventTypeNormal,
+			"MySQL_Upgrade", "innodb_fast_shutdown = 0 to prepare MySQL upgrade")
+		log.Infof("innodb_fast_shutdown = 0 to prepare MySQL upgrade on pod %s", pod.Name)
+	}
+	return nil
+}
+
+var mysqlImageVersion = regexp.MustCompile(`^(\d+)\.(\d+)\.(\d+)`)
+
+func safeMysqldUpgrade(currentImage, desiredImage string) (bool, error) {
+	if currentImage == "" || desiredImage == "" {
+		// No action if we have unknown versions.
+		return false, nil
+	}
+
+	// Quick check so no regexp matching is needed for the most common
+	// case where nothing changes.
+	if desiredImage == currentImage {
+		return false, nil
+	}
+
+	currentParts := strings.SplitN(currentImage, ":", 2)
+	if len(currentParts) != 2 {
+		return false, nil
+	}
+
+	desiredParts := strings.SplitN(desiredImage, ":", 2)
+	if len(desiredParts) != 2 {
+		return false, nil
+	}
+
+	current := currentParts[1]
+	desired := desiredParts[1]
+
+	curStrParts := mysqlImageVersion.FindStringSubmatch(current)
+	if len(curStrParts) != 4 {
+		// Invalid version, assume that we need to do a safe upgrade.
+		return true, nil
+	}
+	dstStrParts := mysqlImageVersion.FindStringSubmatch(desired)
+	if len(dstStrParts) != 4 {
+		// Invalid version, assume that we need to do a safe upgrade.
+		return true, nil
+	}
+	if slices.Equal(curStrParts, dstStrParts) {
+		return false, nil
+	}
+	dstParts := make([]int, len(dstStrParts)-1)
+	curParts := make([]int, len(curStrParts)-1)
+	for i, part := range dstStrParts[1:] {
+		// We already matched with `\d_` so there's no
+		// way this can trigger an error.
+		dstParts[i], _ = strconv.Atoi(part)
+	}
+
+	for i, part := range curStrParts[1:] {
+		// We already matched with `\d_` so there's no
+		// way this can trigger an error.
+		curParts[i], _ = strconv.Atoi(part)
+	}
+
+	if dstParts[0] < curParts[0] {
+		return false, fmt.Errorf("cannot downgrade major version from %s to %s", current, desired)
+	}
+	if dstParts[0] == curParts[1] && dstParts[1] < curParts[1] {
+		return false, fmt.Errorf("cannot downgrade minor version from %s to %s", current, desired)
+	}
+
+	// Alright, here it gets more tricky. MySQL has had a complicated release history. For the 8.0 series,
+	// up to 8.0.34 at least (known at this point), it was not supported to downgrade patch releases
+	// as patch release could also include on-disk data format changes. This happened a number of times
+	// in practice as well, so this concern is real.
+	//
+	// MySQL though has announced a new release strategy, see:
+	// https://dev.mysql.com/blog-archive/introducing-mysql-innovation-and-long-term-support-lts-versions/
+	//
+	// With that release strategy, it will become possible that patch releases will be safe to downgrade
+	// as well and since the data format doesn't change on-disk anymore, it's also safe to upgrade with
+	// fast shutdown enabled.
+	// Specifically, it calls out that "MySQL 8.0.34+ will become bugfix only release (red)". This means
+	// that we use that version as a cut-off point here for when we need to disable fast shutdown or not.
+	if dstParts[0] == 8 && dstParts[1] == 0 && curParts[0] == 8 && curParts[1] == 0 {
+		// Our upgrade process stays within the 8.0.x version range.
+		if dstParts[2] >= 34 && curParts[2] >= 34 {
+			// No need for safe upgrade if both versions are 8.0.34 or higher.
+			return false, nil
+		}
+		// We can't downgrade within the 8.0.x series before 8.0.34.
+		if dstParts[2] < curParts[2] {
+			return false, fmt.Errorf("cannot downgrade patch version from %s to %s", current, desired)
+		}
+		// Always need safe upgrade if we change the patch release for 8.0.x before 8.0.34.
+		return dstParts[2] != curParts[2], nil
+	}
+
+	// For any major or minor version change we always need safe upgrade.
+	return dstParts[0] != curParts[0] || dstParts[1] != curParts[1], nil
+}
+
 type candidateInfo struct {
 	tablet   *topo.TabletInfo
 	position replication.Position

pkg/controller/vitessshardreplication/reconcile_drain_test.go

@@ -0,0 +1,144 @@
+/*
+Copyright 2023 PlanetScale Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package vitessshardreplication
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSafeMysqldUpgrade(t *testing.T) {
+	tests := []struct {
+		name      string
+		current   string
+		desired   string
+		needsSafe bool
+		err       string
+	}{
+		{
+			name:      "no desired and current",
+			needsSafe: false,
+		},
+		{
+			name:      "no desired",
+			desired:   "docker.io/vitess/mysql:8.0.23",
+			needsSafe: false,
+		},
+		{
+			name:      "no current",
+			current:   "docker.io/vitess/mysql:8.0.23",
+			needsSafe: false,
+		},
+		{
+			name:      "equal version with same registry",
+			current:   "docker.io/vitess/mysql:8.0.23",
+			desired:   "docker.io/vitess/mysql:8.0.23",
+			needsSafe: false,
+		},
+		{
+			name:      "no explicit current version",
+			current:   "docker.io/vitess/mysql:latest",
+			desired:   "docker.io/vitess/mysql:8.9.23",
+			needsSafe: true,
+		},
+		{
+			name:      "no explicit desired version",
+			current:   "docker.io/vitess/mysql:8.0.23",
+			desired:   "docker.io/vitess/mysql:latest",
+			needsSafe: true,
+		},
+		{
+			name:    "downgrade version",
+			current: "docker.io/vitess/mysql:8.0.23",
+			desired: "docker.io/vitess/mysql:8.0.22",
+			err:     "cannot downgrade patch version from 8.0.23 to 8.0.22",
+		},
+		{
+			name:      "newer version",
+			current:   "docker.io/vitess/mysql:8.0.23",
+			desired:   "docker.io/vitess/mysql:8.0.24",
+			needsSafe: true,
+		},
+		{
+			name:      "newer version until 8.0.34",
+			current:   "docker.io/vitess/mysql:8.0.33",
+			desired:   "docker.io/vitess/mysql:8.0.34",
+			needsSafe: true,
+		},
+		{
+			name:    "downgrade with 8.0.34",
+			current: "docker.io/vitess/mysql:8.0.34",
+			desired: "docker.io/vitess/mysql:8.0.33",
+			err:     "cannot downgrade patch version from 8.0.34 to 8.0.33",
+		},
+		{
+			name:      "newer version skipping 8.0.34",
+			current:   "docker.io/vitess/mysql:8.0.33",
+			desired:   "docker.io/vitess/mysql:8.0.35",
+			needsSafe: true,
+		},
+		{
+			name:    "downgrade skipping 8.0.34",
+			current: "docker.io/vitess/mysql:8.0.35",
+			desired: "docker.io/vitess/mysql:8.0.33",
+			err:     "cannot downgrade patch version from 8.0.35 to 8.0.33",
+		},
+		{
+			name:      "newer version after 8.0.34",
+			current:   "docker.io/vitess/mysql:8.0.34",
+			desired:   "docker.io/vitess/mysql:8.0.35",
+			needsSafe: false,
+		},
+		{
+			name:      "older version after 8.0.35",
+			current:   "docker.io/vitess/mysql:8.0.35",
+			desired:   "docker.io/vitess/mysql:8.0.34",
+			needsSafe: false,
+		},
+		{
+			name:      "early major upgrade",
+			current:   "docker.io/vitess/mysql:5.7.35",
+			desired:   "docker.io/vitess/mysql:8.0.34",
+			needsSafe: true,
+		},
+		{
+			name:      "late major upgrade",
+			current:   "docker.io/vitess/mysql:8.2.35",
+			desired:   "docker.io/vitess/mysql:9.0.21",
+			needsSafe: true,
+		},
+		{
+			name:      "minor upgrade",
+			current:   "docker.io/vitess/mysql:8.2.25",
+			desired:   "docker.io/vitess/mysql:8.4.12",
+			needsSafe: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			needsSafe, err := safeMysqldUpgrade(tt.current, tt.desired)
+			if tt.err != "" {
+				assert.EqualError(t, err, tt.err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.needsSafe, needsSafe)
+			}
+		})
+	}
+}

pkg/controller/vitessshardreplication/vitessshardreplication_controller.go

@@ -204,7 +204,7 @@ func (r *ReconcileVitessShard) Reconcile(cctx context.Context, request reconcile
 	resultBuilder.Merge(initReplicationResult, err)
 
 	// Check if we've been asked to do a planned reparent.
-	drainResult, err := r.reconcileDrain(ctx, vts, wr)
+	drainResult, err := r.reconcileDrain(ctx, vts, wr, log)
 	resultBuilder.Merge(drainResult, err)
 
 	// Request a periodic resync for the shard so we can recheck replication

pkg/operator/vttablet/constants.go

@@ -27,7 +27,7 @@ const (
 	vtbackupContainerName = "vtbackup"
 	vtbackupCommand       = "/vt/bin/vtbackup"
 
-	mysqldContainerName = "mysqld"
+	MysqldContainerName = "mysqld"
 	mysqldCommand       = "/vt/bin/mysqlctld"
 
 	mysqldExporterContainerName      = "mysqld-exporter"

pkg/operator/vttablet/pod.go

@@ -166,7 +166,7 @@ func UpdatePod(obj *corev1.Pod, spec *Spec) {
 
 	if spec.Mysqld != nil {
 		mysqldContainer = &corev1.Container{
-			Name:            mysqldContainerName,
+			Name:            MysqldContainerName,
 			Image:           spec.Images.Mysqld.Image(),
 			ImagePullPolicy: spec.ImagePullPolicies.Mysqld,
 			Command:         []string{mysqldCommand},