Base the update complete marker on persisted data.
The update complete marker was stored in /var/run, a fixed volatile
location. The marker would signal that an update was already applied
even after an update_engine crash and subsequent restart.
This location, while quite standard on the Unix FHS, is not
available in Android. This patch achieves the same goal by storing the
boot_id in the persisted prefs directory.
Bug: 24868648
Test: Unittests. Restarted update_engine after an update, keeps saying NEED_REBOOT.
Change-Id: I4dc2cbaeaeb0fd3197fa89168deaa042cb776d61
diff --git a/constants.cc b/constants.cc
index d724564..c44e615 100644
--- a/constants.cc
+++ b/constants.cc
@@ -72,6 +72,8 @@
const char kPrefsTotalBytesDownloaded[] = "total-bytes-downloaded";
const char kPrefsUpdateCheckCount[] = "update-check-count";
const char kPrefsUpdateCheckResponseHash[] = "update-check-response-hash";
+const char kPrefsUpdateCompletedBootTime[] = "update-completed-boot-time";
+const char kPrefsUpdateCompletedOnBootId[] = "update-completed-on-boot-id";
const char kPrefsUpdateDurationUptime[] = "update-duration-uptime";
const char kPrefsUpdateFirstSeenAt[] = "update-first-seen-at";
const char kPrefsUpdateOverCellularPermission[] =
diff --git a/constants.h b/constants.h
index 211dc96..a22b98e 100644
--- a/constants.h
+++ b/constants.h
@@ -73,6 +73,8 @@
extern const char kPrefsTotalBytesDownloaded[];
extern const char kPrefsUpdateCheckCount[];
extern const char kPrefsUpdateCheckResponseHash[];
+extern const char kPrefsUpdateCompletedBootTime[];
+extern const char kPrefsUpdateCompletedOnBootId[];
extern const char kPrefsUpdateDurationUptime[];
extern const char kPrefsUpdateFirstSeenAt[];
extern const char kPrefsUpdateOverCellularPermission[];
diff --git a/update_attempter.cc b/update_attempter.cc
index 6ae0f94..1610ec4 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -69,7 +69,6 @@
using base::Bind;
using base::Callback;
-using base::StringPrintf;
using base::Time;
using base::TimeDelta;
using base::TimeTicks;
@@ -89,9 +88,6 @@
namespace {
const int kMaxConsecutiveObeyProxyRequests = 20;
-const char kUpdateCompletedMarker[] =
- "/var/run/update_engine_autoupdate_completed";
-
// By default autest bypasses scattering. If we want to test scattering,
// use kScheduledAUTestURLRequest. The URL used is same in both cases, but
// different params are passed to CheckForUpdate().
@@ -125,25 +121,10 @@
SystemState* system_state,
LibCrosProxy* libcros_proxy,
org::chromium::debugdProxyInterface* debugd_proxy)
- : UpdateAttempter(system_state, libcros_proxy, debugd_proxy,
- kUpdateCompletedMarker) {}
-
-UpdateAttempter::UpdateAttempter(
- SystemState* system_state,
- LibCrosProxy* libcros_proxy,
- org::chromium::debugdProxyInterface* debugd_proxy,
- const string& update_completed_marker)
: processor_(new ActionProcessor()),
system_state_(system_state),
chrome_proxy_resolver_(libcros_proxy),
- update_completed_marker_(update_completed_marker),
debugd_proxy_(debugd_proxy) {
- if (!update_completed_marker_.empty() &&
- utils::FileExists(update_completed_marker_.c_str())) {
- status_ = UpdateStatus::UPDATED_NEED_REBOOT;
- } else {
- status_ = UpdateStatus::IDLE;
- }
}
UpdateAttempter::~UpdateAttempter() {
@@ -156,6 +137,13 @@
// which requires them all to be constructed prior to it being used.
prefs_ = system_state_->prefs();
omaha_request_params_ = system_state_->request_params();
+
+ // In case of update_engine restart without a reboot we need to restore the
+ // reboot needed state.
+ if (GetBootTimeAtUpdate(nullptr))
+ status_ = UpdateStatus::UPDATED_NEED_REBOOT;
+ else
+ status_ = UpdateStatus::IDLE;
}
void UpdateAttempter::ScheduleUpdates() {
@@ -803,15 +791,13 @@
}
void UpdateAttempter::WriteUpdateCompletedMarker() {
- if (update_completed_marker_.empty())
+ string boot_id;
+ if (!utils::GetBootId(&boot_id))
return;
+ prefs_->SetString(kPrefsUpdateCompletedOnBootId, boot_id);
int64_t value = system_state_->clock()->GetBootTime().ToInternalValue();
- string contents = StringPrintf("%" PRIi64, value);
-
- utils::WriteFile(update_completed_marker_.c_str(),
- contents.c_str(),
- contents.length());
+ prefs_->SetInt64(kPrefsUpdateCompletedBootTime, value);
}
bool UpdateAttempter::RequestPowerManagerReboot() {
@@ -1082,15 +1068,12 @@
case UpdateStatus::UPDATED_NEED_REBOOT: {
bool ret_value = true;
status_ = UpdateStatus::IDLE;
- LOG(INFO) << "Reset Successful";
// Remove the reboot marker so that if the machine is rebooted
// after resetting to idle state, it doesn't go back to
// UpdateStatus::UPDATED_NEED_REBOOT state.
- if (!update_completed_marker_.empty()) {
- if (!base::DeleteFile(base::FilePath(update_completed_marker_), false))
- ret_value = false;
- }
+ ret_value = prefs_->Delete(kPrefsUpdateCompletedOnBootId) && ret_value;
+ ret_value = prefs_->Delete(kPrefsUpdateCompletedBootTime) && ret_value;
// Update the boot flags so the current slot has higher priority.
BootControlInterface* boot_control = system_state_->boot_control();
@@ -1100,6 +1083,7 @@
// Notify the PayloadState that the successful payload was canceled.
system_state_->payload_state()->ResetUpdateStatus();
+ LOG(INFO) << "Reset status " << (ret_value ? "successful" : "failed");
return ret_value;
}
@@ -1515,22 +1499,28 @@
}
bool UpdateAttempter::GetBootTimeAtUpdate(Time *out_boot_time) {
- if (update_completed_marker_.empty())
+ // In case of an update_engine restart without a reboot, we stored the boot_id
+ // when the update was completed by setting a pref, so we can check whether
+ // the last update was on this boot or a previous one.
+ string boot_id;
+ TEST_AND_RETURN_FALSE(utils::GetBootId(&boot_id));
+
+ string update_completed_on_boot_id;
+ if (!prefs_->Exists(kPrefsUpdateCompletedOnBootId) ||
+ !prefs_->GetString(kPrefsUpdateCompletedOnBootId,
+ &update_completed_on_boot_id) ||
+ update_completed_on_boot_id != boot_id)
return false;
- string contents;
- if (!utils::ReadFile(update_completed_marker_, &contents))
- return false;
-
- char *endp;
- int64_t stored_value = strtoll(contents.c_str(), &endp, 10);
- if (*endp != '\0') {
- LOG(ERROR) << "Error parsing file " << update_completed_marker_ << " "
- << "with content '" << contents << "'";
- return false;
+ // Short-circuit avoiding the read in case out_boot_time is nullptr.
+ if (out_boot_time) {
+ int64_t boot_time = 0;
+ // Since the kPrefsUpdateCompletedOnBootId was correctly set, this pref
+ // should not fail.
+ TEST_AND_RETURN_FALSE(
+ prefs_->GetInt64(kPrefsUpdateCompletedBootTime, &boot_time));
+ *out_boot_time = Time::FromInternalValue(boot_time);
}
-
- *out_boot_time = Time::FromInternalValue(stored_value);
return true;
}
diff --git a/update_attempter.h b/update_attempter.h
index 7a5e4b7..fa2c18a 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -31,6 +31,7 @@
#include "debugd/dbus-proxies.h"
#include "update_engine/action_processor.h"
#include "update_engine/chrome_browser_proxy_resolver.h"
+#include "update_engine/client_library/include/update_engine/update_status.h"
#include "update_engine/download_action.h"
#include "update_engine/libcros_proxy.h"
#include "update_engine/omaha_request_params.h"
@@ -39,7 +40,6 @@
#include "update_engine/system_state.h"
#include "update_engine/update_manager/policy.h"
#include "update_engine/update_manager/update_manager.h"
-#include "update_engine/update_status.h"
class MetricsLibraryInterface;
@@ -174,8 +174,9 @@
// from the server asynchronously at its own frequency.
virtual void RefreshDevicePolicy();
- // Returns the boottime (CLOCK_BOOTTIME) recorded at the last
- // successful update. Returns false if the device has not updated.
+ // Stores in |out_boot_time| the boottime (CLOCK_BOOTTIME) recorded at the
+ // time of the last successful update in the current boot. Returns false if
+ // there wasn't a successful update in the current boot.
virtual bool GetBootTimeAtUpdate(base::Time *out_boot_time);
// Returns a version OS version that was being used before the last reboot,
@@ -216,12 +217,7 @@
// Update server URL for automated lab test.
static const char* const kTestUpdateUrl;
- // Special ctor + friend declarations for testing purposes.
- UpdateAttempter(SystemState* system_state,
- LibCrosProxy* libcros_proxy,
- org::chromium::debugdProxyInterface* debugd_proxy,
- const std::string& update_completed_marker);
-
+ // Friend declarations for testing purposes.
friend class UpdateAttempterUnderTest;
friend class UpdateAttempterTest;
FRIEND_TEST(UpdateAttempterTest, ActionCompletedDownloadTest);
@@ -231,7 +227,6 @@
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventResumedTest);
FRIEND_TEST(UpdateAttempterTest, DisableDeltaUpdateIfNeededTest);
FRIEND_TEST(UpdateAttempterTest, MarkDeltaUpdateFailureTest);
- FRIEND_TEST(UpdateAttempterTest, ReadTrackFromPolicy);
FRIEND_TEST(UpdateAttempterTest, PingOmahaTest);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionNoEventTest);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionTest);
@@ -428,7 +423,7 @@
bool download_active_ = false;
// For status:
- UpdateStatus status_;
+ UpdateStatus status_{UpdateStatus::IDLE};
double download_progress_ = 0.0;
int64_t last_checked_time_ = 0;
std::string prev_version_;
@@ -469,10 +464,6 @@
// The current scatter factor as found in the policy setting.
base::TimeDelta scatter_factor_;
- // Update completed marker file. An empty string means this marker is being
- // ignored (nor is it being written), which is useful for testing situations.
- std::string update_completed_marker_;
-
// The number of consecutive failed update checks. Needed for calculating the
// next update check interval.
unsigned int consecutive_failed_update_checks_ = 0;
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index c08f1f8..31f75b6 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -82,10 +82,8 @@
public:
UpdateAttempterUnderTest(SystemState* system_state,
LibCrosProxy* libcros_proxy,
- org::chromium::debugdProxyInterface* debugd_proxy,
- const string& update_completed_marker)
- : UpdateAttempter(system_state, libcros_proxy, debugd_proxy,
- update_completed_marker) {}
+ org::chromium::debugdProxyInterface* debugd_proxy)
+ : UpdateAttempter(system_state, libcros_proxy, debugd_proxy) {}
// Wrap the update scheduling method, allowing us to opt out of scheduled
// updates for testing purposes.
@@ -204,8 +202,7 @@
LibCrosProxy libcros_proxy_;
UpdateAttempterUnderTest attempter_{&fake_system_state_,
&libcros_proxy_,
- &debugd_proxy_mock_,
- ""};
+ &debugd_proxy_mock_};
NiceMock<MockActionProcessor>* processor_;
NiceMock<MockPrefs>* prefs_; // Shortcut to fake_system_state_->mock_prefs().
@@ -261,18 +258,14 @@
}
TEST_F(UpdateAttempterTest, ConstructWithUpdatedMarkerTest) {
- string test_update_completed_marker;
- CHECK(utils::MakeTempFile(
- "update_attempter_unittest-update_completed_marker-XXXXXX",
- &test_update_completed_marker,
- nullptr));
- ScopedPathUnlinker completed_marker_unlinker(test_update_completed_marker);
- const base::FilePath marker(test_update_completed_marker);
- EXPECT_EQ(0, base::WriteFile(marker, "", 0));
- UpdateAttempterUnderTest attempter(&fake_system_state_,
- nullptr,
- &debugd_proxy_mock_,
- test_update_completed_marker);
+ FakePrefs fake_prefs;
+ string boot_id;
+ EXPECT_TRUE(utils::GetBootId(&boot_id));
+ fake_prefs.SetString(kPrefsUpdateCompletedOnBootId, boot_id);
+ fake_system_state_.set_prefs(&fake_prefs);
+ UpdateAttempterUnderTest attempter(&fake_system_state_, nullptr,
+ &debugd_proxy_mock_);
+ attempter.Init();
EXPECT_EQ(UpdateStatus::UPDATED_NEED_REBOOT, attempter.status());
}
@@ -936,15 +929,15 @@
}
TEST_F(UpdateAttempterTest, BootTimeInUpdateMarkerFile) {
- const string update_completed_marker = test_dir_ + "/update-completed-marker";
UpdateAttempterUnderTest attempter{&fake_system_state_,
nullptr, // libcros_proxy
- &debugd_proxy_mock_,
- update_completed_marker};
-
+ &debugd_proxy_mock_};
FakeClock fake_clock;
fake_clock.SetBootTime(Time::FromTimeT(42));
fake_system_state_.set_clock(&fake_clock);
+ FakePrefs fake_prefs;
+ fake_system_state_.set_prefs(&fake_prefs);
+ attempter.Init();
Time boot_time;
EXPECT_FALSE(attempter.GetBootTimeAtUpdate(&boot_time));