From ea0cf5084c1fa918c9a73e08d7eeb3ba26ba54f1 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 17 Feb 2020 16:23:20 +0900 Subject: [PATCH 1/3] [NS D05] Rework how to tear down networks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Networks are torn down if they don't satisfy requests but only if they are found unable to ever do so. This is important because general-purpose networks typically turn up unvalidated, which means they would get torn down immediately in the presence of some worse network that is already validated. Note that functionally it's the same as before for the existing scores, except that • An exiting network that doesn't satisfy any request is always torn down immediately – this is WAI • An unmetered network is never torn down when compared to a metered network – this is a bugfix (previously the platform would tear down an unmetered cellular on grounds that it could not beat the performance of a metered Ethernet, but that's wrong ; the bug was never realized because Android always thinks Ethernet is unmetered) • An unvalidated network will not be torn down if the current satisfier is also unvalidated, even if the satisfier is much faster. This is the reason for the change in the test. It's wrong to tear down in this case because even if much slower the slower network should win if it validates and the other doesn't. Test: ConnectivityServiceTest Change-Id: Ic9a3d336306a25d1272976ce467aa7c908af7bef --- .../android/server/ConnectivityService.java | 20 +++--- .../server/connectivity/NetworkAgentInfo.java | 67 ++++++++++++------- .../server/ConnectivityServiceTest.java | 9 +-- 3 files changed, 57 insertions(+), 39 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 7083281eaa7e9..f7eabac3b21f6 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3430,16 +3430,16 @@ public class ConnectivityService extends IConnectivityManager.Stub // there is hope for it to become one if it validated, then it is needed. ensureRunningOnConnectivityServiceThread(); if (nri.request.isRequest() && nai.satisfies(nri.request) && - (nai.isSatisfyingRequest(nri.request.requestId) || - // Note that this catches two important cases: - // 1. Unvalidated cellular will not be reaped when unvalidated WiFi - // is currently satisfying the request. This is desirable when - // cellular ends up validating but WiFi does not. - // 2. Unvalidated WiFi will not be reaped when validated cellular - // is currently satisfying the request. This is desirable when - // WiFi ends up validating and out scoring cellular. - nri.mSatisfier.getCurrentScore() - < nai.getCurrentScoreAsValidated())) { + (nai.isSatisfyingRequest(nri.request.requestId) + // Note that canPossiblyBeat catches two important cases: + // 1. Unvalidated slow networks will not be reaped when an unvalidated fast + // network is currently satisfying the request. This is desirable for example + // when cellular ends up validating but WiFi/Ethernet does not. + // 2. Fast networks will not be reaped when a validated slow network is + // currently satisfying the request. This is desirable for example when + // Ethernet ends up validating and out scoring WiFi, or WiFi/Ethernet ends + // up validating and out scoring cellular. + || nai.canPossiblyBeat(nri.mSatisfier))) { return false; } } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index 4612cfd0f7cbb..3860904a3fed9 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -16,6 +16,8 @@ package com.android.server.connectivity; +import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED; +import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; import static android.net.NetworkCapabilities.transportNamesOf; import android.annotation.NonNull; @@ -475,24 +477,16 @@ public class NetworkAgentInfo implements Comparable { return networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN); } - private int getCurrentScore(boolean pretendValidated) { - // TODO: We may want to refactor this into a NetworkScore class that takes a base score from - // the NetworkAgent and signals from the NetworkAgent and uses those signals to modify the - // score. The NetworkScore class would provide a nice place to centralize score constants - // so they are not scattered about the transports. - + /** Gets the current score */ + public int getCurrentScore() { // If this network is explicitly selected and the user has decided to use it even if it's - // unvalidated, give it the maximum score. Also give it the maximum score if it's explicitly - // selected and we're trying to see what its score could be. This ensures that we don't tear - // down an explicitly selected network before the user gets a chance to prefer it when - // a higher-scoring network (e.g., Ethernet) is available. - if (networkAgentConfig.explicitlySelected - && (networkAgentConfig.acceptUnvalidated || pretendValidated)) { + // unvalidated, give it the maximum score. + if (networkAgentConfig.explicitlySelected && networkAgentConfig.acceptUnvalidated) { return ConnectivityConstants.EXPLICITLY_SELECTED_NETWORK_SCORE; } int score = mNetworkScore.getLegacyScore(); - if (!lastValidated && !pretendValidated && !ignoreWifiUnvalidationPenalty() && !isVPN()) { + if (!lastValidated && !ignoreWifiUnvalidationPenalty() && !isVPN()) { score -= ConnectivityConstants.UNVALIDATED_SCORE_PENALTY; } if (score < 0) score = 0; @@ -508,18 +502,6 @@ public class NetworkAgentInfo implements Comparable { return isWifi && !avoidBadWifi && everValidated; } - // Get the current score for this Network. This may be modified from what the - // NetworkAgent sent, as it has modifiers applied to it. - public int getCurrentScore() { - return getCurrentScore(false); - } - - // Get the current score for this Network as if it was validated. This may be modified from - // what the NetworkAgent sent, as it has modifiers applied to it. - public int getCurrentScoreAsValidated() { - return getCurrentScore(true); - } - public void setNetworkScore(@NonNull NetworkScore ns) { mNetworkScore = ns; } @@ -629,6 +611,41 @@ public class NetworkAgentInfo implements Comparable { mLingering = false; } + /** + * Returns whether this NAI has any chance of ever beating this other agent. + * + * The chief use case of this is the decision to tear down this network. ConnectivityService + * tears down networks that don't satisfy any request, unless they have a chance to beat any + * existing satisfier. + * + * @param other the agent to beat + * @return whether this should be given more time to try and beat the other agent + * TODO : remove this and migrate to a ranker-based approach + */ + public boolean canPossiblyBeat(@NonNull final NetworkAgentInfo other) { + // Any explicitly selected network should be held on. + if (networkAgentConfig.explicitlySelected) return true; + // An outscored exiting network should be torn down. + if (mNetworkScore.isExiting()) return false; + // If this network is validated it can be torn down as it can't hope to be better than + // it already is. + if (networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)) return false; + // If neither network is validated, keep both until at least one does. + if (!other.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)) return true; + // If this network is not metered but the other is, it should be preferable if it validates. + if (networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED) + && !other.networkCapabilities.hasCapability(NET_CAPABILITY_NOT_METERED)) { + return true; + } + + // If the control comes here : + // • This network is neither exiting or explicitly selected + // • This network is not validated, but the other is + // • This network is metered, or both networks are unmetered + // Keep it if it's expected to be faster than the other., should it validate. + return mNetworkScore.probablyFasterThan(other.mNetworkScore); + } + public void dumpLingerTimers(PrintWriter pw) { for (LingerTimer timer : mLingerTimers) { pw.println(timer); } } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 08869751b93e7..5aabc60b229f5 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -2054,14 +2054,15 @@ public class ConnectivityServiceTest { assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - // Bring up wifi with a score of 70. + // Bring up validated wifi. // Cell is lingered because it would not satisfy any request, even if it validated. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - mWiFiNetworkAgent.adjustScore(50); - mWiFiNetworkAgent.connect(false); // Score: 70 + mWiFiNetworkAgent.connect(true); // Score: 60 callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + // TODO: Investigate sending validated before losing. callback.expectCallback(CallbackEntry.LOSING, mCellNetworkAgent); - defaultCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); + defaultCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); From e9fd5a72298729c5ea46cadbea5ee89edfa06a90 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 14 Feb 2020 23:33:47 +0900 Subject: [PATCH 2/3] [NS D06] Implement more policies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Namely : • Explicitly selected policy • VPN policy • Validated policy These go together to avoid breaking any test, because multiple tests rely on all of these working. Test: ConnectivityServiceTest Change-Id: I7d815f87320c2becbfc93a60a3c54346ff4f47c9 --- .../server/connectivity/NetworkRanker.java | 48 +++++++++++++++++-- .../server/connectivity/NetworkRankerTest.kt | 28 +++++++++-- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index c536ab25e9254..fd5a4e82deb77 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -16,6 +16,9 @@ package com.android.server.connectivity; +import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; +import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; +import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkScore.POLICY_IGNORE_ON_WIFI; import static com.android.internal.util.FunctionalUtils.findFirst; @@ -42,8 +45,15 @@ public class NetworkRanker { @NonNull final Collection nais) { final ArrayList candidates = new ArrayList<>(nais); candidates.removeIf(nai -> !nai.satisfies(request)); - // Enforce policy. - filterBadWifiAvoidancePolicy(candidates); + + // Enforce policy. The order in which the policy is computed is essential, because each + // step may remove some of the candidates. For example, filterValidated drops non-validated + // networks in presence of validated networks for INTERNET requests, but the bad wifi + // avoidance policy takes priority over this, so it must be done before. + filterVpn(candidates); + filterExplicitlySelected(candidates); + filterBadWifiAvoidance(candidates); + filterValidated(request, candidates); NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; @@ -57,9 +67,27 @@ public class NetworkRanker { return bestNetwork; } - // If some network with wifi transport is present, drop all networks with POLICY_IGNORE_ON_WIFI. - private void filterBadWifiAvoidancePolicy( + // If a network is a VPN it has priority. + private void filterVpn(@NonNull final ArrayList candidates) { + final NetworkAgentInfo vpn = findFirst(candidates, + nai -> nai.networkCapabilities.hasTransport(TRANSPORT_VPN)); + if (null == vpn) return; // No VPN : this policy doesn't apply. + candidates.removeIf(nai -> !nai.networkCapabilities.hasTransport(TRANSPORT_VPN)); + } + + // If some network is explicitly selected and set to accept unvalidated connectivity, then + // drop all networks that are not explicitly selected. + private void filterExplicitlySelected( @NonNull final ArrayList candidates) { + final NetworkAgentInfo explicitlySelected = findFirst(candidates, + nai -> nai.networkAgentConfig.explicitlySelected + && nai.networkAgentConfig.acceptUnvalidated); + if (null == explicitlySelected) return; // No explicitly selected network accepting unvalid + candidates.removeIf(nai -> !nai.networkAgentConfig.explicitlySelected); + } + + // If some network with wifi transport is present, drop all networks with POLICY_IGNORE_ON_WIFI. + private void filterBadWifiAvoidance(@NonNull final ArrayList candidates) { final NetworkAgentInfo wifi = findFirst(candidates, nai -> nai.networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) && nai.everValidated @@ -71,4 +99,16 @@ public class NetworkRanker { if (null == wifi) return; // No wifi : this policy doesn't apply candidates.removeIf(nai -> nai.getNetworkScore().hasPolicy(POLICY_IGNORE_ON_WIFI)); } + + // If some network is validated and the request asks for INTERNET, drop all networks that are + // not validated. + private void filterValidated(@NonNull final NetworkRequest request, + @NonNull final ArrayList candidates) { + if (!request.hasCapability(NET_CAPABILITY_INTERNET)) return; + final NetworkAgentInfo validated = findFirst(candidates, + nai -> nai.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)); + if (null == validated) return; // No validated network + candidates.removeIf(nai -> + !nai.networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED)); + } } diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt index a6b371a23b587..2b0c2c7215197 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -16,8 +16,14 @@ package com.android.server.connectivity +import android.net.ConnectivityManager.TYPE_WIFI +import android.net.LinkProperties +import android.net.Network +import android.net.NetworkAgentConfig import android.net.NetworkCapabilities +import android.net.NetworkInfo import android.net.NetworkRequest +import android.net.NetworkScore import androidx.test.filters.SmallTest import androidx.test.runner.AndroidJUnit4 import org.junit.Test @@ -33,10 +39,24 @@ import kotlin.test.assertNull class NetworkRankerTest { private val ranker = NetworkRanker() - private fun makeNai(satisfy: Boolean, score: Int) = mock(NetworkAgentInfo::class.java).also { - doReturn(satisfy).`when`(it).satisfies(any()) - doReturn(score).`when`(it).currentScore - it.networkCapabilities = NetworkCapabilities() + private fun makeNai(satisfy: Boolean, score: Int) = object : NetworkAgentInfo( + null /* messenger */, + null /* asyncChannel*/, + Network(100), + NetworkInfo(TYPE_WIFI, 0 /* subtype */, "" /* typename */, "" /* subtypename */), + LinkProperties(), + NetworkCapabilities(), + NetworkScore.Builder().setLegacyScore(score).build(), + null /* context */, + null /* handler */, + NetworkAgentConfig(), + null /* connectivityService */, + null /* netd */, + null /* dnsResolver */, + null /* networkManagementService */, + 0 /* factorySerialNumber */) { + override fun satisfies(request: NetworkRequest?): Boolean = satisfy + override fun getCurrentScore(): Int = score } @Test From ff83b0e467094490e7f3b08bbff91d36a443a7ce Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 17 Feb 2020 19:11:04 +0900 Subject: [PATCH 3/3] [NS D07] Use the unmodified legacy score Ranking used to make use of the various adjustments in ConnectivityService. These are now implemented in policy. Test: ConnectivityServiceTest Change-Id: I56109847678ea5cda1752511123ba652c0f4fe36 --- .../java/com/android/server/connectivity/NetworkRanker.java | 2 +- tests/net/java/com/android/server/ConnectivityServiceTest.java | 2 +- .../java/com/android/server/connectivity/NetworkRankerTest.kt | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java index fd5a4e82deb77..80d46e0370b4d 100644 --- a/services/core/java/com/android/server/connectivity/NetworkRanker.java +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -58,7 +58,7 @@ public class NetworkRanker { NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; for (final NetworkAgentInfo nai : candidates) { - final int score = nai.getCurrentScore(); + final int score = nai.getNetworkScore().getLegacyScore(); if (score > bestScore) { bestNetwork = nai; bestScore = score; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 5aabc60b229f5..220cdce0d1785 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5846,7 +5846,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); - trustedCallback.expectAvailableDoubleValidatedCallbacks(mWiFiNetworkAgent); + trustedCallback.expectAvailableThenValidatedCallbacks(mWiFiNetworkAgent); verify(mNetworkManagementService).setDefaultNetId(eq(mWiFiNetworkAgent.getNetwork().netId)); reset(mNetworkManagementService); diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt index 2b0c2c7215197..d2532c2ce3d32 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -28,8 +28,6 @@ import androidx.test.filters.SmallTest import androidx.test.runner.AndroidJUnit4 import org.junit.Test import org.junit.runner.RunWith -import org.mockito.ArgumentMatchers.any -import org.mockito.Mockito.doReturn import org.mockito.Mockito.mock import kotlin.test.assertEquals import kotlin.test.assertNull