do not demote a new primary after backup completion

[olyazavr]

Description

We have had several outages when the following occurred:

1. a replica has xtrabackup running
2. that replica becomes promoted to primary
3. when xtrabackup completes, that primary is reverted back to replica
4. no primary is available, downtime ensues

I tracked this down to this line in vtctld where we do SetReplicationSource after a backup completes. This moves that into the backup itself, and only after it is done and engine.ShouldDrainForBackup() is true, we then refetch the tablet status, and then perform the equivalent to SetReplicationSource. Here we also check if the tablet itself has been promoted to a primary or not. More context here at #12856 (comment)

Related Issue(s)

Fixes #12754

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

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
• If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

• Is it really necessary to add this flag?
• Flag names should be clear and intuitive (as far as possible)
• Help text should be descriptive.
• Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow should be required, the maintainer team should be notified.

Bug fixes

• There should be at least one unit or end-to-end test.
• The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

• There should be some code comments as to why things are implemented the way they are.

New/Existing features

• Should be documented, either by modifying the existing documentation or creating new documentation.
• New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• vtctl command output order should be stable and awk-able.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from VTop, if used there.

[deepthi]

I believe the right fix is to actually not call SetReplicationSource here at all. The ChangeTabletType code path already effectively does that here
https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletmanager/rpc_actions.go#L100

[deepthi]

Though.. I'm guessing that this line is present to handle the case where the identity of the primary changed while the replica was taking a backup.
Even then, it doesn't belong at this outer layer, it should all be handled inside the call to tmc.Backup because that is where we are making the decisions of whether the tablet type needs to change or not, whether we stop MySQL or not etc.

[deepthi]

Also, it will be good if you can create a unit test to validate the final change. Either in https://github.com/vitessio/vitess/blob/main/go/vt/vtctl/grpcvtctldserver/server_test.go#L510 or in the tabletmanager package.

[GuptaManan100]

Hello Olga!,
I looked at the PR today and realized that the bug you are running into came to be because of a PR I did. #9725 was the PR that added the SetReplicationSource RPC at the end of the Backup call. The motivation for the change was to fix the semi-sync settings on the vttablets after the Backup process has ended. If a vttablet becomes a REPLICA type from BACKUP, it might need to start sending semi-sync ACKs (If the primary or the durability changed).
The addition of this however broke backups from xtrabackup since the tablet itself can become a PRIMARY which the code doesn't take into account.

As @deepthi said, it doesn't look like a good idea to keep adding code here in the vtctld layer. Instead, we should remove the SetReplicationSource call entirely from here and instead, fix the semi-sync and replication settings on the vttablet directly.

I think the best place for these changes would be https://github.com/vitessio/vitess/blob/main/go/vt/vttablet/tabletmanager/rpc_backup.go#L101. Here we can read the shard record and keyspace record to find the current primary and make necessary actions.

Regarding testing, I think tests can be added in https://github.com/vitessio/vitess/blob/main/go/vt/wrangler/testlib/backup_test.go as the original PR does.

[olyazavr]

I've moved the SetReplicationSource call out of vtctl and into vttablet, now trying to wrangle the tests

[olyazavr]

I'm having a hard time with the test- there doesn't seem to be a way to set xtrabackup, and for it to be a real test of the issue, there would have to be a reparent to make the replica tablet primary to ensure that we re-check. I'm trying to do this in an async go func to make it simultaneous with the backup

[olyazavr]

@deepthi @GuptaManan100 could you give this another look? I tested this with our setup and didn't see the bug

[GuptaManan100]

The changes look good to me! Does the newly added test fail without these changes? I tried running it without the changes in the PR and it still passed.

[GuptaManan100]

Also, the description needs to be changed to reflect the code changes that have happened

[olyazavr]

@GuptaManan100 I'm looking for help with the test- there doesn't seem to be a way to set xtrabackup, and for it to be a real test of the issue, there would have to be a reparent to make the replica tablet primary to ensure that we re-check. I tried to do this in an async go func to make it simultaneous with the backup, but it didn't work and anyhow seems flakey

