update_engine: Call dlcservice's Install/Update Completion DBus APIs

When an Install/Update completes, update_engine will now let dlcservice
know all the DLCs that were installed/updated + verified.

Update_engine will also track during install/update for DLCs which did
not install/update so dlcservice receives the correct list of DLC IDs.

BUG=chromium:1059126
TEST=FEATURES=test emerge-$B update_engine update_engine-client

Cq-Depend: chromium:2141191
Change-Id: Id57f66c7c6957d34870d27119d9a6482fe902503
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2146104
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Andrew Lassalle <andrewlassalle@chromium.org>
Auto-Submit: Jae Hoon Kim <kimjae@chromium.org>
diff --git a/common/dlcservice_interface.h b/common/dlcservice_interface.h
index aa24105..3524d50 100644
--- a/common/dlcservice_interface.h
+++ b/common/dlcservice_interface.h
@@ -34,6 +34,14 @@
   // On failure it returns false.
   virtual bool GetInstalled(std::vector<std::string>* dlc_module_ids) = 0;
 
+  // Returns true if dlcservice successfully handled the install completion
+  // method call, otherwise false.
+  virtual bool InstallCompleted(const std::vector<std::string>& ids) = 0;
+
+  // Returns true if dlcservice successfully handled the update completion
+  // method call, otherwise false.
+  virtual bool UpdateCompleted(const std::vector<std::string>& ids) = 0;
+
  protected:
   DlcServiceInterface() = default;
 
diff --git a/common/dlcservice_stub.cc b/common/dlcservice_stub.cc
index c5f9306..3dcb2e0 100644
--- a/common/dlcservice_stub.cc
+++ b/common/dlcservice_stub.cc
@@ -33,4 +33,11 @@
   return true;
 }
 
+bool DlcServiceStub::InstallCompleted(const vector<string>& ids) {
+  return true;
+}
+bool DlcServiceStub::UpdateCompleted(const vector<string>& ids) {
+  return true;
+}
+
 }  // namespace chromeos_update_engine
diff --git a/common/dlcservice_stub.h b/common/dlcservice_stub.h
index 4e12c11..9b27971 100644
--- a/common/dlcservice_stub.h
+++ b/common/dlcservice_stub.h
@@ -32,6 +32,8 @@
 
   // BootControlInterface overrides.
   bool GetInstalled(std::vector<std::string>* dlc_module_ids) override;
+  bool InstallCompleted(const std::vector<std::string>& ids) override;
+  bool UpdateCompleted(const std::vector<std::string>& ids) override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DlcServiceStub);
diff --git a/dlcservice_chromeos.cc b/dlcservice_chromeos.cc
index ad5806a..3c76b2a 100644
--- a/dlcservice_chromeos.cc
+++ b/dlcservice_chromeos.cc
@@ -16,6 +16,7 @@
 
 #include "update_engine/dlcservice_chromeos.h"
 
+#include <brillo/errors/error.h>
 #include <dlcservice/proto_bindings/dlcservice.pb.h>
 // NOLINTNEXTLINE(build/include_alpha) "dbus-proxies.h" needs "dlcservice.pb.h"
 #include <dlcservice/dbus-proxies.h>
@@ -28,6 +29,12 @@
 
 namespace chromeos_update_engine {
 
+namespace {
+org::chromium::DlcServiceInterfaceProxy GetDlcServiceProxy() {
+  return {DBusConnection::Get()->GetDBus()};
+}
+}  // namespace
+
 std::unique_ptr<DlcServiceInterface> CreateDlcService() {
   return std::make_unique<DlcServiceChromeOS>();
 }
@@ -37,11 +44,8 @@
     return false;
   dlc_module_ids->clear();
 
-  org::chromium::DlcServiceInterfaceProxy dlcservice_proxy(
-      DBusConnection::Get()->GetDBus());
-
   dlcservice::DlcModuleList dlc_module_list;
-  if (!dlcservice_proxy.GetInstalled(&dlc_module_list, nullptr)) {
+  if (!GetDlcServiceProxy().GetInstalled(&dlc_module_list, nullptr)) {
     LOG(ERROR) << "dlcservice does not return installed DLC module list.";
     return false;
   }
@@ -51,4 +55,24 @@
   return true;
 }
 
+bool DlcServiceChromeOS::InstallCompleted(const std::vector<std::string>& ids) {
+  brillo::ErrorPtr err;
+  if (!GetDlcServiceProxy().InstallCompleted(ids, &err)) {
+    LOG(ERROR) << "dlcservice failed to complete install. ErrCode="
+               << err->GetCode() << ", ErrMsg=" << err->GetMessage();
+    return false;
+  }
+  return true;
+}
+
+bool DlcServiceChromeOS::UpdateCompleted(const std::vector<std::string>& ids) {
+  brillo::ErrorPtr err;
+  if (!GetDlcServiceProxy().UpdateCompleted(ids, &err)) {
+    LOG(ERROR) << "dlcservice failed to complete updated. ErrCode="
+               << err->GetCode() << ", ErrMsg=" << err->GetMessage();
+    return false;
+  }
+  return true;
+}
+
 }  // namespace chromeos_update_engine
