update_engine: Ditch UpdateCheckScheduler, use UpdateCheckAllowed instead.
This change removes the update_check_scheduler module and replaces it
with async requests to the UpdateCheckAllowed policy, done by the
UpdateAttempter directly.
* A new UpdateAttempter::ScheduleUpdates() is used as a replacement for
UpdateCheckScheduler::Run() and rescheduling of periodic checks inside
UpdateCheckScheduler. The callback
UpdateAttempter::OnUpdateScheduled() handles both periodic and
interactive checks.
* The UpdateAttempter keeps track of whether or not an update check is
being waited for (waiting_for_scheduled_check_) so that we can ensure
liveness. This is a similar check to the one performed inside the
UpdateCheckScheduler.
* Inference of the update target version prefix and channel (via device
policy), as well as update disabled, are now performed by the
UpdateManager policy. Also eliminating reference to the list of
network types allowed by policy, which is not enforced anyway and will
be superceded by another policy request (UpdateDownloadAllowed).
* Since update check scheduling is now performed relative to the last
update check time (as recorded by the UpdateAttempter), we care to
update this time as soon as the request is issued (in addition to when
a response is received). This ensures that we won't be scheduling
back-to-back update requests in the case where a response was not
received. Updating the last check time is delegated to a method call;
we replace raw use of time(2) with the ClockInterface abstraction.
* Handling of forced update checks has been revised: the UpdateAttempter
keeps track of the most recent app_version and omaha_url values that
were received through DBus events; it notifies the UpdateManager not
only of whether or not a forced (formerly, "interactive") update
request is pending, but also whether or not it is indeed interactive
or should be treated as a normal periodic one. The UpdateManager
reflects this back to the updater via the result output of
UpdateCheckAllowed, which tells the UpdateManager whether the custom
app_version and omaha_url should be used (interactive) or not.
BUG=chromium:358269
TEST=Unit tests.
Change-Id: Ifa9857b98e58fdd974f91a0fec674fa4472e3a9d
Reviewed-on: https://chromium-review.googlesource.com/209101
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/update_manager/boxed_value.cc b/update_manager/boxed_value.cc
index 56b42b8..773431c 100644
--- a/update_manager/boxed_value.cc
+++ b/update_manager/boxed_value.cc
@@ -24,56 +24,56 @@
// Keep in sync with boxed_value_unitttest.cc.
template<>
-string BoxedValue::ValuePrinter<string>(const void *value) {
+string BoxedValue::ValuePrinter<string>(const void* value) {
const string* val = reinterpret_cast<const string*>(value);
return *val;
}
template<>
-string BoxedValue::ValuePrinter<int>(const void *value) {
+string BoxedValue::ValuePrinter<int>(const void* value) {
const int* val = reinterpret_cast<const int*>(value);
return base::IntToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<unsigned int>(const void *value) {
+string BoxedValue::ValuePrinter<unsigned int>(const void* value) {
const unsigned int* val = reinterpret_cast<const unsigned int*>(value);
return base::UintToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<int64_t>(const void *value) {
+string BoxedValue::ValuePrinter<int64_t>(const void* value) {
const int64_t* val = reinterpret_cast<const int64_t*>(value);
return base::Int64ToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<uint64_t>(const void *value) {
+string BoxedValue::ValuePrinter<uint64_t>(const void* value) {
const uint64_t* val =
reinterpret_cast<const uint64_t*>(value);
return base::Uint64ToString(static_cast<uint64_t>(*val));
}
template<>
-string BoxedValue::ValuePrinter<bool>(const void *value) {
+string BoxedValue::ValuePrinter<bool>(const void* value) {
const bool* val = reinterpret_cast<const bool*>(value);
return *val ? "true" : "false";
}
template<>
-string BoxedValue::ValuePrinter<double>(const void *value) {
+string BoxedValue::ValuePrinter<double>(const void* value) {
const double* val = reinterpret_cast<const double*>(value);
return base::DoubleToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<base::Time>(const void *value) {
+string BoxedValue::ValuePrinter<base::Time>(const void* value) {
const base::Time* val = reinterpret_cast<const base::Time*>(value);
return chromeos_update_engine::utils::ToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<base::TimeDelta>(const void *value) {
+string BoxedValue::ValuePrinter<base::TimeDelta>(const void* value) {
const base::TimeDelta* val = reinterpret_cast<const base::TimeDelta*>(value);
return chromeos_update_engine::utils::FormatTimeDelta(*val);
}
@@ -98,13 +98,13 @@
}
template<>
-string BoxedValue::ValuePrinter<ConnectionType>(const void *value) {
+string BoxedValue::ValuePrinter<ConnectionType>(const void* value) {
const ConnectionType* val = reinterpret_cast<const ConnectionType*>(value);
return ConnectionTypeToString(*val);
}
template<>
-string BoxedValue::ValuePrinter<set<ConnectionType>>(const void *value) {
+string BoxedValue::ValuePrinter<set<ConnectionType>>(const void* value) {
string ret = "";
const set<ConnectionType>* val =
reinterpret_cast<const set<ConnectionType>*>(value);
@@ -118,7 +118,7 @@
}
template<>
-string BoxedValue::ValuePrinter<ConnectionTethering>(const void *value) {
+string BoxedValue::ValuePrinter<ConnectionTethering>(const void* value) {
const ConnectionTethering* val =
reinterpret_cast<const ConnectionTethering*>(value);
switch (*val) {
@@ -136,7 +136,7 @@
}
template<>
-string BoxedValue::ValuePrinter<Stage>(const void *value) {
+string BoxedValue::ValuePrinter<Stage>(const void* value) {
const Stage* val = reinterpret_cast<const Stage*>(value);
switch (*val) {
case Stage::kIdle:
@@ -162,4 +162,20 @@
return "Unknown";
}
+template<>
+string BoxedValue::ValuePrinter<UpdateRequestStatus>(const void* value) {
+ const UpdateRequestStatus* val =
+ reinterpret_cast<const UpdateRequestStatus*>(value);
+ switch (*val) {
+ case UpdateRequestStatus::kNone:
+ return "None";
+ case UpdateRequestStatus::kInteractive:
+ return "Interactive";
+ case UpdateRequestStatus::kPeriodic:
+ return "Periodic";
+ }
+ NOTREACHED();
+ return "Unknown";
+}
+
} // namespace chromeos_update_manager
diff --git a/update_manager/boxed_value_unittest.cc b/update_manager/boxed_value_unittest.cc
index dff6738..77b44c2 100644
--- a/update_manager/boxed_value_unittest.cc
+++ b/update_manager/boxed_value_unittest.cc
@@ -74,7 +74,7 @@
TEST(UmBoxedValueTest, MixedList) {
list<BoxedValue> lst;
// This is mostly a compile test.
- lst.emplace_back(new const int(42)); // NOLINT(readability/casting)
+ lst.emplace_back(new const int{42});
lst.emplace_back(new const string("Hello world!"));
bool marker;
lst.emplace_back(new const DeleterMarker(&marker));
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index 0dc0ad6..67799ce 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -168,6 +168,7 @@
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) {
+ LOG(INFO) << "Booted from removable device, disabling update checks.";
result->updates_enabled = false;
return EvalStatus::kSucceeded;
}
@@ -178,8 +179,10 @@
// Check whether updates are disabled by policy.
const bool* update_disabled_p = ec->GetValue(
dp_provider->var_update_disabled());
- if (update_disabled_p && *update_disabled_p)
+ if (update_disabled_p && *update_disabled_p) {
+ LOG(INFO) << "Updates disabled by policy, blocking update checks.";
return EvalStatus::kAskMeAgainLater;
+ }
// Determine whether a target version prefix is dictated by policy.
const string* target_version_prefix_p = ec->GetValue(
@@ -199,10 +202,15 @@
}
// 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;
+ const UpdateRequestStatus* forced_update_requested_p = ec->GetValue(
+ updater_provider->var_forced_update_requested());
+ if (forced_update_requested_p &&
+ *forced_update_requested_p != UpdateRequestStatus::kNone) {
+ result->is_interactive =
+ (*forced_update_requested_p == UpdateRequestStatus::kInteractive);
+ LOG(INFO) << "Forced update signaled ("
+ << (result->is_interactive ? "interactive" : "periodic")
+ << "), allowing update check.";
return EvalStatus::kSucceeded;
}
@@ -214,6 +222,7 @@
const bool* is_official_build_p = ec->GetValue(
system_provider->var_is_official_build());
if (is_official_build_p && !(*is_official_build_p)) {
+ LOG(INFO) << "Unofficial build, blocking periodic update checks.";
return EvalStatus::kAskMeAgainLater;
}
@@ -223,8 +232,10 @@
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))
+ if (is_oobe_complete_p && !(*is_oobe_complete_p)) {
+ LOG(INFO) << "OOBE not completed, blocking update checks.";
return EvalStatus::kAskMeAgainLater;
+ }
}
// Ensure that periodic update checks are timed properly.
@@ -233,10 +244,14 @@
EvalStatus::kSucceeded) {
return EvalStatus::kFailed;
}
- if (!ec->IsWallclockTimeGreaterThan(next_update_check))
+ if (!ec->IsWallclockTimeGreaterThan(next_update_check)) {
+ LOG(INFO) << "Periodic check interval not satisfied, blocking until "
+ << chromeos_update_engine::utils::ToString(next_update_check);
return EvalStatus::kAskMeAgainLater;
+ }
// It is time to check for an update.
+ LOG(INFO) << "Allowing update check.";
return EvalStatus::kSucceeded;
}
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index d231a7c..25a091e 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -47,11 +47,11 @@
fake_state_.updater_provider()->var_last_checked_time()->reset(
new Time(fake_clock_.GetWallclockTime()));
fake_state_.updater_provider()->var_consecutive_failed_update_checks()->
- reset(new unsigned int(0)); // NOLINT(readability/casting)
+ reset(new unsigned int{0});
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)
+ reset(new unsigned int{0});
+ fake_state_.updater_provider()->var_forced_update_requested()->
+ reset(new UpdateRequestStatus{UpdateRequestStatus::kNone});
fake_state_.random_provider()->var_seed()->reset(
new uint64_t(4)); // chosen by fair dice roll.
@@ -199,7 +199,7 @@
Time next_update_check;
fake_state_.updater_provider()->var_consecutive_failed_update_checks()->
- reset(new unsigned int(2)); // NOLINT(readability/casting)
+ reset(new unsigned int{2});
ExpectPolicyStatus(EvalStatus::kSucceeded,
&ChromeOSPolicy::NextUpdateCheckTime, &next_update_check);
@@ -221,10 +221,10 @@
const unsigned int kInterval = ChromeOSPolicy::kTimeoutPeriodicInterval * 4;
fake_state_.updater_provider()->var_server_dictated_poll_interval()->
- reset(new unsigned int(kInterval)); // NOLINT(readability/casting)
+ reset(new unsigned int{kInterval});
// We should not be backing off in this case.
fake_state_.updater_provider()->var_consecutive_failed_update_checks()->
- reset(new unsigned int(2)); // NOLINT(readability/casting)
+ reset(new unsigned int{2});
ExpectPolicyStatus(EvalStatus::kSucceeded,
&ChromeOSPolicy::NextUpdateCheckTime, &next_update_check);
@@ -243,7 +243,7 @@
Time next_update_check;
fake_state_.updater_provider()->var_consecutive_failed_update_checks()->
- reset(new unsigned int(100)); // NOLINT(readability/casting)
+ reset(new unsigned int{100});
ExpectPolicyStatus(EvalStatus::kSucceeded,
&ChromeOSPolicy::NextUpdateCheckTime, &next_update_check);
@@ -394,13 +394,14 @@
&Policy::UpdateCheckAllowed, &result);
}
-TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedInteractiveUpdateRequested) {
- // UpdateCheckAllowed should return true because an interactive update request
- // was signaled.
+TEST_F(UmChromeOSPolicyTest,
+ UpdateCheckAllowedForcedUpdateRequestedInteractive) {
+ // UpdateCheckAllowed should return true because a forced update request was
+ // signaled for an interactive update.
SetUpdateCheckAllowed(true);
- fake_state_.updater_provider()->var_interactive_update_requested()->reset(
- new bool(true));
+ fake_state_.updater_provider()->var_forced_update_requested()->reset(
+ new UpdateRequestStatus(UpdateRequestStatus::kInteractive));
UpdateCheckParams result;
ExpectPolicyStatus(EvalStatus::kSucceeded,
@@ -409,6 +410,21 @@
EXPECT_TRUE(result.is_interactive);
}
+TEST_F(UmChromeOSPolicyTest, UpdateCheckAllowedForcedUpdateRequestedPeriodic) {
+ // UpdateCheckAllowed should return true because a forced update request was
+ // signaled for a periodic check.
+
+ SetUpdateCheckAllowed(true);
+ fake_state_.updater_provider()->var_forced_update_requested()->reset(
+ new UpdateRequestStatus(UpdateRequestStatus::kPeriodic));
+
+ UpdateCheckParams result;
+ ExpectPolicyStatus(EvalStatus::kSucceeded,
+ &Policy::UpdateCheckAllowed, &result);
+ EXPECT_TRUE(result.updates_enabled);
+ EXPECT_FALSE(result.is_interactive);
+}
+
TEST_F(UmChromeOSPolicyTest, UpdateCanStartFailsCheckAllowedError) {
// The UpdateCanStart policy fails, not being able to query
// UpdateCheckAllowed.
diff --git a/update_manager/fake_updater_provider.h b/update_manager/fake_updater_provider.h
index f2295a5..0820858 100644
--- a/update_manager/fake_updater_provider.h
+++ b/update_manager/fake_updater_provider.h
@@ -69,8 +69,8 @@
return &var_server_dictated_poll_interval_;
}
- FakeVariable<bool>* var_interactive_update_requested() override {
- return &var_interactive_update_requested_;
+ FakeVariable<UpdateRequestStatus>* var_forced_update_requested() override {
+ return &var_forced_update_requested_;
}
private:
@@ -104,9 +104,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};
+ FakeVariable<UpdateRequestStatus>
+ var_forced_update_requested_{ // NOLINT(whitespace/braces)
+ "forced_update_requested", kVariableModeAsync};
DISALLOW_COPY_AND_ASSIGN(FakeUpdaterProvider);
};
diff --git a/update_manager/real_updater_provider.cc b/update_manager/real_updater_provider.cc
index 004b51a..ac70746 100644
--- a/update_manager/real_updater_provider.cc
+++ b/update_manager/real_updater_provider.cc
@@ -350,35 +350,39 @@
DISALLOW_COPY_AND_ASSIGN(ServerDictatedPollIntervalVariable);
};
-// An async variable that tracks changes to interactive update requests.
-class InteractiveUpdateRequestedVariable : public UpdaterVariableBase<bool> {
+// An async variable that tracks changes to forced update requests.
+class ForcedUpdateRequestedVariable
+ : public UpdaterVariableBase<UpdateRequestStatus> {
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,
+ ForcedUpdateRequestedVariable(const string& name, SystemState* system_state)
+ : UpdaterVariableBase<UpdateRequestStatus>::UpdaterVariableBase(
+ name, kVariableModeAsync, system_state) {
+ system_state->update_attempter()->set_forced_update_pending_callback(
+ new base::Callback<void(bool, bool)>( // NOLINT(readability/function)
+ base::Bind(&ForcedUpdateRequestedVariable::Reset,
base::Unretained(this))));
}
private:
- const bool* GetValue(TimeDelta /* timeout */,
- string* /* errmsg */) override {
- return new bool(interactive_update_requested_);
+ const UpdateRequestStatus* GetValue(TimeDelta /* timeout */,
+ string* /* errmsg */) override {
+ return new UpdateRequestStatus(update_request_status_);
}
- void Reset(bool value) {
- if (interactive_update_requested_ != value) {
- interactive_update_requested_ = value;
+ void Reset(bool forced_update_requested, bool is_interactive) {
+ UpdateRequestStatus new_value = UpdateRequestStatus::kNone;
+ if (forced_update_requested)
+ new_value = (is_interactive ? UpdateRequestStatus::kInteractive :
+ UpdateRequestStatus::kPeriodic);
+ if (update_request_status_ != new_value) {
+ update_request_status_ = new_value;
NotifyValueChanged();
}
}
- bool interactive_update_requested_ = false;
+ UpdateRequestStatus update_request_status_ = UpdateRequestStatus::kNone;
- DISALLOW_COPY_AND_ASSIGN(InteractiveUpdateRequestedVariable);
+ DISALLOW_COPY_AND_ASSIGN(ForcedUpdateRequestedVariable);
};
// RealUpdaterProvider methods.
@@ -413,8 +417,8 @@
var_server_dictated_poll_interval_(
new ServerDictatedPollIntervalVariable(
"server_dictated_poll_interval", system_state_)),
- var_interactive_update_requested_(
- new InteractiveUpdateRequestedVariable(
- "interactive_update_requested", system_state_)) {}
+ var_forced_update_requested_(
+ new ForcedUpdateRequestedVariable(
+ "forced_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 5c40acd..c61032e 100644
--- a/update_manager/real_updater_provider.h
+++ b/update_manager/real_updater_provider.h
@@ -81,8 +81,8 @@
return var_server_dictated_poll_interval_.get();
}
- Variable<bool>* var_interactive_update_requested() override {
- return var_interactive_update_requested_.get();
+ Variable<UpdateRequestStatus>* var_forced_update_requested() override {
+ return var_forced_update_requested_.get();
}
private:
@@ -103,7 +103,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_;
+ scoped_ptr<Variable<UpdateRequestStatus>> var_forced_update_requested_;
DISALLOW_COPY_AND_ASSIGN(RealUpdaterProvider);
};
diff --git a/update_manager/updater_provider.h b/update_manager/updater_provider.h
index ec1ca8a..535b3e7 100644
--- a/update_manager/updater_provider.h
+++ b/update_manager/updater_provider.h
@@ -26,6 +26,12 @@
kAttemptingRollback,
};
+enum class UpdateRequestStatus {
+ kNone,
+ kInteractive,
+ kPeriodic,
+};
+
// Provider for Chrome OS update related information.
class UpdaterProvider : public Provider {
public:
@@ -82,9 +88,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;
+ // A variable denoting whether a forced update was request but no update check
+ // performed yet; also tells whether this request is for an interactive or
+ // scheduled update.
+ virtual Variable<UpdateRequestStatus>* var_forced_update_requested() = 0;
protected:
UpdaterProvider() {}