update_engine: Report VPD write failure to UMA Currently we see many first actives to come from non-FSI images. But we have not been able to figure out why. This CL, reports a new error kFirstActiveOmahaPingSentPersistenceError when writing the first active omaha flag into VPD fails. This allows us to see if that is the actual cause of the problem. CL:1062659 adds the enum value on the Chrome side. BUG=chromium:833980 TEST=unittests TEST=precq Change-Id: I65e233c5f895489ba905494fb20d7b00d0c4af10 Reviewed-on: https://chromium-review.googlesource.com/1062662 Commit-Ready: ChromeOS CL Exonerator Bot <[email protected]> Tested-by: Amin Hassani <[email protected]> Reviewed-by: Amin Hassani <[email protected]> Reviewed-by: Ben Chan <[email protected]>
diff --git a/common/error_code.h b/common/error_code.h index c301155..a7fee2a 100644 --- a/common/error_code.h +++ b/common/error_code.h
@@ -78,6 +78,7 @@ kUpdatedButNotActive = 52, kNoUpdate = 53, kRollbackNotPossible = 54, + kFirstActiveOmahaPingSentPersistenceError = 55, // VERY IMPORTANT! When adding new error codes: //
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc index a0e75f0..2a2a0a3 100644 --- a/common/error_code_utils.cc +++ b/common/error_code_utils.cc
@@ -152,6 +152,8 @@ return "ErrorCode::kNoUpdate"; case ErrorCode::kRollbackNotPossible: return "ErrorCode::kRollbackNotPossible"; + case ErrorCode::kFirstActiveOmahaPingSentPersistenceError: + return "ErrorCode::kFirstActiveOmahaPingSentPersistenceError"; // Don't add a default case to let the compiler warn about newly added // error codes which should be added here. }
diff --git a/common/fake_hardware.h b/common/fake_hardware.h index d699fb7..d68b0f8 100644 --- a/common/fake_hardware.h +++ b/common/fake_hardware.h
@@ -110,8 +110,9 @@ return first_active_omaha_ping_sent_; } - void SetFirstActiveOmahaPingSent() override { + bool SetFirstActiveOmahaPingSent() override { first_active_omaha_ping_sent_ = true; + return true; } // Setters
diff --git a/common/hardware_interface.h b/common/hardware_interface.h index 4d7c162..239e7c8 100644 --- a/common/hardware_interface.h +++ b/common/hardware_interface.h
@@ -110,9 +110,9 @@ // |SetFirstActiveOmahaPingSent()|. virtual bool GetFirstActiveOmahaPingSent() const = 0; - // Persist the fact that first active ping was sent to omaha. It bails out if - // it fails. - virtual void SetFirstActiveOmahaPingSent() = 0; + // Persist the fact that first active ping was sent to omaha and returns false + // if failed to persist it. + virtual bool SetFirstActiveOmahaPingSent() = 0; }; } // namespace chromeos_update_engine
diff --git a/hardware_android.cc b/hardware_android.cc index 7958fbf..0e5abaa 100644 --- a/hardware_android.cc +++ b/hardware_android.cc
@@ -214,9 +214,10 @@ return false; } -void HardwareAndroid::SetFirstActiveOmahaPingSent() { - LOG(WARNING) << "STUB: Assuming first active omaha is never set."; - return; +bool HardwareAndroid::SetFirstActiveOmahaPingSent() { + LOG(WARNING) << "STUB: Assuming first active omaha is set."; + // We will set it true, so its failure doesn't cause escalation. + return true; } } // namespace chromeos_update_engine
diff --git a/hardware_android.h b/hardware_android.h index a6c9f6a..981f033 100644 --- a/hardware_android.h +++ b/hardware_android.h
@@ -51,7 +51,7 @@ bool GetNonVolatileDirectory(base::FilePath* path) const override; bool GetPowerwashSafeDirectory(base::FilePath* path) const override; bool GetFirstActiveOmahaPingSent() const override; - void SetFirstActiveOmahaPingSent() override; + bool SetFirstActiveOmahaPingSent() override; private: DISALLOW_COPY_AND_ASSIGN(HardwareAndroid);
diff --git a/hardware_chromeos.cc b/hardware_chromeos.cc index c0f2b67..08303d0 100644 --- a/hardware_chromeos.cc +++ b/hardware_chromeos.cc
@@ -287,7 +287,7 @@ return static_cast<bool>(active_ping); } -void HardwareChromeOS::SetFirstActiveOmahaPingSent() { +bool HardwareChromeOS::SetFirstActiveOmahaPingSent() { int exit_code = 0; string output; vector<string> vpd_set_cmd = { @@ -297,7 +297,7 @@ LOG(ERROR) << "Failed to set vpd key for " << kActivePingKey << " with exit code: " << exit_code << " with error: " << output; - return; + return false; } vector<string> vpd_dump_cmd = { "dump_vpd_log", "--force" }; @@ -306,7 +306,9 @@ LOG(ERROR) << "Failed to cache " << kActivePingKey<< " using dump_vpd_log" << " with exit code: " << exit_code << " with error: " << output; + return false; } + return true; } } // namespace chromeos_update_engine
diff --git a/hardware_chromeos.h b/hardware_chromeos.h index 8e9ad1e..6bdd37a 100644 --- a/hardware_chromeos.h +++ b/hardware_chromeos.h
@@ -56,7 +56,7 @@ bool GetNonVolatileDirectory(base::FilePath* path) const override; bool GetPowerwashSafeDirectory(base::FilePath* path) const override; bool GetFirstActiveOmahaPingSent() const override; - void SetFirstActiveOmahaPingSent() override; + bool SetFirstActiveOmahaPingSent() override; private: friend class HardwareChromeOSTest;
diff --git a/metrics_reporter_android.h b/metrics_reporter_android.h index ee94e43..44f770e 100644 --- a/metrics_reporter_android.h +++ b/metrics_reporter_android.h
@@ -79,6 +79,8 @@ void ReportInstallDateProvisioningSource(int source, int max) override {} + void ReportInternalErrorCode(ErrorCode error_code) override {} + private: DISALLOW_COPY_AND_ASSIGN(MetricsReporterAndroid); };
diff --git a/metrics_reporter_interface.h b/metrics_reporter_interface.h index 2c7ce5b..13f5a54 100644 --- a/metrics_reporter_interface.h +++ b/metrics_reporter_interface.h
@@ -193,6 +193,12 @@ // // |kMetricInstallDateProvisioningSource| virtual void ReportInstallDateProvisioningSource(int source, int max) = 0; + + // Helper function to report an internal error code. The following metrics are + // reported: + // + // |kMetricAttemptInternalErrorCode| + virtual void ReportInternalErrorCode(ErrorCode error_code) = 0; }; } // namespace chromeos_update_engine
diff --git a/metrics_reporter_omaha.cc b/metrics_reporter_omaha.cc index 0397b83..9c81088 100644 --- a/metrics_reporter_omaha.cc +++ b/metrics_reporter_omaha.cc
@@ -269,12 +269,7 @@ static_cast<int>(metrics::AttemptResult::kNumConstants)); if (internal_error_code != ErrorCode::kSuccess) { - metric = metrics::kMetricAttemptInternalErrorCode; - LOG(INFO) << "Uploading " << internal_error_code << " for metric " - << metric; - metrics_lib_->SendEnumToUMA(metric, - static_cast<int>(internal_error_code), - static_cast<int>(ErrorCode::kUmaReportedMax)); + ReportInternalErrorCode(internal_error_code); } base::TimeDelta time_since_last; @@ -535,4 +530,12 @@ max); } +void MetricsReporterOmaha::ReportInternalErrorCode(ErrorCode error_code) { + auto metric = metrics::kMetricAttemptInternalErrorCode; + LOG(INFO) << "Uploading " << error_code << " for metric " << metric; + metrics_lib_->SendEnumToUMA(metric, + static_cast<int>(error_code), + static_cast<int>(ErrorCode::kUmaReportedMax)); +} + } // namespace chromeos_update_engine
diff --git a/metrics_reporter_omaha.h b/metrics_reporter_omaha.h index c19fe86..344cff8 100644 --- a/metrics_reporter_omaha.h +++ b/metrics_reporter_omaha.h
@@ -143,6 +143,8 @@ void ReportInstallDateProvisioningSource(int source, int max) override; + void ReportInternalErrorCode(ErrorCode error_code) override; + private: friend class MetricsReporterOmahaTest;
diff --git a/metrics_utils.cc b/metrics_utils.cc index c84aa8f..b85f257 100644 --- a/metrics_utils.cc +++ b/metrics_utils.cc
@@ -116,6 +116,7 @@ case ErrorCode::kOmahaRequestXMLHasEntityDecl: case ErrorCode::kOmahaUpdateIgnoredOverCellular: case ErrorCode::kNoUpdate: + case ErrorCode::kFirstActiveOmahaPingSentPersistenceError: return metrics::AttemptResult::kInternalError; // Special flags. These can't happen (we mask them out above) but @@ -220,6 +221,7 @@ case ErrorCode::kUpdatedButNotActive: case ErrorCode::kNoUpdate: case ErrorCode::kRollbackNotPossible: + case ErrorCode::kFirstActiveOmahaPingSentPersistenceError: break; // Special flags. These can't happen (we mask them out above) but
diff --git a/mock_metrics_reporter.h b/mock_metrics_reporter.h index a0f164b..a4e0a12 100644 --- a/mock_metrics_reporter.h +++ b/mock_metrics_reporter.h
@@ -76,6 +76,8 @@ MOCK_METHOD1(ReportTimeToReboot, void(int time_to_reboot_minutes)); MOCK_METHOD2(ReportInstallDateProvisioningSource, void(int source, int max)); + + MOCK_METHOD1(ReportInternalErrorCode, void(ErrorCode error_code)); }; } // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc index 0d529a9..72a7d84 100644 --- a/omaha_request_action.cc +++ b/omaha_request_action.cc
@@ -1188,7 +1188,10 @@ // their a=-1 in the past and we have to set first_active_omaha_ping_sent for // future checks. if (!system_state_->hardware()->GetFirstActiveOmahaPingSent()) { - system_state_->hardware()->SetFirstActiveOmahaPingSent(); + if (!system_state_->hardware()->SetFirstActiveOmahaPingSent()) { + system_state_->metrics_reporter()->ReportInternalErrorCode( + ErrorCode::kFirstActiveOmahaPingSentPersistenceError); + } } if (!HasOutputPipe()) {
diff --git a/payload_state.cc b/payload_state.cc index 03f74af..c07fe7a 100644 --- a/payload_state.cc +++ b/payload_state.cc
@@ -360,6 +360,7 @@ case ErrorCode::kUpdatedButNotActive: case ErrorCode::kNoUpdate: case ErrorCode::kRollbackNotPossible: + case ErrorCode::kFirstActiveOmahaPingSentPersistenceError: LOG(INFO) << "Not incrementing URL index or failure count for this error"; break;
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc index d56a22e..04d9680 100644 --- a/update_manager/chromeos_policy.cc +++ b/update_manager/chromeos_policy.cc
@@ -145,6 +145,7 @@ case ErrorCode::kUpdatedButNotActive: case ErrorCode::kNoUpdate: case ErrorCode::kRollbackNotPossible: + case ErrorCode::kFirstActiveOmahaPingSentPersistenceError: LOG(INFO) << "Not changing URL index or failure count due to error " << chromeos_update_engine::utils::ErrorCodeToString(err_code) << " (" << static_cast<int>(err_code) << ")";