HBASE-28882 Backup restores are broken if the backup has moved locations

[rmdmattingly]

https://issues.apache.org/jira/browse/HBASE-28882

My company runs a few hundred HBase clusters. We want to take backups everyday in one public cloud region, and then use said cloud's native replication solution to "backup our backups" in a secondary region. This is how we plan for region-wide disaster recovery.

This system should work, but doesn't because of the way that BackupManifests are constructed.

Backing up a bit (no pun intended): when we replicate backups verbatim, the manifest file continues to point to the original backup root. This shouldn't matter, because when taking a restore one passes a RestoreRequest to the RestoreTablesClient — and this RestoreRequest includes a BackupRootDir field. This works as you would expect initially, but eventually we build a BackupManifest that fails to interpolate this provided root directory and, instead, falls back to what it finds on disk in the backup (which would point back to the primary backup location, even if reading a replicated backup).

To fix this, I'm proposing that we properly interpolate the request's root directory field when building BackupManifests.

I've added a unit test for the new behavior, and I've used this updated client to successfully run a table restore of a replicated backup in our test environment.

cc @ndimiduk@charlesconnell@aalhour@ksravista@DieterDP-ng

[DieterDP-ng]

A question (before having had the time to go through your changes):
When testing backup functionality for my company, I believe I tested the ability to restore a backup on a new (empty) HBase cluster, which worked as expected. So a restore should be possible without using any local BackupInfo on the target cluster. But this PR seems to imply that local BackupInfo is in fact used. Is this a case of use-if-present, or am I missing something?

[rmdmattingly]

Did your test also involve moving the backup location? For example, moving the backup from an S3 bucket in us-east-1 to us-east-2 prior to attempting the restore?

Edit to clarify further: the simple backup case that you've described above definitely does work. Taking a backup and then restoring from that backup works well. But the more complicated case of:

1. Take a backup
2. Move that backup's location
3. Initiate a restore specifying the new location

Does not work because we stop using the specified location at this point in the restore internals.

[rmdmattingly]

Converting to a draft PR while I investigate what looks like a related test failures (testBackupHistory)

Edit: I made a precondition too strict for the variety of paths that might be passed into a BackupManifest constructor. Working on an update now, and will test more thoroughly locally

[DieterDP-ng]

Did your test also involve moving the backup location? For example, moving the backup from an S3 bucket in us-east-1 to us-east-2 prior to attempting the restore?

No, my testcase was "can a fresh HBase cluster restore a backup".

Does not work because we stop using the specified location at this point in the restore internals.

