UM: Policy for deciding download URL.
This adds a new private policy (UpdateDownloadUrl) for determining which
download URL should be used for obtaining the update payload. We further
extend an existing public policy (UpdateCanStart) to return the download
URL details, based on the current URL index and the number of failures
associated with it. This renders the explicit notion of "HTTP download
allowed" in the return value unnecessary: If HTTP is not allowed, then
HTTP URLs will not be considered.
We also implement logic for logging the start/end of a policy
evaluation, so that intermediate log messages emitted during evaluation
have a clear context.
BUG=chromium:358329
TEST=Unit tests.
Change-Id: Ib5343417480d8825082f83bed2630a6611360b61
Reviewed-on: https://chromium-review.googlesource.com/203373
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 646c3f5..fc73d0d 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -9,18 +9,144 @@
#include <string>
#include <base/logging.h>
+#include <base/strings/string_util.h>
#include <base/time/time.h>
+#include "update_engine/error_code.h"
#include "update_engine/update_manager/device_policy_provider.h"
#include "update_engine/update_manager/policy_utils.h"
#include "update_engine/update_manager/shill_provider.h"
+#include "update_engine/utils.h"
using base::Time;
using base::TimeDelta;
+using chromeos_update_engine::ErrorCode;
+using std::max;
using std::min;
using std::set;
using std::string;
+namespace {
+
+// Increment |url_idx|, |url_num_failures| or none of them based on the provided
+// error code. If |url_idx| is incremented, then sets |url_num_failures| to zero
+// and returns true; otherwise, returns false.
+//
+// TODO(garnold) Adapted from PayloadState::UpdateFailed() (to be retired).
+bool HandleErrorCode(ErrorCode err_code, int* url_idx, int* url_num_failures) {
+ err_code = chromeos_update_engine::utils::GetBaseErrorCode(err_code);
+ switch (err_code) {
+ // Errors which are good indicators of a problem with a particular URL or
+ // the protocol used in the URL or entities in the communication channel
+ // (e.g. proxies). We should try the next available URL in the next update
+ // check to quickly recover from these errors.
+ case ErrorCode::kPayloadHashMismatchError:
+ case ErrorCode::kPayloadSizeMismatchError:
+ case ErrorCode::kDownloadPayloadVerificationError:
+ case ErrorCode::kDownloadPayloadPubKeyVerificationError:
+ case ErrorCode::kSignedDeltaPayloadExpectedError:
+ case ErrorCode::kDownloadInvalidMetadataMagicString:
+ case ErrorCode::kDownloadSignatureMissingInManifest:
+ case ErrorCode::kDownloadManifestParseError:
+ case ErrorCode::kDownloadMetadataSignatureError:
+ case ErrorCode::kDownloadMetadataSignatureVerificationError:
+ case ErrorCode::kDownloadMetadataSignatureMismatch:
+ case ErrorCode::kDownloadOperationHashVerificationError:
+ case ErrorCode::kDownloadOperationExecutionError:
+ case ErrorCode::kDownloadOperationHashMismatch:
+ case ErrorCode::kDownloadInvalidMetadataSize:
+ case ErrorCode::kDownloadInvalidMetadataSignature:
+ case ErrorCode::kDownloadOperationHashMissingError:
+ case ErrorCode::kDownloadMetadataSignatureMissingError:
+ case ErrorCode::kPayloadMismatchedType:
+ case ErrorCode::kUnsupportedMajorPayloadVersion:
+ case ErrorCode::kUnsupportedMinorPayloadVersion:
+ LOG(INFO) << "Advancing download URL due to error "
+ << chromeos_update_engine::utils::CodeToString(err_code)
+ << " (" << static_cast<int>(err_code) << ")";
+ *url_idx += 1;
+ *url_num_failures = 0;
+ return true;
+
+ // Errors which seem to be just transient network/communication related
+ // failures and do not indicate any inherent problem with the URL itself.
+ // So, we should keep the current URL but just increment the
+ // failure count to give it more chances. This way, while we maximize our
+ // chances of downloading from the URLs that appear earlier in the response
+ // (because download from a local server URL that appears earlier in a
+ // response is preferable than downloading from the next URL which could be
+ // a internet URL and thus could be more expensive).
+ case ErrorCode::kError:
+ case ErrorCode::kDownloadTransferError:
+ case ErrorCode::kDownloadWriteError:
+ case ErrorCode::kDownloadStateInitializationError:
+ case ErrorCode::kOmahaErrorInHTTPResponse: // Aggregate for HTTP errors.
+ LOG(INFO) << "Incrementing URL failure count due to error "
+ << chromeos_update_engine::utils::CodeToString(err_code)
+ << " (" << static_cast<int>(err_code) << ")";
+ *url_num_failures += 1;
+ return false;
+
+ // Errors which are not specific to a URL and hence shouldn't result in
+ // the URL being penalized. This can happen in two cases:
+ // 1. We haven't started downloading anything: These errors don't cost us
+ // anything in terms of actual payload bytes, so we should just do the
+ // regular retries at the next update check.
+ // 2. We have successfully downloaded the payload: In this case, the
+ // payload attempt number would have been incremented and would take care
+ // of the backoff at the next update check.
+ // In either case, there's no need to update URL index or failure count.
+ case ErrorCode::kOmahaRequestError:
+ case ErrorCode::kOmahaResponseHandlerError:
+ case ErrorCode::kPostinstallRunnerError:
+ case ErrorCode::kFilesystemCopierError:
+ case ErrorCode::kInstallDeviceOpenError:
+ case ErrorCode::kKernelDeviceOpenError:
+ case ErrorCode::kDownloadNewPartitionInfoError:
+ case ErrorCode::kNewRootfsVerificationError:
+ case ErrorCode::kNewKernelVerificationError:
+ case ErrorCode::kPostinstallBootedFromFirmwareB:
+ case ErrorCode::kPostinstallFirmwareRONotUpdatable:
+ case ErrorCode::kOmahaRequestEmptyResponseError:
+ case ErrorCode::kOmahaRequestXMLParseError:
+ case ErrorCode::kOmahaResponseInvalid:
+ case ErrorCode::kOmahaUpdateIgnoredPerPolicy:
+ case ErrorCode::kOmahaUpdateDeferredPerPolicy:
+ case ErrorCode::kOmahaUpdateDeferredForBackoff:
+ case ErrorCode::kPostinstallPowerwashError:
+ case ErrorCode::kUpdateCanceledByChannelChange:
+ LOG(INFO) << "Not changing URL index or failure count due to error "
+ << chromeos_update_engine::utils::CodeToString(err_code)
+ << " (" << static_cast<int>(err_code) << ")";
+ return false;
+
+ case ErrorCode::kSuccess: // success code
+ case ErrorCode::kUmaReportedMax: // not an error code
+ case ErrorCode::kOmahaRequestHTTPResponseBase: // aggregated already
+ case ErrorCode::kDevModeFlag: // not an error code
+ case ErrorCode::kResumedFlag: // not an error code
+ case ErrorCode::kTestImageFlag: // not an error code
+ case ErrorCode::kTestOmahaUrlFlag: // not an error code
+ case ErrorCode::kSpecialFlags: // not an error code
+ // These shouldn't happen. Enumerating these explicitly here so that we
+ // can let the compiler warn about new error codes that are added to
+ // action_processor.h but not added here.
+ LOG(WARNING) << "Unexpected error "
+ << chromeos_update_engine::utils::CodeToString(err_code)
+ << " (" << static_cast<int>(err_code) << ")";
+ // Note: Not adding a default here so as to let the compiler warn us of
+ // any new enums that were added in the .h but not listed in this switch.
+ }
+ return false;
+}
+
+// Checks whether |download_url| can be used under given download restrictions.
+bool DownloadUrlIsUsable(const string& download_url, bool http_allowed) {
+ return http_allowed || !StartsWithASCII(download_url, "http://", false);
+}
+
+} // namespace
+
namespace chromeos_update_manager {
EvalStatus ChromeOSPolicy::UpdateCheckAllowed(
@@ -49,12 +175,13 @@
const UpdateState& update_state) const {
// Set the default return values.
result->update_can_start = true;
- result->http_allowed = true;
result->p2p_allowed = false;
result->target_channel.clear();
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
result->scatter_wait_period = kZeroInterval;
result->scatter_check_threshold = 0;
+ result->download_url_idx = -1;
+ result->download_url_num_failures = 0;
// Make sure that we're not due for an update check.
UpdateCheckParams check_result;
@@ -69,7 +196,6 @@
}
DevicePolicyProvider* const dp_provider = state->device_policy_provider();
- SystemProvider* const system_provider = state->system_provider();
const bool* device_policy_is_loaded_p = ec->GetValue(
dp_provider->var_device_policy_is_loaded());
@@ -99,7 +225,7 @@
scattering_applies = true;
} else {
const bool* is_oobe_complete_p = ec->GetValue(
- system_provider->var_is_oobe_complete());
+ state->system_provider()->var_is_oobe_complete());
scattering_applies = (is_oobe_complete_p && *is_oobe_complete_p);
}
}
@@ -121,17 +247,6 @@
}
}
- // Determine whether HTTP downloads are forbidden by policy. This only
- // applies to official system builds; otherwise, HTTP is always enabled.
- const bool* is_official_build_p = ec->GetValue(
- system_provider->var_is_official_build());
- if (is_official_build_p && *is_official_build_p) {
- const bool* policy_http_downloads_enabled_p = ec->GetValue(
- dp_provider->var_http_downloads_enabled());
- result->http_allowed =
- !policy_http_downloads_enabled_p || *policy_http_downloads_enabled_p;
- }
-
// Determine whether use of P2P is allowed by policy.
const bool* policy_au_p2p_enabled_p = ec->GetValue(
dp_provider->var_au_p2p_enabled());
@@ -155,6 +270,23 @@
result->p2p_allowed = updater_p2p_enabled_p && *updater_p2p_enabled_p;
}
+ // Determine the URL to download the update from. Note that a failure to find
+ // a download URL will only fail this policy iff no other means of download
+ // (such as P2P) are enabled.
+ UpdateDownloadUrlResult download_url_result;
+ EvalStatus download_url_status = UpdateDownloadUrl(
+ ec, state, error, &download_url_result, update_state);
+ if (download_url_status == EvalStatus::kSucceeded) {
+ result->download_url_idx = download_url_result.url_idx;
+ result->download_url_num_failures = download_url_result.url_num_failures;
+ } else if (!result->p2p_allowed) {
+ if (download_url_status != EvalStatus::kFailed) {
+ result->update_can_start = false;
+ result->cannot_start_reason = UpdateCannotStartReason::kCannotDownload;
+ }
+ return download_url_status;
+ }
+
return EvalStatus::kSucceeded;
}
@@ -303,11 +435,72 @@
DCHECK_GE(fuzz, 0);
int half_fuzz = fuzz / 2;
// This guarantees the output interval is non negative.
- int interval_min = std::max(interval - half_fuzz, 0);
+ int interval_min = max(interval - half_fuzz, 0);
int interval_max = interval + half_fuzz;
return TimeDelta::FromSeconds(prng->RandMinMax(interval_min, interval_max));
}
+EvalStatus ChromeOSPolicy::UpdateDownloadUrl(
+ EvaluationContext* ec, State* state, std::string* error,
+ UpdateDownloadUrlResult* result, const UpdateState& update_state) const {
+ int url_idx = 0;
+ int url_num_failures = 0;
+ if (update_state.num_checks > 1) {
+ // Ignore negative URL indexes, which indicate that no previous suitable
+ // download URL was found.
+ url_idx = max(0, update_state.download_url_idx);
+ url_num_failures = update_state.download_url_num_failures;
+ }
+
+ // Preconditions / sanity checks.
+ DCHECK_GE(update_state.download_failures_max, 0);
+ DCHECK_LT(url_idx, static_cast<int>(update_state.download_urls.size()));
+ DCHECK_LE(url_num_failures, update_state.download_failures_max);
+
+ // Determine whether HTTP downloads are forbidden by policy. This only
+ // applies to official system builds; otherwise, HTTP is always enabled.
+ bool http_allowed = true;
+ const bool* is_official_build_p = ec->GetValue(
+ state->system_provider()->var_is_official_build());
+ if (is_official_build_p && *is_official_build_p) {
+ DevicePolicyProvider* const dp_provider = state->device_policy_provider();
+ const bool* device_policy_is_loaded_p = ec->GetValue(
+ dp_provider->var_device_policy_is_loaded());
+ if (device_policy_is_loaded_p && *device_policy_is_loaded_p) {
+ const bool* policy_http_downloads_enabled_p = ec->GetValue(
+ dp_provider->var_http_downloads_enabled());
+ http_allowed = (!policy_http_downloads_enabled_p ||
+ *policy_http_downloads_enabled_p);
+ }
+ }
+
+ // Process recent failures, stop if the URL index advances.
+ for (auto& err_code : update_state.download_url_error_codes) {
+ if (HandleErrorCode(err_code, &url_idx, &url_num_failures))
+ break;
+ if (url_num_failures > update_state.download_failures_max) {
+ url_idx++;
+ url_num_failures = 0;
+ break;
+ }
+ }
+ url_idx %= update_state.download_urls.size();
+
+ // Scan through URLs until a usable one is found.
+ const int start_url_idx = url_idx;
+ while (!DownloadUrlIsUsable(update_state.download_urls[url_idx],
+ http_allowed)) {
+ url_idx = (url_idx + 1) % update_state.download_urls.size();
+ url_num_failures = 0;
+ if (url_idx == start_url_idx)
+ return EvalStatus::kFailed; // No usable URLs.
+ }
+
+ result->url_idx = url_idx;
+ result->url_num_failures = url_num_failures;
+ return EvalStatus::kSucceeded;
+}
+
EvalStatus ChromeOSPolicy::UpdateScattering(
EvaluationContext* ec,
State* state,
diff --git a/update_manager/chromeos_policy.h b/update_manager/chromeos_policy.h
index 3c47c60..68cda65 100644
--- a/update_manager/chromeos_policy.h
+++ b/update_manager/chromeos_policy.h
@@ -13,6 +13,12 @@
namespace chromeos_update_manager {
+// Parameters for update download URL, as determined by UpdateDownloadUrl.
+struct UpdateDownloadUrlResult {
+ int url_idx;
+ int url_num_failures;
+};
+
// Parameters for update scattering, as determined by UpdateNotScattering.
struct UpdateScatteringResult {
bool is_scattering;
@@ -45,6 +51,12 @@
std::string* error,
bool* result) const override;
+ protected:
+ // Policy override.
+ virtual std::string PolicyName() const override {
+ return "ChromeOSPolicy";
+ }
+
private:
friend class UmChromeOSPolicyTest;
FRIEND_TEST(UmChromeOSPolicyTest,
@@ -86,18 +98,34 @@
// TimeDelta.
static base::TimeDelta FuzzedInterval(PRNG* prng, int interval, int fuzz);
+ // A private policy for determining which download URL to use. Within
+ // |update_state|, |download_urls| should contain the download URLs as listed
+ // in the Omaha response; |download_failures_max| the maximum number of
+ // failures per URL allowed per the response; |download_url_idx| the index of
+ // the previously used URL; |download_url_num_failures| the previously known
+ // number of failures associated with that URL; and |download_url_error_codes|
+ // the list of failures occurring since the latest evaluation.
+ //
+ // Upon successly deciding a URL to use, returns |EvalStatus::kSucceeded| and
+ // writes the current URL index and the number of failures associated with it
+ // in |result|. Otherwise, returns |EvalStatus::kFailed|.
+ EvalStatus UpdateDownloadUrl(EvaluationContext* ec, State* state,
+ std::string* error,
+ UpdateDownloadUrlResult* result,
+ const UpdateState& update_state) const;
+
// A private policy for checking whether scattering is due. Writes in |result|
// the decision as to whether or not to scatter; a wallclock-based scatter
// wait period, which ranges from zero (do not wait) and no greater than the
// current scatter factor provided by the device policy (if available) or the
// maximum wait period determined by Omaha; and an update check-based
// threshold between zero (no threshold) and the maximum number determined by
- // the update engine. Within |update_state|, |wait_period| should contain the
- // last scattering period returned by this function, or zero if no wait period
- // is known; |check_threshold| is the last update check threshold, or zero if
- // no such threshold is known. If not scattering, or if any of the scattering
- // values has changed, returns |EvalStatus::kSucceeded|; otherwise,
- // |EvalStatus::kAskMeAgainLater|.
+ // the update engine. Within |update_state|, |scatter_wait_period| should
+ // contain the last scattering period returned by this function, or zero if no
+ // wait period is known; |scatter_check_threshold| is the last update check
+ // threshold, or zero if no such threshold is known. If not scattering, or if
+ // any of the scattering values has changed, returns |EvalStatus::kSucceeded|;
+ // otherwise, |EvalStatus::kAskMeAgainLater|.
EvalStatus UpdateScattering(EvaluationContext* ec, State* state,
std::string* error,
UpdateScatteringResult* result,
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index 0fa96b3..140b6ba 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -6,6 +6,7 @@
#include <set>
#include <string>
+#include <vector>
#include <base/time/time.h>
#include <gtest/gtest.h>
@@ -17,9 +18,11 @@
using base::Time;
using base::TimeDelta;
+using chromeos_update_engine::ErrorCode;
using chromeos_update_engine::FakeClock;
using std::set;
using std::string;
+using std::vector;
namespace chromeos_update_manager {
@@ -68,8 +71,8 @@
reset(new ConnectionTethering(ConnectionTethering::kNotDetected));
}
- // Sets up a default device policy that does not impose any restrictions, nor
- // enables any features (HTTP, P2P).
+ // Sets up a default device policy that does not impose any restrictions
+ // (HTTP) nor enables any features (P2P).
void SetUpDefaultDevicePolicy() {
fake_state_.device_policy_provider()->var_device_policy_is_loaded()->reset(
new bool(true));
@@ -80,7 +83,7 @@
fake_state_.device_policy_provider()->var_scatter_factor()->reset(
new TimeDelta());
fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
- new bool(false));
+ new bool(true));
fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
new bool(false));
fake_state_.device_policy_provider()->var_release_channel_delegated()->
@@ -106,12 +109,14 @@
}
// Returns a default UpdateState structure: first seen time is calculated
- // backward from the current wall clock time, update was seen just once, there
- // is no scattering wait period and the max allowed is 7 days, there is no
- // check threshold and none is allowed.
+ // backward from the current wall clock time, update was seen just once;
+ // there's a single HTTP download URL with a maximum of 10 allowed failures;
+ // there is no scattering wait period and the max allowed is 7 days, there is
+ // no check threshold and none is allowed.
UpdateState GetDefaultUpdateState(TimeDelta update_first_seen_period) {
UpdateState update_state = {
fake_clock_.GetWallclockTime() - update_first_seen_period, 1,
+ vector<string>(1, "http://fake/url/"), 10, 0, 0, vector<ErrorCode>(),
TimeDelta(), TimeDelta::FromDays(7), 0, 0, 0
};
return update_state;
@@ -242,9 +247,10 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, false, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.http_allowed);
EXPECT_FALSE(result.p2p_allowed);
EXPECT_TRUE(result.target_channel.empty());
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedBlankPolicy) {
@@ -259,9 +265,10 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCanStart, &result, false, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.http_allowed);
EXPECT_FALSE(result.p2p_allowed);
EXPECT_TRUE(result.target_channel.empty());
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest, UpdateCanStartNotAllowedUpdatesDisabled) {
@@ -419,6 +426,8 @@
EXPECT_TRUE(result.update_can_start);
EXPECT_EQ(TimeDelta(), result.scatter_wait_period);
EXPECT_EQ(0, result.scatter_check_threshold);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest,
@@ -443,6 +452,8 @@
EXPECT_TRUE(result.update_can_start);
EXPECT_EQ(TimeDelta(), result.scatter_wait_period);
EXPECT_EQ(0, result.scatter_check_threshold);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest,
@@ -468,6 +479,8 @@
EXPECT_TRUE(result.update_can_start);
EXPECT_EQ(TimeDelta(), result.scatter_wait_period);
EXPECT_EQ(0, result.scatter_check_threshold);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithAttributes) {
@@ -492,9 +505,10 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.http_allowed);
EXPECT_TRUE(result.p2p_allowed);
EXPECT_EQ("foo-channel", result.target_channel);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithP2PFromUpdater) {
@@ -505,10 +519,6 @@
SetUpdateCheckAllowed(false);
// Override specific device policy attributes.
- fake_state_.device_policy_provider()->var_release_channel_delegated()->
- reset(new bool(false));
- fake_state_.device_policy_provider()->var_release_channel()->
- reset(new string("foo-channel"));
fake_state_.updater_provider()->var_p2p_enabled()->reset(new bool(true));
// Check that the UpdateCanStart returns true.
@@ -517,12 +527,13 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_FALSE(result.http_allowed);
EXPECT_TRUE(result.p2p_allowed);
- EXPECT_EQ("foo-channel", result.target_channel);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
-TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithHttpForUnofficialBuild) {
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCanStartAllowedWithHttpUrlForUnofficialBuild) {
// The UpdateCanStart policy returns true; device policy forbids both HTTP and
// P2P updates, but marking this an unofficial build overrules the HTTP
// setting.
@@ -530,10 +541,8 @@
SetUpdateCheckAllowed(false);
// Override specific device policy attributes.
- fake_state_.device_policy_provider()->var_release_channel_delegated()->
- reset(new bool(false));
- fake_state_.device_policy_provider()->var_release_channel()->
- reset(new string("foo-channel"));
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(false));
fake_state_.system_provider()->var_is_official_build()->
reset(new bool(false));
@@ -543,9 +552,152 @@
ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
false, update_state);
EXPECT_TRUE(result.update_can_start);
- EXPECT_TRUE(result.http_allowed);
EXPECT_FALSE(result.p2p_allowed);
- EXPECT_EQ("foo-channel", result.target_channel);
+ EXPECT_EQ(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithHttpsUrl) {
+ // The UpdateCanStart policy returns true; device policy forbids both HTTP and
+ // P2P updates, but an HTTPS URL is provided and selected for download.
+
+ SetUpdateCheckAllowed(false);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(false));
+
+ // Add an HTTPS URL.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.download_urls.push_back("https://secure/url/");
+
+ // Check that the UpdateCanStart returns true.
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ false, 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_failures);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithSecondUrlMaxExceeded) {
+ // The UpdateCanStart policy returns true; the first URL exceeded the maximum
+ // allowed number of failures, but a second URL is available.
+
+ SetUpdateCheckAllowed(false);
+
+ // Add a second URL; update with this URL attempted and failed enough times to
+ // disqualify the current (first) URL. This tests both the previously
+ // accounted failures (download_url_num_failures) as well as those occurring
+ // since the last call (download_url_error_codes).
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.num_checks = 10;
+ update_state.download_urls.push_back("http://another/fake/url/");
+ update_state.download_url_num_failures = 9;
+ update_state.download_url_error_codes.push_back(
+ ErrorCode::kDownloadTransferError);
+ update_state.download_url_error_codes.push_back(
+ ErrorCode::kDownloadWriteError);
+
+ // Check that the UpdateCanStart returns true.
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ false, 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_failures);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedWithSecondUrlHardError) {
+ // The UpdateCanStart policy returns true; the first URL fails with a hard
+ // error, but a second URL is available.
+
+ SetUpdateCheckAllowed(false);
+
+ // Add a second URL; update with this URL attempted and failed in a way that
+ // causes it to switch directly to the next URL.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.num_checks = 10;
+ update_state.download_urls.push_back("http://another/fake/url/");
+ update_state.download_url_error_codes.push_back(
+ ErrorCode::kPayloadHashMismatchError);
+
+ // Check that the UpdateCanStart returns true.
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ false, 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_failures);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedUrlWrapsAround) {
+ // The UpdateCanStart policy returns true; URL search properly wraps around
+ // the last one on the list.
+
+ SetUpdateCheckAllowed(false);
+
+ // Add a second URL; update with this URL attempted and failed in a way that
+ // causes it to switch directly to the next URL.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ update_state.num_checks = 10;
+ update_state.download_urls.push_back("http://another/fake/url/");
+ update_state.download_url_idx = 1;
+ update_state.download_url_error_codes.push_back(
+ ErrorCode::kPayloadHashMismatchError);
+
+ // Check that the UpdateCanStart returns true.
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ false, 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_failures);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartNotAllowedNoUsableUrls) {
+ // The UpdateCanStart policy returns false; there's a single HTTP URL but its
+ // use is forbidden by policy.
+
+ SetUpdateCheckAllowed(false);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(false));
+
+ // Check that the UpdateCanStart returns false.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kFailed, &Policy::UpdateCanStart, &result,
+ false, update_state);
+}
+
+TEST_F(UmChromeOSPolicyTest, UpdateCanStartAllowedNoUsableUrlsButP2PEnabled) {
+ // The UpdateCanStart policy returns true; there's a single HTTP URL but its
+ // use is forbidden by policy, however P2P is enabled. The result indicates
+ // that no URL can be used.
+
+ SetUpdateCheckAllowed(false);
+
+ // Override specific device policy attributes.
+ fake_state_.device_policy_provider()->var_au_p2p_enabled()->reset(
+ new bool(true));
+ fake_state_.device_policy_provider()->var_http_downloads_enabled()->reset(
+ new bool(false));
+
+ // Check that the UpdateCanStart returns true.
+ UpdateState update_state = GetDefaultUpdateState(TimeDelta::FromMinutes(10));
+ UpdateCanStartResult result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded, &Policy::UpdateCanStart, &result,
+ false, update_state);
+ EXPECT_TRUE(result.update_can_start);
+ EXPECT_TRUE(result.p2p_allowed);
+ EXPECT_GT(0, result.download_url_idx);
+ EXPECT_EQ(0, result.download_url_num_failures);
}
TEST_F(UmChromeOSPolicyTest, UpdateCurrentConnectionAllowedEthernetDefault) {
diff --git a/update_manager/default_policy.h b/update_manager/default_policy.h
index b3767dc..f9f4011 100644
--- a/update_manager/default_policy.h
+++ b/update_manager/default_policy.h
@@ -35,9 +35,10 @@
const bool interactive,
const UpdateState& update_state) const override {
result->update_can_start = true;
- result->http_allowed = false;
result->p2p_allowed = false;
result->target_channel.clear();
+ result->download_url_idx = 0;
+ result->download_url_num_failures = 0;
result->cannot_start_reason = UpdateCannotStartReason::kUndefined;
result->scatter_wait_period = base::TimeDelta();
result->scatter_check_threshold = 0;
@@ -53,6 +54,12 @@
return EvalStatus::kSucceeded;
}
+ protected:
+ // Policy override.
+ virtual std::string PolicyName() const override {
+ return "DefaultPolicy";
+ }
+
private:
DISALLOW_COPY_AND_ASSIGN(DefaultPolicy);
};
diff --git a/update_manager/policy.h b/update_manager/policy.h
index 65f236c..6515b77 100644
--- a/update_manager/policy.h
+++ b/update_manager/policy.h
@@ -6,7 +6,9 @@
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_UPDATE_MANAGER_POLICY_H_
#include <string>
+#include <vector>
+#include "update_engine/error_code.h"
#include "update_engine/update_manager/evaluation_context.h"
#include "update_engine/update_manager/state.h"
@@ -31,10 +33,30 @@
//
// A snapshot of the state of the current update process.
struct UpdateState {
+ // Information pertaining to the Omaha update response.
+ //
// Time when update was first offered by Omaha.
base::Time first_seen;
// Number of update checks returning the current update.
int num_checks;
+
+ // Information pertaining to the update download URL.
+ //
+ // An array of download URLs provided by Omaha.
+ std::vector<std::string> download_urls;
+ // Max number of failures allowed per download URL.
+ int download_failures_max;
+ // The index of the URL to use, as previously determined by the policy. This
+ // number is significant iff |num_checks| is greater than 1.
+ int download_url_idx;
+ // The number of failures already associated with this URL.
+ int download_url_num_failures;
+ // An array of failure error codes that occurred since the latest reported
+ // ones (included in the number above).
+ std::vector<chromeos_update_engine::ErrorCode> download_url_error_codes;
+
+ // Information pertaining to update scattering.
+ //
// Scattering wallclock-based wait period, as returned by the policy.
base::TimeDelta scatter_wait_period;
// Maximum wait period allowed for this update, as determined by Omaha.
@@ -57,17 +79,24 @@
kCheckDue,
kDisabledByPolicy,
kScattering,
+ kCannotDownload,
};
struct UpdateCanStartResult {
// Whether the update attempt is allowed to proceed.
bool update_can_start;
+
// Attributes pertaining to the case where update is allowed. The update
// engine uses them to choose the means for downloading and applying an
// update.
- bool http_allowed;
bool p2p_allowed;
std::string target_channel;
+ // The index of the download URL to use, and the number of failures associated
+ // with this URL. An index value of -1 indicates that no suitable URL is
+ // available, but there may be other means for download (like P2P).
+ int download_url_idx;
+ int download_url_num_failures;
+
// Attributes pertaining to the case where update is not allowed. Some are
// needed for storing values to persistent storage, others for
// logging/metrics.
@@ -86,6 +115,31 @@
public:
virtual ~Policy() {}
+ // Returns the name of a public policy request.
+ // IMPORTANT: Be sure to add a conditional for each new public policy that is
+ // being added to this class in the future.
+ template<typename R, typename... Args>
+ std::string PolicyRequestName(
+ EvalStatus (Policy::*policy_method)(EvaluationContext*, State*,
+ std::string*, R*,
+ Args...) const) const {
+ std::string class_name = PolicyName() + "::";
+
+ if (reinterpret_cast<typeof(&Policy::UpdateCheckAllowed)>(
+ policy_method) == &Policy::UpdateCheckAllowed)
+ return class_name + "UpdateCheckAllowed";
+ if (reinterpret_cast<typeof(&Policy::UpdateCanStart)>(
+ policy_method) == &Policy::UpdateCanStart)
+ return class_name + "UpdateCanStart";
+ if (reinterpret_cast<typeof(&Policy::UpdateCurrentConnectionAllowed)>(
+ policy_method) == &Policy::UpdateCurrentConnectionAllowed)
+ return class_name + "UpdateCurrentConnectionAllowed";
+
+ NOTREACHED();
+ return class_name + "(unknown)";
+ }
+
+
// List of policy requests. A policy request takes an EvaluationContext as the
// first argument, a State instance, a returned error message, a returned
// value and optionally followed by one or more arbitrary constant arguments.
@@ -128,6 +182,9 @@
protected:
Policy() {}
+ // Returns the name of the actual policy class.
+ virtual std::string PolicyName() const = 0;
+
private:
DISALLOW_COPY_AND_ASSIGN(Policy);
};
diff --git a/update_manager/update_manager-inl.h b/update_manager/update_manager-inl.h
index 544a371..362d4c5 100644
--- a/update_manager/update_manager-inl.h
+++ b/update_manager/update_manager-inl.h
@@ -22,8 +22,10 @@
Args...) const,
R* result, Args... args) {
std::string error;
+ const std::string policy_name = policy_->PolicyRequestName(policy_method);
// First try calling the actual policy.
+ LOG(INFO) << "Evaluating " << policy_name << " START";
EvalStatus status = (policy_.get()->*policy_method)(ec, state_.get(), &error,
result, args...);
@@ -37,6 +39,8 @@
LOG(WARNING) << "Request to default policy also failed: " << error;
}
}
+ LOG(INFO) << "Evaluating " << policy_name << " END";
+
// TODO(deymo): Log the actual state used from the EvaluationContext.
return status;
}
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 95fd3e0..fa6f9dd 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -25,6 +25,7 @@
using base::Callback;
using base::Time;
using base::TimeDelta;
+using chromeos_update_engine::ErrorCode;
using chromeos_update_engine::FakeClock;
using std::pair;
using std::string;
@@ -106,7 +107,9 @@
TEST_F(UmUpdateManagerTest, PolicyRequestCallUpdateCanStart) {
const UpdateState update_state = {
- FixedTime(), 1, TimeDelta::FromSeconds(15), TimeDelta::FromSeconds(60),
+ FixedTime(), 1,
+ vector<string>(1, "http://fake/url/"), 10, 0, 0, vector<ErrorCode>(),
+ TimeDelta::FromSeconds(15), TimeDelta::FromSeconds(60),
4, 2, 8
};
UpdateCanStartResult result;