diff --git a/dlcservice_chromeos.h b/dlcservice_chromeos.h
index 73442e6..b56b495 100644
--- a/dlcservice_chromeos.h
+++ b/dlcservice_chromeos.h
@@ -32,13 +32,20 @@
   DlcServiceChromeOS() = default;
   ~DlcServiceChromeOS() = default;
 
-  // BootControlInterface overrides.
+  // DlcServiceInterface overrides.
+
   // Will clear the |dlc_module_ids|, passed to be modified. Clearing by
   // default has the added benefit of avoiding indeterminate behavior in the
   // case that |dlc_module_ids| wasn't empty to begin which would lead to
   // possible duplicates and cases when error was not checked it's still safe.
   bool GetInstalled(std::vector<std::string>* dlc_module_ids) override;
 
+  // Call into dlcservice for it to mark the DLC IDs as being installed.
+  bool InstallCompleted(const std::vector<std::string>& ids) override;
+
+  // Call into dlcservice for it to mark the DLC IDs as being updated.
+  bool UpdateCompleted(const std::vector<std::string>& ids) override;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(DlcServiceChromeOS);
 };
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 50fe3cc..85699d8 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -782,6 +782,7 @@
       if (params_->IsDlcAppId(app.id)) {
         LOG(INFO) << "No update for <app> " << app.id
                   << " but update continuing since a DLC.";
+        params_->SetDlcNoUpdate(app.id);
         continue;
       }
       // Don't update if any app has status="noupdate".
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index 8863392..5267598 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -258,4 +258,11 @@
   return dlc_apps_params().find(app_id) != dlc_apps_params().end();
 }
 
+void OmahaRequestParams::SetDlcNoUpdate(const string& app_id) {
+  auto itr = dlc_apps_params_.find(app_id);
+  if (itr == dlc_apps_params_.end())
+    return;
+  itr->second.updated = false;
+}
+
 }  // namespace chromeos_update_engine
diff --git a/omaha_request_params.h b/omaha_request_params.h
index 14f3eaf..b33d0b1 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -76,6 +76,9 @@
     int64_t ping_date_last_active;
     int64_t ping_date_last_rollcall;
     bool send_ping;
+    // |updated| is only used for DLCs to decide sending DBus message to
+    // dlcservice on an install/update completion.
+    bool updated = true;
   };
 
   // Setters and getters for the various properties.
@@ -232,6 +235,9 @@
   // request parameters.
   virtual bool IsDlcAppId(const std::string& app_id) const;
 
+  // If the App ID is a DLC App ID will set to no update.
+  void SetDlcNoUpdate(const std::string& app_id);
+
   // Suggested defaults
   static const char kOsVersion[];
   static const int64_t kDefaultMinUpdateChecks = 0;
diff --git a/update_attempter.cc b/update_attempter.cc
index 29d256c..c45fe4f 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -1219,8 +1219,18 @@
   }
 }
 
