restore: Run PITR through mysqlctl #13123

dbussink · 2023-05-22T09:41:58Z

When we run a restore remotely on the vttablet, we need to run the PITR logic through mysqlctl to ensure we run it with the correct MySQL binaries.

It adds additional safety checks for using the Mysqld type because it's reused between both the local MySQL interaction and mysqlctl but also as a remote interface used from vttablet.

This safety check means that we should never run certain things when running remotely inside vttablet, which is identified by the socketFile being set or not. When it is set, we now panic on certain actions we know are not going to work.

By adding this panic we can ensure we catch uses like the restore usage earlier since it would break if we do anything unsupported.

Related Issue(s)

Fixes #13124

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

vitess-bot · 2023-05-22T09:42:01Z

When we run a retore remotely on the vttablet, we need to run the partial restore logic through mysqlctl to ensure we run it with the correct MySQL binaries. It adds additional safety checks for using `Mysqld` because it's reused between both the local MySQL interaction and `mysqlctl` but also as a remote interface used from `vttablet`. This safety check means that we should never run certain things when running remotely inside `vttablet`, which is identified by the `socketFile` being set or not. When it is set, we now panic on certain actions we know are not going to work. By adding this panic we can ensure we catch uses like the restore usage earlier since it would break if we do anything unsupported. Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink · 2023-05-22T14:34:28Z

go/vt/mysqlctl/capabilityset.go

-func (c *capabilitySet) hasDisableRedoLog() bool {
-	return c.isMySQLLike() && c.version.atLeast(ServerVersion{Major: 8, Minor: 0, Patch: 21})
-}
-


I have removed this function since nothing is using atm. I'd like to reduce anything that checks capabilities to as little as possible, since this PR also further restricts that these capabilities can only be used properly inside mysqlctl and with less surface, we have less potential issues where it can be used wrong.

dbussink · 2023-05-22T14:35:31Z

go/vt/mysqlctl/mysqld.go

+	if socketFile != "" {
+		log.Info("mysqld is remote. Skipping flavor detection")
+		return result
+	}


This ensures we don't set up capabilities when we can't correctly detect them.

dbussink · 2023-05-22T14:35:51Z

go/vt/mysqlctl/mysqld.go

@@ -286,7 +293,7 @@ func (mysqld *Mysqld) RunMysqlUpgrade() error {
 			return fmt.Errorf("can't dial mysqlctld: %v", err)
 		}
 		defer client.Close()
-		return client.RunMysqlUpgrade(context.TODO())
+		return client.RunMysqlUpgrade(ctx)
 	}


Seems simplest to pass in the context we already have in all the callers further up in the call chain.

dbussink · 2023-05-22T14:36:38Z

go/vt/mysqlctl/mysqld.go

@@ -635,6 +642,7 @@ func execCmd(name string, args, env []string, dir string, input io.Reader) (cmd
 // binaryPath does a limited path lookup for a command,
 // searching only within sbin and bin in the given root.
 func binaryPath(root, binary string) (string, error) {
+	noSocketFile()


Anywhere we're going to check if a certain binary is available, implies we need to be running inside mysqlctl. This ensures we error out hard when we try to end up calling this somewhere else.

Everything is correct here today, this is a safety mechanism so that we don't accidentally add broken usage in the future.

dbussink · 2023-05-22T14:37:55Z

go/vt/mysqlctl/mysqld.go

+	}
+	res := qr.Named().Row()
+	version, _ := res.ToString("@@global.version")
+	return version
 }


Similar to GetVersionComment, we get the version number here at runtime from the actual running MySQL instance. This ensures we get the right version and not the local binary.

dbussink · 2023-05-22T14:38:18Z

go/vt/mysqlctl/mysqld.go

+		}
+		defer client.Close()
+		return client.ApplyBinlogFile(ctx, binlogFile, mysql.EncodePosition(restorePos))
+	}


This is now part of the interface and also has a remote version so we can run this correctly for restores through mysqlctl.

dbussink · 2023-05-22T14:39:14Z

go/vt/mysqlctl/mysqld.go

 // $ mysqlbinlog --include-gtids binlog.file | mysql