[GuptaManan100]

@olyazavr I see! I agree with you that a unit test isn't sufficient for testing these changes. I think we should add an end-to-end test, especially because we have the ability to run xtrabackup there. You can take a look at https://github.com/vitessio/vitess/blob/main/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go and add a test here which runs a xtrabackup test and reparents the primary before the backup completes.
You can look at https://github.com/vitessio/vitess/blob/main/go/test/endtoend/reparent/plannedreparent/reparent_test.go to get some utility functions for running PRS, etc.
Let me know if you run into any issues and I'll try to help!

[olyazavr]

@GuptaManan100 I added an e2e test, though it also passes in main. The way I tried to simultaneously reparent and backup on that tablet is by launching two go functions, but I can't get it to print any output locally, so not sure how it's passing (perhaps it's a race here), could you give me some pointers?

[GuptaManan100]

@olyazavr I looked at the end to end test that you added. I noticed something right off the bat, you aren't setting up the xtrabackup flags in the vttablets. So they're probably running normal builtin backups. You can look at

vitess/go/test/endtoend/backup/vtctlbackup/backup_utils.go

Lines 140 to 158 in fa0761e

	if setupType == XtraBackup {
	useXtrabackup = true
	xtrabackupArgs := []string{
	"--backup_engine_implementation", "xtrabackup",
	fmt.Sprintf("--xtrabackup_stream_mode=%s", streamMode),
	"--xtrabackup_user=vt_dba",
	fmt.Sprintf("--xtrabackup_stripes=%d", stripes),
	"--xtrabackup_backup_flags", fmt.Sprintf("--password=%s", dbPassword),
	}
	// if streamMode is xbstream, add some additional args to test other xtrabackup flags
	if streamMode == "xbstream" {
	xtrabackupArgs = append(xtrabackupArgs, "--xtrabackup_prepare_flags", fmt.Sprintf("--use-memory=100M")) //nolint
	}
	commonTabletArg = append(commonTabletArg, xtrabackupArgs...)
	}

to see how the xtrabackup tests setup the flags. You'll need to put these into clusterInstance.VttabletExtraArgs before starting the vttablets.

Also, if you setup your VTDATAROOT environment variable before running the tests, you can find all the logs in that directory.

[olyazavr]

@GuptaManan100 added in the backup args to the test

[GuptaManan100]

This won't work because the cluster has already been setup. The vttablets are already running by the time this code runs. You should add a parameter to the function you're using and then put these configurations before the vttablets are launched.

[olyazavr]

Ok with enough finagling, I finally got this test to work 🎉

[GuptaManan100]

Everything LGTM now!

[GuptaManan100]

I have also tested the added test locally to make sure it isn't flaky!

[olyazavr]

this is an interesting failure when doing GetTablet:
Expected nil, but got: &json.UnmarshalTypeError{Value:"string", Type:(*reflect.rtype)(0x1049c60), Offset:364, Struct:"Time", Field:"primary_term_start_time.seconds"}

output of GetTablet is

{
        	            	  "alias": {
        	            	    "cell": "zone1",
        	            	    "uid": 9810
        	            	  },
        	            	  "hostname": "localhost",
        	            	  "port_map": {
        	            	    "grpc": 16408,
        	            	    "vt": 16407
        	            	  },
        	            	  "keyspace": "ks",
        	            	  "shard": "0",
        	            	  "key_range": null,
        	            	  "type": 1,
        	            	  "db_name_override": "",
        	            	  "tags": {},
        	            	  "mysql_hostname": "localhost",
        	            	  "mysql_port": 16409,
        	            	  "primary_term_start_time": {
        	            	    "seconds": "1684500897",
        	            	    "nanoseconds": 980657271
        	            	  },
        	            	  "default_conn_collation": 255
        	            	}
        	            	```

[GuptaManan100]

json2 is a package in Vitess "vitess.io/vitess/go/json2". Using that unmarshaller fixes the problem. 😄

[olyazavr]

I've rebased and fixed tests, this is good to go!

[GuptaManan100]

And just in time for RC-1. ❤️ it!