From a7de6bf8bf195d300871dcd45d503240de32ba35 Mon Sep 17 00:00:00 2001 From: Sarp Misoglu Date: Mon, 3 Oct 2022 16:10:57 +0100 Subject: [PATCH] Add a class for retrieving server flag values Also adding two flags for previously hardcoded timeout values to test out the new flag system. The reason for adding a util class is to avoid calling DeviceConfig and passing in the same strings every time a flag value is retrieved. Bug: 210083465 Test: atest BackupAndRestoreFeatureFlagsTest.java Manual: adb shell device_config put backup_and_restore backup_transport_callback_timeout_millis 1 Observe that transport status callbacks fail with a timeout error adb shell device_config put backup_and_restore backup_transport_future_timeout_millis 1 Observe that many transport futures timeout Note that the timeouts that turned into flags are not unit-testable since they use the system clock to keep track of time. AFAIU this can't be mocked. Change-Id: I6953fd480ec217a854a57e3c369cadb0947bd55c --- .../backup/BackupAndRestoreFeatureFlags.java | 55 ++++++++++++++ .../transport/BackupTransportClient.java | 4 +- .../transport/TransportStatusCallback.java | 6 +- .../tests/mockingservicestests/Android.bp | 1 + .../BackupAndRestoreFeatureFlagsTest.java | 73 +++++++++++++++++++ 5 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java create mode 100644 services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java diff --git a/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java b/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java new file mode 100644 index 0000000000000..042bcbd0a0bbe --- /dev/null +++ b/services/backup/java/com/android/server/backup/BackupAndRestoreFeatureFlags.java @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed 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 com.android.server.backup; + +import android.Manifest; +import android.annotation.RequiresPermission; +import android.provider.DeviceConfig; + +/** + * Retrieves values of feature flags. + * + *

These flags are intended to be configured server-side and their values should be set in {@link + * DeviceConfig} by a service that periodically syncs with the server. + * + *

