Tablet init super read only #77

rsajwani · 2023-01-18T18:15:51Z

Description

Please review https://github.com/vitessio/vitess/pull/12206/files instead.

This change is build on top of vitessio#11520, where instead of using withddl we use declarative approach. Using declarative approach helps us condense all our schema changes to one place. This provide us an opportunity to apply super-read-only changes to our DB. As of withddl is used across schema engines with make it hard to do super-read-only change.

What is super-read-only change

As of today all replicas comes up in read-only mode, however this doesn't prevent user's like root and vt_dba who have SUPER privileges to change the database anytime anywhere. We want to leverage GLOBAL super_read_only configuration in order to protect against errant GTIDs. This will make sure that apart from primary no other component or offline system can mutate DB resulting in errant GTIDs that are then lying in wait to cause later failures, as you can see in vitessio#9312 and vitessio#10094

Furthermore not all the uses-cases are ensuring that we end up replica in read-only mode. During ERS there are cases where we don't set the read-only. With super-read-only change we will make sure that we cover those cases as well.

Details

This PR usese vitessio#11520 as a building block. I am listing major changes below to help you review the PR.

my.cnf contains super-read-only, to ensure all Mysql instances comes up in super-read-only mode.
During init_db we switch super-read-only off temporarily in order to perform some mutations like creating necessary users and permission.
Since MariaDB doesn't have super-read-only, therefore we need to come up with a separate init_db for it.
Adding # add custom sql here inside init-db.sql file. This is because in our test we modify the file and add custom SQL like (change password of DBA user etc). Now that we expect super_read_only to execute at the end, we need these custom sql to be added before that last line. See example go/test/endtoend/backup/vtbackup/main_test.go
During PRS & ERS. We make sure that only primary turn-off super-read-only and rest of replica are strictly in super-read-only mode.
For unit test I introduced a separate init_db.sql so that we don't run into complexity of super-read-only.
In some of tests I have to execute commands with super-read-only privileges, given the way the test are written.

For Reference

Some related work items done in the past

vitessio#10363
vitessio#11706
vitessio#10094
vitessio#9312
vitessio#10448

Unresolved Issued during code changes:

I have filed few issues which I will need to resolve once I check-in this PR.

Related Issue(s)

vitessio#12180

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

rsajwani · 2023-01-26T03:03:13Z

.github/workflows/upgrade_downgrade_test_backups_e2e.yml

@@ -161,8 +161,10 @@ jobs:
      run: |
        source build.env

-        rm -f $PWD/bin/vttablet
+        rm -f $PWD/bin/vttablet $PWD/bin/mysqlctl $PWD/bin/mysqlctld


vttablet + mysqlctl + mysqlctld needs to be always at the same version during upgrades and downgrades. I am taking this assumption after talking to different folks and it look logical to me as well.

rohit-nayak-ps

I have done a quick read through of the changes without really following the new flow. Hope to do that tomorrow. I will also look at all the tests where we are expecting different results now.

Suggested some minor typos/stylistic changes and asked some clarifications.

rohit-nayak-ps · 2023-01-26T19:40:59Z

config/embed.go

@@ -16,3 +16,6 @@ var MycnfMySQL57 string

 //go:embed mycnf/mysql80.cnf
 var MycnfMySQL80 string
+


Did you try checking if the underlying database is mysql or mariadb inside init_db.sql?

I discussed it initially with other team members .. and I concluded it will make code complex if we put that logic in .sql file .. or we might need to create templates. Furthermore Maria DB is going to be deprecated anyways in future release.

go/test/endtoend/backup/vtbackup/main_test.go

rohit-nayak-ps · 2023-01-26T19:54:53Z

go/test/endtoend/backup/vtbackup/main_test.go

+			return 1, err
+		}
+		log.Infof("cluster.VtTabletMajorVersion: %d", vtTabletVersion)
+		if vtTabletVersion <= 15 {


Why do we have conditional code for a tablet with version less than 16?

For downgrade / upgrade test, tablet version < 16 will not have super read only code handling. Therefore we are explicitly setting super-read-only to false here.

rohit-nayak-ps · 2023-01-26T20:09:58Z

go/vt/vtexplain/vtexplain_vttablet.go

 		return callback(emptyResult)
 	}
