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 <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
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) << ")";