This class must ensure that the namespace, flag name, and default value passed into {@link + * DeviceConfig} matches what's declared on the server. The namespace is shared for all backup and + * restore flags. + */ +public class BackupAndRestoreFeatureFlags { + private static final String NAMESPACE = "backup_and_restore"; + + private BackupAndRestoreFeatureFlags() {} + + /** Retrieves the value of the flag "backup_transport_future_timeout_millis". */ + @RequiresPermission(Manifest.permission.READ_DEVICE_CONFIG) + public static long getBackupTransportFutureTimeoutMillis() { + return DeviceConfig.getLong( + NAMESPACE, + /* name= */ "backup_transport_future_timeout_millis", + /* defaultValue= */ 600000); // 10 minutes + } + + /** Retrieves the value of the flag "backup_transport_callback_timeout_millis". */ + @RequiresPermission(Manifest.permission.READ_DEVICE_CONFIG) + public static long getBackupTransportCallbackTimeoutMillis() { + return DeviceConfig.getLong( + NAMESPACE, + /* name= */ "backup_transport_callback_timeout_millis", + /* defaultValue= */ 300000); // 5 minutes + } +} diff --git a/services/backup/java/com/android/server/backup/transport/BackupTransportClient.java b/services/backup/java/com/android/server/backup/transport/BackupTransportClient.java index 40d7cad9405ac..21005bbf8af9e 100644 --- a/services/backup/java/com/android/server/backup/transport/BackupTransportClient.java +++ b/services/backup/java/com/android/server/backup/transport/BackupTransportClient.java @@ -30,6 +30,7 @@ import android.util.Slog; import com.android.internal.backup.IBackupTransport; import com.android.internal.infra.AndroidFuture; +import com.android.server.backup.BackupAndRestoreFeatureFlags; import java.util.ArrayDeque; import java.util.HashSet; @@ -385,7 +386,8 @@ public class BackupTransportClient { private T getFutureResult(AndroidFuture future) { try { - return future.get(600, TimeUnit.SECONDS); + return future.get(BackupAndRestoreFeatureFlags.getBackupTransportFutureTimeoutMillis(), + TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException | TimeoutException | CancellationException e) { Slog.w(TAG, "Failed to get result from transport:", e); diff --git a/services/backup/java/com/android/server/backup/transport/TransportStatusCallback.java b/services/backup/java/com/android/server/backup/transport/TransportStatusCallback.java index fb98825f13438..deaa86c09b36a 100644 --- a/services/backup/java/com/android/server/backup/transport/TransportStatusCallback.java +++ b/services/backup/java/com/android/server/backup/transport/TransportStatusCallback.java @@ -23,13 +23,13 @@ import android.util.Slog; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.backup.ITransportStatusCallback; +import com.android.server.backup.BackupAndRestoreFeatureFlags; public class TransportStatusCallback extends ITransportStatusCallback.Stub { private static final String TAG = "TransportStatusCallback"; - private static final int TIMEOUT_MILLIS = 300 * 1000; // 5 minutes. private static final int OPERATION_STATUS_DEFAULT = 0; - private final int mOperationTimeout; + private final long mOperationTimeout; @GuardedBy("this") private int mOperationStatus = OPERATION_STATUS_DEFAULT; @@ -37,7 +37,7 @@ public class TransportStatusCallback extends ITransportStatusCallback.Stub { private boolean mHasCompletedOperation = false; public TransportStatusCallback() { - mOperationTimeout = TIMEOUT_MILLIS; + mOperationTimeout = BackupAndRestoreFeatureFlags.getBackupTransportCallbackTimeoutMillis(); } @VisibleForTesting diff --git a/services/tests/mockingservicestests/Android.bp b/services/tests/mockingservicestests/Android.bp index 73b1907c9f429..681bfcf68cc38 100644 --- a/services/tests/mockingservicestests/Android.bp +++ b/services/tests/mockingservicestests/Android.bp @@ -56,6 +56,7 @@ android_test { "service-jobscheduler", "service-permission.impl", "service-sdksandbox.impl", + "services.backup", "services.companion", "services.core", "services.devicepolicy", diff --git a/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java b/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java new file mode 100644 index 0000000000000..f53599731e10a --- /dev/null +++ b/services/tests/mockingservicestests/src/com/android/server/backup/BackupAndRestoreFeatureFlagsTest.java @@ -0,0 +1,73 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed 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 com.android.server.backup; + +import android.platform.test.annotations.Presubmit; +import android.provider.DeviceConfig; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import com.android.modules.utils.testing.TestableDeviceConfig; + +import static com.google.common.truth.Truth.assertThat; + + +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +@Presubmit +@RunWith(AndroidJUnit4.class) +public class BackupAndRestoreFeatureFlagsTest { + @Rule + public TestableDeviceConfig.TestableDeviceConfigRule + mDeviceConfigRule = new TestableDeviceConfig.TestableDeviceConfigRule(); + + @Test + public void getBackupTransportFutureTimeoutMillis_notSet_returnsDefault() { + assertThat( + BackupAndRestoreFeatureFlags.getBackupTransportFutureTimeoutMillis()).isEqualTo( + 600000); + } + + @Test + public void getBackupTransportFutureTimeoutMillis_set_returnsSetValue() { + DeviceConfig.setProperty("backup_and_restore", "backup_transport_future_timeout_millis", + "1234", false); + + assertThat( + BackupAndRestoreFeatureFlags.getBackupTransportFutureTimeoutMillis()).isEqualTo( + 1234); + } + + @Test + public void getBackupTransportCallbackTimeoutMillis_notSet_returnsDefault() { + assertThat( + BackupAndRestoreFeatureFlags.getBackupTransportCallbackTimeoutMillis()).isEqualTo( + 300000); + } + + @Test + public void getBackupTransportCallbackTimeoutMillis_set_returnsSetValue() { + DeviceConfig.setProperty("backup_and_restore", "backup_transport_callback_timeout_millis", + "5678", false); + + assertThat( + BackupAndRestoreFeatureFlags.getBackupTransportCallbackTimeoutMillis()).isEqualTo( + 5678); + } +}