-	if strings.HasPrefix(query, "use _vt") || strings.HasPrefix(query, "CREATE TABLE _vt") || strings.HasPrefix(query, "ALTER TABLE _vt") || strings.HasPrefix(query, "CREATE TABLE IF NOT EXISTS _vt") {
+	if //strings.HasPrefix(query, "use `fakesqldb`") ||


The fakesqldb condition is commented out: intended?

There's a lot of stuff commented out (mixing of multi-line and single line comments, etc) and it's not clear what is intended and why.

@Rohit, this was commented out to show you, these changes from you PR is not needed .. But I have removed it now... You don't need to add these specific checks here instead I use existing structure of fakesqldb..

go/vt/vttablet/tabletserver/tx_engine.go

go/vt/sidecardb/sidecardb.go

go/vt/vttest/environment.go

rohit-nayak-ps · 2023-01-26T20:31:37Z

go/mysql/collations/integration/helpers_test.go

@@ -137,7 +137,7 @@ func verifyWeightString(t *testing.T, local collations.Collation, remote *remote
 }

 func exec(t *testing.T, conn *mysql.Conn, query string) *sqltypes.Result {
-	res, err := conn.ExecuteFetch(query, -1, true)
+	res, err := conn.ExecuteFetchWithSuperReadOnlyHandling(query, -1, true)


why do we need super readonly handling here: does the test run DDLs?

Yeah these test does not have vttablets. They just create mysql instance and run test against them...

rohit-nayak-ps · 2023-01-26T20:40:17Z

config/init_db.sql

@@ -77,3 +83,8 @@ FLUSH PRIVILEGES;

 RESET SLAVE ALL;
 RESET MASTER;
+
+# add custom sql here


instead of adding this, can't we just look for the line in the file that sets the readonly and add our custom commands above that?

I thought that would be more risky ... we don't know how would it react if client provide their own int_db.sql and it has some statements which does turns out true for our condition of readonly... Hence I thought this will be much cleaner approach...

rohit-nayak-ps · 2023-01-26T20:42:30Z

go/mysql/query.go

+	// Note: MariaDB does not have super_read_only but support for it is EOL in v14.0+
+	turnSuperReadOnly := false
+	if !c.IsMariaDB() {
+		if err := c.WriteComQuery("SELECT @@global.super_read_only"); err != nil {


we should extract this code which is repeated in multiple functions here, into a utility function

mattlord · 2023-01-26T23:18:23Z

config/embed.go

@@ -16,3 +16,6 @@ var MycnfMySQL57 string

 //go:embed mycnf/mysql80.cnf
 var MycnfMySQL80 string
+
+//go:embed init_maria_db.sql
+var DefaultInitMariaDB string


We can remove the MariaDB specific stuff in v16+ vitessio#9518. We still want to support importing/migrating from MariaDB into Vitess, but in that case the MariaDB instance would be external with an unmanaged tablet so init would not apply.

The import/migration test is using init_db.sql which now have super-read-only and therefore it fails. This is added to make sure our import/migrate test uses init_db specific to maria db...

mattlord · 2023-01-26T23:22:01Z

config/init_testserver_db.sql

@@ -0,0 +1,88 @@
+# This file is executed immediately after mysql_install_db,


What is this file for? Is it for vttestserver/vtcombo? What's unique about that compared to standard and then below, unit test cases? We can add a comment at the top to explain. It feels like this is part of a larger problem in that we're not testing super_read_only in many tests.

We are testing super-read-only for all the cases.

super-read-only does not make sense for unit test .. therefore I have come up with new config/init_unit_test_db.sql

testserver/vtcombo does not operate in same manner.. it uses same sql instance for all the vttablet under the hood so we have to get rid of super-read-only in this case as well , hence config/init_testserver_db.sql

Every other test e2e and backup/restore etc uses init_db.sql file, which has super-read-only.

config/mycnf/mysql80.cnf

mattlord · 2023-01-26T23:25:14Z

config/mycnf/test-suite.cnf

@@ -23,3 +23,4 @@ sql_mode = STRICT_TRANS_TABLES

 # set a short heartbeat interval in order to detect failures quickly
 slave_net_timeout = 4
+super-read-only = false


This change can have wide impacts. IMO, we should test it in as many ways as possible. i.e. we should use super-read-only in all of the tests, if possible.

test-suite is used only in vttestserver. VtTestserver uses vtcombo which has one sql instance for all vttablet. We cannot turn 'on/'off' super-read-only because it will make vtcombo confuse due to one instance of mysql for all vttablets.

Are we using vttestserver in our test/e2e , i don't see instances where we are...

go/cmd/vtbackup/vtbackup.go

mattlord · 2023-01-27T00:41:12Z

go/test/endtoend/vtorc/utils/utils.go

+// RunSQLWithSuperReadOnly is used to run a SQL statement on the given tablet with super read only set to true or false
+func RunSQLWithSuperReadOnly(t *testing.T, sql string, tablet *cluster.Vttablet, db string, withSuperReadOnly bool) (*sqltypes.Result, error) {
+	// Get Connection
+	tabletParams := getMysqlConnParam(tablet, db)
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+	conn, err := mysql.Connect(ctx, &tabletParams)
+	require.Nil(t, err)
+	defer conn.Close()
+
+	// RunSQL
+	return execute(t, conn, sql, withSuperReadOnly)
+}


This is another case where it feels like we're avoiding testing the fact that Vitess works properly when super_read_only is enabled. No?

if you closely look at I have introduced a flag 'withSuperReadOnly' ... during test setup we introduce errantGTID (https://github.com/planetscale/vitess/pull/77/files#diff-9162b364b991658ff1575922d4a008aecc15e44fd27e64678f24cce330033d9fR108) thats the only place where we pass true to this flag. For all other places we keep it false. Hence the tests are running with super-read-only false.

go/test/endtoend/vtorc/readtopologyinstance/main_test.go

go/vt/mysqlctl/mysqld.go

mattlord · 2023-01-27T00:45:40Z

go/vt/mysqlctl/mysqld.go

+		// have super_read_only This is safe, since we're restarting MySQL after the restore anyway
+		log.Infof("Restore: disabling super_read_only")
+		if err := mysqld.SetSuperReadOnly(false); err != nil {
+			if strings.Contains(err.Error(), strconv.Itoa(mysql.ERUnknownSystemVariable)) {


Better to cast it to a MySQLError and check the error code.

this is how we are doing check for something not part of specific mysql version... we want to warn if super-read-only is not supported in case of db like mariadb and erorr in all other cases. We are not checking any specific mysql erorr.

mattlord · 2023-01-27T00:46:07Z

go/vt/mysqlctl/mysqld.go

+		// We disable super_read_only, in case it is in the default MySQL startup
+		// parameters.  We do it blindly, since this will fail on MariaDB, which doesn't
+		// have super_read_only This is safe, since we're restarting MySQL after the restore anyway
+		log.Infof("Restore: disabling super_read_only")
+		if err := mysqld.SetSuperReadOnly(false); err != nil {
+			if strings.Contains(err.Error(), strconv.Itoa(mysql.ERUnknownSystemVariable)) {
+				log.Warningf("Restore: server does not know about super_read_only, continuing anyway...")
+			} else {
+				log.Errorf("Restore: unexpected error while trying to set super_read_only: %v", err)
+				return err
+			}
+		}


How is this function directly/exclusively tied to restores? If someone runs the equivalent of mysqldump foo.10 | mysql, it would be bad if mysqld automatically disabled super_read_only rather than protecting you as it should.

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

rsajwani · 2023-02-03T02:42:03Z

please review vitessio#12206 instead.

rsajwani requested review from harshit-gangal, systay, shlomi-noach, rohit-nayak-ps, deepthi, GuptaManan100, ajm188, mattlord and frouioui as code owners January 18, 2023 18:15

rsajwani changed the title ~~Tablet init super read only~~ [Don't Review] Tablet init super read only Jan 18, 2023

rsajwani self-assigned this Jan 19, 2023

rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch from b730d1f to 7a426aa Compare January 20, 2023 19:59

rohit-nayak-ps requested a review from notfelineit as a January 20, 2023 19:59

rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch 7 times, most recently from 404ed69 to fcea2d8 Compare January 20, 2023 23:35

rsajwani force-pushed the tablet-init-super-read-only branch from 058cd6b to bc58cb5 Compare January 21, 2023 00:15

rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch from b3c996d to 21cc81e Compare January 25, 2023 19:15

rsajwani force-pushed the tablet-init-super-read-only branch from 0904832 to 58a1e92 Compare January 26, 2023 00:45

rsajwani commented Jan 26, 2023

View reviewed changes

rsajwani changed the title ~~[Don't Review] Tablet init super read only~~ Tablet init super read only Jan 26, 2023

rsajwani mentioned this pull request Jan 26, 2023

[Don't review] Issue#10363 ii vitessio/vitess#11706

Closed

3 tasks

rohit-nayak-ps reviewed Jan 26, 2023

View reviewed changes

mattlord reviewed Jan 27, 2023

View reviewed changes

rohit-nayak-ps force-pushed the rn-tablet-init-refactor branch from 5952836 to d03a02b Compare January 28, 2023 18:47

rsajwani force-pushed the tablet-init-super-read-only branch from 8eaceaf to c9b7c05 Compare January 29, 2023 03:54

rsajwani and others added 25 commits February 1, 2023 00:26

fixing schema sql

4c99a57

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing unit tests

a64fd4d

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing vttest server test cases

80879a2

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing more downgrad upgrade test

30aa865

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing reparent upgrade downgrade test

5c7eb2e

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Bug fix for replication position

ee952a4

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Fixing vtexplain tests

881f50b

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Fixing mariadb initialization

a8b230a

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing upgrade downgrade e2e test

605a308

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Fixing assignment of init sql db file

3bedb2b

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

code clean up

1256c71

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Fixing backup test

211253e

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

adding super read only api for sql demon

c025af6

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

converting read-only calls to super-read-only calls

32789e6

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

restructure vtexplain code to be more generic

c857d25

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

forgot to make proto vtadmin

c263a57

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Code review part-1

7f5d871

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

remove code for redolog in test

3d7bf35

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fix ensureDB test

dca3838

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

code review part-2

7d43d0b

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Remove setSuperReadOnly checks

cfa97fe

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

code review part-3

fad2b7d

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Try restoring without super_read_only

473fe4a

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

test: simplify vtorc test to not require turning off super-read-only

0f4b189

Signed-off-by: Manan Gupta <manan@planetscale.com>

code review part-4

fe21638

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani force-pushed the tablet-init-super-read-only branch from f824ef0 to fe21638 Compare February 1, 2023 08:30

rsajwani and others added 2 commits February 1, 2023 08:43

Fixing typo

62a4a22

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

test: fix vtorc test

9330333

Signed-off-by: Manan Gupta <manan@planetscale.com>

rsajwani closed this Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tablet init super read only #77

Tablet init super read only #77

rsajwani commented Jan 18, 2023 •

edited

Loading

rsajwani Jan 26, 2023

rohit-nayak-ps left a comment

rohit-nayak-ps Jan 26, 2023

rsajwani Jan 27, 2023

rohit-nayak-ps Jan 26, 2023

rsajwani Jan 27, 2023

rohit-nayak-ps Jan 26, 2023

mattlord Jan 26, 2023

rsajwani Jan 27, 2023

rohit-nayak-ps Jan 26, 2023

rsajwani Jan 28, 2023

rohit-nayak-ps Jan 26, 2023

rsajwani Jan 28, 2023

rohit-nayak-ps Jan 26, 2023

mattlord Jan 26, 2023

rsajwani Jan 28, 2023

mattlord Jan 26, 2023

rsajwani Jan 28, 2023

mattlord Jan 26, 2023

rsajwani Jan 28, 2023

rsajwani Jan 28, 2023 •

edited

Loading

mattlord Jan 27, 2023

rsajwani Jan 28, 2023

mattlord Jan 27, 2023

rsajwani Jan 28, 2023

mattlord Jan 27, 2023

rsajwani commented Feb 3, 2023

		@@ -16,3 +16,6 @@ var MycnfMySQL57 string

		//go:embed mycnf/mysql80.cnf
		var MycnfMySQL80 string

		@@ -0,0 +1,88 @@
		# This file is executed immediately after mysql_install_db,

Tablet init super read only #77

Tablet init super read only #77

Conversation

rsajwani commented Jan 18, 2023 • edited Loading

Description

What is super-read-only change

Details

For Reference

Unresolved Issued during code changes:

Related Issue(s)

Checklist

Deployment Notes

Choose a reason for hiding this comment

rohit-nayak-ps left a 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

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

rsajwani Jan 28, 2023 • edited Loading

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

rsajwani commented Feb 3, 2023

rsajwani commented Jan 18, 2023 •

edited

Loading

rsajwani Jan 28, 2023 •

edited

Loading