update_engine: UM: Policy to distinguish between P2P downloading and sharing.
Previously, the UpdateCanStart policy returned a single Boolean,
indicating whether "P2P is allowed". However, the policy has been
incorrectly reproducing decisions made by current code, whereas P2P
sharing is always allowed (if P2P is enabled) but P2P downloading has
additional limitations. This CL introduces distinct flags for both
downloading and sharing, and reasons about them specifically.
This also adds a constraints whereas P2P downloading is allowed for
non-interactive updates only.
BUG=chromium:420732
TEST=Unit tests.
Change-Id: Ife4b6b2830c999745a0d4239089034c863de6388
Reviewed-on: https://chromium-review.googlesource.com/222262
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 86a2dc4..c2026b2 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -278,7 +278,8 @@
result->update_can_start = true;
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
result->download_url_idx = -1;
- result->p2p_allowed = false;
+ result->p2p_downloading_allowed = false;
+ result->p2p_sharing_allowed = false;
result->do_increment_failures = false;
result->backoff_expiry = update_state.backoff_expiry;
result->scatter_wait_period = update_state.scatter_wait_period;
@@ -317,22 +318,6 @@
return backoff_url_status;
}
- // Check whether P2P has reached its number of attempts and/or time limits. In
- // which case, set a flag preventing us from enabling P2P later on.
- bool is_p2p_blocked = false;
- if (update_state.p2p_num_attempts >= kMaxP2PAttempts) {
- is_p2p_blocked = true;
- LOG(INFO) << "Blocking P2P as it's been attempted too many times.";
- } else if (!update_state.p2p_first_attempted.is_null()) {
- Time p2p_expiry = (
- update_state.p2p_first_attempted +
- TimeDelta::FromSeconds(kMaxP2PAttemptsPeriodInSeconds));
- if (ec->IsWallclockTimeGreaterThan(p2p_expiry)) {
- is_p2p_blocked = true;
- LOG(INFO) << "Blocking P2P as its usage timespan exceeds the limit.";
- }
- }
-
DevicePolicyProvider* const dp_provider = state->device_policy_provider();
const bool* device_policy_is_loaded_p = ec->GetValue(
@@ -381,31 +366,47 @@
// Determine whether use of P2P is allowed by policy. Even if P2P is not
// explicitly allowed, we allow it if the device is enterprise enrolled
// (that is, missing or empty owner string).
- if (!is_p2p_blocked) {
- const bool* policy_au_p2p_enabled_p = ec->GetValue(
- dp_provider->var_au_p2p_enabled());
- if (policy_au_p2p_enabled_p) {
- result->p2p_allowed = *policy_au_p2p_enabled_p;
- } else {
- const string* policy_owner_p = ec->GetValue(dp_provider->var_owner());
- if (!policy_owner_p || policy_owner_p->empty())
- result->p2p_allowed = true;
- }
+ const bool* policy_au_p2p_enabled_p = ec->GetValue(
+ dp_provider->var_au_p2p_enabled());
+ if (policy_au_p2p_enabled_p) {
+ result->p2p_sharing_allowed = *policy_au_p2p_enabled_p;
+ } else {
+ const string* policy_owner_p = ec->GetValue(dp_provider->var_owner());
+ if (!policy_owner_p || policy_owner_p->empty())
+ result->p2p_sharing_allowed = true;
}
}
// Enable P2P, if so mandated by the updater configuration. This is additive
// to whether or not P2P is allowed per device policy (see above).
- if (!(is_p2p_blocked || result->p2p_allowed)) {
+ if (!result->p2p_sharing_allowed) {
const bool* updater_p2p_enabled_p = ec->GetValue(
state->updater_provider()->var_p2p_enabled());
- result->p2p_allowed = updater_p2p_enabled_p && *updater_p2p_enabled_p;
+ result->p2p_sharing_allowed =
+ updater_p2p_enabled_p && *updater_p2p_enabled_p;
+ }
+
+ // Finally, download via P2P is enabled iff P2P is enabled (sharing allowed),
+ // an update is not interactive, and other limits haven't been reached.
+ if (result->p2p_sharing_allowed) {
+ if (update_state.is_interactive) {
+ LOG(INFO) << "Blocked P2P download because update is interactive.";
+ } else if (update_state.p2p_num_attempts >= kMaxP2PAttempts) {
+ LOG(INFO) << "Blocked P2P download as it was attempted too many times.";
+ } else if (!update_state.p2p_first_attempted.is_null() &&
+ ec->IsWallclockTimeGreaterThan(
+ update_state.p2p_first_attempted +
+ TimeDelta::FromSeconds(kMaxP2PAttemptsPeriodInSeconds))) {
+ LOG(INFO) << "Blocked P2P download as its usage timespan exceeds limit.";
+ } else {
+ result->p2p_downloading_allowed = true;
+ }
}
// Store the download URL and failure increment flag. Note that no URL will
// only terminate evaluation if no other means of download (such as P2P) are
// enabled.
- if (result->download_url_idx < 0 && !result->p2p_allowed) {
+ if (result->download_url_idx < 0 && !result->p2p_downloading_allowed) {
result->update_can_start = false;
result->cannot_start_reason = UpdateCannotStartReason::kCannotDownload;
}
diff --git a/update_manager/chromeos_policy.h b/update_manager/chromeos_policy.h
index e3e9c5f..5108bc2 100644
--- a/update_manager/chromeos_policy.h
+++ b/update_manager/chromeos_policy.h
@@ -86,9 +86,9 @@
FRIEND_TEST(UmChromeOSPolicyTest,
UpdateCanStartAllowedInteractivePreventsScattering);
FRIEND_TEST(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PBlockedDueToNumAttempts);
+ UpdateCanStartAllowedP2PDownloadBlockedDueToNumAttempts);
FRIEND_TEST(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PBlockedDueToAttemptsPeriod);
+ UpdateCanStartAllowedP2PDownloadBlockedDueToAttemptsPeriod);
// Auxiliary constant (zero by default).
const base::TimeDelta kZeroInterval;
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index b99074c..443a298 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -494,7 +494,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
+ EXPECT_FALSE(result.p2p_downloading_allowed);
+ EXPECT_FALSE(result.p2p_sharing_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -512,7 +513,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
+ EXPECT_FALSE(result.p2p_downloading_allowed);
+ EXPECT_FALSE(result.p2p_sharing_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -949,7 +951,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_TRUE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -971,15 +974,18 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_TRUE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
}
-TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedP2PBlockedDueToNumAttempts) {
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCanStartAllowedP2PDownloadBlockedDueToNumAttempts) {
// The UpdateCanStart policy returns true; device policy permits HTTP but
- // blocks P2P, because the max number of P2P downloads have been attempted.
+ // blocks P2P download, because the max number of P2P downloads have been
+ // attempted. P2P sharing is still permitted.
SetUpdateCheckAllowed(false);
@@ -996,14 +1002,15 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
+ EXPECT_FALSE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
}
TEST_F(UmChromeOSPolicyTest,
- UpdateCanStartAllowedP2PBlockedDueToAttemptsPeriod) {
+ UpdateCanStartAllowedP2PDownloadBlockedDueToAttemptsPeriod) {
// The UpdateCanStart policy returns true; device policy permits HTTP but
- // blocks P2P, because the max period for attempt to download via P2P has
- // elapsed.
+ // blocks P2P download, because the max period for attempt to download via P2P
+ // has elapsed. P2P sharing is still permitted.
SetUpdateCheckAllowed(false);
@@ -1024,7 +1031,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
+ EXPECT_FALSE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
}
TEST_F(UmChromeOSPolicyTest,
@@ -1047,7 +1055,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1072,7 +1079,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(1, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1102,7 +1108,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(5, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1131,7 +1136,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(1, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1157,7 +1161,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(1, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1185,7 +1188,6 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.p2p_allowed);
EXPECT_EQ(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_TRUE(result.do_increment_failures);
@@ -1239,7 +1241,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_TRUE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
EXPECT_GT(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
@@ -1269,7 +1272,8 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_TRUE(result.p2p_downloading_allowed);
+ EXPECT_TRUE(result.p2p_sharing_allowed);
EXPECT_GT(0, result.download_url_idx);
EXPECT_EQ(0, result.download_url_num_errors);
EXPECT_FALSE(result.do_increment_failures);
diff --git a/update_manager/default_policy.cc b/update_manager/default_policy.cc
index 13de5bd..3de9755 100644
--- a/update_manager/default_policy.cc
+++ b/update_manager/default_policy.cc
@@ -51,7 +51,8 @@
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
result->download_url_idx = 0;
result->download_url_num_errors = 0;
- result->p2p_allowed = false;
+ result->p2p_downloading_allowed = false;
+ result->p2p_sharing_allowed = false;
result->do_increment_failures = false;
result->backoff_expiry = base::Time();
result->scatter_wait_period = base::TimeDelta();
diff --git a/update_manager/policy.h b/update_manager/policy.h
index 2f88996..b801d91 100644
--- a/update_manager/policy.h
+++ b/update_manager/policy.h
@@ -145,8 +145,9 @@
// needs to be persisted and handed back to the policy on the next time it is
// called.
int download_url_num_errors;
- // Whether P2P downloads are allowed.
- bool p2p_allowed;
+ // Whether P2P download and sharing are allowed.
+ bool p2p_downloading_allowed;
+ bool p2p_sharing_allowed;
// Other values that need to be persisted and handed to the policy as need on
// the next call.