From a35145b461572a9cbfc44c36520829f4430ab6df Mon Sep 17 00:00:00 2001 From: DieterDP <90392398+DieterDP-ng@users.noreply.github.com> Date: Thu, 6 Jun 2024 13:48:46 +0200 Subject: [PATCH] HBASE-28562 Correct backup ancestor calculation (#5868) The ancestor calculation was wrong for incremental backups: when requesting the ancestors for an incremental backup X, the ancestors could include both full and incremental backups that predate the full backup on which X is built. This caused a crash in incremental backup creation when data of old incremental backups was deleted through other means than the HBase API (i.e. without the HBase backup system table being updated). Reviewed-by: Ray Mattingly Signed-off-by: Nick Dimiduk --- .../hbase/backup/impl/BackupManager.java | 99 ------------------- .../hbase/backup/impl/BackupManifest.java | 98 ------------------ .../backup/impl/FullTableBackupClient.java | 2 +- .../impl/IncrementalTableBackupClient.java | 2 +- .../hbase/backup/impl/TableBackupClient.java | 68 +++++++++++-- .../hadoop/hbase/backup/TestBackupBase.java | 4 +- .../TestIncrementalBackupWithDataLoss.java | 85 ++++++++++++++++ 7 files changed, 151 insertions(+), 207 deletions(-) create mode 100644 hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index ed1755ad5021..f0c93db4b4c2 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -25,7 +25,6 @@ import java.util.Map; import java.util.Set; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.backup.BackupHFileCleaner; @@ -34,8 +33,6 @@ import org.apache.hadoop.hbase.backup.BackupObserver; import org.apache.hadoop.hbase.backup.BackupRestoreConstants; import org.apache.hadoop.hbase.backup.BackupType; -import org.apache.hadoop.hbase.backup.HBackupFileSystem; -import org.apache.hadoop.hbase.backup.impl.BackupManifest.BackupImage; import org.apache.hadoop.hbase.backup.master.BackupLogCleaner; import org.apache.hadoop.hbase.backup.master.LogRollMasterProcedureManager; import org.apache.hadoop.hbase.backup.regionserver.LogRollRegionServerProcedureManager; @@ -268,102 +265,6 @@ public void setBackupInfo(BackupInfo backupInfo) { this.backupInfo = backupInfo; } - /** - * Get direct ancestors of the current backup. - * @param backupInfo The backup info for the current backup - * @return The ancestors for the current backup - * @throws IOException exception - */ - public ArrayList getAncestors(BackupInfo backupInfo) throws IOException { - LOG.debug("Getting the direct ancestors of the current backup {}", backupInfo.getBackupId()); - - ArrayList ancestors = new ArrayList<>(); - - // full backup does not have ancestor - if (backupInfo.getType() == BackupType.FULL) { - LOG.debug("Current backup is a full backup, no direct ancestor for it."); - return ancestors; - } - - // get all backup history list in descending order - ArrayList allHistoryList = getBackupHistory(true); - for (BackupInfo backup : allHistoryList) { - - BackupImage.Builder builder = BackupImage.newBuilder(); - - BackupImage image = builder.withBackupId(backup.getBackupId()).withType(backup.getType()) - .withRootDir(backup.getBackupRootDir()).withTableList(backup.getTableNames()) - .withStartTime(backup.getStartTs()).withCompleteTime(backup.getCompleteTs()).build(); - - // Only direct ancestors for a backup are required and not entire history of backup for this - // table resulting in verifying all of the previous backups which is unnecessary and backup - // paths need not be valid beyond the lifetime of a backup. - // - // RootDir is way of grouping a single backup including one full and many incremental backups - if (!image.getRootDir().equals(backupInfo.getBackupRootDir())) { - continue; - } - - // add the full backup image as an ancestor until the last incremental backup - if (backup.getType().equals(BackupType.FULL)) { - // check the backup image coverage, if previous image could be covered by the newer ones, - // then no need to add - if (!BackupManifest.canCoverImage(ancestors, image)) { - ancestors.add(image); - } - } else { - // found last incremental backup, if previously added full backup ancestor images can cover - // it, then this incremental ancestor is not the dependent of the current incremental - // backup, that is to say, this is the backup scope boundary of current table set. - // Otherwise, this incremental backup ancestor is the dependent ancestor of the ongoing - // incremental backup - if (BackupManifest.canCoverImage(ancestors, image)) { - LOG.debug("Met the backup boundary of the current table set:"); - for (BackupImage image1 : ancestors) { - LOG.debug(" BackupID={}, BackupDir={}", image1.getBackupId(), image1.getRootDir()); - } - } else { - Path logBackupPath = - HBackupFileSystem.getBackupPath(backup.getBackupRootDir(), backup.getBackupId()); - LOG.debug( - "Current backup has an incremental backup ancestor, " - + "touching its image manifest in {}" + " to construct the dependency.", - logBackupPath.toString()); - BackupManifest lastIncrImgManifest = new BackupManifest(conf, logBackupPath); - BackupImage lastIncrImage = lastIncrImgManifest.getBackupImage(); - ancestors.add(lastIncrImage); - - LOG.debug("Last dependent incremental backup image: {BackupID={}" + "BackupDir={}}", - lastIncrImage.getBackupId(), lastIncrImage.getRootDir()); - } - } - } - LOG.debug("Got {} ancestors for the current backup.", ancestors.size()); - return ancestors; - } - - /** - * Get the direct ancestors of this backup for one table involved. - * @param backupInfo backup info - * @param table table - * @return backupImages on the dependency list - * @throws IOException exception - */ - public ArrayList getAncestors(BackupInfo backupInfo, TableName table) - throws IOException { - ArrayList ancestors = getAncestors(backupInfo); - ArrayList tableAncestors = new ArrayList<>(); - for (BackupImage image : ancestors) { - if (image.hasTable(table)) { - tableAncestors.add(image); - if (image.getType() == BackupType.FULL) { - break; - } - } - } - return tableAncestors; - } - /* * backup system table operations */ diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java index 237d8686ab79..d66b5886794c 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java @@ -548,104 +548,6 @@ public ArrayList getDependentListByTable(TableName table) { return tableImageList; } - /** - * Get the full dependent image list in the whole dependency scope for a specific table of this - * backup in time order from old to new. - * @param table table - * @return the full backup image list for a table in time order in the whole scope of the - * dependency of this image - */ - public ArrayList getAllDependentListByTable(TableName table) { - ArrayList tableImageList = new ArrayList<>(); - ArrayList imageList = getRestoreDependentList(false); - for (BackupImage image : imageList) { - if (image.hasTable(table)) { - tableImageList.add(image); - } - } - return tableImageList; - } - - /** - * Check whether backup image1 could cover backup image2 or not. - * @param image1 backup image 1 - * @param image2 backup image 2 - * @return true if image1 can cover image2, otherwise false - */ - public static boolean canCoverImage(BackupImage image1, BackupImage image2) { - // image1 can cover image2 only when the following conditions are satisfied: - // - image1 must not be an incremental image; - // - image1 must be taken after image2 has been taken; - // - table set of image1 must cover the table set of image2. - if (image1.getType() == BackupType.INCREMENTAL) { - return false; - } - if (image1.getStartTs() < image2.getStartTs()) { - return false; - } - List image1TableList = image1.getTableNames(); - List image2TableList = image2.getTableNames(); - boolean found; - for (int i = 0; i < image2TableList.size(); i++) { - found = false; - for (int j = 0; j < image1TableList.size(); j++) { - if (image2TableList.get(i).equals(image1TableList.get(j))) { - found = true; - break; - } - } - if (!found) { - return false; - } - } - - LOG.debug("Backup image " + image1.getBackupId() + " can cover " + image2.getBackupId()); - return true; - } - - /** - * Check whether backup image set could cover a backup image or not. - * @param fullImages The backup image set - * @param image The target backup image - * @return true if fullImages can cover image, otherwise false - */ - public static boolean canCoverImage(ArrayList fullImages, BackupImage image) { - // fullImages can cover image only when the following conditions are satisfied: - // - each image of fullImages must not be an incremental image; - // - each image of fullImages must be taken after image has been taken; - // - sum table set of fullImages must cover the table set of image. - for (BackupImage image1 : fullImages) { - if (image1.getType() == BackupType.INCREMENTAL) { - return false; - } - if (image1.getStartTs() < image.getStartTs()) { - return false; - } - } - - ArrayList image1TableList = new ArrayList<>(); - for (BackupImage image1 : fullImages) { - List tableList = image1.getTableNames(); - for (TableName table : tableList) { - image1TableList.add(table.getNameAsString()); - } - } - ArrayList image2TableList = new ArrayList<>(); - List tableList = image.getTableNames(); - for (TableName table : tableList) { - image2TableList.add(table.getNameAsString()); - } - - for (int i = 0; i < image2TableList.size(); i++) { - if (image1TableList.contains(image2TableList.get(i)) == false) { - return false; - } - } - - LOG.debug("Full image set can cover image " + image.getBackupId()); - return true; - } - public BackupInfo toBackupInfo() { BackupInfo info = new BackupInfo(); info.setType(backupImage.getType()); diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java index fee2e825728e..06dad8880b5b 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java @@ -190,7 +190,7 @@ public void execute() throws IOException { backupManager.writeBackupStartCode(newStartCode); // backup complete - completeBackup(conn, backupInfo, backupManager, BackupType.FULL, conf); + completeBackup(conn, backupInfo, BackupType.FULL, conf); } catch (Exception e) { failBackup(conn, backupInfo, backupManager, e, "Unexpected BackupException : ", BackupType.FULL, conf); diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java index 211e9f96c89c..b7d1c4a95cc6 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java @@ -310,7 +310,7 @@ public void execute() throws IOException { handleBulkLoad(backupInfo.getTableNames()); // backup complete - completeBackup(conn, backupInfo, backupManager, BackupType.INCREMENTAL, conf); + completeBackup(conn, backupInfo, BackupType.INCREMENTAL, conf); } catch (IOException e) { failBackup(conn, backupInfo, backupManager, e, "Unexpected Exception : ", diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java index e758ced3f846..0aa6516fe4f3 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java @@ -19,8 +19,12 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Set; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; @@ -271,8 +275,8 @@ public static void cleanupAndRestoreBackupSystem(Connection conn, BackupInfo bac * @param backupInfo The current backup info * @throws IOException exception */ - protected void addManifest(BackupInfo backupInfo, BackupManager backupManager, BackupType type, - Configuration conf) throws IOException { + protected void addManifest(BackupInfo backupInfo, BackupType type, Configuration conf) + throws IOException { // set the overall backup phase : store manifest backupInfo.setPhase(BackupPhase.STORE_MANIFEST); @@ -281,13 +285,65 @@ protected void addManifest(BackupInfo backupInfo, BackupManager backupManager, B // set the table region server start and end timestamps for incremental backup manifest.setIncrTimestampMap(backupInfo.getIncrTimestampMap()); } - ArrayList ancestors = backupManager.getAncestors(backupInfo); + List ancestors = getAncestors(backupInfo); for (BackupImage image : ancestors) { manifest.addDependentImage(image); } manifest.store(conf); } + /** + * Gets the direct ancestors of the currently being created backup. + * @param backupInfo The backup info for the backup being created + */ + protected List getAncestors(BackupInfo backupInfo) throws IOException { + LOG.debug("Getting the direct ancestors of the current backup {}", backupInfo.getBackupId()); + + // Full backups do not have ancestors + if (backupInfo.getType() == BackupType.FULL) { + LOG.debug("Current backup is a full backup, no direct ancestor for it."); + return Collections.emptyList(); + } + + List ancestors = new ArrayList<>(); + Set tablesToCover = new HashSet<>(backupInfo.getTables()); + + // Go over the backup history list from newest to oldest + List allHistoryList = backupManager.getBackupHistory(true); + for (BackupInfo backup : allHistoryList) { + // If the image has a different rootDir, it cannot be an ancestor. + if (!Objects.equals(backup.getBackupRootDir(), backupInfo.getBackupRootDir())) { + continue; + } + + BackupImage.Builder builder = BackupImage.newBuilder(); + BackupImage image = builder.withBackupId(backup.getBackupId()).withType(backup.getType()) + .withRootDir(backup.getBackupRootDir()).withTableList(backup.getTableNames()) + .withStartTime(backup.getStartTs()).withCompleteTime(backup.getCompleteTs()).build(); + + // The ancestors consist of the most recent FULL backups that cover the list of tables + // required in the new backup and all INCREMENTAL backups that came after one of those FULL + // backups. + if (backup.getType().equals(BackupType.INCREMENTAL)) { + ancestors.add(image); + LOG.debug("Dependent incremental backup image: {BackupID={}}", image.getBackupId()); + } else { + if (tablesToCover.removeAll(new HashSet<>(image.getTableNames()))) { + ancestors.add(image); + LOG.debug("Dependent full backup image: {BackupID={}}", image.getBackupId()); + + if (tablesToCover.isEmpty()) { + LOG.debug("Got {} ancestors for the current backup.", ancestors.size()); + return Collections.unmodifiableList(ancestors); + } + } + } + } + + throw new IllegalStateException( + "Unable to find full backup that contains tables: " + tablesToCover); + } + /** * Get backup request meta data dir as string. * @param backupInfo backup info @@ -312,15 +368,15 @@ protected String obtainBackupMetaDataStr(BackupInfo backupInfo) { * @param backupInfo backup info * @throws IOException exception */ - protected void completeBackup(final Connection conn, BackupInfo backupInfo, - BackupManager backupManager, BackupType type, Configuration conf) throws IOException { + protected void completeBackup(final Connection conn, BackupInfo backupInfo, BackupType type, + Configuration conf) throws IOException { // set the complete timestamp of the overall backup backupInfo.setCompleteTs(EnvironmentEdgeManager.currentTime()); // set overall backup status: complete backupInfo.setState(BackupState.COMPLETE); backupInfo.setProgress(100); // add and store the manifest for the backup - addManifest(backupInfo, backupManager, type, conf); + addManifest(backupInfo, type, conf); // compose the backup complete data String backupCompleteData = diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java index 7b5095a897e2..e9c1cfd9c323 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java @@ -160,7 +160,7 @@ public void execute() throws IOException { failStageIf(Stage.stage_4); // backup complete - completeBackup(conn, backupInfo, backupManager, BackupType.INCREMENTAL, conf); + completeBackup(conn, backupInfo, BackupType.INCREMENTAL, conf); } catch (Exception e) { failBackup(conn, backupInfo, backupManager, e, "Unexpected Exception : ", @@ -244,7 +244,7 @@ public void execute() throws IOException { backupManager.writeBackupStartCode(newStartCode); failStageIf(Stage.stage_4); // backup complete - completeBackup(conn, backupInfo, backupManager, BackupType.FULL, conf); + completeBackup(conn, backupInfo, BackupType.FULL, conf); } catch (Exception e) { diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java new file mode 100644 index 000000000000..cf442f5f0dd7 --- /dev/null +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.hadoop.hbase.backup; + +import static org.junit.Assert.assertTrue; + +import java.util.List; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.backup.impl.BackupAdminImpl; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; + +@Category(LargeTests.class) +public class TestIncrementalBackupWithDataLoss extends TestBackupBase { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestIncrementalBackupWithDataLoss.class); + + private static final Logger LOG = + LoggerFactory.getLogger(TestIncrementalBackupWithDataLoss.class); + + @Test + public void testFullBackupBreaksDependencyOnOlderBackups() throws Exception { + LOG.info("test creation of backups after backup data was lost"); + + try (Connection conn = ConnectionFactory.createConnection(conf1)) { + BackupAdminImpl client = new BackupAdminImpl(conn); + List tables = Lists.newArrayList(table1); + + insertIntoTable(conn, table1, famName, 1, 1).close(); + String backup1 = + client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 2, 1).close(); + String backup2 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + + assertTrue(checkSucceeded(backup1)); + assertTrue(checkSucceeded(backup2)); + + // Simulate data loss on the backup storage + TEST_UTIL.getTestFileSystem().delete(new Path(BACKUP_ROOT_DIR, backup2), true); + + insertIntoTable(conn, table1, famName, 4, 1).close(); + String backup4 = + client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 5, 1).close(); + String backup5 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 6, 1).close(); + String backup6 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + + assertTrue(checkSucceeded(backup4)); + assertTrue(checkSucceeded(backup5)); + assertTrue(checkSucceeded(backup6)); + } + } + +}