update_engine: Clear lingering install indication
When an install for DLC is initiated, the member variable |is_install_|
is set to true, however the issue arises from that fact that it is never
reverted to be false. This can cause periodic updates to still have the
lingering |is_install_| as true from a previous install. The state must
be correctly cleaned up/cleared.
|ProcessingDone()| will be called at the end of an install/update. This
also depends on the fact that |is_install_| will not be changed by the
time |OnUpdateScheduled()| is scheduled periodically or forced up until
|ProcessingDone()| for an install or update is finished.
The new approach is to have |ProcessingDone()| dispatch to two different
methods (|ProcessingDoneInstall()| and |ProcessingDoneUpdate()|) based
on what the action finished on (an install or update indicated by
|is_install_|). Then those dispatched actions will be performed, then
the |is_install_| can be reset.
BUG=chromium:982929
TEST=FEATURES="test" P2_TEST_FILTER="*UpdateAttempterTest.*-*RunAsRoot*" emerge-$BOARD update_engine
TEST=FEATURES="test" emerge-$BOARD update_engine
Change-Id: I1fb9387dd416ed0815964cfcb22a6559ab81fa80
diff --git a/update_attempter.cc b/update_attempter.cc
index 31d728b..dc7a4b5 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -1038,11 +1038,8 @@
}
}
-// Delegate methods:
-void UpdateAttempter::ProcessingDone(const ActionProcessor* processor,
- ErrorCode code) {
- LOG(INFO) << "Processing Done.";
-
+void UpdateAttempter::ProcessingDoneInternal(const ActionProcessor* processor,
+ ErrorCode code) {
// Reset cpu shares back to normal.
cpu_limiter_.StopLimiter();
@@ -1064,84 +1061,101 @@
attempt_error_code_ = utils::GetBaseErrorCode(code);
- if (code == ErrorCode::kSuccess) {
- // For install operation, we do not mark update complete since we do not
- // need reboot.
- if (!is_install_)
- WriteUpdateCompletedMarker();
- ReportTimeToUpdateAppliedMetric();
-
- prefs_->SetInt64(kPrefsDeltaUpdateFailures, 0);
- prefs_->SetString(kPrefsPreviousVersion,
- omaha_request_params_->app_version());
- DeltaPerformer::ResetUpdateProgress(prefs_, false);
-
- system_state_->payload_state()->UpdateSucceeded();
-
- // Since we're done with scattering fully at this point, this is the
- // safest point delete the state files, as we're sure that the status is
- // set to reboot (which means no more updates will be applied until reboot)
- // This deletion is required for correctness as we want the next update
- // check to re-create a new random number for the update check count.
- // Similarly, we also delete the wall-clock-wait period that was persisted
- // so that we start with a new random value for the next update check
- // after reboot so that the same device is not favored or punished in any
- // way.
- prefs_->Delete(kPrefsUpdateCheckCount);
- system_state_->payload_state()->SetScatteringWaitPeriod(TimeDelta());
- system_state_->payload_state()->SetStagingWaitPeriod(TimeDelta());
- prefs_->Delete(kPrefsUpdateFirstSeenAt);
-
- if (is_install_) {
- LOG(INFO) << "DLC successfully installed, no reboot needed.";
- SetStatusAndNotify(UpdateStatus::IDLE);
- ScheduleUpdates();
+ if (code != ErrorCode::kSuccess) {
+ if (ScheduleErrorEventAction()) {
return;
}
-
- SetStatusAndNotify(UpdateStatus::UPDATED_NEED_REBOOT);
+ LOG(INFO) << "No update.";
+ SetStatusAndNotify(UpdateStatus::IDLE);
ScheduleUpdates();
- LOG(INFO) << "Update successfully applied, waiting to reboot.";
-
- // |install_plan_| is null during rollback operations, and the stats don't
- // make much sense then anyway.
- if (install_plan_) {
- // Generate an unique payload identifier.
- string target_version_uid;
- for (const auto& payload : install_plan_->payloads) {
- target_version_uid +=
- brillo::data_encoding::Base64Encode(payload.hash) + ":" +
- payload.metadata_signature + ":";
- }
-
- // If we just downloaded a rollback image, we should preserve this fact
- // over the following powerwash.
- if (install_plan_->is_rollback) {
- system_state_->payload_state()->SetRollbackHappened(true);
- system_state_->metrics_reporter()->ReportEnterpriseRollbackMetrics(
- /*success=*/true, install_plan_->version);
- }
-
- // Expect to reboot into the new version to send the proper metric during
- // next boot.
- system_state_->payload_state()->ExpectRebootInNewVersion(
- target_version_uid);
- } else {
- // If we just finished a rollback, then we expect to have no Omaha
- // response. Otherwise, it's an error.
- if (system_state_->payload_state()->GetRollbackVersion().empty()) {
- LOG(ERROR) << "Can't send metrics because there was no Omaha response";
- }
- }
return;
}
- if (ScheduleErrorEventAction()) {
- return;
+ ReportTimeToUpdateAppliedMetric();
+ prefs_->SetInt64(kPrefsDeltaUpdateFailures, 0);
+ prefs_->SetString(kPrefsPreviousVersion,
+ omaha_request_params_->app_version());
+ DeltaPerformer::ResetUpdateProgress(prefs_, false);
+
+ system_state_->payload_state()->UpdateSucceeded();
+
+ // Since we're done with scattering fully at this point, this is the
+ // safest point delete the state files, as we're sure that the status is
+ // set to reboot (which means no more updates will be applied until reboot)
+ // This deletion is required for correctness as we want the next update
+ // check to re-create a new random number for the update check count.
+ // Similarly, we also delete the wall-clock-wait period that was persisted
+ // so that we start with a new random value for the next update check
+ // after reboot so that the same device is not favored or punished in any
+ // way.
+ prefs_->Delete(kPrefsUpdateCheckCount);
+ system_state_->payload_state()->SetScatteringWaitPeriod(TimeDelta());
+ system_state_->payload_state()->SetStagingWaitPeriod(TimeDelta());
+ prefs_->Delete(kPrefsUpdateFirstSeenAt);
+
+ // Note: below this comment should only be on |ErrorCode::kSuccess|.
+ if (is_install_) {
+ ProcessingDoneInstall(processor, code);
+ } else {
+ ProcessingDoneUpdate(processor, code);
}
- LOG(INFO) << "No update.";
+}
+
+void UpdateAttempter::ProcessingDoneInstall(const ActionProcessor* processor,
+ ErrorCode code) {
SetStatusAndNotify(UpdateStatus::IDLE);
ScheduleUpdates();
+ LOG(INFO) << "DLC successfully installed, no reboot needed.";
+}
+
+void UpdateAttempter::ProcessingDoneUpdate(const ActionProcessor* processor,
+ ErrorCode code) {
+ WriteUpdateCompletedMarker();
+
+ SetStatusAndNotify(UpdateStatus::UPDATED_NEED_REBOOT);
+ ScheduleUpdates();
+ LOG(INFO) << "Update successfully applied, waiting to reboot.";
+
+ // |install_plan_| is null during rollback operations, and the stats don't
+ // make much sense then anyway.
+ if (install_plan_) {
+ // Generate an unique payload identifier.
+ string target_version_uid;
+ for (const auto& payload : install_plan_->payloads) {
+ target_version_uid += brillo::data_encoding::Base64Encode(payload.hash) +
+ ":" + payload.metadata_signature + ":";
+ }
+
+ // If we just downloaded a rollback image, we should preserve this fact
+ // over the following powerwash.
+ if (install_plan_->is_rollback) {
+ system_state_->payload_state()->SetRollbackHappened(true);
+ system_state_->metrics_reporter()->ReportEnterpriseRollbackMetrics(
+ /*success=*/true, install_plan_->version);
+ }
+
+ // Expect to reboot into the new version to send the proper metric during
+ // next boot.
+ system_state_->payload_state()->ExpectRebootInNewVersion(
+ target_version_uid);
+ } else {
+ // If we just finished a rollback, then we expect to have no Omaha
+ // response. Otherwise, it's an error.
+ if (system_state_->payload_state()->GetRollbackVersion().empty()) {
+ LOG(ERROR) << "Can't send metrics because there was no Omaha response";
+ }
+ }
+}
+
+// Delegate methods:
+void UpdateAttempter::ProcessingDone(const ActionProcessor* processor,
+ ErrorCode code) {
+ LOG(INFO) << "Processing Done.";
+ ProcessingDoneInternal(processor, code);
+
+ // Note: do cleanups here for any variables that need to be reset after a
+ // failure, error, update, or install.
+ is_install_ = false;
}
void UpdateAttempter::ProcessingStopped(const ActionProcessor* processor) {
diff --git a/update_attempter.h b/update_attempter.h
index 880e975..3db4097 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -259,6 +259,8 @@
FRIEND_TEST(UpdateAttempterTest, InstallSetsStatusIdle);
FRIEND_TEST(UpdateAttempterTest, MarkDeltaUpdateFailureTest);
FRIEND_TEST(UpdateAttempterTest, PingOmahaTest);
+ FRIEND_TEST(UpdateAttempterTest, ProcessingDoneInstallError);
+ FRIEND_TEST(UpdateAttempterTest, ProcessingDoneUpdateError);
FRIEND_TEST(UpdateAttempterTest, ReportDailyMetrics);
FRIEND_TEST(UpdateAttempterTest, RollbackNotAllowed);
FRIEND_TEST(UpdateAttempterTest, RollbackAfterInstall);
@@ -284,6 +286,11 @@
// parameters used in the current update attempt.
uint32_t GetErrorCodeFlags();
+ // ActionProcessorDelegate methods |ProcessingDone()| internal helpers.
+ void ProcessingDoneInternal(const ActionProcessor* processor, ErrorCode code);
+ void ProcessingDoneUpdate(const ActionProcessor* processor, ErrorCode code);
+ void ProcessingDoneInstall(const ActionProcessor* processor, ErrorCode code);
+
// CertificateChecker::Observer method.
// Report metrics about the certificate being checked.
void CertificateChecked(ServerToCheck server_to_check,
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 254579c..4b9bc75 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -127,6 +127,19 @@
bool should_update_be_called = false;
};
+struct ProcessingDoneTestParams {
+ // Setups + Inputs:
+ bool is_install = false;
+ UpdateStatus status = UpdateStatus::CHECKING_FOR_UPDATE;
+ ActionProcessor* processor = nullptr;
+ ErrorCode code = ErrorCode::kSuccess;
+
+ // Expects:
+ const bool kExpectedIsInstall = false;
+ bool should_schedule_updates_be_called = true;
+ UpdateStatus expected_exit_status = UpdateStatus::IDLE;
+};
+
class MockDlcService : public DlcServiceInterface {
public:
MOCK_METHOD1(GetInstalled, bool(vector<string>*));
@@ -302,6 +315,9 @@
// |OnUpdateScheduled()| related member functions.
void TestOnUpdateScheduled();
+ // |ProcessingDone()| related member functions.
+ void TestProcessingDone();
+
base::MessageLoopForIO base_loop_;
brillo::BaseMessageLoop loop_{&base_loop_};
@@ -323,6 +339,9 @@
// |OnUpdateScheduled()| test params.
OnUpdateScheduledTestParams ous_params_;
+ // |ProcessingDone()| test params.
+ ProcessingDoneTestParams pd_params_;
+
bool actual_using_p2p_for_downloading_;
bool actual_using_p2p_for_sharing_;
};
@@ -350,6 +369,22 @@
attempter_.WasScheduleUpdatesCalled());
}
+void UpdateAttempterTest::TestProcessingDone() {
+ // Setup
+ attempter_.DisableScheduleUpdates();
+ attempter_.is_install_ = pd_params_.is_install;
+ attempter_.status_ = pd_params_.status;
+
+ // Invocation
+ attempter_.ProcessingDone(pd_params_.processor, pd_params_.code);
+
+ // Verify
+ EXPECT_EQ(pd_params_.kExpectedIsInstall, attempter_.is_install_);
+ EXPECT_EQ(pd_params_.should_schedule_updates_be_called,
+ attempter_.WasScheduleUpdatesCalled());
+ EXPECT_EQ(pd_params_.expected_exit_status, attempter_.status_);
+}
+
void UpdateAttempterTest::ScheduleQuitMainLoop() {
loop_.PostTask(
FROM_HERE,
@@ -1961,6 +1996,106 @@
attempter_.ProcessingDone(nullptr, ErrorCode::kSuccess);
}
+TEST_F(UpdateAttempterTest, ProcessingDoneUpdated) {
+ // GIVEN an update finished.
+
+ // THEN need reboot since update applied.
+ pd_params_.expected_exit_status = UpdateStatus::UPDATED_NEED_REBOOT;
+ // THEN install indication should be false.
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneInstalled) {
+ // GIVEN an install finished.
+ pd_params_.is_install = true;
+
+ // THEN go idle.
+ // THEN install indication should be false.
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneInstallReportingError) {
+ // GIVEN an install finished.
+ pd_params_.is_install = true;
+ // GIVEN a reporting error occurred.
+ pd_params_.status = UpdateStatus::REPORTING_ERROR_EVENT;
+
+ // THEN go idle.
+ // THEN install indication should be false.
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneNoUpdate) {
+ // GIVEN an update finished.
+ // GIVEN an action error occured.
+ pd_params_.code = ErrorCode::kNoUpdate;
+
+ // THEN go idle.
+ // THEN install indication should be false.
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneNoInstall) {
+ // GIVEN an install finished.
+ pd_params_.is_install = true;
+ // GIVEN an action error occured.
+ pd_params_.code = ErrorCode::kNoUpdate;
+
+ // THEN go idle.
+ // THEN install indication should be false.
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneUpdateError) {
+ // GIVEN an update finished.
+ // GIVEN an action error occured.
+ pd_params_.code = ErrorCode::kError;
+ // GIVEN an event error is set.
+ attempter_.error_event_.reset(new OmahaEvent(OmahaEvent::kTypeUpdateComplete,
+ OmahaEvent::kResultError,
+ ErrorCode::kError));
+
+ // THEN indicate a error event.
+ pd_params_.expected_exit_status = UpdateStatus::REPORTING_ERROR_EVENT;
+ // THEN install indication should be false.
+
+ // THEN expect critical actions of |ScheduleErrorEventAction()|.
+ EXPECT_CALL(*processor_, EnqueueAction(Pointee(_))).Times(1);
+ EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ // THEN |ScheduleUpdates()| will be called next |ProcessingDone()| so skip.
+ pd_params_.should_schedule_updates_be_called = false;
+
+ TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneInstallError) {
+ // GIVEN an install finished.
+ pd_params_.is_install = true;
+ // GIVEN an action error occured.
+ pd_params_.code = ErrorCode::kError;
+ // GIVEN an event error is set.
+ attempter_.error_event_.reset(new OmahaEvent(OmahaEvent::kTypeUpdateComplete,
+ OmahaEvent::kResultError,
+ ErrorCode::kError));
+
+ // THEN indicate a error event.
+ pd_params_.expected_exit_status = UpdateStatus::REPORTING_ERROR_EVENT;
+ // THEN install indication should be false.
+
+ // THEN expect critical actions of |ScheduleErrorEventAction()|.
+ EXPECT_CALL(*processor_, EnqueueAction(Pointee(_))).Times(1);
+ EXPECT_CALL(*processor_, StartProcessing()).Times(1);
+ // THEN |ScheduleUpdates()| will be called next |ProcessingDone()| so skip.
+ pd_params_.should_schedule_updates_be_called = false;
+
+ TestProcessingDone();
+}
+
void UpdateAttempterTest::UpdateToQuickFixBuildStart(bool set_token) {
// Tests that checks if |device_quick_fix_build_token| arrives when
// policy is set and the device is enterprise enrolled based on |set_token|.