update_engine: UM: UpdateCheckAllowed now considers interactive update requests.
This is necessary so we can delegate handling of all update checks to
the UpdateManager, allowing us to share logic between the two cases and
eliminate multiple entry point to UpdateAttempter::Update() and handling
of interference between these two processes. Instead, these are all
handled naturally by the UpdateManager.
BUG=chromium:394389
TEST=Unit tests.
Change-Id: I32a1ab917e5aeb5c2da1953d8b0ffa8c9d8d62f9
Reviewed-on: https://chromium-review.googlesource.com/209100
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/update_attempter.cc b/update_attempter.cc
index a31e135..e2e1201 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -225,6 +225,10 @@
// appropriate to use as a hook for reporting daily metrics.
CheckAndReportDailyMetrics();
+ // Notify of the new update attempt, clearing prior interactive requests.
+ if (interactive_update_pending_callback_.get())
+ interactive_update_pending_callback_->Run(false);
+
chrome_proxy_resolver_.Init();
fake_update_success_ = false;
if (status_ == UPDATE_STATUS_UPDATED_NEED_REBOOT) {
@@ -797,7 +801,9 @@
void UpdateAttempter::CheckForUpdate(const string& app_version,
const string& omaha_url,
bool interactive) {
- LOG(INFO) << "New update check requested";
+ LOG(INFO) << "Interactive update check requested.";
+ if (interactive_update_pending_callback_.get())
+ interactive_update_pending_callback_->Run(true);
if (status_ != UPDATE_STATUS_IDLE) {
LOG(INFO) << "Skipping update check because current status is "
diff --git a/update_attempter.h b/update_attempter.h
index 037fc24..8ea348a 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -12,6 +12,7 @@
#include <vector>
#include <utility>
+#include <base/bind.h>
#include <base/time/time.h>
#include <glib.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
@@ -205,6 +206,16 @@
return server_dictated_poll_interval_;
}
+ // Sets a callback to be used when either an interactive update request is
+ // received (true) or cleared by an update attempt (false). Takes ownership of
+ // the callback object. A null value disables callback on these events. Note
+ // that only one callback can be set, so effectively at most one client can be
+ // notified.
+ virtual void set_interactive_update_pending_callback(
+ base::Callback<void(bool)>* callback) { // NOLINT(readability/function)
+ interactive_update_pending_callback_.reset(callback);
+ }
+
private:
// Update server URL for automated lab test.
static const char* const kTestUpdateUrl;
@@ -467,6 +478,11 @@
// otherwise. This is needed for calculating the update check interval.
unsigned int server_dictated_poll_interval_ = 0;
+ // A callback to use when either an interactive update request is received
+ // (true) or cleared by an update attempt (false).
+ scoped_ptr<base::Callback<void(bool)>> // NOLINT(readability/function)
+ interactive_update_pending_callback_;
+
DISALLOW_COPY_AND_ASSIGN(UpdateAttempter);
};
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index c49e3a7..7032074 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -155,19 +155,14 @@
// Set the default return values.
result->updates_enabled = true;
result->target_channel.clear();
+ result->is_interactive = false;
DevicePolicyProvider* const dp_provider = state->device_policy_provider();
+ UpdaterProvider* const updater_provider = state->updater_provider();
SystemProvider* const system_provider = state->system_provider();
- // Unofficial builds should not perform any automatic update checks.
- const bool* is_official_build_p = ec->GetValue(
- system_provider->var_is_official_build());
- if (is_official_build_p && !(*is_official_build_p)) {
- result->updates_enabled = false;
- return EvalStatus::kSucceeded;
- }
-
- // Do not perform any automatic update checks if booted from removable device.
+ // Do not perform any updates if booted from removable device. This decision
+ // is final.
const bool* is_boot_device_removable_p = ec->GetValue(
system_provider->var_is_boot_device_removable());
if (is_boot_device_removable_p && *is_boot_device_removable_p) {
@@ -175,16 +170,6 @@
return EvalStatus::kSucceeded;
}
- // If OOBE is enabled, wait until it is completed.
- const bool* is_oobe_enabled_p = ec->GetValue(
- state->config_provider()->var_is_oobe_enabled());
- if (is_oobe_enabled_p && *is_oobe_enabled_p) {
- const bool* is_oobe_complete_p = ec->GetValue(
- system_provider->var_is_oobe_complete());
- if (is_oobe_complete_p && !(*is_oobe_complete_p))
- return EvalStatus::kAskMeAgainLater;
- }
-
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) {
@@ -205,7 +190,36 @@
}
}
- // Ensure that update checks are timed properly.
+ // First, check to see if an interactive update was requested.
+ const bool* interactive_update_requested_p = ec->GetValue(
+ updater_provider->var_interactive_update_requested());
+ if (interactive_update_requested_p && *interactive_update_requested_p) {
+ result->is_interactive = true;
+ return EvalStatus::kSucceeded;
+ }
+
+ // The logic thereafter applies to periodic updates. Bear in mind that we
+ // should not return a final "no" if any of these criteria are not satisfied,
+ // because the system may still update due to an interactive update request.
+
+ // Unofficial builds should not perform periodic update checks.
+ const bool* is_official_build_p = ec->GetValue(
+ system_provider->var_is_official_build());
+ if (is_official_build_p && !(*is_official_build_p)) {
+ return EvalStatus::kAskMeAgainLater;
+ }
+
+ // If OOBE is enabled, wait until it is completed.
+ const bool* is_oobe_enabled_p = ec->GetValue(
+ state->config_provider()->var_is_oobe_enabled());
+ if (is_oobe_enabled_p && *is_oobe_enabled_p) {
+ const bool* is_oobe_complete_p = ec->GetValue(
+ system_provider->var_is_oobe_complete());
+ if (is_oobe_complete_p && !(*is_oobe_complete_p))
+ return EvalStatus::kAskMeAgainLater;
+ }
+
+ // Ensure that periodic update checks are timed properly.
Time next_update_check;
if (NextUpdateCheckTime(ec, state, error, &next_update_check) !=
EvalStatus::kSucceeded) {
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index ef609c1..f79c4fc 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -50,6 +50,8 @@
reset(new unsigned int(0)); // NOLINT(readability/casting)
fake_state_.updater_provider()->var_server_dictated_poll_interval()->
reset(new unsigned int(0)); // NOLINT(readability/casting)
+ fake_state_.updater_provider()->var_interactive_update_requested()->
+ reset(new bool(false)); // NOLINT(readability/casting)
fake_state_.random_provider()->var_seed()->reset(
new uint64_t(4)); // chosen by fair dice roll.
@@ -290,6 +292,7 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCheckAllowed, &result);
EXPECT_TRUE(result.updates_enabled);
+ EXPECT_FALSE(result.is_interactive);
}
TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedWaitsForOOBE) {
@@ -326,6 +329,7 @@
ExpectPolicyStatus(EvalStatus::kSucceeded,
&Policy::UpdateCheckAllowed, &result);
EXPECT_TRUE(result.updates_enabled);
+ EXPECT_FALSE(result.is_interactive);
}
TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedWithAttributes) {
@@ -344,20 +348,20 @@
&Policy::UpdateCheckAllowed, &result);
EXPECT_TRUE(result.updates_enabled);
EXPECT_EQ("foo-channel", result.target_channel);
+ EXPECT_FALSE(result.is_interactive);
}
TEST_F(UmChromeOSPolicyTest,
UpdateCheckAllowedUpdatesDisabledForUnofficialBuilds) {
- // UpdateCheckAllowed should return false (kSucceeded) if this is an
- // unofficial build; we don't want periodic update checks on developer images.
+ // UpdateCheckAllowed should return kAskMeAgainLater if this is an unofficial
+ // build; we don't want periodic update checks on developer images.
fake_state_.system_provider()->var_is_official_build()->reset(
new bool(false));
UpdateCheckParams result;
- ExpectPolicyStatus(EvalStatus::kSucceeded,
+ ExpectPolicyStatus(EvalStatus::kAskMeAgainLater,
&Policy::UpdateCheckAllowed, &result);
- EXPECT_FALSE(result.updates_enabled);
}
TEST_F(UmChromeOSPolicyTest,
@@ -387,6 +391,21 @@
&Policy::UpdateCheckAllowed, &result);
}
+TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedInteractiveUpdateRequested) {
+ // UpdateCheckAllowed should return true because an interactive update request
+ // was signaled.
+
+ SetUpdateCheckAllowed(true);
+ fake_state_.updater_provider()->var_interactive_update_requested()->reset(
+ new bool(true));
+
+ UpdateCheckParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded,
+ &Policy::UpdateCheckAllowed, &result);
+ EXPECT_TRUE(result.updates_enabled);
+ EXPECT_TRUE(result.is_interactive);
+}
+
TEST_F(UmChromeOSPolicyTest, UpdateCanStartFailsCheckAllowedError) {
// The UpdateCanStart policy fails, not being able to query
// UpdateCheckAllowed.
diff --git a/update_manager/default_policy.cc b/update_manager/default_policy.cc
index 2c75d13..91b1bba 100644
--- a/update_manager/default_policy.cc
+++ b/update_manager/default_policy.cc
@@ -24,6 +24,7 @@
UpdateCheckParams* result) const {
result->updates_enabled = true;
result->target_channel.clear();
+ result->is_interactive = false;
// Ensure that the minimum interval is set. If there's no clock, this defaults
// to always allowing the update.
diff --git a/update_manager/fake_updater_provider.h b/update_manager/fake_updater_provider.h
index e4c1866..fa3b869 100644
--- a/update_manager/fake_updater_provider.h
+++ b/update_manager/fake_updater_provider.h
@@ -71,6 +71,10 @@
return &var_server_dictated_poll_interval_;
}
+ virtual FakeVariable<bool>* var_interactive_update_requested() override {
+ return &var_interactive_update_requested_;
+ }
+
private:
FakeVariable<base::Time>
var_updater_started_time_{ // NOLINT(whitespace/braces)
@@ -102,6 +106,9 @@
FakeVariable<unsigned int>
var_server_dictated_poll_interval_{ // NOLINT(whitespace/braces)
"server_dictated_poll_interval", kVariableModePoll};
+ FakeVariable<bool>
+ var_interactive_update_requested_{ // NOLINT(whitespace/braces)
+ "interactive_update_requested", kVariableModeAsync};
DISALLOW_COPY_AND_ASSIGN(FakeUpdaterProvider);
};
diff --git a/update_manager/policy.h b/update_manager/policy.h
index df21fde..9a7d620 100644
--- a/update_manager/policy.h
+++ b/update_manager/policy.h
@@ -32,6 +32,9 @@
//
// A target channel, if so imposed by policy; otherwise, an empty string.
std::string target_channel;
+
+ // Whether the allowed update is interactive (user-initiated) or periodic.
+ bool is_interactive;
};
// Input arguments to UpdateCanStart.
diff --git a/update_manager/real_updater_provider.cc b/update_manager/real_updater_provider.cc
index 47d5dfa..7dae714 100644
--- a/update_manager/real_updater_provider.cc
+++ b/update_manager/real_updater_provider.cc
@@ -8,6 +8,7 @@
#include <string>
+#include <base/bind.h>
#include <base/strings/stringprintf.h>
#include <base/time/time.h>
#include <chromeos/dbus/service_constants.h>
@@ -31,8 +32,9 @@
template<typename T>
class UpdaterVariableBase : public Variable<T> {
public:
- UpdaterVariableBase(const string& name, SystemState* system_state)
- : Variable<T>(name, kVariableModePoll), system_state_(system_state) {}
+ UpdaterVariableBase(const string& name, VariableMode mode,
+ SystemState* system_state)
+ : Variable<T>(name, mode), system_state_(system_state) {}
protected:
// The system state used for pulling information from the updater.
@@ -72,7 +74,8 @@
// A variable reporting the time when a last update check was issued.
class LastCheckedTimeVariable : public UpdaterVariableBase<Time> {
public:
- using UpdaterVariableBase<Time>::UpdaterVariableBase;
+ LastCheckedTimeVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<Time>(name, kVariableModePoll, system_state) {}
private:
virtual const Time* GetValue(TimeDelta /* timeout */,
@@ -91,7 +94,8 @@
// between 0.0 and 1.0.
class ProgressVariable : public UpdaterVariableBase<double> {
public:
- using UpdaterVariableBase<double>::UpdaterVariableBase;
+ ProgressVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<double>(name, kVariableModePoll, system_state) {}
private:
virtual const double* GetValue(TimeDelta /* timeout */,
@@ -117,7 +121,8 @@
// A variable reporting the stage in which the update process is.
class StageVariable : public UpdaterVariableBase<Stage> {
public:
- using UpdaterVariableBase<Stage>::UpdaterVariableBase;
+ StageVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<Stage>(name, kVariableModePoll, system_state) {}
private:
struct CurrOpStrToStage {
@@ -166,7 +171,8 @@
// A variable reporting the version number that an update is updating to.
class NewVersionVariable : public UpdaterVariableBase<string> {
public:
- using UpdaterVariableBase<string>::UpdaterVariableBase;
+ NewVersionVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<string>(name, kVariableModePoll, system_state) {}
private:
virtual const string* GetValue(TimeDelta /* timeout */,
@@ -184,7 +190,8 @@
// A variable reporting the size of the update being processed in bytes.
class PayloadSizeVariable : public UpdaterVariableBase<int64_t> {
public:
- using UpdaterVariableBase<int64_t>::UpdaterVariableBase;
+ PayloadSizeVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<int64_t>(name, kVariableModePoll, system_state) {}
private:
virtual const int64_t* GetValue(TimeDelta /* timeout */,
@@ -214,7 +221,8 @@
// policy request.
class UpdateCompletedTimeVariable : public UpdaterVariableBase<Time> {
public:
- using UpdaterVariableBase<Time>::UpdaterVariableBase;
+ UpdateCompletedTimeVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<Time>(name, kVariableModePoll, system_state) {}
private:
virtual const Time* GetValue(TimeDelta /* timeout */,
@@ -244,7 +252,8 @@
// Variables reporting the current image channel.
class CurrChannelVariable : public UpdaterVariableBase<string> {
public:
- using UpdaterVariableBase<string>::UpdaterVariableBase;
+ CurrChannelVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<string>(name, kVariableModePoll, system_state) {}
private:
virtual const string* GetValue(TimeDelta /* timeout */,
@@ -265,7 +274,8 @@
// Variables reporting the new image channel.
class NewChannelVariable : public UpdaterVariableBase<string> {
public:
- using UpdaterVariableBase<string>::UpdaterVariableBase;
+ NewChannelVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<string>(name, kVariableModePoll, system_state) {}
private:
virtual const string* GetValue(TimeDelta /* timeout */,
@@ -288,7 +298,7 @@
public:
BooleanPrefVariable(const string& name, SystemState* system_state,
const char* key, bool default_val)
- : UpdaterVariableBase<bool>(name, system_state),
+ : UpdaterVariableBase<bool>(name, kVariableModePoll, system_state),
key_(key), default_val_(default_val) {}
private:
@@ -315,7 +325,10 @@
class ConsecutiveFailedUpdateChecksVariable
: public UpdaterVariableBase<unsigned int> {
public:
- using UpdaterVariableBase<unsigned int>::UpdaterVariableBase;
+ ConsecutiveFailedUpdateChecksVariable(const string& name,
+ SystemState* system_state)
+ : UpdaterVariableBase<unsigned int>(name, kVariableModePoll,
+ system_state) {}
private:
virtual const unsigned int* GetValue(TimeDelta /* timeout */,
@@ -331,7 +344,10 @@
class ServerDictatedPollIntervalVariable
: public UpdaterVariableBase<unsigned int> {
public:
- using UpdaterVariableBase<unsigned int>::UpdaterVariableBase;
+ ServerDictatedPollIntervalVariable(const string& name,
+ SystemState* system_state)
+ : UpdaterVariableBase<unsigned int>(name, kVariableModePoll,
+ system_state) {}
private:
virtual const unsigned int* GetValue(TimeDelta /* timeout */,
@@ -343,6 +359,37 @@
DISALLOW_COPY_AND_ASSIGN(ServerDictatedPollIntervalVariable);
};
+// An async variable that tracks changes to interactive update requests.
+class InteractiveUpdateRequestedVariable : public UpdaterVariableBase<bool> {
+ public:
+ InteractiveUpdateRequestedVariable(const string& name,
+ SystemState* system_state)
+ : UpdaterVariableBase<bool>::UpdaterVariableBase(name, kVariableModeAsync,
+ system_state) {
+ system_state->update_attempter()->set_interactive_update_pending_callback(
+ new base::Callback<void(bool)>( // NOLINT(readability/function)
+ base::Bind(&InteractiveUpdateRequestedVariable::Reset,
+ base::Unretained(this))));
+ }
+
+ private:
+ virtual const bool* GetValue(TimeDelta /* timeout */,
+ string* /* errmsg */) override {
+ return new bool(interactive_update_requested_);
+ }
+
+ void Reset(bool value) {
+ if (interactive_update_requested_ != value) {
+ interactive_update_requested_ = value;
+ NotifyValueChanged();
+ }
+ }
+
+ bool interactive_update_requested_ = false;
+
+ DISALLOW_COPY_AND_ASSIGN(InteractiveUpdateRequestedVariable);
+};
+
// RealUpdaterProvider methods.
RealUpdaterProvider::RealUpdaterProvider(SystemState* system_state)
@@ -374,6 +421,9 @@
"consecutive_failed_update_checks", system_state_)),
var_server_dictated_poll_interval_(
new ServerDictatedPollIntervalVariable(
- "server_dictated_poll_interval", system_state_)) {}
+ "server_dictated_poll_interval", system_state_)),
+ var_interactive_update_requested_(
+ new InteractiveUpdateRequestedVariable(
+ "interactive_update_requested", system_state_)) {}
} // namespace chromeos_update_manager
diff --git a/update_manager/real_updater_provider.h b/update_manager/real_updater_provider.h
index 2bc30ae..6a3bc19 100644
--- a/update_manager/real_updater_provider.h
+++ b/update_manager/real_updater_provider.h
@@ -83,6 +83,10 @@
return var_server_dictated_poll_interval_.get();
}
+ virtual Variable<bool>* var_interactive_update_requested() override {
+ return var_interactive_update_requested_.get();
+ }
+
private:
// A pointer to the update engine's system state aggregator.
chromeos_update_engine::SystemState* system_state_;
@@ -101,6 +105,7 @@
scoped_ptr<Variable<bool>> var_cellular_enabled_;
scoped_ptr<Variable<unsigned int>> var_consecutive_failed_update_checks_;
scoped_ptr<Variable<unsigned int>> var_server_dictated_poll_interval_;
+ scoped_ptr<Variable<bool>> var_interactive_update_requested_;
DISALLOW_COPY_AND_ASSIGN(RealUpdaterProvider);
};
diff --git a/update_manager/update_manager_unittest.cc b/update_manager/update_manager_unittest.cc
index 048a0d9..32fd5ac 100644
--- a/update_manager/update_manager_unittest.cc
+++ b/update_manager/update_manager_unittest.cc
@@ -198,8 +198,8 @@
umut_->set_policy(new FailingPolicy());
vector<pair<EvalStatus, UpdateCheckParams>> calls;
- Callback<void(EvalStatus, const UpdateCheckParams& result)> callback =
- Bind(AccumulateCallsCallback<UpdateCheckParams>, &calls);
+ Callback<void(EvalStatus, const UpdateCheckParams&)> callback = Bind(
+ AccumulateCallsCallback<UpdateCheckParams>, &calls);
umut_->AsyncPolicyRequest(callback, base::TimeDelta::FromSeconds(5),
&Policy::UpdateCheckAllowed);
diff --git a/update_manager/updater_provider.h b/update_manager/updater_provider.h
index f7a7648..ec1ca8a 100644
--- a/update_manager/updater_provider.h
+++ b/update_manager/updater_provider.h
@@ -82,6 +82,10 @@
// A server-dictated update check interval in seconds, if one was given.
virtual Variable<unsigned int>* var_server_dictated_poll_interval() = 0;
+ // A variable denoting whether a request for an interactive update was
+ // received but no update check performed yet.
+ virtual Variable<bool>* var_interactive_update_requested() = 0;
+
protected:
UpdaterProvider() {}