HBASE-29776: Log filtering in IncrementalBackupManager can lead to data loss #8694
There are no files selected for viewing
|
hgromer marked this conversation as resolved.
hgromer marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,10 @@ | |
| 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.Set; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FileStatus; | ||
| import org.apache.hadoop.fs.FileSystem; | ||
|
|
@@ -54,12 +56,14 @@ public IncrementalBackupManager(Connection conn, Configuration conf) throws IOEx | |
| /** | ||
| * Obtain the list of logs that need to be copied out for this incremental backup. The list is set | ||
| * in BackupInfo. | ||
| * @return The new map of RS log time stamps for this incremental backup, as computed by | ||
| * {@link BackupUtils#computeLogBoundaries}: the included logs are covered, the logs held | ||
| * back for a later backup (for example, logs still being split) are pending, and a host | ||
| * that still has logs keeps its previous boundary if this backup gives it no new one. | ||
| * @throws IOException exception | ||
| */ | ||
| public Map<String, Long> getIncrBackupLogFileMap() throws IOException { | ||
| List<String> logList; | ||
| Map<String, Long> previousTimestampMins = | ||
| BackupUtils.getRSLogTimestampMins(readLogTimestampMap()); | ||
|
|
||
|
|
@@ -69,18 +73,28 @@ public Map<String, Long> getIncrBackupLogFileMap() throws IOException { | |
| + "In order to create an incremental backup, at least one full backup is needed."); | ||
| } | ||
|
|
||
| Map<String, Long> previousLogRollByHost = readRegionServerLastLogRollResult(); | ||
| LOG.info("Execute roll log procedure for incremental backup ..."); | ||
| BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf); | ||
|
|
||
| Map<String, Long> rolledHosts = | ||
| BackupUtils.getRolledHosts(previousLogRollByHost, readRegionServerLastLogRollResult()); | ||
|
|
||
| LogFileSelection selection = getLogFilesForNewBackup(previousTimestampMins, rolledHosts, conf); | ||
| logList = excludeProcV2WALs(selection.included()); | ||
| backupInfo.setIncrBackupFileList(logList); | ||
|
|
||
| Map<String, Long> newTimestamps = BackupUtils.computeLogBoundaries(rolledHosts, | ||
| previousTimestampMins, selection.hostsWithLogs(), logList, selection.heldBack()); | ||
| LOG.debug("Log boundaries for incremental backup {}: {}", backupInfo.getBackupId(), | ||
| newTimestamps); | ||
| return newTimestamps; | ||
| } | ||
|
|
||
| private record LogFileSelection(List<String> included, List<String> heldBack, | ||
| Set<String> hostsWithLogs) { | ||
| } | ||
|
|
||
| private List<String> excludeProcV2WALs(List<String> logList) { | ||
| List<String> list = new ArrayList<>(); | ||
| for (int i = 0; i < logList.size(); i++) { | ||
|
|
@@ -97,15 +111,17 @@ private List<String> excludeProcV2WALs(List<String> logList) { | |
| } | ||
|
|
||
| /** | ||
| * Gather all log files that either: 1) are newer than the older timestamps, but not newer than | ||
| * the newest timestamps, or 2) are archived logs whose host name does not occur in the newest | ||
| * timestamps. | ||
| * @param olderTimestamps the timestamp for each region server of the last backup. | ||
| * @param newestTimestamps the timestamp for each region server that the backup should lead to. | ||
| * @param conf the Hadoop and Hbase configuration | ||
| * @return the log files to be backed up, the log files held back for a later backup, and the | ||
| * hosts that have any log files, including ones not backed up | ||
| * @throws IOException exception | ||
| */ | ||
| private LogFileSelection getLogFilesForNewBackup(Map<String, Long> olderTimestamps, | ||
|
Contributor
Author
There was a problem hiding this comment. If a server crashes as we're doing log rolls + backups, its WALs will sit in the We will keep the boundary at before the oldest held back WAL, ensuring that the log cleaner doesn't delete it. These WALs will be backed up in the next incremental. |
||
| Map<String, Long> newestTimestamps, Configuration conf) throws IOException { | ||
| LOG.debug("In getLogFilesForNewBackup()\n" + "olderTimestamps: " + olderTimestamps | ||
| + "\n newestTimestamps: " + newestTimestamps); | ||
|
|
@@ -119,6 +135,7 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps, | |
|
|
||
| List<String> resultLogFiles = new ArrayList<>(); | ||
| List<String> newestLogs = new ArrayList<>(); | ||
| Set<String> hostsWithLogs = new HashSet<>(); | ||
|
|
||
| /* | ||
| * The old region servers and timestamps info we kept in backup system table may be out of sync | ||
|
|
@@ -143,6 +160,7 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps, | |
| if (host == null) { | ||
| continue; | ||
| } | ||
| hostsWithLogs.add(host); | ||
| FileStatus[] logs; | ||
| oldTimeStamp = olderTimestamps.get(host); | ||
| // It is possible that there is no old timestamp in backup system table for this host if | ||
|
|
@@ -198,6 +216,7 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps, | |
| if (host == null) { | ||
| continue; | ||
| } | ||
| hostsWithLogs.add(host); | ||
| currentLogTS = BackupUtils.getCreationTime(p); | ||
| oldTimeStamp = olderTimestamps.get(host); | ||
| /* | ||
|
|
@@ -217,18 +236,14 @@ private List<String> getLogFilesForNewBackup(Map<String, Long> olderTimestamps, | |
| resultLogFiles.add(currentLogFile); | ||
| } | ||
|
|
||
| Long newTimestamp = newestTimestamps.get(host); | ||
|
Contributor
Author
There was a problem hiding this comment. I know there was debate whether this was necessary or not here, and want to emphasize that this is a very important change. Without this change, we're subject to data loss anytime a new RS joins the cluster. If we add just this code snippet back, our There was some discussion about modifying this check due to hypothetical scenarios. I think it's a lot safer to simply remove this block which can cause data loss and provides no value. Simply removing this check keeps things safe. And I don't think the scenario here warrants potential data loss. Even if this scenario did play out, the only consequence is that we include a WAL that we don't technically need in a backup. This is a non-issue, given that our restore process handles this without any issues. Finally, there was dicussion around trying to ensure "cross-table consistency" and data being out of sync which I disagree with as well. This backup is a point-in-time snapshot. In a distributed system, each RS is going to have it's own point in time. This is similarly true for snapshots, where one region's snapshot procedure can happen minutes prior to another one. Regardless, I think preventing data loss scenarios and simplifying the code should be the priority here. There was a problem hiding this comment. I still argue to not delete this check, but to change it to Two things have to occur on a region server during an incremental backup to cause data loss:
Of these 2 conditions, I expect (2) to occur commonly. For (1): The window runs from the backup's roll to the end of the oldWALs listing, and that's normally a few seconds. Two rolls might fit in it when one of these holds:
A rare case, but one that can occur on any incremental backup on any region server. For a large cluster with daily backups, it is bound to happen eventually. The result is a WAL that never gets included in a backup, leading to data loss when restoring. I agree that simpler is better, and I wished lots of the backup code was simpler, but in this case the complexity is worth it. Example test case (breaks on the current PR, succeeds on the current PR + suggested adjustment): // paste me in TestIncrementalBackupManager
/**
* WALs can be archived out of order. This test sets up the WAL directories as an incremental
* backup would find them if, after its log roll, a newer WAL got archived while an older one was
* still in the WALs directory. The older WAL must still end up in a backup.
*/
@Test
public void testOutOfOrderArchivedWALDoesNotSkipOlderWAL() throws Exception {
List<TableName> tables = List.of(table1);
HRegionServer rs = TEST_UTIL.getMiniHBaseCluster().getRegionServer(0);
ServerName serverName = rs.getServerName();
Path walRootDir = CommonFSUtils.getWALRootDir(conf1);
FileSystem fs = walRootDir.getFileSystem(conf1);
try (Connection conn = ConnectionFactory.createConnection(conf1);
BackupAdminImpl backupAdmin = new BackupAdminImpl(conn)) {
String fullBackupId =
backupAdmin.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR));
assertTrue(checkSucceeded(fullBackupId));
// Both test WALs are newer than any WAL that exists at this point, so they are newer than the
// WAL reported by the log roll of the next incremental backup.
long olderWALTs = EnvironmentEdgeManager.currentTime() + 1;
long newerWALTs = olderWALTs + 1;
Path walDir =
new Path(walRootDir, AbstractFSWALProvider.getWALDirectoryName(serverName.toString()));
Path olderWAL =
new Path(walDir, serverName.toString() + BackupUtils.LOGNAME_SEPARATOR + olderWALTs);
Path archiveDir = new Path(walRootDir,
AbstractFSWALProvider.getWALArchiveDirectoryName(conf1, serverName.toString()));
Path newerArchivedWAL =
new Path(archiveDir, "wal" + BackupUtils.LOGNAME_SEPARATOR + newerWALTs);
fs.create(olderWAL).close();
fs.mkdirs(archiveDir);
fs.create(newerArchivedWAL).close();
try {
List<String> firstBackupFiles;
try (IncrementalBackupManager manager = new IncrementalBackupManager(conn, conf1)) {
BackupInfo backupInfo = manager.createBackupInfo("backup_incr_1",
BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR, -1, -1, false, false);
Map<String, Long> boundaries = manager.getIncrBackupLogFileMap();
manager.writeRegionServerLogTimestamp(backupInfo.getTables(), boundaries);
firstBackupFiles = backupInfo.getIncrBackupFileList();
}
// Roll once more, so the log roll of the next backup reports a WAL that is newer than both
// test WALs.
TEST_UTIL.waitFor(30_000, () -> EnvironmentEdgeManager.currentTime() > newerWALTs);
rs.getWalRoller().requestRollAll();
rs.getWalRoller().waitUntilWalRollFinished();
List<String> secondBackupFiles;
try (IncrementalBackupManager manager = new IncrementalBackupManager(conn, conf1)) {
BackupInfo backupInfo = manager.createBackupInfo("backup_incr_2",
BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR, -1, -1, false, false);
manager.getIncrBackupLogFileMap();
secondBackupFiles = backupInfo.getIncrBackupFileList();
}
assertTrue(
firstBackupFiles.contains(olderWAL.toString())
|| secondBackupFiles.contains(olderWAL.toString()),
"WAL " + olderWAL + " was not included in any backup. First backup: " + firstBackupFiles
+ ", second backup: " + secondBackupFiles);
} finally {
fs.delete(olderWAL, false);
fs.delete(newerArchivedWAL, false);
}
}
}
Contributor
Author
There was a problem hiding this comment. Hmm, okay I think I see your point, I didn't realize that WAL archival was random. This comment makes sense. In that case, I think your proposed change makes sense, while still preventing the data loss scenario that I'm trying to address |
||
| if (newTimestamp != null && currentLogTS > newTimestamp) { | ||
| newestLogs.add(currentLogFile); | ||
| } | ||
| } | ||
| // remove newest log per host because they are still in use | ||
| resultLogFiles.removeAll(newestLogs); | ||
| return new LogFileSelection(resultLogFiles, newestLogs, hostsWithLogs); | ||
| } | ||
|
|
||
| static class NewestLogFilter implements PathFilter { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, I can't think of a much better way to accomplish consistency here. The HMaster may be initializing when we query for live RS, and unfortunately that means we may get a partial list of RS back.
This sanity check is a little brittle, but I believe that it's robust enough to do what we need. Given that we just rolled WAL files, if we don't see any of those hosts in the live RS list we got back from the HMaster, we can likely assume the HMaster didn't return a comprehensive list, and we should abort.