update_engine: Providing testing capability for periodic update checks
Currently, we are not able to properly test periodic update checks
because these update checks are disabled on test images. To solve this
problem this CL introduces a new pref test-update-check-interval-timeout
that contains the number of seconds between periodic update checks. The
tests can put this file in /var/lib/update_engine/prefs and restart the
update_engine. The update_engine should start checking for update after
the number of seconds identified in the above pref and continue checking
for update with that interval. The tests also need to make sure this
file is deleted at the end so it doesn't interfere with future device
updates. This pref internally is deleted after it has been read/used 3
times so it can't be abused. For the same reason, the maximum value that
can be set in the pref is limited to 10 minutes.
BUG=chromium:953471
TEST=FEATURES=test emerge-reef update_engine
TEST=flashed a device with this new image, put the pref with value of 10
seconds and restarted the update_engine, the update check happened.
Change-Id: I3ad0e300f7908f17da26b0eb0d1510348a2d2435
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2333308
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Andrew Lassalle <andrewlassalle@chromium.org>
diff --git a/update_manager/chromeos_policy_unittest.cc b/update_manager/chromeos_policy_unittest.cc
index f4ad165..996db2b 100644
--- a/update_manager/chromeos_policy_unittest.cc
+++ b/update_manager/chromeos_policy_unittest.cc
@@ -341,6 +341,26 @@
EvalStatus::kAskMeAgainLater, &Policy::UpdateCheckAllowed, &result);
}
+TEST_F(UmChromeOSPolicyTest, TestUpdateCheckIntervalTimeout) {
+ fake_state_.updater_provider()
+ ->var_test_update_check_interval_timeout()
+ ->reset(new int64_t(10));
+ fake_state_.system_provider()->var_is_official_build()->reset(
+ new bool(false));
+
+ // The first time, update should not be allowed.
+ UpdateCheckParams result;
+ ExpectPolicyStatus(
+ EvalStatus::kAskMeAgainLater, &Policy::UpdateCheckAllowed, &result);
+
+ // After moving the time forward more than the update check interval, it
+ // should now allow for update.
+ fake_clock_.SetWallclockTime(fake_clock_.GetWallclockTime() +
+ TimeDelta::FromSeconds(11));
+ ExpectPolicyStatus(
+ EvalStatus::kSucceeded, &Policy::UpdateCheckAllowed, &result);
+}
+
TEST_F(UmChromeOSPolicyTest,
UpdateCheckAllowedUpdatesDisabledWhenNotEnoughSlotsAbUpdates) {
// UpdateCheckAllowed should return false (kSucceeded) if the image booted
diff --git a/update_manager/evaluation_context-inl.h b/update_manager/evaluation_context-inl.h
index 59d85da..82861fa 100644
--- a/update_manager/evaluation_context-inl.h
+++ b/update_manager/evaluation_context-inl.h
@@ -39,7 +39,7 @@
std::string errmsg;
const T* result =
var->GetValue(RemainingTime(evaluation_monotonic_deadline_), &errmsg);
- if (result == nullptr) {
+ if (result == nullptr && !var->IsMissingOk()) {
LOG(WARNING) << "Error reading Variable " << var->GetName() << ": \""
<< errmsg << "\"";
}
diff --git a/update_manager/fake_updater_provider.h b/update_manager/fake_updater_provider.h
index 7295765..d967f42 100644
--- a/update_manager/fake_updater_provider.h
+++ b/update_manager/fake_updater_provider.h
@@ -83,6 +83,10 @@
return &var_update_restrictions_;
}
+ FakeVariable<int64_t>* var_test_update_check_interval_timeout() override {
+ return &var_test_update_check_interval_timeout_;
+ }
+
private:
FakeVariable<base::Time> var_updater_started_time_{"updater_started_time",
kVariableModePoll};
@@ -108,6 +112,8 @@
"forced_update_requested", kVariableModeAsync};
FakeVariable<UpdateRestrictions> var_update_restrictions_{
"update_restrictions", kVariableModePoll};
+ FakeVariable<int64_t> var_test_update_check_interval_timeout_{
+ "test_update_check_interval_timeout", kVariableModePoll};
DISALLOW_COPY_AND_ASSIGN(FakeUpdaterProvider);
};
diff --git a/update_manager/next_update_check_policy_impl.cc b/update_manager/next_update_check_policy_impl.cc
index 6f9748e..0a78718 100644
--- a/update_manager/next_update_check_policy_impl.cc
+++ b/update_manager/next_update_check_policy_impl.cc
@@ -72,6 +72,11 @@
ec->GetValue(updater_provider->var_updater_started_time());
POLICY_CHECK_VALUE_AND_FAIL(updater_started_time, error);
+ // This value is used for testing only and it will get deleted after the first
+ // time it is read.
+ const int64_t* interval_timeout =
+ ec->GetValue(updater_provider->var_test_update_check_interval_timeout());
+
const Time* last_checked_time =
ec->GetValue(updater_provider->var_last_checked_time());
@@ -83,13 +88,21 @@
// If this is the first attempt, compute and return an initial value.
if (last_checked_time == nullptr ||
*last_checked_time < *updater_started_time) {
- *next_update_check = *updater_started_time +
- FuzzedInterval(&prng,
- constants.timeout_initial_interval,
- constants.timeout_regular_fuzz);
+ TimeDelta time_diff =
+ interval_timeout == nullptr
+ ? FuzzedInterval(&prng,
+ constants.timeout_initial_interval,
+ constants.timeout_regular_fuzz)
+ : TimeDelta::FromSeconds(*interval_timeout);
+ *next_update_check = *updater_started_time + time_diff;
return EvalStatus::kSucceeded;
}
+ if (interval_timeout != nullptr) {
+ *next_update_check =
+ *last_checked_time + TimeDelta::FromSeconds(*interval_timeout);
+ return EvalStatus::kSucceeded;
+ }
// Check whether the server is enforcing a poll interval; if not, this value
// will be zero.
const unsigned int* server_dictated_poll_interval =
diff --git a/update_manager/official_build_check_policy_impl.cc b/update_manager/official_build_check_policy_impl.cc
index 096f7bf..e80c09f 100644
--- a/update_manager/official_build_check_policy_impl.cc
+++ b/update_manager/official_build_check_policy_impl.cc
@@ -27,8 +27,16 @@
const bool* is_official_build_p =
ec->GetValue(state->system_provider()->var_is_official_build());
if (is_official_build_p != nullptr && !(*is_official_build_p)) {
- LOG(INFO) << "Unofficial build, blocking periodic update checks.";
- return EvalStatus::kAskMeAgainLater;
+ const int64_t* interval_timeout_p = ec->GetValue(
+ state->updater_provider()->var_test_update_check_interval_timeout());
+ // The |interval_timeout | is used for testing only to test periodic
+ // update checks on unofficial images.
+ if (interval_timeout_p == nullptr) {
+ LOG(INFO) << "Unofficial build, blocking periodic update checks.";
+ return EvalStatus::kAskMeAgainLater;
+ }
+ LOG(INFO) << "Unofficial build, but periodic update check interval "
+ << "timeout is defined, so update is not blocked.";
}
return EvalStatus::kContinue;
}
diff --git a/update_manager/real_updater_provider.cc b/update_manager/real_updater_provider.cc
index 134db69..268f3bb 100644
--- a/update_manager/real_updater_provider.cc
+++ b/update_manager/real_updater_provider.cc
@@ -18,6 +18,7 @@
#include <inttypes.h>
+#include <algorithm>
#include <string>
#include <base/bind.h>
@@ -439,6 +440,46 @@
DISALLOW_COPY_AND_ASSIGN(UpdateRestrictionsVariable);
};
+// A variable class for reading timeout interval prefs value.
+class TestUpdateCheckIntervalTimeoutVariable : public Variable<int64_t> {
+ public:
+ TestUpdateCheckIntervalTimeoutVariable(
+ const string& name, chromeos_update_engine::PrefsInterface* prefs)
+ : Variable<int64_t>(name, kVariableModePoll),
+ prefs_(prefs),
+ read_count_(0) {
+ SetMissingOk();
+ }
+ ~TestUpdateCheckIntervalTimeoutVariable() = default;
+
+ private:
+ const int64_t* GetValue(TimeDelta /* timeout */,
+ string* /* errmsg */) override {
+ auto key = chromeos_update_engine::kPrefsTestUpdateCheckIntervalTimeout;
+ int64_t result;
+ if (prefs_ && prefs_->Exists(key) && prefs_->GetInt64(key, &result)) {
+ // This specific value is used for testing only. So it should not be kept
+ // around and should be deleted after a few reads.
+ if (++read_count_ > 2)
+ prefs_->Delete(key);
+
+ // Limit the timeout interval to 10 minutes so it is not abused if it is
+ // seen on official images.
+ return new int64_t(std::min(result, static_cast<int64_t>(10 * 60)));
+ }
+ return nullptr;
+ }
+
+ chromeos_update_engine::PrefsInterface* prefs_;
+
+ // Counts how many times this variable is read. This is used to delete the
+ // underlying file defining the variable after a certain number of reads in
+ // order to prevent any abuse of this variable.
+ int read_count_;
+
+ DISALLOW_COPY_AND_ASSIGN(TestUpdateCheckIntervalTimeoutVariable);
+};
+
// RealUpdaterProvider methods.
RealUpdaterProvider::RealUpdaterProvider(SystemState* system_state)
@@ -472,6 +513,9 @@
"server_dictated_poll_interval", system_state_)),
var_forced_update_requested_(new ForcedUpdateRequestedVariable(
"forced_update_requested", system_state_)),
- var_update_restrictions_(new UpdateRestrictionsVariable(
- "update_restrictions", system_state_)) {}
+ var_update_restrictions_(
+ new UpdateRestrictionsVariable("update_restrictions", system_state_)),
+ var_test_update_check_interval_timeout_(
+ new TestUpdateCheckIntervalTimeoutVariable(
+ "test_update_check_interval_timeout", system_state_->prefs())) {}
} // namespace chromeos_update_manager
diff --git a/update_manager/real_updater_provider.h b/update_manager/real_updater_provider.h
index 1b46895..0819357 100644
--- a/update_manager/real_updater_provider.h
+++ b/update_manager/real_updater_provider.h
@@ -94,6 +94,10 @@
return var_update_restrictions_.get();
}
+ Variable<int64_t>* var_test_update_check_interval_timeout() override {
+ return var_test_update_check_interval_timeout_.get();
+ }
+
private:
// A pointer to the update engine's system state aggregator.
chromeos_update_engine::SystemState* system_state_;
@@ -114,6 +118,7 @@
std::unique_ptr<Variable<unsigned int>> var_server_dictated_poll_interval_;
std::unique_ptr<Variable<UpdateRequestStatus>> var_forced_update_requested_;
std::unique_ptr<Variable<UpdateRestrictions>> var_update_restrictions_;
+ std::unique_ptr<Variable<int64_t>> var_test_update_check_interval_timeout_;
DISALLOW_COPY_AND_ASSIGN(RealUpdaterProvider);
};
diff --git a/update_manager/real_updater_provider_unittest.cc b/update_manager/real_updater_provider_unittest.cc
index fb7a763..e31f6f3 100644
--- a/update_manager/real_updater_provider_unittest.cc
+++ b/update_manager/real_updater_provider_unittest.cc
@@ -445,4 +445,30 @@
UmTestUtils::ExpectVariableHasValue(UpdateRestrictions::kNone,
provider_->var_update_restrictions());
}
+
+TEST_F(UmRealUpdaterProviderTest, TestUpdateCheckIntervalTimeout) {
+ UmTestUtils::ExpectVariableNotSet(
+ provider_->var_test_update_check_interval_timeout());
+ fake_prefs_.SetInt64(
+ chromeos_update_engine::kPrefsTestUpdateCheckIntervalTimeout, 1);
+ UmTestUtils::ExpectVariableHasValue(
+ static_cast<int64_t>(1),
+ provider_->var_test_update_check_interval_timeout());
+
+ // Make sure the value does not exceed a threshold of 10 minutes.
+ fake_prefs_.SetInt64(
+ chromeos_update_engine::kPrefsTestUpdateCheckIntervalTimeout, 11 * 60);
+ UmTestUtils::ExpectVariableHasValue(
+ static_cast<int64_t>(10 * 60),
+ provider_->var_test_update_check_interval_timeout());
+ UmTestUtils::ExpectVariableHasValue(
+ static_cast<int64_t>(10 * 60),
+ provider_->var_test_update_check_interval_timeout());
+
+ // Just to make sure it is not cached anywhere and deleted. The variable is
+ // allowd to be read 3 times.
+ UmTestUtils::ExpectVariableNotSet(
+ provider_->var_test_update_check_interval_timeout());
+}
+
} // namespace chromeos_update_manager
diff --git a/update_manager/updater_provider.h b/update_manager/updater_provider.h
index cb62623..98fd6d1 100644
--- a/update_manager/updater_provider.h
+++ b/update_manager/updater_provider.h
@@ -115,6 +115,10 @@
// for all updates.
virtual Variable<UpdateRestrictions>* var_update_restrictions() = 0;
+ // A variable that returns the number of seconds for the first update check to
+ // happen.
+ virtual Variable<int64_t>* var_test_update_check_interval_timeout() = 0;
+
protected:
UpdaterProvider() {}
diff --git a/update_manager/variable.h b/update_manager/variable.h
index 6c7d350..9ac7dae 100644
--- a/update_manager/variable.h
+++ b/update_manager/variable.h
@@ -83,6 +83,10 @@
// variable. In other case, it returns 0.
base::TimeDelta GetPollInterval() const { return poll_interval_; }
+ // Returns true, if the value for this variable is expected to be missing
+ // sometimes so we can avoid printing confusing error logs.
+ bool IsMissingOk() const { return missing_ok_; }
+
// Adds and removes observers for value changes on the variable. This only
// works for kVariableAsync variables since the other modes don't track value
// changes. Adding the same observer twice has no effect.
@@ -115,6 +119,8 @@
poll_interval_ = poll_interval;
}
+ void SetMissingOk() { missing_ok_ = true; }
+
// Calls ValueChanged on all the observers.
void NotifyValueChanged() {
// Fire all the observer methods from the main loop as single call. In order
@@ -140,7 +146,8 @@
: name_(name),
mode_(mode),
poll_interval_(mode == kVariableModePoll ? poll_interval
- : base::TimeDelta()) {}
+ : base::TimeDelta()),
+ missing_ok_(false) {}
void OnValueChangedNotification() {
// A ValueChanged() method can change the list of observers, for example
@@ -174,6 +181,9 @@
// The list of value changes observers.
std::list<BaseVariable::ObserverInterface*> observer_list_;
+ // Defines whether this variable is expected to have no value.
+ bool missing_ok_;
+
DISALLOW_COPY_AND_ASSIGN(BaseVariable);
};