From 7ab8dc312867a060f5536c71c20abd9512aacb2c Mon Sep 17 00:00:00 2001 From: Al Sutton Date: Thu, 16 Jan 2020 12:38:45 +0000 Subject: [PATCH] Notify transports of empty K/V backups In order to allow transports to know when a K/V backup would have been performed, but was not due to no data changes being reported, we need to introduce some code to find the K/V backup participants, remove any which are backed up in this sweep or are currently failing to back up due to an error, and then inform the transport of the remaing set. This CL introduces the code to do that. We use a state file to determine if a package backed up without an error on the last run, if the backup fails we remove the file for that package. When all packages with changed data have been backed up we get a list of all packages which have the file set (i.e. were successful), remove the set of packages backed up on this run, and inform the transport the rest had no data changes. Bug:147481066 Test: m -j RunBackupFrameworksServicesRoboTests Change-Id: I5c9f94c925096faf7b65307c0be1a7aba48c1cfb --- .../backup/keyvalue/KeyValueBackupTask.java | 196 ++++++++++++++++++ .../keyvalue/KeyValueBackupTaskTest.java | 94 +++++++++ 2 files changed, 290 insertions(+) diff --git a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java index bda0e3bb39fc..5e10916c4491 100644 --- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java +++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java @@ -47,6 +47,7 @@ import android.os.RemoteException; import android.os.SELinux; import android.os.UserHandle; import android.os.WorkSource; +import android.util.Log; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; @@ -65,14 +66,18 @@ import com.android.server.backup.remote.RemoteCall; import com.android.server.backup.remote.RemoteCallable; import com.android.server.backup.remote.RemoteResult; import com.android.server.backup.transport.TransportClient; +import com.android.server.backup.transport.TransportNotAvailableException; import com.android.server.backup.utils.AppBackupUtils; +import libcore.io.IoUtils; + import java.io.Closeable; import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.File; import java.io.FileDescriptor; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; import java.lang.annotation.Retention; @@ -80,8 +85,10 @@ import java.lang.annotation.RetentionPolicy; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; @@ -169,10 +176,14 @@ import java.util.concurrent.atomic.AtomicInteger; // TODO: Consider having the caller responsible for some clean-up (like resetting state) // TODO: Distinguish between cancel and time-out where possible for logging/monitoring/observing public class KeyValueBackupTask implements BackupRestoreTask, Runnable { + private static final String TAG = "KVBT"; + private static final int THREAD_PRIORITY = Process.THREAD_PRIORITY_BACKGROUND; private static final AtomicInteger THREAD_COUNT = new AtomicInteger(); private static final String BLANK_STATE_FILE_NAME = "blank_state"; private static final String PM_PACKAGE = UserBackupManagerService.PACKAGE_MANAGER_SENTINEL; + private static final String SUCCESS_STATE_SUBDIR = "backing-up"; + @VisibleForTesting static final String NO_DATA_END_SENTINEL = "@end@"; @VisibleForTesting public static final String STAGING_FILE_SUFFIX = ".data"; @VisibleForTesting public static final String NEW_STATE_FILE_SUFFIX = ".new"; @@ -336,6 +347,7 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { mHasDataToBackup = false; + Set backedUpApps = new HashSet<>(); int status = BackupTransport.TRANSPORT_OK; try { startTask(); @@ -347,13 +359,18 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { } else { backupPackage(packageName); } + setSuccessState(packageName, true); + backedUpApps.add(packageName); } catch (AgentException e) { + setSuccessState(packageName, false); if (e.isTransitory()) { // We try again this package in the next backup pass. mBackupManagerService.dataChangedImpl(packageName); } } } + + informTransportOfUnchangedApps(backedUpApps); } catch (TaskException e) { if (e.isStateCompromised()) { mBackupManagerService.resetBackupState(mStateDirectory); @@ -364,6 +381,185 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable { finishTask(status); } + /** + * Tell the transport about all of the packages which have successfully backed up but + * have not informed the framework that they have new data. This allows transports to + * differentiate between packages which are not backing data up due to an error and + * packages which are not backing up data because nothing has changed. + * + * The current implementation involves creating a state file when a backup succeeds, + * on subsequent runs the existence of the file indicates the backup ran successfully + * but there was no data. If a backup fails with an error, or if the package is not + * eligible for backup by the transport any more, the status file is removed and the + * "no data" message will not be sent to the transport until another successful data + * changed backup has succeeded. + * + * @param appsBackedUp The Set of apps backed up during this run so we can exclude them + * from the list of successfully backed up apps that we signal to + * the transport have no data. + */ + private void informTransportOfUnchangedApps(Set appsBackedUp) { + String[] succeedingPackages = getSucceedingPackages(); + if (succeedingPackages == null) { + // Nothing is succeeding, so end early. + return; + } + + int flags = BackupTransport.FLAG_DATA_NOT_CHANGED; + if (mUserInitiated) { + flags |= BackupTransport.FLAG_USER_INITIATED; + } + + boolean noDataPackageEncountered = false; + try { + IBackupTransport transport = + mTransportClient.connectOrThrow("KVBT.informTransportOfEmptyBackups()"); + + for (String packageName : succeedingPackages) { + if (appsBackedUp.contains(packageName)) { + Log.v(TAG, "Skipping package which was backed up this time :" + packageName); + // Skip packages we backed up in this run. + continue; + } + + PackageInfo packageInfo; + try { + packageInfo = mPackageManager.getPackageInfo(packageName, /* flags */ 0); + if (!isEligibleForNoDataCall(packageInfo)) { + // If the package isn't eligible any more we can forget about it and move + // on. + clearStatus(packageName); + continue; + } + } catch (PackageManager.NameNotFoundException e) { + // If the package has been uninstalled we can forget about it and move on. + clearStatus(packageName); + continue; + } + + sendNoDataChangedTo(transport, packageInfo, flags); + noDataPackageEncountered = true; + } + + if (noDataPackageEncountered) { + // If we've notified the transport of an unchanged package we need to + // tell it that it's seen all of the unchanged packages. We do this by + // reporting the end sentinel package as unchanged. + PackageInfo endSentinal = new PackageInfo(); + endSentinal.packageName = NO_DATA_END_SENTINEL; + sendNoDataChangedTo(transport, endSentinal, flags); + } + } catch (TransportNotAvailableException | RemoteException e) { + Log.e(TAG, "Could not inform transport of all unchanged apps", e); + } + } + + /** Determine if a package is eligible to be backed up to the transport */ + private boolean isEligibleForNoDataCall(PackageInfo packageInfo) { + return AppBackupUtils.appIsKeyValueOnly(packageInfo) + && AppBackupUtils.appIsRunningAndEligibleForBackupWithTransport(mTransportClient, + packageInfo.packageName, mPackageManager, mUserId); + } + + /** Send the "no data changed" message to a transport for a specific package */ + private void sendNoDataChangedTo(IBackupTransport transport, PackageInfo packageInfo, int flags) + throws RemoteException { + ParcelFileDescriptor pfd; + try { + pfd = ParcelFileDescriptor.open(mBlankStateFile, MODE_READ_ONLY | MODE_CREATE); + } catch (FileNotFoundException e) { + Log.e(TAG, "Unable to find blank state file, aborting unchanged apps signal."); + return; + } + try { + int result = transport.performBackup(packageInfo, pfd, flags); + if (result == BackupTransport.TRANSPORT_ERROR + || result == BackupTransport.TRANSPORT_NOT_INITIALIZED) { + Log.w( + TAG, + "Aborting informing transport of unchanged apps, transport" + " errored"); + return; + } + + transport.finishBackup(); + } finally { + IoUtils.closeQuietly(pfd); + } + } + + /** Get the list of package names which are marked as having previously succeeded */ + private String[] getSucceedingPackages() { + File stateDirectory = getTopLevelSuccessStateDirectory(/* createIfMissing */ false); + if (stateDirectory == null) { + // getSuccessStateFileFor logs when we can't use the state area + return null; + } + + return stateDirectory.list(); + } + + /** Sets the indicator that a package backup is succeeding */ + private void setSuccessState(String packageName, boolean success) { + File successStateFile = getSuccessStateFileFor(packageName); + if (successStateFile == null) { + // The error will have been logged by getSuccessStateFileFor(). + return; + } + + if (successStateFile.exists() != success) { + // If there's been a change of state + if (!success) { + // Clear the status if we're now failing + clearStatus(packageName, successStateFile); + return; + } + + // For succeeding packages we want the file + try { + if (!successStateFile.createNewFile()) { + Log.w(TAG, "Unable to permanently record success for " + packageName); + } + } catch (IOException e) { + Log.w(TAG, "Unable to permanently record success for " + packageName, e); + } + } + } + + /** Clear the status file for a specific package */ + private void clearStatus(String packageName) { + File successStateFile = getSuccessStateFileFor(packageName); + if (successStateFile == null) { + // The error will have been logged by getSuccessStateFileFor(). + return; + } + clearStatus(packageName, successStateFile); + } + + /** Clear the status file for a package once we have the File representation */ + private void clearStatus(String packageName, File successStateFile) { + if (successStateFile.exists()) { + if (!successStateFile.delete()) { + Log.w(TAG, "Unable to remove status file for " + packageName); + } + } + } + + /** Get the backup state file for a package **/ + private File getSuccessStateFileFor(String packageName) { + File stateDirectory = getTopLevelSuccessStateDirectory(/* createIfMissing */ true); + return stateDirectory == null ? null : new File(stateDirectory, packageName); + } + + /** The top level directory for success state files */ + private File getTopLevelSuccessStateDirectory(boolean createIfMissing) { + File directory = new File(mStateDirectory, SUCCESS_STATE_SUBDIR); + if (!directory.exists() && createIfMissing && !directory.mkdirs()) { + Log.e(TAG, "Unable to create backing-up state directory"); + return null; + } + return directory; + } + /** Returns transport status. */ private int sendDataToTransport(@Nullable PackageInfo packageInfo) throws AgentException, TaskException { diff --git a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java index 2510b60955d3..ec56e1ebc8e0 100644 --- a/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java +++ b/services/robotests/backup/src/com/android/server/backup/keyvalue/KeyValueBackupTaskTest.java @@ -131,6 +131,7 @@ import org.junit.runner.RunWith; import org.mockito.ArgumentMatcher; import org.mockito.InOrder; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -2339,6 +2340,85 @@ public class KeyValueBackupTaskTest { assertThat(mBackupManagerService.getCurrentToken()).isEqualTo(0L); } + /** Do not inform transport of an empty backup if the app hasn't backed up before */ + @Test + public void testRunTask_whenNoDataToBackupOnFirstBackup_doesNotTellTransportOfBackup() + throws Exception { + TransportMock transportMock = setUpInitializedTransport(mTransport); + mBackupManagerService.setCurrentToken(0L); + when(transportMock.transport.getCurrentRestoreSet()).thenReturn(1234L); + setUpAgent(PACKAGE_1); + KeyValueBackupTask task = createKeyValueBackupTask(transportMock, true, PACKAGE_1); + + runTask(task); + + verify(transportMock.transport, never()) + .performBackup( + argThat(packageInfo(PACKAGE_1)), any(ParcelFileDescriptor.class), anyInt()); + } + + /** Let the transport know if there are no changes for a KV backed-up package. */ + @Test + public void testRunTask_whenBackupHasCompletedAndThenNoDataChanges_transportGetsNotified() + throws Exception { + TransportMock transportMock = setUpInitializedTransport(mTransport); + when(transportMock.transport.getCurrentRestoreSet()).thenReturn(1234L); + when(transportMock.transport.isAppEligibleForBackup( + argThat(packageInfo(PACKAGE_1)), eq(false))) + .thenReturn(true); + when(transportMock.transport.isAppEligibleForBackup( + argThat(packageInfo(PACKAGE_2)), eq(false))) + .thenReturn(true); + setUpAgentWithData(PACKAGE_1); + setUpAgentWithData(PACKAGE_2); + + PackageInfo endSentinel = new PackageInfo(); + endSentinel.packageName = KeyValueBackupTask.NO_DATA_END_SENTINEL; + + // Perform First Backup run, which should backup both packages + KeyValueBackupTask task = createKeyValueBackupTask(transportMock, PACKAGE_1, PACKAGE_2); + runTask(task); + InOrder order = Mockito.inOrder(transportMock.transport); + order.verify(transportMock.transport) + .performBackup( + argThat(packageInfo(PACKAGE_1)), + any(), + eq(BackupTransport.FLAG_NON_INCREMENTAL)); + order.verify(transportMock.transport).finishBackup(); + order.verify(transportMock.transport) + .performBackup( + argThat(packageInfo(PACKAGE_2)), + any(), + eq(BackupTransport.FLAG_NON_INCREMENTAL)); + order.verify(transportMock.transport).finishBackup(); + + // Run again with new data for package 1, but nothing new for package 2 + task = createKeyValueBackupTask(transportMock, PACKAGE_1); + runTask(task); + + // Now for the second run we performed one incremental backup (package 1) and + // made one "no change" call (package 2) before sending the end sentinel. + order.verify(transportMock.transport) + .performBackup( + argThat(packageInfo(PACKAGE_1)), + any(), + eq(BackupTransport.FLAG_INCREMENTAL)); + order.verify(transportMock.transport).finishBackup(); + order.verify(transportMock.transport) + .performBackup( + argThat(packageInfo(PACKAGE_2)), + any(), + eq(BackupTransport.FLAG_DATA_NOT_CHANGED)); + order.verify(transportMock.transport).finishBackup(); + order.verify(transportMock.transport) + .performBackup( + argThat(packageInfo(endSentinel)), + any(), + eq(BackupTransport.FLAG_DATA_NOT_CHANGED)); + order.verify(transportMock.transport).finishBackup(); + order.verifyNoMoreInteractions(); + } + private void runTask(KeyValueBackupTask task) { // Pretend we are not on the main-thread to prevent RemoteCall from complaining mShadowMainLooper.setCurrentThread(false); @@ -2576,6 +2656,20 @@ public class KeyValueBackupTaskTest { packageInfo != null && packageData.packageName.equals(packageInfo.packageName); } + /** Matches {@link PackageInfo} whose package name is {@code packageData.packageName}. */ + private static ArgumentMatcher packageInfo(PackageInfo packageData) { + // We have to test for packageInfo nulity because of Mockito's own stubbing with argThat(). + // E.g. if you do: + // + // 1. when(object.method(argThat(str -> str.equals("foo")))).thenReturn(0) + // 2. when(object.method(argThat(str -> str.equals("bar")))).thenReturn(2) + // + // The second line will throw NPE because it will call lambda 1 with null, since argThat() + // returns null. So we guard against that by checking for null. + return packageInfo -> + packageInfo != null && packageInfo.packageName.equals(packageInfo.packageName); + } + /** Matches {@link ApplicationInfo} whose package name is {@code packageData.packageName}. */ private static ArgumentMatcher applicationInfo(PackageData packageData) { return applicationInfo -> -- GitLab