Make checkpointing atomic
Update Engine currently has a bug where if it the process is interrupted
during checkpointing, we are unable to resume progress from where we
were previously. This is because prefs are reset at the beginning of
checkpointing and then slowly updated during checkpointing, if
update_engine is interrupted during this stage, it loses its state. This
change allows us to update the prefs non-destructively and then
atomically replace the old prefs with the new ones.
Test: m update_engine. update_device.py tested resume ota
Change-Id: I3a7edf7498be9cde5c6f34e3cb259a7853f4f443
diff --git a/common/fake_prefs.h b/common/fake_prefs.h
index 7ae9fb9..721cf24 100644
--- a/common/fake_prefs.h
+++ b/common/fake_prefs.h
@@ -59,6 +59,9 @@
void AddObserver(std::string_view key, ObserverInterface* observer) override;
void RemoveObserver(std::string_view key,
ObserverInterface* observer) override;
+ bool StartTransaction() override { return false; }
+ bool CancelTransaction() override { return false; }
+ bool SubmitTransaction() override { return false; }
private:
enum class PrefType {
diff --git a/common/mock_prefs.h b/common/mock_prefs.h
index f308074..028650d 100644
--- a/common/mock_prefs.h
+++ b/common/mock_prefs.h
@@ -47,6 +47,9 @@
MOCK_METHOD2(AddObserver, void(std::string_view key, ObserverInterface*));
MOCK_METHOD2(RemoveObserver, void(std::string_view key, ObserverInterface*));
+ MOCK_METHOD(bool, StartTransaction, (), (override));
+ MOCK_METHOD(bool, CancelTransaction, (), (override));
+ MOCK_METHOD(bool, SubmitTransaction, (), (override));
};
} // namespace chromeos_update_engine
diff --git a/common/prefs.cc b/common/prefs.cc
index a070302..57d4dcd 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -17,6 +17,8 @@
#include "update_engine/common/prefs.h"
#include <algorithm>
+#include <filesystem>
+#include <unistd.h>
#include <base/files/file_enumerator.h>
#include <base/files/file_util.h>
@@ -25,6 +27,7 @@
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
+#include "update_engine/common/platform_constants.h"
#include "update_engine/common/utils.h"
using std::string;
@@ -160,8 +163,62 @@
return file_storage_.Init(prefs_dir);
}
+bool PrefsBase::StartTransaction() {
+ return storage_->CreateTemporaryPrefs();
+}
+
+bool PrefsBase::CancelTransaction() {
+ return storage_->DeleteTemporaryPrefs();
+}
+
+bool PrefsBase::SubmitTransaction() {
+ return storage_->SwapPrefs();
+}
+
+std::string Prefs::FileStorage::GetTemporaryDir() const {
+ return prefs_dir_.value() + "_tmp";
+}
+
+bool Prefs::FileStorage::CreateTemporaryPrefs() {
+ // Delete any existing prefs_tmp
+ DeleteTemporaryPrefs();
+ // Get the paths to the source and destination directories.
+ std::filesystem::path source_directory(prefs_dir_.value());
+ std::filesystem::path destination_directory(GetTemporaryDir());
+
+ if (!std::filesystem::exists(source_directory)) {
+ LOG(ERROR) << "prefs directory does not exist: " << source_directory;
+ return false;
+ }
+ // Copy the directory.
+ std::filesystem::copy(source_directory, destination_directory);
+
+ return true;
+}
+
+bool Prefs::FileStorage::DeleteTemporaryPrefs() {
+ std::filesystem::path destination_directory(GetTemporaryDir());
+
+ if (std::filesystem::exists(destination_directory)) {
+ return std::filesystem::remove_all(destination_directory);
+ }
+ return true;
+}
+
+bool Prefs::FileStorage::SwapPrefs() {
+ if (std::filesystem::exists(prefs_dir_.value())) {
+ std::filesystem::remove_all(prefs_dir_.value());
+ }
+ if (rename(GetTemporaryDir().c_str(), prefs_dir_.value().c_str()) != 0) {
+ LOG(ERROR) << "Error replacing prefs with prefs_tmp" << strerror(errno);
+ return false;
+ }
+ return true;
+}
+
bool Prefs::FileStorage::Init(const base::FilePath& prefs_dir) {
prefs_dir_ = prefs_dir;
+
// Delete empty directories. Ignore errors when deleting empty directories.
DeleteEmptyDirectories(prefs_dir_);
return true;
@@ -231,8 +288,14 @@
for (char c : key)
TEST_AND_RETURN_FALSE(base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) ||
c == '_' || c == '-' || c == kKeySeparator);
- *filename = prefs_dir_.Append(
- base::FilePath::StringPieceType(key.data(), key.size()));
+ if (std::filesystem::exists(GetTemporaryDir())) {
+ *filename =
+ base::FilePath(GetTemporaryDir())
+ .Append(base::FilePath::StringPieceType(key.data(), key.size()));
+ } else {
+ *filename = prefs_dir_.Append(
+ base::FilePath::StringPieceType(key.data(), key.size()));
+ }
return true;
}
diff --git a/common/prefs.h b/common/prefs.h
index c3105c6..d32e732 100644
--- a/common/prefs.h
+++ b/common/prefs.h
@@ -60,6 +60,17 @@
// key was deleted.
virtual bool DeleteKey(std::string_view key) = 0;
+ // Makes a copy of prefs directory called prefs_tmp, which is modified
+ // during update_engine checkpointing
+ virtual bool CreateTemporaryPrefs() { return false; }
+
+ // Deletes prefs_tmp directory
+ virtual bool DeleteTemporaryPrefs() { return false; }
+
+ // Replaces prefs with prefs_tmp to make update_engine checkpointing more
+ // atomic
+ virtual bool SwapPrefs() { return false; }
+
private:
DISALLOW_COPY_AND_ASSIGN(StorageInterface);
};
@@ -73,6 +84,9 @@
bool SetInt64(std::string_view key, const int64_t value) override;
bool GetBoolean(std::string_view key, bool* value) const override;
bool SetBoolean(std::string_view key, const bool value) override;
+ bool StartTransaction() override;
+ bool CancelTransaction() override;
+ bool SubmitTransaction() override;
bool Exists(std::string_view key) const override;
bool Delete(std::string_view key) override;
@@ -128,6 +142,9 @@
bool SetKey(std::string_view key, std::string_view value) override;
bool KeyExists(std::string_view key) const override;
bool DeleteKey(std::string_view key) override;
+ bool CreateTemporaryPrefs() override;
+ bool DeleteTemporaryPrefs() override;
+ bool SwapPrefs() override;
private:
FRIEND_TEST(PrefsTest, GetFileNameForKey);
@@ -139,6 +156,9 @@
bool GetFileNameForKey(std::string_view key,
base::FilePath* filename) const;
+ // Returns path of prefs_tmp used during update_engine checkpointing
+ std::string GetTemporaryDir() const;
+
// Preference store directory.
base::FilePath prefs_dir_;
};
diff --git a/common/prefs_interface.h b/common/prefs_interface.h
index 69ccf68..d56435f 100644
--- a/common/prefs_interface.h
+++ b/common/prefs_interface.h
@@ -105,6 +105,17 @@
virtual void RemoveObserver(std::string_view key,
ObserverInterface* observer) = 0;
+ // create tmp_prefs to write to during checkpointing. Flush will replace prefs
+ // with tmp_prefs
+ virtual bool StartTransaction() = 0;
+
+ // cancel any pending transaction by deleting tmp_prefs
+ virtual bool CancelTransaction() = 0;
+
+ // swap prefs with tmp_prefs checkpointing will use new tmp prefs to resume
+ // updates, otherwise we fall back on old prefs
+ virtual bool SubmitTransaction() = 0;
+
protected:
// Key separator used to create sub key and get file names,
static const char kKeySeparator = '/';
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 3b9f2b6..3bd131d 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -1404,9 +1404,12 @@
return false;
}
Terminator::set_exit_blocked(true);
+ LOG_IF(WARNING, !prefs_->StartTransaction())
+ << "unable to start transaction in checkpointing";
+ DEFER {
+ prefs_->CancelTransaction();
+ };
if (last_updated_operation_num_ != next_operation_num_ || force) {
- // Resets the progress in case we die in the middle of the state update.
- ResetUpdateProgress(prefs_, true);
if (!signatures_message_data_.empty()) {
// Save the signature blob because if the update is interrupted after the
// download phase we don't go through this path anymore. Some alternatives
@@ -1458,6 +1461,9 @@
}
TEST_AND_RETURN_FALSE(
prefs_->SetInt64(kPrefsUpdateStateNextOperation, next_operation_num_));
+ if (!prefs_->SubmitTransaction()) {
+ LOG(ERROR) << "Failed to submit transaction in checkpointing";
+ }
return true;
}
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 12238a7..ce37bd1 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -151,9 +151,9 @@
// Attempts to parse the update metadata starting from the beginning of
// |payload|. On success, returns kMetadataParseSuccess. Returns
- // kMetadataParseInsufficientData if more data is needed to parse the complete
- // metadata. Returns kMetadataParseError if the metadata can't be parsed given
- // the payload.
+ // kMetadataParseInsufficientData if more data is needed to parse the
+ // complete metadata. Returns kMetadataParseError if the metadata can't be
+ // parsed given the payload.
MetadataParseResult ParsePayloadMetadata(const brillo::Blob& payload,
ErrorCode* error);