Send an UMA metric when failed to boot into the new partition.
When a payload is successfully applied, the /other/ partition
is marked as valid and a reboot is needed, the reboot into this
new partition can fail due to several reasons. If than happens,
the firmware can rollback to the previous partition.
When this happens, this fix sends a new UMA metric with the
attempt number of this failing payload.
In order to test this functionality we need to fake the
utils::BootDevice() to emulate a reboot into the same or
a different partition. To achieve this, this function is
moved to a new "HardwareInterface" that can be faked
using the FakeHardware class that can hold similar hardware
related functions. Implementations and unittest were
refactored as needed.
BUG=chromium:243572
TEST=unittests
Change-Id: I1a4242df0bd61e2718ab881ead603b1d3705b877
Reviewed-on: https://gerrit.chromium.org/gerrit/61815
Commit-Queue: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/SConstruct b/SConstruct
index fe6170d..c60f1d0 100644
--- a/SConstruct
+++ b/SConstruct
@@ -265,6 +265,7 @@
full_update_generator.cc
gpio_handler.cc
graph_utils.cc
+ hardware.cc
http_common.cc
http_fetcher.cc
install_plan.cc
@@ -308,6 +309,7 @@
extent_mapper_unittest.cc
extent_ranges_unittest.cc
extent_writer_unittest.cc
+ fake_hardware.cc
file_writer_unittest.cc
filesystem_copier_action_unittest.cc
filesystem_iterator_unittest.cc
@@ -316,6 +318,7 @@
gpio_mock_file_descriptor.cc
gpio_mock_udev_interface.cc
graph_utils_unittest.cc
+ hardware_unittest.cc
http_fetcher_unittest.cc
metadata_unittest.cc
mock_http_fetcher.cc
diff --git a/constants.cc b/constants.cc
index 7878a85..874a004 100644
--- a/constants.cc
+++ b/constants.cc
@@ -43,6 +43,9 @@
const char kPrefsResumedUpdateFailures[] = "resumed-update-failures";
const char kPrefsRollbackVersion[] = "rollback-version";
const char kPrefsSystemUpdatedMarker[] = "system-updated-marker";
+const char kPrefsTargetVersionAttempt[] = "target-version-attempt";
+const char kPrefsTargetVersionInstalledFrom[] = "target-version-installed-from";
+const char kPrefsTargetVersionUniqueId[] = "target-version-unique-id";
const char kPrefsTotalBytesDownloaded[] = "total-bytes-downloaded";
const char kPrefsUpdateCheckCount[] = "update-check-count";
const char kPrefsUpdateCheckResponseHash[] = "update-check-response-hash";
diff --git a/constants.h b/constants.h
index 076aeae..7b3dca9 100644
--- a/constants.h
+++ b/constants.h
@@ -46,6 +46,9 @@
extern const char kPrefsResumedUpdateFailures[];
extern const char kPrefsRollbackVersion[];
extern const char kPrefsSystemUpdatedMarker[];
+extern const char kPrefsTargetVersionAttempt[];
+extern const char kPrefsTargetVersionInstalledFrom[];
+extern const char kPrefsTargetVersionUniqueId[];
extern const char kPrefsTotalBytesDownloaded[];
extern const char kPrefsUpdateCheckCount[];
extern const char kPrefsUpdateCheckResponseHash[];
diff --git a/fake_hardware.cc b/fake_hardware.cc
new file mode 100644
index 0000000..604358d
--- /dev/null
+++ b/fake_hardware.cc
@@ -0,0 +1,24 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/fake_hardware.h"
+
+using std::string;
+
+namespace chromeos_update_engine {
+
+const string FakeHardware::KernelDeviceOfBootDevice(
+ const string& boot_device) {
+ KernelDevicesMap::iterator it = kernel_devices_.find(boot_device);
+ if (it == kernel_devices_.end())
+ return "";
+ return it->second;
+}
+
+void FakeHardware::SetKernelDeviceOfBootDevice(const string& boot_device,
+ const string& kernel_device) {
+ kernel_devices_[boot_device] = kernel_device;
+}
+
+} // namespace chromeos_update_engine
diff --git a/fake_hardware.h b/fake_hardware.h
new file mode 100644
index 0000000..8b570b2
--- /dev/null
+++ b/fake_hardware.h
@@ -0,0 +1,43 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_HARDWARE_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_HARDWARE_H__
+
+#include "update_engine/hardware_interface.h"
+
+#include "base/basictypes.h"
+
+#include <map>
+
+namespace chromeos_update_engine {
+
+// Implements a fake hardware interface used for testing.
+class FakeHardware : public HardwareInterface {
+ public:
+ FakeHardware() {}
+
+ // HardwareInterface methods.
+ virtual const std::string BootDevice() { return boot_device_; }
+ virtual const std::string KernelDeviceOfBootDevice(
+ const std::string& boot_device);
+
+ // Setters
+ void SetBootDevice(const std::string boot_device) {
+ boot_device_ = boot_device;
+ }
+ void SetKernelDeviceOfBootDevice(const std::string& boot_device,
+ const std::string& kernel_device);
+
+ private:
+ std::string boot_device_;
+ typedef std::map<std::string, std::string> KernelDevicesMap;
+ KernelDevicesMap kernel_devices_;
+
+ DISALLOW_COPY_AND_ASSIGN(FakeHardware);
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_FAKE_HARDWARE_H__
diff --git a/filesystem_copier_action.cc b/filesystem_copier_action.cc
index 3e5160e..0aeac36 100644
--- a/filesystem_copier_action.cc
+++ b/filesystem_copier_action.cc
@@ -21,7 +21,9 @@
#include <glib.h>
#include "update_engine/filesystem_iterator.h"
+#include "update_engine/hardware_interface.h"
#include "update_engine/subprocess.h"
+#include "update_engine/system_state.h"
#include "update_engine/utils.h"
using std::map;
@@ -36,6 +38,7 @@
} // namespace {}
FilesystemCopierAction::FilesystemCopierAction(
+ SystemState* system_state,
bool copying_kernel_install_path,
bool verify_hash)
: copying_kernel_install_path_(copying_kernel_install_path),
@@ -45,7 +48,8 @@
read_done_(false),
failed_(false),
cancelled_(false),
- filesystem_size_(kint64max) {
+ filesystem_size_(kint64max),
+ system_state_(system_state) {
// A lot of code works on the implicit assumption that processing is done on
// exactly 2 ping-pong buffers.
COMPILE_ASSERT(arraysize(buffer_) == 2 &&
@@ -86,8 +90,9 @@
string source = verify_hash_ ? destination : copy_source_;
if (source.empty()) {
source = copying_kernel_install_path_ ?
- utils::BootKernelDevice(utils::BootDevice()) :
- utils::BootDevice();
+ system_state_->hardware()->KernelDeviceOfBootDevice(
+ system_state_->hardware()->BootDevice()) :
+ system_state_->hardware()->BootDevice();
}
int src_fd = open(source.c_str(), O_RDONLY);
if (src_fd < 0) {
diff --git a/filesystem_copier_action.h b/filesystem_copier_action.h
index 4a61298..c01fdeb 100644
--- a/filesystem_copier_action.h
+++ b/filesystem_copier_action.h
@@ -24,10 +24,13 @@
namespace chromeos_update_engine {
+class SystemState;
class FilesystemCopierAction : public InstallPlanAction {
public:
- FilesystemCopierAction(bool copying_kernel_install_path, bool verify_hash);
+ FilesystemCopierAction(SystemState* system_state,
+ bool copying_kernel_install_path,
+ bool verify_hash);
void PerformAction();
void TerminateProcessing();
@@ -130,6 +133,9 @@
// bytes get copied.
int64_t filesystem_size_;
+ // The global context for update_engine.
+ SystemState* system_state_;
+
DISALLOW_COPY_AND_ASSIGN(FilesystemCopierAction);
};
diff --git a/filesystem_copier_action_unittest.cc b/filesystem_copier_action_unittest.cc
index 0db1640..685c546 100644
--- a/filesystem_copier_action_unittest.cc
+++ b/filesystem_copier_action_unittest.cc
@@ -16,6 +16,7 @@
#include "update_engine/filesystem_copier_action.h"
#include "update_engine/filesystem_iterator.h"
+#include "update_engine/mock_system_state.h"
#include "update_engine/omaha_hash_calculator.h"
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -39,6 +40,8 @@
}
void TearDown() {
}
+
+ MockSystemState mock_system_state_;
};
class FilesystemCopierActionTestDelegate : public ActionProcessorDelegate {
@@ -196,7 +199,8 @@
ActionProcessor processor;
ObjectFeederAction<InstallPlan> feeder_action;
- FilesystemCopierAction copier_action(use_kernel_partition,
+ FilesystemCopierAction copier_action(&mock_system_state_,
+ use_kernel_partition,
verify_hash != 0);
ObjectCollectorAction<InstallPlan> collector_action;
@@ -290,7 +294,7 @@
processor.set_delegate(&delegate);
- FilesystemCopierAction copier_action(false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&copier_action, &collector_action);
@@ -313,7 +317,7 @@
const char* kUrl = "http://some/url";
InstallPlan install_plan(false, true, kUrl, 0, "", 0, "", "", "");
feeder_action.set_obj(install_plan);
- FilesystemCopierAction copier_action(false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&feeder_action, &copier_action);
@@ -346,7 +350,7 @@
"/no/such/file",
"/no/such/file");
feeder_action.set_obj(install_plan);
- FilesystemCopierAction copier_action(false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&copier_action, &collector_action);
@@ -395,7 +399,7 @@
for (int i = 0; i < 2; ++i) {
bool is_kernel = i == 1;
- FilesystemCopierAction action(is_kernel, false);
+ FilesystemCopierAction action(&mock_system_state_, is_kernel, false);
EXPECT_EQ(kint64max, action.filesystem_size_);
{
int fd = HANDLE_EINTR(open(img.c_str(), O_RDONLY));
diff --git a/hardware.cc b/hardware.cc
new file mode 100644
index 0000000..07901f5
--- /dev/null
+++ b/hardware.cc
@@ -0,0 +1,47 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/hardware.h"
+
+#include <base/logging.h>
+#include <rootdev/rootdev.h>
+
+using std::string;
+
+namespace chromeos_update_engine {
+
+const string Hardware::BootDevice() {
+ char boot_path[PATH_MAX];
+ // Resolve the boot device path fully, including dereferencing
+ // through dm-verity.
+ int ret = rootdev(boot_path, sizeof(boot_path), true, false);
+
+ if (ret < 0) {
+ LOG(ERROR) << "rootdev failed to find the root device";
+ return "";
+ }
+ LOG_IF(WARNING, ret > 0) << "rootdev found a device name with no device node";
+
+ // This local variable is used to construct the return string and is not
+ // passed around after use.
+ return boot_path;
+}
+
+const string Hardware::KernelDeviceOfBootDevice(
+ const std::string& boot_device) {
+ // Currently this assumes the last digit of the boot device is
+ // 3, 5, or 7, and changes it to 2, 4, or 6, respectively, to
+ // get the kernel device.
+ string ret = boot_device;
+ if (ret.empty())
+ return ret;
+ char last_char = ret[ret.size() - 1];
+ if (last_char == '3' || last_char == '5' || last_char == '7') {
+ ret[ret.size() - 1] = last_char - 1;
+ return ret;
+ }
+ return "";
+}
+
+} // namespace chromeos_update_engine
diff --git a/hardware.h b/hardware.h
new file mode 100644
index 0000000..3c42524
--- /dev/null
+++ b/hardware.h
@@ -0,0 +1,30 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_H__
+
+#include "update_engine/hardware_interface.h"
+
+#include "base/basictypes.h"
+
+namespace chromeos_update_engine {
+
+// Implements the real interface with the hardware.
+class Hardware : public HardwareInterface {
+ public:
+ Hardware() {}
+
+ // HardwareInterface methods.
+ virtual const std::string BootDevice();
+ virtual const std::string KernelDeviceOfBootDevice(
+ const std::string& boot_device);
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(Hardware);
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_H__
diff --git a/hardware_interface.h b/hardware_interface.h
new file mode 100644
index 0000000..98773d9
--- /dev/null
+++ b/hardware_interface.h
@@ -0,0 +1,44 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_INTERFACE_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_INTERFACE_H__
+
+#include <string>
+
+namespace chromeos_update_engine {
+
+// The hardware interface allows access to the following parts of the system,
+// closely related to the hardware:
+// * crossystem exposed properties: firmware, hwid, etc.
+// * Physical disk: partition booted from and partition name conversions.
+// These stateless functions are tied together in this interface to facilitate
+// unit testing.
+class HardwareInterface {
+ public:
+ // Returns the currently booted device. "/dev/sda3", for example.
+ // This will not interpret LABEL= or UUID=. You'll need to use findfs
+ // or something with equivalent funcionality to interpret those.
+ virtual const std::string BootDevice() = 0;
+
+ // Returns the kernel device associated with the given boot device,
+ // for example, this function returns "/dev/sda2" if |boot_device| is
+ // "/dev/sda3".
+ // To obtain the current booted kernel device, the suggested calling
+ // convention is KernelDeviceOfBootDevice(BootDevice()).
+ // This function works by doing string modification on |boot_device|.
+ // Returns empty string on failure.
+ virtual const std::string KernelDeviceOfBootDevice(
+ const std::string& boot_device) = 0;
+
+ // TODO(deymo): Move other hardware-dependant functions to this interface:
+ // GetECVersion, GetFirmwareVersion, GetHardwareClass, IsNormalBootMode and
+ // IsOfficialBuild.
+
+ virtual ~HardwareInterface() {}
+};
+
+} // namespace chromeos_update_engine
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_HARDWARE_INTERFACE_H__
diff --git a/hardware_unittest.cc b/hardware_unittest.cc
new file mode 100644
index 0000000..3e49fd5
--- /dev/null
+++ b/hardware_unittest.cc
@@ -0,0 +1,49 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "update_engine/test_utils.h"
+#include "update_engine/hardware.h"
+
+using std::string;
+using std::vector;
+
+namespace chromeos_update_engine {
+
+class HardwareTest : public ::testing::Test {
+ protected:
+ void SetUp() {
+ }
+ void TearDown() {
+ }
+
+ // Hardware object under test.
+ Hardware hwut_;
+};
+
+TEST_F(HardwareTest, BootDeviceTest) {
+ // Pretty lame test...
+ EXPECT_FALSE(hwut_.BootDevice().empty());
+}
+
+TEST_F(HardwareTest, KernelDeviceOfBootDevice) {
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("foo"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda0"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda1"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda2"));
+ EXPECT_EQ("/dev/sda2", hwut_.KernelDeviceOfBootDevice("/dev/sda3"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda4"));
+ EXPECT_EQ("/dev/sda4", hwut_.KernelDeviceOfBootDevice("/dev/sda5"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda6"));
+ EXPECT_EQ("/dev/sda6", hwut_.KernelDeviceOfBootDevice("/dev/sda7"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda8"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/sda9"));
+
+ EXPECT_EQ("/dev/mmcblk0p2", hwut_.KernelDeviceOfBootDevice("/dev/mmcblk0p3"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/mmcblk0p4"));
+
+ EXPECT_EQ("/dev/ubi2", hwut_.KernelDeviceOfBootDevice("/dev/ubi3"));
+ EXPECT_EQ("", hwut_.KernelDeviceOfBootDevice("/dev/ubi4"));
+}
+
+} // namespace chromeos_update_engine
diff --git a/mock_payload_state.h b/mock_payload_state.h
index 02db28b..0415a80 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -25,9 +25,12 @@
MOCK_METHOD0(UpdateRestarted, void());
MOCK_METHOD0(UpdateSucceeded, void());
MOCK_METHOD1(UpdateFailed, void(ErrorCode error));
+ MOCK_METHOD0(ResetUpdateStatus, void());
MOCK_METHOD0(ShouldBackoffDownload, bool());
MOCK_METHOD0(UpdateEngineStarted, void());
MOCK_METHOD0(Rollback, void());
+ MOCK_METHOD1(ExpectRebootInNewVersion,
+ void(const std::string& target_version_uid));
// Getters.
MOCK_METHOD0(GetResponseSignature, std::string());
diff --git a/mock_system_state.cc b/mock_system_state.cc
index d76de05..0ee9e5e 100644
--- a/mock_system_state.cc
+++ b/mock_system_state.cc
@@ -11,9 +11,10 @@
// OOBE is completed even when there's no such marker file, etc.
MockSystemState::MockSystemState()
: default_request_params_(this),
+ clock_(&default_clock_),
+ hardware_(&default_hardware_),
prefs_(&mock_prefs_),
powerwash_safe_prefs_(&mock_powerwash_safe_prefs_) {
- clock_ = &default_clock_;
request_params_ = &default_request_params_;
mock_payload_state_.Initialize(this);
mock_gpio_handler_ = new testing::NiceMock<MockGpioHandler>();
diff --git a/mock_system_state.h b/mock_system_state.h
index 8b04f7e..a666a66 100644
--- a/mock_system_state.h
+++ b/mock_system_state.h
@@ -14,6 +14,7 @@
#include "update_engine/mock_gpio_handler.h"
#include "update_engine/mock_payload_state.h"
#include "update_engine/clock.h"
+#include "update_engine/hardware.h"
#include "update_engine/prefs_mock.h"
#include "update_engine/system_state.h"
@@ -44,6 +45,10 @@
return connection_manager_;
}
+ inline virtual HardwareInterface* hardware() {
+ return hardware_;
+ }
+
inline virtual MetricsLibraryInterface* metrics_lib() {
return &mock_metrics_lib_;
}
@@ -83,6 +88,10 @@
clock_ = clock;
}
+ inline void set_hardware(HardwareInterface* hardware) {
+ hardware_ = hardware;
+ }
+
inline void set_prefs(PrefsInterface* prefs) {
prefs_ = prefs;
}
@@ -119,10 +128,12 @@
// These are the other object we own.
Clock default_clock_;
+ Hardware default_hardware_;
OmahaRequestParams default_request_params_;
// These are pointers to objects which caller can override.
ClockInterface* clock_;
+ HardwareInterface* hardware_;
PrefsInterface* prefs_;
PrefsInterface* powerwash_safe_prefs_;
ConnectionManager* connection_manager_;
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 923c62d..d0853c8 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -11,6 +11,7 @@
#include "update_engine/constants.h"
#include "update_engine/delta_performer.h"
+#include "update_engine/hardware_interface.h"
#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
#include "update_engine/utils.h"
@@ -89,10 +90,12 @@
install_plan_.is_full_update = !response.is_delta_payload;
TEST_AND_RETURN(utils::GetInstallDev(
- (!boot_device_.empty() ? boot_device_ : utils::BootDevice()),
+ (!boot_device_.empty() ? boot_device_ :
+ system_state_->hardware()->BootDevice()),
&install_plan_.install_path));
install_plan_.kernel_install_path =
- utils::BootKernelDevice(install_plan_.install_path);
+ system_state_->hardware()->KernelDeviceOfBootDevice(
+ install_plan_.install_path);
OmahaRequestParams* params = system_state_->request_params();
if (params->to_more_stable_channel() && params->is_powerwash_allowed())
diff --git a/payload_state.cc b/payload_state.cc
index 8c2547f..c519b4f 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -12,6 +12,8 @@
#include "update_engine/clock.h"
#include "update_engine/constants.h"
+#include "update_engine/hardware_interface.h"
+#include "update_engine/install_plan.h"
#include "update_engine/prefs.h"
#include "update_engine/system_state.h"
#include "update_engine/utils.h"
@@ -1106,6 +1108,86 @@
}
prefs_->Delete(kPrefsSystemUpdatedMarker);
}
+ // Check if it is needed to send metrics about a failed reboot into a new
+ // version.
+ ReportFailedBootIfNeeded();
+}
+
+void PayloadState::ReportFailedBootIfNeeded() {
+ // If the kPrefsTargetVersionInstalledFrom is present, a successfully applied
+ // payload was marked as ready immediately before the last reboot, and we
+ // need to check if such payload successfully rebooted or not.
+ if (prefs_->Exists(kPrefsTargetVersionInstalledFrom)) {
+ string installed_from;
+ if (!prefs_->GetString(kPrefsTargetVersionInstalledFrom, &installed_from)) {
+ LOG(ERROR) << "Error reading TargetVersionInstalledFrom on reboot.";
+ return;
+ }
+ if (installed_from ==
+ utils::PartitionNumber(system_state_->hardware()->BootDevice())) {
+ // A reboot was pending, but the chromebook is again in the same
+ // BootDevice where the update was installed from.
+ int64_t target_attempt;
+ if (!prefs_->GetInt64(kPrefsTargetVersionAttempt, &target_attempt)) {
+ LOG(ERROR) << "Error reading TargetVersionAttempt when "
+ "TargetVersionInstalledFrom was present.";
+ target_attempt = 1;
+ }
+
+ // Report the UMA metric of the current boot failure.
+ string metric = "Installer.RebootToNewPartitionAttempt";
+
+ LOG(INFO) << "Uploading " << target_attempt
+ << " (count) for metric " << metric;
+ system_state_->metrics_lib()->SendToUMA(
+ metric,
+ target_attempt,
+ 1, // min value
+ 50, // max value
+ kNumDefaultUmaBuckets);
+ } else {
+ prefs_->Delete(kPrefsTargetVersionAttempt);
+ prefs_->Delete(kPrefsTargetVersionUniqueId);
+ }
+ prefs_->Delete(kPrefsTargetVersionInstalledFrom);
+ }
+}
+
+void PayloadState::ExpectRebootInNewVersion(const string& target_version_uid) {
+ // Expect to boot into the new partition in the next reboot setting the
+ // TargetVersion* flags in the Prefs.
+ string stored_target_version_uid;
+ string target_version_id;
+ string target_partition;
+ int64_t target_attempt;
+
+ if (prefs_->Exists(kPrefsTargetVersionUniqueId) &&
+ prefs_->GetString(kPrefsTargetVersionUniqueId,
+ &stored_target_version_uid) &&
+ stored_target_version_uid == target_version_uid) {
+ if (!prefs_->GetInt64(kPrefsTargetVersionAttempt, &target_attempt))
+ target_attempt = 0;
+ } else {
+ prefs_->SetString(kPrefsTargetVersionUniqueId, target_version_uid);
+ target_attempt = 0;
+ }
+ prefs_->SetInt64(kPrefsTargetVersionAttempt, target_attempt + 1);
+
+ prefs_->SetString(kPrefsTargetVersionInstalledFrom,
+ utils::PartitionNumber(
+ system_state_->hardware()->BootDevice()));
+}
+
+void PayloadState::ResetUpdateStatus() {
+ // Remove the TargetVersionInstalledFrom pref so that if the machine is
+ // rebooted the next boot is not flagged as failed to rebooted into the
+ // new applied payload.
+ prefs_->Delete(kPrefsTargetVersionInstalledFrom);
+
+ // Also decrement the attempt number if it exists.
+ int64_t target_attempt;
+ if (prefs_->GetInt64(kPrefsTargetVersionAttempt, &target_attempt))
+ prefs_->SetInt64(kPrefsTargetVersionAttempt, target_attempt-1);
}
} // namespace chromeos_update_engine
diff --git a/payload_state.h b/payload_state.h
index 8c3133c..64d134f 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -6,6 +6,7 @@
#define CHROMEOS_PLATFORM_UPDATE_ENGINE_PAYLOAD_STATE_H__
#include <base/time.h>
+#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "update_engine/payload_state_interface.h"
#include "update_engine/prefs_interface.h"
@@ -39,8 +40,10 @@
virtual void UpdateRestarted();
virtual void UpdateSucceeded();
virtual void UpdateFailed(ErrorCode error);
+ virtual void ResetUpdateStatus();
virtual bool ShouldBackoffDownload();
virtual void Rollback();
+ virtual void ExpectRebootInNewVersion(const std::string& target_version_uid);
virtual inline std::string GetResponseSignature() {
return response_signature_;
@@ -97,6 +100,12 @@
}
private:
+ friend class PayloadStateTest;
+ FRIEND_TEST(PayloadStateTest, RebootAfterUpdateFailedMetric);
+ FRIEND_TEST(PayloadStateTest, RebootAfterUpdateSucceed);
+ FRIEND_TEST(PayloadStateTest, RebootAfterCanceledUpdate);
+ FRIEND_TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs);
+
// Increments the payload attempt number used for metrics.
void IncrementPayloadAttemptNumber();
@@ -147,6 +156,13 @@
// Reports the various metrics related to update attempts counts.
void ReportAttemptsCountMetrics();
+ // Checks if we were expecting to be running in the new version but the
+ // boot into the new version failed for some reason. If that's the case, an
+ // UMA metric is sent reporting the number of attempts the same applied
+ // payload was attempted to reboot. This function is called by UpdateAttempter
+ // every time the update engine starts and there's no reboot pending.
+ void ReportFailedBootIfNeeded();
+
// Resets all the persisted state values which are maintained relative to the
// current response signature. The response signature itself is not reset.
void ResetPersistedState();
diff --git a/payload_state_interface.h b/payload_state_interface.h
index 4323b4f..fcb6d2b 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -58,9 +58,22 @@
// depending on the type of the error that happened.
virtual void UpdateFailed(ErrorCode error) = 0;
+ // This method should be called whenever a succeeded update is canceled, and
+ // thus can only be called after UpdateSucceeded(). This is currently used
+ // only for manual testing using the update_engine_client.
+ virtual void ResetUpdateStatus() = 0;
+
// This method should be called every time we initiate a Rollback.
virtual void Rollback() = 0;
+ // Sets the expectations to boot into the new version in the next reboot.
+ // This function is called every time a new update is marked as ready by
+ // UpdateSuccess(). |target_version_uid| is an unique identifier of the
+ // applied payload. It can be any string, as long as the same string is used
+ // for the same payload.
+ virtual void ExpectRebootInNewVersion(
+ const std::string& target_version_uid) = 0;
+
// Returns true if we should backoff the current download attempt.
// False otherwise.
virtual bool ShouldBackoffDownload() = 0;
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index f7665ae..5a8722f 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -12,6 +12,7 @@
#include "update_engine/constants.h"
#include "update_engine/fake_clock.h"
+#include "update_engine/fake_hardware.h"
#include "update_engine/mock_system_state.h"
#include "update_engine/omaha_request_action.h"
#include "update_engine/payload_state.h"
@@ -24,11 +25,12 @@
using base::TimeDelta;
using std::string;
using testing::_;
+using testing::AnyNumber;
+using testing::AtLeast;
+using testing::Mock;
using testing::NiceMock;
using testing::Return;
using testing::SetArgumentPointee;
-using testing::AtLeast;
-using testing::AnyNumber;
namespace chromeos_update_engine {
@@ -1165,4 +1167,157 @@
payload_state.UpdateSucceeded();
}
+TEST(PayloadStateTest, RebootAfterUpdateFailedMetric) {
+ FakeHardware fake_hardware;
+ MockSystemState mock_system_state;
+ OmahaResponse response;
+ PayloadState payload_state;
+ Prefs prefs;
+ string temp_dir;
+
+ // Setup an environment with persistent prefs across simulated reboots.
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateReboot.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+ mock_system_state.set_prefs(&prefs);
+
+ fake_hardware.SetBootDevice("/dev/sda3");
+ fake_hardware.SetKernelDeviceOfBootDevice("/dev/sda3", "/dev/sda2");
+ mock_system_state.set_hardware(&fake_hardware);
+
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+ SetupPayloadStateWith2Urls("Hash3141", true, &payload_state, &response);
+
+ // Simulate a successful download and update.
+ payload_state.DownloadComplete();
+ payload_state.UpdateSucceeded();
+ payload_state.ExpectRebootInNewVersion("Version:12345678");
+
+ // Reboot into the same environment to get an UMA metric with a value of 1.
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", 1, _, _, _));
+ payload_state.ReportFailedBootIfNeeded();
+ Mock::VerifyAndClearExpectations(mock_system_state.mock_metrics_lib());
+
+ // Simulate a second update and reboot into the same environment, this should
+ // send a value of 2.
+ payload_state.ExpectRebootInNewVersion("Version:12345678");
+
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", 2, _, _, _));
+ payload_state.ReportFailedBootIfNeeded();
+ Mock::VerifyAndClearExpectations(mock_system_state.mock_metrics_lib());
+
+ // Simulate a third failed reboot to new version, but this time for a
+ // different payload. This should send a value of 1 this time.
+ payload_state.ExpectRebootInNewVersion("Version:3141592");
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", 1, _, _, _));
+ payload_state.ReportFailedBootIfNeeded();
+ Mock::VerifyAndClearExpectations(mock_system_state.mock_metrics_lib());
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+TEST(PayloadStateTest, RebootAfterUpdateSucceed) {
+ FakeHardware fake_hardware;
+ MockSystemState mock_system_state;
+ OmahaResponse response;
+ PayloadState payload_state;
+ Prefs prefs;
+ string temp_dir;
+
+ // Setup an environment with persistent prefs across simulated reboots.
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateReboot.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+ mock_system_state.set_prefs(&prefs);
+
+ fake_hardware.SetKernelDeviceOfBootDevice("/dev/sda3", "/dev/sda2");
+ fake_hardware.SetKernelDeviceOfBootDevice("/dev/sda5", "/dev/sda4");
+ fake_hardware.SetBootDevice("/dev/sda3");
+ mock_system_state.set_hardware(&fake_hardware);
+
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+ SetupPayloadStateWith2Urls("Hash3141", true, &payload_state, &response);
+
+ // Simulate a successful download and update.
+ payload_state.DownloadComplete();
+ payload_state.UpdateSucceeded();
+ payload_state.ExpectRebootInNewVersion("Version:12345678");
+
+ // Change the BootDevice to a different one, no metric should be sent.
+ fake_hardware.SetBootDevice("/dev/sda5");
+
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", _, _, _, _))
+ .Times(0);
+ payload_state.ReportFailedBootIfNeeded();
+
+ // A second reboot in eiher partition should not send a metric.
+ payload_state.ReportFailedBootIfNeeded();
+ fake_hardware.SetBootDevice("/dev/sda3");
+ payload_state.ReportFailedBootIfNeeded();
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+TEST(PayloadStateTest, RebootAfterCanceledUpdate) {
+ MockSystemState mock_system_state;
+ OmahaResponse response;
+ PayloadState payload_state;
+ Prefs prefs;
+ string temp_dir;
+
+ // Setup an environment with persistent prefs across simulated reboots.
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateReboot.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+ mock_system_state.set_prefs(&prefs);
+
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+ SetupPayloadStateWith2Urls("Hash3141", true, &payload_state, &response);
+
+ // Simulate a successful download and update.
+ payload_state.DownloadComplete();
+ payload_state.UpdateSucceeded();
+ payload_state.ExpectRebootInNewVersion("Version:12345678");
+
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", _, _, _, _))
+ .Times(0);
+
+ // Cancel the applied update.
+ payload_state.ResetUpdateStatus();
+
+ // Simulate a reboot.
+ payload_state.ReportFailedBootIfNeeded();
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
+TEST(PayloadStateTest, UpdateSuccessWithWipedPrefs) {
+ MockSystemState mock_system_state;
+ PayloadState payload_state;
+ Prefs prefs;
+ string temp_dir;
+
+ // Setup an environment with persistent but initially empty prefs.
+ EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PayloadStateReboot.XXXXXX",
+ &temp_dir));
+ prefs.Init(FilePath(temp_dir));
+ mock_system_state.set_prefs(&prefs);
+
+ EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
+
+ EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
+ "Installer.RebootToNewPartitionAttempt", _, _, _, _))
+ .Times(0);
+
+ // Simulate a reboot in this environment.
+ payload_state.ReportFailedBootIfNeeded();
+
+ EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
+}
+
}
diff --git a/real_system_state.h b/real_system_state.h
index 9ced628..c51094e 100644
--- a/real_system_state.h
+++ b/real_system_state.h
@@ -10,6 +10,7 @@
#include <update_engine/clock.h>
#include <update_engine/connection_manager.h>
#include <update_engine/gpio_handler.h>
+#include <update_engine/hardware.h>
#include <update_engine/payload_state.h>
#include <update_engine/prefs.h>
#include <update_engine/update_attempter.h>
@@ -44,6 +45,10 @@
return &connection_manager_;
}
+ virtual inline HardwareInterface* hardware() {
+ return &hardware_;
+ }
+
virtual inline MetricsLibraryInterface* metrics_lib() {
return &metrics_lib_;
}
@@ -93,6 +98,9 @@
// decisions depending on the current type of connection.
ConnectionManager connection_manager_;
+ // Interface for the hardware functions.
+ Hardware hardware_;
+
// The Metrics Library interface for reporting UMA stats.
MetricsLibrary metrics_lib_;
diff --git a/system_state.h b/system_state.h
index 56d38a6..08092f1 100644
--- a/system_state.h
+++ b/system_state.h
@@ -19,6 +19,7 @@
// the required classes.
class ClockInterface;
class ConnectionManager;
+class HardwareInterface;
class PrefsInterface;
class PayloadStateInterface;
class GpioHandler;
@@ -54,6 +55,9 @@
// Gets the connection manager object.
virtual ConnectionManager* connection_manager() = 0;
+ // Gets the hardware interface object.
+ virtual HardwareInterface* hardware() = 0;
+
// Gets the Metrics Library interface for reporting UMA stats.
virtual MetricsLibraryInterface* metrics_lib() = 0;
diff --git a/update_attempter.cc b/update_attempter.cc
index 6809108..0f8abfb 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -21,6 +21,7 @@
#include "update_engine/download_action.h"
#include "update_engine/filesystem_copier_action.h"
#include "update_engine/gpio_handler.h"
+#include "update_engine/hardware_interface.h"
#include "update_engine/libcurl_http_fetcher.h"
#include "update_engine/multi_range_http_fetcher.h"
#include "update_engine/omaha_request_action.h"
@@ -463,9 +464,9 @@
shared_ptr<OmahaResponseHandlerAction> response_handler_action(
new OmahaResponseHandlerAction(system_state_));
shared_ptr<FilesystemCopierAction> filesystem_copier_action(
- new FilesystemCopierAction(false, false));
+ new FilesystemCopierAction(system_state_, false, false));
shared_ptr<FilesystemCopierAction> kernel_filesystem_copier_action(
- new FilesystemCopierAction(true, false));
+ new FilesystemCopierAction(system_state_, true, false));
shared_ptr<OmahaRequestAction> download_started_action(
new OmahaRequestAction(system_state_,
new OmahaEvent(
@@ -491,9 +492,9 @@
is_test_mode_),
false));
shared_ptr<FilesystemCopierAction> filesystem_verifier_action(
- new FilesystemCopierAction(false, true));
+ new FilesystemCopierAction(system_state_, false, true));
shared_ptr<FilesystemCopierAction> kernel_filesystem_verifier_action(
- new FilesystemCopierAction(true, true));
+ new FilesystemCopierAction(system_state_, true, true));
shared_ptr<OmahaRequestAction> update_complete_action(
new OmahaRequestAction(system_state_,
new OmahaEvent(OmahaEvent::kTypeUpdateComplete),
@@ -566,15 +567,17 @@
LOG(INFO) << "Setting rollback options.";
InstallPlan install_plan;
if (install_path == NULL) {
- TEST_AND_RETURN_FALSE(utils::GetInstallDev(utils::BootDevice(),
- &install_plan.install_path));
+ TEST_AND_RETURN_FALSE(utils::GetInstallDev(
+ system_state_->hardware()->BootDevice(),
+ &install_plan.install_path));
}
else {
install_plan.install_path = *install_path;
}
- install_plan.kernel_install_path = utils::BootKernelDevice(
- install_plan.install_path);
+ install_plan.kernel_install_path =
+ system_state_->hardware()->KernelDeviceOfBootDevice(
+ install_plan.install_path);
install_plan.powerwash_required = powerwash;
if (powerwash) {
// Enterprise-enrolled devices have an empty owner in their device policy.
@@ -703,6 +706,17 @@
kUpdateNoticeUnspecified);
LOG(INFO) << "Update successfully applied, waiting to reboot.";
+ const InstallPlan& install_plan = response_handler_action_->install_plan();
+
+ // Generate an unique payload identifier.
+ const string target_version_uid =
+ install_plan.payload_hash + ":" + install_plan.metadata_signature;
+
+ // Expect to reboot into the new version to send the proper metric during
+ // next boot.
+ system_state_->payload_state()->ExpectRebootInNewVersion(
+ target_version_uid);
+
// Also report the success code so that the percentiles can be
// interpreted properly for the remaining error codes in UMA.
utils::SendErrorCodeToUma(system_state_, code);
@@ -844,6 +858,9 @@
if (!file_util::Delete(kUpdateCompletedMarkerPath, false))
ret_value = false;
+ // Notify the PayloadState that the successful payload was canceled.
+ system_state_->payload_state()->ResetUpdateStatus();
+
return ret_value;
}
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 98fc1a8..b5df68c 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -183,7 +183,8 @@
EXPECT_EQ(kErrorCodeOmahaResponseHandlerError,
GetErrorCodeForAction(&omaha_response_handler_action,
kErrorCodeError));
- FilesystemCopierAction filesystem_copier_action(false, false);
+ FilesystemCopierAction filesystem_copier_action(
+ &mock_system_state_, false, false);
EXPECT_EQ(kErrorCodeFilesystemCopierError,
GetErrorCodeForAction(&filesystem_copier_action, kErrorCodeError));
PostinstallRunnerAction postinstall_runner_action;
diff --git a/update_check_scheduler.cc b/update_check_scheduler.cc
index e77b5bb..425ce3e 100644
--- a/update_check_scheduler.cc
+++ b/update_check_scheduler.cc
@@ -5,6 +5,7 @@
#include "update_engine/update_check_scheduler.h"
#include "update_engine/certificate_checker.h"
+#include "update_engine/hardware_interface.h"
#include "update_engine/http_common.h"
#include "update_engine/gpio_handler.h"
#include "update_engine/system_state.h"
@@ -58,7 +59,8 @@
}
bool UpdateCheckScheduler::IsBootDeviceRemovable() {
- return utils::IsRemovableDevice(utils::RootDevice(utils::BootDevice()));
+ return utils::IsRemovableDevice(utils::RootDevice(
+ system_state_->hardware()->BootDevice()));
}
bool UpdateCheckScheduler::IsOfficialBuild() {
diff --git a/utils.cc b/utils.cc
index 74e0627..e3579bf 100644
--- a/utils.cc
+++ b/utils.cc
@@ -31,7 +31,6 @@
#include <base/stringprintf.h>
#include <glib.h>
#include <google/protobuf/stubs/common.h>
-#include <rootdev/rootdev.h>
#include "update_engine/constants.h"
#include "update_engine/file_writer.h"
@@ -554,38 +553,6 @@
return 0 == str.compare(0, prefix.size(), prefix);
}
-const std::string BootDevice() {
- char boot_path[PATH_MAX];
- // Resolve the boot device path fully, including dereferencing
- // through dm-verity.
- int ret = rootdev(boot_path, sizeof(boot_path), true, false);
-
- if (ret < 0) {
- LOG(ERROR) << "rootdev failed to find the root device";
- return "";
- }
- LOG_IF(WARNING, ret > 0) << "rootdev found a device name with no device node";
-
- // This local variable is used to construct the return string and is not
- // passed around after use.
- return boot_path;
-}
-
-const string BootKernelDevice(const std::string& boot_device) {
- // Currently this assumes the last digit of the boot device is
- // 3, 5, or 7, and changes it to 2, 4, or 6, respectively, to
- // get the kernel device.
- string ret = boot_device;
- if (ret.empty())
- return ret;
- char last_char = ret[ret.size() - 1];
- if (last_char == '3' || last_char == '5' || last_char == '7') {
- ret[ret.size() - 1] = last_char - 1;
- return ret;
- }
- return "";
-}
-
bool MountFilesystem(const string& device,
const string& mountpoint,
unsigned long mountflags) {
diff --git a/utils.h b/utils.h
index 9b93134..412252c 100644
--- a/utils.h
+++ b/utils.h
@@ -254,18 +254,6 @@
}
}
-// Returns the currently booted device. "/dev/sda3", for example.
-// This will not interpret LABEL= or UUID=. You'll need to use findfs
-// or something with equivalent funcionality to interpret those.
-const std::string BootDevice();
-
-// Returns the currently booted kernel device, "dev/sda2", for example.
-// Client must pass in the boot device. The suggested calling convention
-// is: BootKernelDevice(BootDevice()).
-// This function works by doing string modification on boot_device.
-// Returns empty string on failure.
-const std::string BootKernelDevice(const std::string& boot_device);
-
// Cgroups cpu shares constants. 1024 is the default shares a standard process
// gets and 2 is the minimum value. We set High as a value that gives the
// update-engine 2x the cpu share of a standard process.
diff --git a/utils_unittest.cc b/utils_unittest.cc
index e10e2cb..09b6667 100644
--- a/utils_unittest.cc
+++ b/utils_unittest.cc
@@ -139,25 +139,6 @@
EXPECT_FALSE(utils::StringHasPrefix("abcdefgh", "gh"));
}
-TEST(UtilsTest, BootDeviceTest) {
- // Pretty lame test...
- EXPECT_FALSE(utils::BootDevice().empty());
-}
-
-TEST(UtilsTest, BootKernelDeviceTest) {
- EXPECT_EQ("", utils::BootKernelDevice("foo"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda0"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda1"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda2"));
- EXPECT_EQ("/dev/sda2", utils::BootKernelDevice("/dev/sda3"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda4"));
- EXPECT_EQ("/dev/sda4", utils::BootKernelDevice("/dev/sda5"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda6"));
- EXPECT_EQ("/dev/sda6", utils::BootKernelDevice("/dev/sda7"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda8"));
- EXPECT_EQ("", utils::BootKernelDevice("/dev/sda9"));
-}
-
TEST(UtilsTest, RecursiveUnlinkDirTest) {
string first_dir_name;
ASSERT_TRUE(utils::MakeTempDirectory("RecursiveUnlinkDirTest-a-XXXXXX",