+vector<string> UpdateAttempter::GetSuccessfulDlcIds() {
+  vector<string> dlc_ids;
+  for (const auto& pr : omaha_request_params_->dlc_apps_params())
+    if (pr.second.updated)
+      dlc_ids.push_back(pr.second.name);
+  return dlc_ids;
+}
+
 void UpdateAttempter::ProcessingDoneInstall(const ActionProcessor* processor,
                                             ErrorCode code) {
+  if (!system_state_->dlcservice()->InstallCompleted(GetSuccessfulDlcIds()))
+    LOG(WARNING) << "dlcservice didn't successfully handle install completion.";
   SetStatusAndNotify(UpdateStatus::IDLE);
   ScheduleUpdates();
   LOG(INFO) << "DLC successfully installed, no reboot needed.";
@@ -1230,6 +1240,8 @@
                                            ErrorCode code) {
   WriteUpdateCompletedMarker();
 
+  if (!system_state_->dlcservice()->UpdateCompleted(GetSuccessfulDlcIds()))
+    LOG(WARNING) << "dlcservice didn't successfully handle update completion.";
   SetStatusAndNotify(UpdateStatus::UPDATED_NEED_REBOOT);
   ScheduleUpdates();
   LOG(INFO) << "Update successfully applied, waiting to reboot.";
diff --git a/update_attempter.h b/update_attempter.h
index c364de3..3c6f4a1 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -292,6 +292,7 @@
   FRIEND_TEST(UpdateAttempterTest, UpdateAttemptFlagsCachedAtUpdateStart);
   FRIEND_TEST(UpdateAttempterTest, UpdateDeferredByPolicyTest);
   FRIEND_TEST(UpdateAttempterTest, UpdateIsNotRunningWhenUpdateAvailable);
+  FRIEND_TEST(UpdateAttempterTest, GetSuccessfulDlcIds);
 
   // Returns the special flags to be added to ErrorCode values based on the
   // parameters used in the current update attempt.
@@ -450,6 +451,10 @@
   // parameter on the |omaha_request_params_| object.
   void CalculateDlcParams();
 
+  // Returns the list of DLC IDs that were installed/updated, excluding the ones
+  // which had "noupdate" in the Omaha response.
+  std::vector<std::string> GetSuccessfulDlcIds();
+
   // Last status notification timestamp used for throttling. Use monotonic
   // TimeTicks to ensure that notifications are sent even if the system clock is
   // set back in the middle of an update.
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index d65a556..5849c38 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -19,7 +19,9 @@
 #include <stdint.h>
 
 #include <limits>
+#include <map>
 #include <memory>
+#include <string>
 #include <unordered_set>
 
 #include <base/files/file_util.h>
@@ -64,12 +66,14 @@
 using chromeos_update_manager::StagingSchedule;
 using chromeos_update_manager::UpdateCheckParams;
 using policy::DevicePolicy;
+using std::map;
 using std::string;
 using std::unique_ptr;
 using std::unordered_set;
 using std::vector;
 using testing::_;
 using testing::DoAll;
+using testing::ElementsAre;
 using testing::Field;
 using testing::InSequence;
 using testing::Invoke;
@@ -135,16 +139,23 @@
   UpdateStatus status = UpdateStatus::CHECKING_FOR_UPDATE;
   ActionProcessor* processor = nullptr;
   ErrorCode code = ErrorCode::kSuccess;
+  map<string, OmahaRequestParams::AppParams> dlc_apps_params;
 
   // Expects:
   const bool kExpectedIsInstall = false;
   bool should_schedule_updates_be_called = true;
   UpdateStatus expected_exit_status = UpdateStatus::IDLE;
+  bool should_install_completed_be_called = false;
+  bool should_update_completed_be_called = false;
+  vector<string> args_to_install_completed;
+  vector<string> args_to_update_completed;
 };
 
 class MockDlcService : public DlcServiceInterface {
  public:
   MOCK_METHOD1(GetInstalled, bool(vector<string>*));
+  MOCK_METHOD1(InstallCompleted, bool(const vector<string>&));
+  MOCK_METHOD1(UpdateCompleted, bool(const vector<string>&));
 };
 
 }  // namespace
@@ -376,6 +387,22 @@
   attempter_.DisableScheduleUpdates();
   attempter_.is_install_ = pd_params_.is_install;
   attempter_.status_ = pd_params_.status;
+  attempter_.omaha_request_params_->set_dlc_apps_params(
+      pd_params_.dlc_apps_params);
+
+  // Expects
+  if (pd_params_.should_install_completed_be_called)
+    EXPECT_CALL(mock_dlcservice_,
+                InstallCompleted(pd_params_.args_to_install_completed))
+        .WillOnce(Return(true));
+  else
+    EXPECT_CALL(mock_dlcservice_, InstallCompleted(_)).Times(0);
+  if (pd_params_.should_update_completed_be_called)
+    EXPECT_CALL(mock_dlcservice_,
+                UpdateCompleted(pd_params_.args_to_update_completed))
+        .WillOnce(Return(true));
+  else
+    EXPECT_CALL(mock_dlcservice_, UpdateCompleted(_)).Times(0);
 
   // Invocation
   attempter_.ProcessingDone(pd_params_.processor, pd_params_.code);
