From cedd0bec7dceaf038d26fff91065f6b827dff11a Mon Sep 17 00:00:00 2001 From: Piyush Mehrotra Date: Thu, 23 Mar 2023 17:05:48 +0000 Subject: [PATCH] Control behavior of unified restore pipeline after transport failure during K/V restore with flag Currently unified restore pipeline finishes immediately if restore of a package fails due to transport error. This is mostly fine as transport level errors usually happen when transport is broken. But there are some situations where such an error is package specific - like when system app goes from KV backups to full backups, and the bundled version of the app is unable to handle Full data restore (as bundled version could be older version). To take this into account, we allow the restore pipeline to continue, when a transport level error happens in K/V restore, based on the value of flag - `unified_restore_should_continue_after_transport_level_failure_in_keyvalue_restore` Bug: 128499560 Test: atest BackupAndRestoreFeatureFlagsTest atest PerformUnifiedRestoreTaskTest Change-Id: I799d259387bd914877066920b8168fd11b5bfd45 --- .../backup/BackupAndRestoreFeatureFlags.java | 15 +++ .../restore/PerformUnifiedRestoreTask.java | 39 ++++++- .../BackupAndRestoreFeatureFlagsTest.java | 20 ++++ .../PerformUnifiedRestoreTaskTest.java | 102 ++++++++++++++++-- 4 files changed, 163 insertions(+), 13 deletions(-) diff --git a/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java b/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java index 1990fe277af9e..98aebddddac90 100644 --- a/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java +++ b/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java @@ -77,4 +77,19 @@ public class BackupAndRestoreFeatureFlags { /* name= */ "full_backup_utils_route_buffer_size_bytes", /* defaultValue= */ 32 * 1024); // 32 KB } + + /** + * Retrieves the value of the flag + * "unified_restore_continue_after_transport_failure_in_kv_restore". + * If true, Unified restore task will continue to next package if key-value restore of a + * package fails due to Transport-level failure. See b/128499560 for more context. + */ + @RequiresPermission(Manifest.permission.READ_DEVICE_CONFIG) + public static boolean getUnifiedRestoreContinueAfterTransportFailureInKvRestore() { + return DeviceConfig.getBoolean( + NAMESPACE, + /* name= */ + "unified_restore_continue_after_transport_failure_in_kv_restore", + /* defaultValue= */ true); + } } diff --git a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java index 18e28de757828..1656b6f0ab9b1 100644 --- a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java +++ b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java @@ -57,6 +57,7 @@ import com.android.server.AppWidgetBackupBridge; import com.android.server.EventLogTags; import com.android.server.LocalServices; import com.android.server.backup.BackupAgentTimeoutParameters; +import com.android.server.backup.BackupAndRestoreFeatureFlags; import com.android.server.backup.BackupRestoreTask; import com.android.server.backup.BackupUtils; import com.android.server.backup.OperationStorage; @@ -168,11 +169,13 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { private final BackupEligibilityRules mBackupEligibilityRules; @VisibleForTesting - PerformUnifiedRestoreTask(UserBackupManagerService backupManagerService) { + PerformUnifiedRestoreTask( + UserBackupManagerService backupManagerService, + TransportConnection transportConnection) { mListener = null; mAgentTimeoutParameters = null; mOperationStorage = null; - mTransportConnection = null; + mTransportConnection = transportConnection; mTransportManager = null; mEphemeralOpToken = 0; mUserId = 0; @@ -731,13 +734,18 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { ParcelFileDescriptor.MODE_TRUNCATE); if (transport.getRestoreData(stage) != BackupTransport.TRANSPORT_OK) { - // Transport-level failure, so we wind everything up and - // terminate the restore operation. + // Transport-level failure. This failure could be specific to package currently in + // restore. Slog.e(TAG, "Error getting restore data for " + packageName); EventLog.writeEvent(EventLogTags.RESTORE_TRANSPORT_FAILURE); stage.close(); downloadFile.delete(); - executeNextState(UnifiedRestoreState.FINAL); + UnifiedRestoreState nextState = + BackupAndRestoreFeatureFlags + .getUnifiedRestoreContinueAfterTransportFailureInKvRestore() + ? UnifiedRestoreState.RUNNING_QUEUE + : UnifiedRestoreState.FINAL; + executeNextState(nextState); return; } @@ -1358,6 +1366,7 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { executeNextState(UnifiedRestoreState.RUNNING_QUEUE); } + @VisibleForTesting void executeNextState(UnifiedRestoreState nextState) { if (MORE_DEBUG) { Slog.i(TAG, " => executing next step on " @@ -1369,6 +1378,26 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { backupManagerService.getBackupHandler().sendMessage(msg); } + @VisibleForTesting + UnifiedRestoreState getCurrentUnifiedRestoreStateForTesting() { + return mState; + } + + @VisibleForTesting + void setCurrentUnifiedRestoreStateForTesting(UnifiedRestoreState state) { + mState = state; + } + + @VisibleForTesting + void setStateDirForTesting(File stateDir) { + mStateDir = stateDir; + } + + @VisibleForTesting + void initiateOneRestoreForTesting(PackageInfo app, long appVersionCode) { + initiateOneRestore(app, appVersionCode); + } + // restore observer support void sendStartRestore(int numPackages) { if (mObserver != null) { diff --git a/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java b/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java index 201da359aeb62..9f685b479d474 100644 --- a/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java @@ -104,4 +104,24 @@ public class BackupAndRestoreFeatureFlagsTest { assertThat(BackupAndRestoreFeatureFlags.getFullBackupUtilsRouteBufferSizeBytes()) .isEqualTo(5678); } + + @Test + public void getUnifiedRestoreContinueAfterTransportFailureInKvRestore_notSet_returnsDefault() { + assertThat( + BackupAndRestoreFeatureFlags + .getUnifiedRestoreContinueAfterTransportFailureInKvRestore()) + .isEqualTo(true); + } + + @Test + public void getUnifiedRestoreContinueAfterTransportFailureInKvRestore_set_returnsSetValue() { + DeviceConfig.setProperty(/*namespace=*/ "backup_and_restore", + /*name=*/ "unified_restore_continue_after_transport_failure_in_kv_restore", + /*value=*/ "false", /*makeDefault=*/ false); + + assertThat( + BackupAndRestoreFeatureFlags + .getUnifiedRestoreContinueAfterTransportFailureInKvRestore()) + .isEqualTo(false); + } } diff --git a/services/tests/mockingservicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java b/services/tests/mockingservicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java index 017c93975286c..c84797febcfd6 100644 --- a/services/tests/mockingservicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java @@ -25,20 +25,33 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; +import android.app.backup.BackupDataInput; +import android.app.backup.BackupDataOutput; +import android.app.backup.BackupTransport; +import android.content.Context; +import android.content.pm.PackageInfo; +import android.os.Message; +import android.os.RemoteException; +import android.platform.test.annotations.Presubmit; +import android.provider.DeviceConfig; + import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; -import android.app.backup.BackupDataInput; -import android.app.backup.BackupDataOutput; -import android.platform.test.annotations.Presubmit; - +import com.android.modules.utils.testing.TestableDeviceConfig; import com.android.server.backup.UserBackupManagerService; +import com.android.server.backup.internal.BackupHandler; +import com.android.server.backup.transport.BackupTransportClient; +import com.android.server.backup.transport.TransportConnection; +import com.android.server.backup.transport.TransportNotAvailableException; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; @@ -62,9 +75,14 @@ public class PerformUnifiedRestoreTaskTest { private static final String SYSTEM_PACKAGE_NAME = "android"; private static final String NON_SYSTEM_PACKAGE_NAME = "package"; - @Mock private BackupDataInput mBackupDataInput; - @Mock private BackupDataOutput mBackupDataOutput; - @Mock private UserBackupManagerService mBackupManagerService; + @Mock + private BackupDataInput mBackupDataInput; + @Mock + private BackupDataOutput mBackupDataOutput; + @Mock + private UserBackupManagerService mBackupManagerService; + @Mock + private TransportConnection mTransportConnection; private Set mExcludedkeys = new HashSet<>(); private Map mBackupData = new HashMap<>(); @@ -74,12 +92,20 @@ public class PerformUnifiedRestoreTaskTest { private Set mBackupDataDump; private PerformUnifiedRestoreTask mRestoreTask; + @Rule + public TestableDeviceConfig.TestableDeviceConfigRule + mDeviceConfigRule = new TestableDeviceConfig.TestableDeviceConfigRule(); + + private Context mContext; + @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); populateTestData(); + mContext = InstrumentationRegistry.getInstrumentation().getTargetContext(); + mBackupDataSource = new ArrayDeque<>(mBackupData.keySet()); when(mBackupDataInput.readNextHeader()).then(new Answer() { @Override @@ -106,7 +132,7 @@ public class PerformUnifiedRestoreTaskTest { } }); - mRestoreTask = new PerformUnifiedRestoreTask(mBackupManagerService); + mRestoreTask = new PerformUnifiedRestoreTask(mBackupManagerService, mTransportConnection); } private void populateTestData() { @@ -179,4 +205,64 @@ public class PerformUnifiedRestoreTaskTest { assertTrue(mRestoreTask.shouldStageBackupData(SYSTEM_PACKAGE_NAME)); } + + @Test + public void testFailedKeyValueRestore_continueAfterFeatureEnabled_nextStateIsRunningQueue() + throws TransportNotAvailableException, RemoteException { + DeviceConfig.setProperty( + "backup_and_restore", + "unified_restore_continue_after_transport_failure_in_kv_restore", + "true", + false); + + setupForRestoreKeyValueState(BackupTransport.TRANSPORT_ERROR); + + mRestoreTask.setCurrentUnifiedRestoreStateForTesting(UnifiedRestoreState.RESTORE_KEYVALUE); + mRestoreTask.setStateDirForTesting(mContext.getCacheDir()); + + PackageInfo testPackageInfo = new PackageInfo(); + testPackageInfo.packageName = "test.package.name"; + mRestoreTask.initiateOneRestoreForTesting(testPackageInfo, 0L); + assertTrue( + mRestoreTask.getCurrentUnifiedRestoreStateForTesting() + == UnifiedRestoreState.RUNNING_QUEUE); + } + + @Test + public void testFailedKeyValueRestore_continueAfterFeatureDisabled_nextStateIsFinal() + throws RemoteException, TransportNotAvailableException { + DeviceConfig.setProperty( + "backup_and_restore", + "unified_restore_continue_after_transport_failure_in_kv_restore", + "false", + false); + + setupForRestoreKeyValueState(BackupTransport.TRANSPORT_ERROR); + + mRestoreTask.setCurrentUnifiedRestoreStateForTesting(UnifiedRestoreState.RESTORE_KEYVALUE); + mRestoreTask.setStateDirForTesting(mContext.getCacheDir()); + + PackageInfo testPackageInfo = new PackageInfo(); + testPackageInfo.packageName = "test.package.name"; + mRestoreTask.initiateOneRestoreForTesting(testPackageInfo, 0L); + assertTrue( + mRestoreTask.getCurrentUnifiedRestoreStateForTesting() + == UnifiedRestoreState.FINAL); + } + + private void setupForRestoreKeyValueState(int transportStatus) + throws RemoteException, TransportNotAvailableException { + // Mock BackupHandler to do nothing when executeNextState() is called + BackupHandler backupHandler = Mockito.mock(BackupHandler.class); + when(backupHandler.obtainMessage(anyInt(), any())).thenReturn(new Message()); + when(backupHandler.sendMessage(any())).thenReturn(true); + + // Return cache directory for any bookkeeping or maintaining persistent state. + when(mBackupManagerService.getDataDir()).thenReturn(mContext.getCacheDir()); + when(mBackupManagerService.getBackupHandler()).thenReturn(backupHandler); + + BackupTransportClient transport = Mockito.mock(BackupTransportClient.class); + when(transport.getRestoreData(any())).thenReturn(transportStatus); + when(mTransportConnection.connectOrThrow(any())).thenReturn(transport); + } }