-func (mysqld *Mysqld) applyBinlogFile(binlogFile string, includeGTIDs mysql.GTIDSet) error {
+func (mysqld *Mysqld) ApplyBinlogFile(ctx context.Context, binlogFile string, restorePos mysql.Position) error {


Changing this to mysql.Position makes it easier to serialize / deserialize and better matches how we use a GTID position in other places here.

shlomi-noach · 2023-05-22T15:36:11Z

go/vt/mysqlctl/grpcmysqlctlclient/client.go

+		BinlogRestorePosition: binlogRestorePosition,
+	}
+	return c.withRetry(ctx, func() error {
+		_, err := c.c.ApplyBinlogFile(ctx, req)


Not a fan of c.c. but unrelated to this PR

Copied this basically from the other things we have to be consistent.

shlomi-noach · 2023-05-22T15:40:29Z

go/vt/mysqlctl/mysqld.go

+		log.Errorf("Unexpected number of rows: %v", qr.Rows)
+		return ""
+	}
+	res := qr.Named().Row()


BTW the function Row() returns nil if the number of rows != 1. So if you wanted, you could rewrite the integrity check above from if len(qr.Rows) != 1 { to (down in this line) if res == nil. Whichever you feel more comfortable with, seeing that the former is more explicit and the latter is implied.

Same here, this is basically a copy paste of how the comment version works and mostly the same to stay consistent with that.

After the changes in vitessio#13123, I realized that there's no cases left where we actually use or depend on `MYSQL_FLAVOR`. This means we can actually remove usages of `MYSQL_FLAVOR`. It doesn't yet remove it from the artifacts we build, because we shouldn't clean that up until v18 then because users might mix versions during the upgrade and we don't want to have old code that still depends on this fail then. Signed-off-by: Dirkjan Bussink <[email protected]>

* mysqlctl: Remove usage of MYSQL_FLAVOR After the changes in #13123, I realized that there's no cases left where we actually use or depend on `MYSQL_FLAVOR`. This means we can actually remove usages of `MYSQL_FLAVOR`. It doesn't yet remove it from the artifacts we build, because we shouldn't clean that up until v18 then because users might mix versions during the upgrade and we don't want to have old code that still depends on this fail then. Signed-off-by: Dirkjan Bussink <[email protected]> * Add release note item for removed `MYSQL_FLAVOR`. Signed-off-by: Dirkjan Bussink <[email protected]> --------- Signed-off-by: Dirkjan Bussink <[email protected]>

vitess-bot bot added the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label May 22, 2023

vitess-bot bot added NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels May 22, 2023

github-actions bot added this to the v17.0.0 milestone May 22, 2023

dbussink force-pushed the dbussink/remote-binlog-restore branch 2 times, most recently from a51a422 to 2f03810 Compare May 22, 2023 13:29

dbussink force-pushed the dbussink/remote-binlog-restore branch from 2f03810 to 8061c55 Compare May 22, 2023 14:01

dbussink commented May 22, 2023

View reviewed changes

dbussink added Type: Bug Component: TabletManager Component: Backup and Restore and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels May 22, 2023

dbussink marked this pull request as ready for review May 22, 2023 14:40

dbussink requested review from deepthi, harshit-gangal, mattlord, systay, shlomi-noach, rohit-nayak-ps, GuptaManan100 and ajm188 as code owners May 22, 2023 14:40

dbussink changed the title ~~restore: Run partial restore through mysqlctl~~ restore: Run PITR restore through mysqlctl May 22, 2023

dbussink changed the title ~~restore: Run PITR restore through mysqlctl~~ restore: Run PITR through mysqlctl May 22, 2023

shlomi-noach approved these changes May 22, 2023

View reviewed changes

harshit-gangal approved these changes May 23, 2023

View reviewed changes

harshit-gangal merged commit 04c4862 into vitessio:main May 23, 2023

harshit-gangal deleted the dbussink/remote-binlog-restore branch May 23, 2023 06:54

dbussink mentioned this pull request May 23, 2023

mysqlctl: Remove usage of MYSQL_FLAVOR #13135

Merged

4 tasks

This was referenced May 30, 2023

Temporarily disable panic in mysqld when socket files are directly used #13198

Merged

MoveTables panics in certain clusters #13203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

restore: Run PITR through mysqlctl #13123

restore: Run PITR through mysqlctl #13123

dbussink commented May 22, 2023 •

edited

Loading

vitess-bot bot commented May 22, 2023 •

edited by deepthi

Loading

dbussink May 22, 2023

dbussink May 22, 2023

dbussink May 22, 2023

dbussink May 22, 2023

dbussink May 22, 2023

dbussink May 22, 2023

dbussink May 22, 2023

shlomi-noach May 22, 2023

dbussink May 22, 2023

shlomi-noach May 22, 2023

dbussink May 22, 2023

restore: Run PITR through mysqlctl #13123

restore: Run PITR through mysqlctl #13123

Conversation

dbussink commented May 22, 2023 • edited Loading

Related Issue(s)

Checklist

vitess-bot bot commented May 22, 2023 • edited by deepthi Loading

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbussink commented May 22, 2023 •

edited

Loading

vitess-bot bot commented May 22, 2023 •

edited by deepthi

Loading