This is where I'm confused. A fresh cluster won't have any record stored of where backups were previously stored. So why is this an issue for a cluster that created the now-moved backup? (To be clear: I believe you saying this is an issue, I just don't understand yet where it comes from.)

[rmdmattingly]

No, my testcase was "can a fresh HBase cluster restore a backup".

👍

fresh cluster won't have any record stored of where backups were previously stored. So why is this an issue for a cluster that created the now-moved backup?

This is a good question that I don't have a certain answer to because the test that I'm running internally actually restores to the existing cluster in-place (it drops the existing table, and then restores from backup).

But I believe it would still be a problem for a restore to a new cluster because we don't load the backup manifest from metadata on the HBase cluster — we load the backup manifest from its on disk persistence in the given backup.

// load and set manifest field from file contentlonglen = subFile.getLen();
byte[] pbBytes = newbyte[(int) len];
try (FSDataInputStreamin = fs.open(subFile.getPath())) {
in.readFully(pbBytes);
} catch (IOExceptione) {
thrownewBackupException(e.getMessage());
}
BackupProtos.BackupImageproto = null;
try {
proto = BackupProtos.BackupImage.parseFrom(pbBytes);
} catch (Exceptione) {
thrownewBackupException(e);
}

For example:

1. cluster-a takes a backup and persists it to /primary/root/dir
2. we move that backup to /secondary/root/dir, verbatim, via S3 replication (or something similar)
3. we decide we want to restore this backup onto cluster-b
4. we send a restore request to cluster-b for this backup, with the root dir specified as /secondary/root/dir
5. the restore will read the BackupManifest file from /secondary/root/dir, as intended, but in the aforementioned code we'll blindly parse a proto that has the root dir listed as /primary/root/dir, both for its own BackupImage and all of the ancestor images
6. the restore will now run awry with an unclear/inconsistent root dir. Maybe it would succeed if your primary root dir still worked, the backup is still there, and your permissions allow access to both — but I don't think that "success" really aligns with the operator's intention of passing in the secondary root. And, more likely, it will fail because you've passed in a secondary root for a reason (perhaps the primary root dir is down, due to a region outage for example. Or perhaps you just enforce good least privilege)

This PR fixes this by intervening in step 5. After we parse the proto we'll hydrate the BackupImage (and its ancestors) with the backup root that was passed into the constructor, and that ensures that we stay aligned on one root

[DieterDP-ng]

I'd simplify this to return backupPath.getParent()

[rmdmattingly]

Good idea

[DieterDP-ng]

Browsed the code, looks good to me.

I now also get why this wasn't an issue for my tests when restoring a backup on a fresh cluster: the backup root hadn't changed in that case, so the paths in the backup manifests were still correct.

We could go one step further though. The core issue is that the protofied(?) BackupImage, which is stored as the backup manifest on the filesystem, contains it's own path. So, it would make sense to simply not store the path.
This would imply losing the functionality that the ancestors of a backup B could have different backuproots than B, but there's currently no functionality to create such backups, so that's not a real loss.
I glanced the code, and found no real need to persist the backuproot path.

[rmdmattingly]

Good idea, I don't obviously see a reason to persist the root on the BackupImage either, and any subtraction in this code is a big addition imo. I'll take a look at cleaning that up

[rmdmattingly]

@DieterDP-ng I've thought about it a little bit more, and I'm less sure about taking the path out of the BackupManifest. I like that the backups can be used, even if the original cluster and/or its backup system table has been lost.

If we removed this metadata from the backup itself, and the system table were lost, then we'd put more burden on the operator to provide additional details/args

[DieterDP-ng]

@DieterDP-ng I've thought about it a little bit more, and I'm less sure about taking the path out of the BackupManifest. I like that the backups can be used, even if the original cluster and/or its backup system table has been lost.

If we removed this metadata from the backup itself, and the system table were lost, then we'd put more burden on the operator to provide additional details/args

I'm not seeing the detriment. What exactly would become harder if the path is no longer stored inside the manifest?

• In case the operator wants to restore a backup, while the system table is still present, they'll have to do a hbase restore s3://.... (If they forget the path, they can retrieve it using hbase backup history.)
• In case the operator want to restore a backup on an empty cluster, they'll have to do a hbase restore s3://....

[rmdmattingly]

Yeah you're right, there wouldn't be a regression in usability — not sure I can even articulate my half-baked thought process from yesterday. Never mind

[rmdmattingly]

Sorry that I've fallen off here. I'll try to get this fixed up for a re-review this week.

[rmdmattingly]

Seems like a legitimate test failure, will dig in

[rmdmattingly]

Ok I've fixed the test failure by ensuring that we persist the merged backup's manifest file when deleting the tmp directory. Also added some logging to help with debuggability, and I think it's worth just keeping that around.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 33s		Docker mode activated.
-0 ⚠️	yetus	0m 4s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 59s		master passed
+1 💚	compile	0m 49s		master passed
+1 💚	javadoc	0m 23s		master passed
+1 💚	shadedjars	5m 36s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 50s		the patch passed
+1 💚	compile	0m 49s		the patch passed
+1 💚	javac	0m 49s		the patch passed
+1 💚	javadoc	0m 21s		the patch passed
+1 💚	shadedjars	5m 35s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	0m 31s		hbase-protocol-shaded in the patch passed.
+1 💚	unit	10m 38s		hbase-backup in the patch passed.
		32m 45s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6294/8/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6294
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 4c37c2c49f08 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 3e6d98a
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6294/8/testReport/
Max. process+thread count	3572 (vs. ulimit of 30000)
modules	C: hbase-protocol-shaded hbase-backup U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6294/8/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 42s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	buf	0m 0s		buf was not available.
+0 🆗	buf	0m 0s		buf was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+0 🆗	mvndep	0m 15s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 53s		master passed
+1 💚	compile	1m 9s		master passed
+1 💚	checkstyle	0m 19s		master passed
+1 💚	spotbugs	2m 42s		master passed
+1 💚	spotless	0m 43s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 49s		the patch passed
+1 💚	compile	1m 10s		the patch passed
+1 💚	cc	1m 10s		the patch passed
-0 ⚠️	javac	0m 31s	/results-compile-javac-hbase-backup.txt	hbase-backup generated 1 new + 101 unchanged - 0 fixed = 102 total (was 101)
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 11s	/results-checkstyle-hbase-backup.txt	hbase-backup: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 💚	spotbugs	2m 59s		the patch passed
+1 💚	hadoopcheck	10m 16s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	hbaseprotoc	1m 1s		the patch passed
+1 💚	spotless	0m 44s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 20s		The patch does not generate ASF License warnings.
		35m 25s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6294/8/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6294
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless cc buflint bufcompat hbaseprotoc
uname	Linux 59aa6b57b282 5.4.0-195-generic #215-Ubuntu SMP Fri Aug 2 18:28:05 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 3e6d98a
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	86 (vs. ulimit of 30000)
modules	C: hbase-protocol-shaded hbase-backup U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6294/8/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

[rmdmattingly]

This becomes necessary because the manifest file would implicitly move to the tmp directory during the merge process, since it follows the application's working root dir rather than its original root dir. So, before this changeset, rewriting the manifest would always update the original root dir (where we want this eventually), but with this changeset rewriting the manifest would put it in the tmp dir. Since the tmp dir will be purged, we need to make sure we copy the manifest file out first

[ndimiduk]

Do we have a test that asserts that we haven't lost any critical components of a backup image during processing it?

[rmdmattingly]

Not so specifically I suppose. But the larger tests do implicitly validate that — for example, I realized this change was necessary due to test failures

[DieterDP-ng]

While I agree with this change, I can't find what was changed that previously put the manifest in the correct location. Just curious.

[rmdmattingly]

Good question, this was confusing to me too. Before we stripped the root directory from the BackupImage, rewriting the manifest would implicitly always use the original root directory. So this only worked because we were unable to relocate backups

[ndimiduk]

Looks good to me. One more ping toward @DieterDP-ng, just in case.