@@ -2001,6 +2028,25 @@
 TEST_F(UpdateAttempterTest, ProcessingDoneUpdated) {
   // GIVEN an update finished.
 
+  // THEN update_engine should call update completion.
+  pd_params_.should_update_completed_be_called = true;
+  // 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, ProcessingDoneUpdatedDlcFilter) {
+  // GIVEN an update finished.
+  // GIVEN DLC |AppParams| list.
+  auto dlc_1 = "dlc_1", dlc_2 = "dlc_2";
+  pd_params_.dlc_apps_params = {{dlc_1, {.name = dlc_1, .updated = false}},
+                                {dlc_2, {.name = dlc_2}}};
+
+  // THEN update_engine should call update completion.
+  pd_params_.should_update_completed_be_called = true;
+  pd_params_.args_to_update_completed = {dlc_2};
   // THEN need reboot since update applied.
   pd_params_.expected_exit_status = UpdateStatus::UPDATED_NEED_REBOOT;
   // THEN install indication should be false.
@@ -2012,6 +2058,25 @@
   // GIVEN an install finished.
   pd_params_.is_install = true;
 
+  // THEN update_engine should call install completion.
+  pd_params_.should_install_completed_be_called = true;
+  // THEN go idle.
+  // THEN install indication should be false.
+
+  TestProcessingDone();
+}
+
+TEST_F(UpdateAttempterTest, ProcessingDoneInstalledDlcFilter) {
+  // GIVEN an install finished.
+  pd_params_.is_install = true;
+  // GIVEN DLC |AppParams| list.
+  auto dlc_1 = "dlc_1", dlc_2 = "dlc_2";
+  pd_params_.dlc_apps_params = {{dlc_1, {.name = dlc_1, .updated = false}},
+                                {dlc_2, {.name = dlc_2}}};
+
+  // THEN update_engine should call install completion.
+  pd_params_.should_install_completed_be_called = true;
+  pd_params_.args_to_install_completed = {dlc_2};
   // THEN go idle.
   // THEN install indication should be false.
 
@@ -2024,6 +2089,7 @@
   // GIVEN a reporting error occurred.
   pd_params_.status = UpdateStatus::REPORTING_ERROR_EVENT;
 
+  // THEN update_engine should not call install completion.
   // THEN go idle.
   // THEN install indication should be false.
 
@@ -2035,6 +2101,7 @@
   // GIVEN an action error occured.
   pd_params_.code = ErrorCode::kNoUpdate;
 
+  // THEN update_engine should not call update completion.
   // THEN go idle.
   // THEN install indication should be false.
 
@@ -2047,6 +2114,7 @@
   // GIVEN an action error occured.
   pd_params_.code = ErrorCode::kNoUpdate;
 
+  // THEN update_engine should not call install completion.
   // THEN go idle.
   // THEN install indication should be false.
 
@@ -2066,6 +2134,7 @@
   pd_params_.expected_exit_status = UpdateStatus::REPORTING_ERROR_EVENT;
   // THEN install indication should be false.
 
+  // THEN update_engine should not call update completion.
   // THEN expect critical actions of |ScheduleErrorEventAction()|.
   EXPECT_CALL(*processor_, EnqueueAction(Pointee(_))).Times(1);
   EXPECT_CALL(*processor_, StartProcessing()).Times(1);
@@ -2089,6 +2158,7 @@
   pd_params_.expected_exit_status = UpdateStatus::REPORTING_ERROR_EVENT;
   // THEN install indication should be false.
 
+  // THEN update_engine should not call install completion.
   // THEN expect critical actions of |ScheduleErrorEventAction()|.
   EXPECT_CALL(*processor_, EnqueueAction(Pointee(_))).Times(1);
   EXPECT_CALL(*processor_, StartProcessing()).Times(1);
@@ -2457,4 +2527,13 @@
   EXPECT_FALSE(base::PathExists(metadata_path_dlc0));
 }
 
+TEST_F(UpdateAttempterTest, GetSuccessfulDlcIds) {
+  auto dlc_1 = "1", dlc_2 = "2", dlc_3 = "3";
+  attempter_.omaha_request_params_->set_dlc_apps_params(
+      {{dlc_1, {.name = dlc_1, .updated = false}},
+       {dlc_2, {.name = dlc_2}},
+       {dlc_3, {.name = dlc_3, .updated = false}}});
+  EXPECT_THAT(attempter_.GetSuccessfulDlcIds(), ElementsAre(dlc_2));
+}
+
 }  // namespace chromeos_update_engine