From ba036ade2a2475a5e0da2c96d6540fc393a98a99 Mon Sep 17 00:00:00 2001 From: Zimuzo Date: Fri, 5 Apr 2019 17:24:29 +0100 Subject: [PATCH 1/2] Make PackageWatchdog variables configurable We leverage DeviceConfig to configure the following variables: watchdog_trigger_failure_duration_millis watchdog_trigger_failure_count watchdog_explicit_health_check_enabled watchdog_explicit_health_check_timeout_millis Fix PackageWatchdogTest accordingly. Bug: 129335707 Test: added Log to PackageWatchdog that prints the received value from DeviceConfig and made sure it matches the expected Test: atest com.android.server.PackageWatchdogTest Change-Id: I8a57b87682a4c636a61afdd7f93700459ccfddeb --- .../watchdog/ExplicitHealthCheckService.java | 1 - .../com/android/server/PackageWatchdog.java | 98 ++++++++++++++++--- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/core/java/android/service/watchdog/ExplicitHealthCheckService.java b/core/java/android/service/watchdog/ExplicitHealthCheckService.java index eeefb4ae10fa8..dc9c858082405 100644 --- a/core/java/android/service/watchdog/ExplicitHealthCheckService.java +++ b/core/java/android/service/watchdog/ExplicitHealthCheckService.java @@ -183,7 +183,6 @@ public abstract class ExplicitHealthCheckService extends Service { */ @SystemApi public static final class PackageConfig implements Parcelable { - // TODO: Receive from DeviceConfig flag private static final long DEFAULT_HEALTH_CHECK_TIMEOUT_MILLIS = TimeUnit.HOURS.toMillis(1); private final String mPackageName; diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index b44009fd68806..e383c522f474e 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -29,6 +29,7 @@ import android.os.Environment; import android.os.Handler; import android.os.Looper; import android.os.SystemClock; +import android.provider.DeviceConfig; import android.text.TextUtils; import android.util.ArrayMap; import android.util.ArraySet; @@ -61,6 +62,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.TimeUnit; /** * Monitors the health of packages on the system and notifies interested observers when packages @@ -69,10 +71,22 @@ import java.util.Set; */ public class PackageWatchdog { private static final String TAG = "PackageWatchdog"; + + static final String PROPERTY_WATCHDOG_TRIGGER_DURATION_MILLIS = + "watchdog_trigger_failure_duration_millis"; + static final String PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT = + "watchdog_trigger_failure_count"; + static final String PROPERTY_WATCHDOG_EXPLICIT_HEALTH_CHECK_ENABLED = + "watchdog_explicit_health_check_enabled"; + // Duration to count package failures before it resets to 0 - private static final int TRIGGER_DURATION_MS = 60000; + private static final int DEFAULT_TRIGGER_FAILURE_DURATION_MS = + (int) TimeUnit.MINUTES.toMillis(1); // Number of package failures within the duration above before we notify observers - static final int TRIGGER_FAILURE_COUNT = 5; + private static final int DEFAULT_TRIGGER_FAILURE_COUNT = 5; + // Whether explicit health checks are enabled or not + private static final boolean DEFAULT_EXPLICIT_HEALTH_CHECK_ENABLED = true; + private static final int DB_VERSION = 1; private static final String TAG_PACKAGE_WATCHDOG = "package-watchdog"; private static final String TAG_PACKAGE = "package"; @@ -83,6 +97,7 @@ public class PackageWatchdog { private static final String ATTR_EXPLICIT_HEALTH_CHECK_DURATION = "health-check-duration"; private static final String ATTR_PASSED_HEALTH_CHECK = "passed-health-check"; + @GuardedBy("PackageWatchdog.class") private static PackageWatchdog sPackageWatchdog; private final Object mLock = new Object(); @@ -100,11 +115,15 @@ public class PackageWatchdog { // File containing the XML data of monitored packages /data/system/package-watchdog.xml private final AtomicFile mPolicyFile; private final ExplicitHealthCheckController mHealthCheckController; - // Flag to control whether explicit health checks are supported or not - @GuardedBy("mLock") - private boolean mIsHealthCheckEnabled = true; @GuardedBy("mLock") private boolean mIsPackagesReady; + // Flag to control whether explicit health checks are supported or not + @GuardedBy("mLock") + private boolean mIsHealthCheckEnabled = DEFAULT_EXPLICIT_HEALTH_CHECK_ENABLED; + @GuardedBy("mLock") + private int mTriggerFailureDurationMs = DEFAULT_TRIGGER_FAILURE_DURATION_MS; + @GuardedBy("mLock") + private int mTriggerFailureCount = DEFAULT_TRIGGER_FAILURE_COUNT; // SystemClock#uptimeMillis when we last executed #syncState // 0 if no prune is scheduled. @GuardedBy("mLock") @@ -153,8 +172,8 @@ public class PackageWatchdog { mHealthCheckController.setCallbacks(packageName -> onHealthCheckPassed(packageName), packages -> onSupportedPackages(packages), () -> syncRequestsAsync()); - // Controller is initially disabled until here where we may enable it and sync our state - setExplicitHealthCheckEnabled(mIsHealthCheckEnabled); + setPropertyChangedListenerLocked(); + updateConfigs(); } } @@ -332,7 +351,6 @@ public class PackageWatchdog { } } - // TODO(b/120598832): Set depending on DeviceConfig flag /** * Enables or disables explicit health checks. *

If explicit health checks are enabled, the health check service is started. @@ -390,6 +408,12 @@ public class PackageWatchdog { String getName(); } + long getTriggerFailureCount() { + synchronized (mLock) { + return mTriggerFailureCount; + } + } + /** * Serializes and syncs health check requests with the {@link ExplicitHealthCheckController}. */ @@ -646,7 +670,7 @@ public class PackageWatchdog { XmlUtils.beginDocument(parser, TAG_PACKAGE_WATCHDOG); int outerDepth = parser.getDepth(); while (XmlUtils.nextElementWithin(parser, outerDepth)) { - ObserverInternal observer = ObserverInternal.read(parser); + ObserverInternal observer = ObserverInternal.read(parser, this); if (observer != null) { mAllObservers.put(observer.mName, observer); } @@ -661,6 +685,48 @@ public class PackageWatchdog { } } + /** Adds a {@link DeviceConfig#OnPropertyChangedListener}. */ + private void setPropertyChangedListenerLocked() { + DeviceConfig.addOnPropertyChangedListener( + DeviceConfig.NAMESPACE_ROLLBACK, + mContext.getMainExecutor(), + (namespace, name, value) -> { + if (!DeviceConfig.NAMESPACE_ROLLBACK.equals(namespace)) { + return; + } + updateConfigs(); + }); + } + + /** + * Health check is enabled or disabled after reading the flags + * from DeviceConfig. + */ + private void updateConfigs() { + synchronized (mLock) { + mTriggerFailureCount = DeviceConfig.getInt( + DeviceConfig.NAMESPACE_ROLLBACK, + PROPERTY_WATCHDOG_TRIGGER_FAILURE_COUNT, + DEFAULT_TRIGGER_FAILURE_COUNT); + if (mTriggerFailureCount <= 0) { + mTriggerFailureCount = DEFAULT_TRIGGER_FAILURE_COUNT; + } + + mTriggerFailureDurationMs = DeviceConfig.getInt( + DeviceConfig.NAMESPACE_ROLLBACK, + PROPERTY_WATCHDOG_TRIGGER_DURATION_MILLIS, + DEFAULT_TRIGGER_FAILURE_DURATION_MS); + if (mTriggerFailureDurationMs <= 0) { + mTriggerFailureDurationMs = DEFAULT_TRIGGER_FAILURE_COUNT; + } + + setExplicitHealthCheckEnabled(DeviceConfig.getBoolean( + DeviceConfig.NAMESPACE_ROLLBACK, + PROPERTY_WATCHDOG_EXPLICIT_HEALTH_CHECK_ENABLED, + DEFAULT_EXPLICIT_HEALTH_CHECK_ENABLED)); + } + } + /** * Persists mAllObservers to file. Threshold information is ignored. */ @@ -805,7 +871,7 @@ public class PackageWatchdog { * #loadFromFile which in turn is only called on construction of the * singleton PackageWatchdog. **/ - public static ObserverInternal read(XmlPullParser parser) { + public static ObserverInternal read(XmlPullParser parser, PackageWatchdog watchdog) { String observerName = null; if (TAG_OBSERVER.equals(parser.getName())) { observerName = parser.getAttributeValue(null, ATTR_NAME); @@ -829,7 +895,7 @@ public class PackageWatchdog { boolean hasPassedHealthCheck = Boolean.parseBoolean( parser.getAttributeValue(null, ATTR_PASSED_HEALTH_CHECK)); if (!TextUtils.isEmpty(packageName)) { - packages.add(new MonitoredPackage(packageName, duration, + packages.add(watchdog.new MonitoredPackage(packageName, duration, healthCheckDuration, hasPassedHealthCheck)); } } catch (NumberFormatException e) { @@ -856,7 +922,7 @@ public class PackageWatchdog { *

Note, the PackageWatchdog#mLock must always be held when reading or writing * instances of this class. */ - static class MonitoredPackage { + class MonitoredPackage { // Health check states // TODO(b/120598832): Prefix with HEALTH_CHECK // mName has not passed health check but has requested a health check @@ -931,7 +997,7 @@ public class PackageWatchdog { public boolean onFailureLocked() { final long now = SystemClock.uptimeMillis(); final long duration = now - mUptimeStartMs; - if (duration > TRIGGER_DURATION_MS) { + if (duration > mTriggerFailureDurationMs) { // TODO(b/120598832): Reseting to 1 is not correct // because there may be more than 1 failure in the last trigger window from now // This is the RescueParty impl, will leave for now @@ -940,7 +1006,7 @@ public class PackageWatchdog { } else { mFailures++; } - boolean failed = mFailures >= TRIGGER_FAILURE_COUNT; + boolean failed = mFailures >= mTriggerFailureCount; if (failed) { mFailures = 0; } @@ -1065,7 +1131,7 @@ public class PackageWatchdog { } /** Returns a {@link String} representation of the current health check state. */ - private static String toString(int state) { + private String toString(int state) { switch (state) { case STATE_ACTIVE: return "ACTIVE"; @@ -1081,7 +1147,7 @@ public class PackageWatchdog { } /** Returns {@code value} if it is greater than 0 or {@link Long#MAX_VALUE} otherwise. */ - private static long toPositive(long value) { + private long toPositive(long value) { return value > 0 ? value : Long.MAX_VALUE; } } From 97d96c6149b6e7be4ec1fe53bccf027dbf801001 Mon Sep 17 00:00:00 2001 From: shafik Date: Wed, 24 Apr 2019 10:31:54 +0100 Subject: [PATCH 2/2] Update PackageWatchdogTest Make PackageWatchdogTest compatible to the changes that added DeviceConfig flags to PackageWatchdog. This includes: * Make PackageWatchdog#setExplicitHealthCheckEnabled private and use DeviceConfig mechanism for changing that value instead * Disable TestLooper#startAutoDispatch * Other minor refinements that solve compatibility issues Bug: 129335707 Test: atest com.android.server.PackageWatchdogTest Change-Id: I7323dc65ec2957aeab128224864441bdf63c6f81 --- .../com/android/server/PackageWatchdog.java | 2 +- .../android/server/PackageWatchdogTest.java | 79 ++++++++++++++----- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java index e383c522f474e..7b5b419a7aa01 100644 --- a/services/core/java/com/android/server/PackageWatchdog.java +++ b/services/core/java/com/android/server/PackageWatchdog.java @@ -357,7 +357,7 @@ public class PackageWatchdog { *

If explicit health checks are disabled, pending explicit health check requests are * passed and the health check service is stopped. */ - public void setExplicitHealthCheckEnabled(boolean enabled) { + private void setExplicitHealthCheckEnabled(boolean enabled) { synchronized (mLock) { mIsHealthCheckEnabled = enabled; mHealthCheckController.setEnabled(enabled); diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java index 13e737e9fed35..eb19361d86a3c 100644 --- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java +++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java @@ -18,25 +18,27 @@ package com.android.server; import static android.service.watchdog.ExplicitHealthCheckService.PackageConfig; -import static com.android.server.PackageWatchdog.MonitoredPackage; -import static com.android.server.PackageWatchdog.TRIGGER_FAILURE_COUNT; - import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import android.Manifest; import android.content.Context; import android.content.pm.VersionedPackage; import android.os.Handler; import android.os.test.TestLooper; +import android.provider.DeviceConfig; import android.util.AtomicFile; import androidx.test.InstrumentationRegistry; +import com.android.server.PackageWatchdog.MonitoredPackage; import com.android.server.PackageWatchdog.PackageHealthObserver; import com.android.server.PackageWatchdog.PackageHealthObserverImpact; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -73,8 +75,13 @@ public class PackageWatchdogTest { public void setUp() throws Exception { new File(InstrumentationRegistry.getContext().getFilesDir(), "package-watchdog.xml").delete(); + adoptShellPermissions(Manifest.permission.READ_DEVICE_CONFIG); mTestLooper = new TestLooper(); - mTestLooper.startAutoDispatch(); + } + + @After + public void tearDown() throws Exception { + dropShellPermissions(); } /** @@ -204,7 +211,7 @@ public class PackageWatchdogTest { // Verify random observer not saved returns null assertNull(watchdog2.getPackages(new TestObserver(OBSERVER_NAME_3))); - // Then regiser observer1 + // Then register observer1 watchdog2.registerHealthObserver(observer1); watchdog2.registerHealthObserver(observer2); @@ -231,7 +238,7 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A below the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT - 1; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount() - 1; i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } @@ -258,7 +265,7 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observer1, Arrays.asList(APP_B), SHORT_DURATION); // Then fail APP_C (not observed) above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_C, VERSION_CODE))); } @@ -292,7 +299,7 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observer, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A (different version) above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList( new VersionedPackage(APP_A, differentVersionCode))); } @@ -331,7 +338,7 @@ public class PackageWatchdogTest { SHORT_DURATION); // Then fail all apps above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE), new VersionedPackage(APP_B, VERSION_CODE), new VersionedPackage(APP_C, VERSION_CODE), @@ -384,7 +391,7 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observerSecond, Arrays.asList(APP_A), LONG_DURATION); // Then fail APP_A above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } // Run handler so package failures are dispatched to observers @@ -401,7 +408,7 @@ public class PackageWatchdogTest { observerSecond.mFailedPackages.clear(); // Then fail APP_A again above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } // Run handler so package failures are dispatched to observers @@ -418,7 +425,7 @@ public class PackageWatchdogTest { observerSecond.mFailedPackages.clear(); // Then fail APP_A again above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } // Run handler so package failures are dispatched to observers @@ -435,7 +442,7 @@ public class PackageWatchdogTest { observerSecond.mFailedPackages.clear(); // Then fail APP_A again above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } // Run handler so package failures are dispatched to observers @@ -462,7 +469,7 @@ public class PackageWatchdogTest { watchdog.startObservingHealth(observer1, Arrays.asList(APP_A), SHORT_DURATION); // Then fail APP_A above the threshold - for (int i = 0; i < TRIGGER_FAILURE_COUNT; i++) { + for (int i = 0; i < watchdog.getTriggerFailureCount(); i++) { watchdog.onPackageFailure(Arrays.asList(new VersionedPackage(APP_A, VERSION_CODE))); } @@ -539,6 +546,10 @@ public class PackageWatchdogTest { */ @Test public void testExplicitHealthCheckStateChanges() throws Exception { + adoptShellPermissions( + Manifest.permission.WRITE_DEVICE_CONFIG, + Manifest.permission.READ_DEVICE_CONFIG); + TestController controller = new TestController(); PackageWatchdog watchdog = createWatchdog(controller, true /* withPackagesReady */); TestObserver observer = new TestObserver(OBSERVER_NAME_1, @@ -559,7 +570,7 @@ public class PackageWatchdogTest { assertEquals(APP_B, requestedPackages.get(1)); // Disable explicit health checks (marks APP_A and APP_B as passed) - watchdog.setExplicitHealthCheckEnabled(false); + setExplicitHealthCheckEnabled(false); // Run handler so requests/cancellations are dispatched to the controller mTestLooper.dispatchAll(); @@ -575,7 +586,7 @@ public class PackageWatchdogTest { assertEquals(0, observer.mFailedPackages.size()); // Re-enable explicit health checks - watchdog.setExplicitHealthCheckEnabled(true); + setExplicitHealthCheckEnabled(true); // Run handler so requests/cancellations are dispatched to the controller mTestLooper.dispatchAll(); @@ -643,11 +654,13 @@ public class PackageWatchdogTest { /** Tests {@link MonitoredPackage} health check state transitions. */ @Test public void testPackageHealthCheckStateTransitions() { - MonitoredPackage m1 = new MonitoredPackage(APP_A, LONG_DURATION, + TestController controller = new TestController(); + PackageWatchdog wd = createWatchdog(controller, true /* withPackagesReady */); + MonitoredPackage m1 = wd.new MonitoredPackage(APP_A, LONG_DURATION, false /* hasPassedHealthCheck */); - MonitoredPackage m2 = new MonitoredPackage(APP_B, LONG_DURATION, false); - MonitoredPackage m3 = new MonitoredPackage(APP_C, LONG_DURATION, false); - MonitoredPackage m4 = new MonitoredPackage(APP_D, LONG_DURATION, SHORT_DURATION, true); + MonitoredPackage m2 = wd.new MonitoredPackage(APP_B, LONG_DURATION, false); + MonitoredPackage m3 = wd.new MonitoredPackage(APP_C, LONG_DURATION, false); + MonitoredPackage m4 = wd.new MonitoredPackage(APP_D, LONG_DURATION, SHORT_DURATION, true); // Verify transition: inactive -> active -> passed // Verify initially inactive @@ -683,6 +696,32 @@ public class PackageWatchdogTest { assertEquals(MonitoredPackage.STATE_PASSED, m4.handleElapsedTimeLocked(LONG_DURATION)); } + private void adoptShellPermissions(String... permissions) { + InstrumentationRegistry + .getInstrumentation() + .getUiAutomation() + .adoptShellPermissionIdentity(permissions); + } + + private void dropShellPermissions() { + InstrumentationRegistry + .getInstrumentation() + .getUiAutomation() + .dropShellPermissionIdentity(); + } + + private void setExplicitHealthCheckEnabled(boolean enabled) { + DeviceConfig.setProperty(DeviceConfig.NAMESPACE_ROLLBACK, + PackageWatchdog.PROPERTY_WATCHDOG_EXPLICIT_HEALTH_CHECK_ENABLED, + Boolean.toString(enabled), /*makeDefault*/false); + //give time for DeviceConfig to broadcast the property value change + try { + Thread.sleep(SHORT_DURATION); + } catch (InterruptedException e) { + fail("Thread.sleep unexpectedly failed!"); + } + } + private PackageWatchdog createWatchdog() { return createWatchdog(new TestController(), true /* withPackagesReady */); }