Fix error code for no update.
PayloadState does not get notified if there's no update, so the error
code will not be updated, move this to UpdateAttempter.
We were using the generic kError for no update, this change added a
new error code kNoUpdate for this, now update_engine_client will show
kNoUpdate instead of kSuccess or a random last error.
Bug: 36946608
Test: update_engine_client --check_for_update --follow
Change-Id: Ie1e94841788437140e0894dc2e73c314a7564108
(cherry picked from commit 89e24c11b406fe048382bdf1c89334e10417b899)
Reviewed-on: https://chromium-review.googlesource.com/1055857
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Nicolas Norvez <norvez@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>
diff --git a/common/error_code.h b/common/error_code.h
index d6c8080..9e7c71d 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -76,6 +76,7 @@
kOmahaUpdateIgnoredOverCellular = 50,
// kPayloadTimestampError = 51,
kUpdatedButNotActive = 52,
+ kNoUpdate = 53,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index 2c485a2..6b72eee 100644
--- a/common/error_code_utils.cc
+++ b/common/error_code_utils.cc
@@ -148,6 +148,8 @@
return "ErrorCode::kOmahaUpdateIgnoredOverCellular";
case ErrorCode::kUpdatedButNotActive:
return "ErrorCode::kUpdatedButNotActive";
+ case ErrorCode::kNoUpdate:
+ return "ErrorCode::kNoUpdate";
// 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_service.cc b/common_service.cc
index 6614622..2fdd700 100644
--- a/common_service.cc
+++ b/common_service.cc
@@ -390,7 +390,8 @@
bool UpdateEngineService::GetLastAttemptError(ErrorPtr* /* error */,
int32_t* out_last_attempt_error) {
- ErrorCode error_code = system_state_->payload_state()->GetAttemptErrorCode();
+ ErrorCode error_code =
+ system_state_->update_attempter()->GetAttemptErrorCode();
*out_last_attempt_error = static_cast<int>(error_code);
return true;
}
diff --git a/metrics_utils.cc b/metrics_utils.cc
index 1500311..0ff4cc9 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -114,6 +114,7 @@
case ErrorCode::kUpdateCanceledByChannelChange:
case ErrorCode::kOmahaRequestXMLHasEntityDecl:
case ErrorCode::kOmahaUpdateIgnoredOverCellular:
+ case ErrorCode::kNoUpdate:
return metrics::AttemptResult::kInternalError;
// Special flags. These can't happen (we mask them out above) but
@@ -216,6 +217,7 @@
case ErrorCode::kUserCanceled:
case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
+ case ErrorCode::kNoUpdate:
break;
// Special flags. These can't happen (we mask them out above) but
diff --git a/mock_payload_state.h b/mock_payload_state.h
index 4cfcac0..f543842 100644
--- a/mock_payload_state.h
+++ b/mock_payload_state.h
@@ -77,7 +77,6 @@
MOCK_CONST_METHOD0(GetUsingP2PForSharing, bool());
MOCK_METHOD0(GetScatteringWaitPeriod, base::TimeDelta());
MOCK_CONST_METHOD0(GetP2PUrl, std::string());
- MOCK_CONST_METHOD0(GetAttemptErrorCode, ErrorCode());
};
} // namespace chromeos_update_engine
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 744a5c8..f1a3310 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -59,6 +59,7 @@
if (!response.update_exists) {
got_no_update_response_ = true;
LOG(INFO) << "There are no updates. Aborting.";
+ completer.set_code(ErrorCode::kNoUpdate);
return;
}
diff --git a/payload_state.cc b/payload_state.cc
index 745ae2e..7c969b3 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -67,7 +67,6 @@
rollback_happened_(false),
attempt_num_bytes_downloaded_(0),
attempt_connection_type_(metrics::ConnectionType::kUnknown),
- attempt_error_code_(ErrorCode::kSuccess),
attempt_type_(AttemptType::kUpdate) {
for (int i = 0; i <= kNumDownloadSources; i++)
total_bytes_downloaded_[i] = current_bytes_downloaded_[i] = 0;
@@ -244,7 +243,6 @@
metrics::RollbackResult::kSuccess);
break;
}
- attempt_error_code_ = ErrorCode::kSuccess;
// Reset the number of responses seen since it counts from the last
// successful update, e.g. now.
@@ -258,7 +256,6 @@
ErrorCode base_error = utils::GetBaseErrorCode(error);
LOG(INFO) << "Updating payload state for error code: " << base_error
<< " (" << utils::ErrorCodeToString(base_error) << ")";
- attempt_error_code_ = base_error;
if (candidate_urls_.size() == 0) {
// This means we got this error even before we got a valid Omaha response
@@ -361,6 +358,7 @@
case ErrorCode::kUserCanceled:
case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
+ case ErrorCode::kNoUpdate:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/payload_state.h b/payload_state.h
index 0868a10..4ea7fdd 100644
--- a/payload_state.h
+++ b/payload_state.h
@@ -155,10 +155,6 @@
return p2p_url_;
}
- inline ErrorCode GetAttemptErrorCode() const override {
- return attempt_error_code_;
- }
-
bool NextPayload() override;
private:
@@ -584,9 +580,6 @@
// The connection type when the attempt started.
metrics::ConnectionType attempt_connection_type_;
- // The attempt error code when the attempt finished.
- ErrorCode attempt_error_code_;
-
// Whether we're currently rolling back.
AttemptType attempt_type_;
diff --git a/payload_state_interface.h b/payload_state_interface.h
index 2460d24..3adc148 100644
--- a/payload_state_interface.h
+++ b/payload_state_interface.h
@@ -202,7 +202,6 @@
// Sets/gets the P2P download URL, if one is to be used.
virtual void SetP2PUrl(const std::string& url) = 0;
virtual std::string GetP2PUrl() const = 0;
- virtual ErrorCode GetAttemptErrorCode() const = 0;
// Switch to next payload.
virtual bool NextPayload() = 0;
diff --git a/update_attempter.cc b/update_attempter.cc
index 9d18932..db4be6b 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -961,6 +961,8 @@
"so requesting reboot from user.";
}
+ attempt_error_code_ = utils::GetBaseErrorCode(code);
+
if (code == ErrorCode::kSuccess) {
WriteUpdateCompletedMarker();
prefs_->SetInt64(kPrefsDeltaUpdateFailures, 0);
@@ -1125,8 +1127,10 @@
break;
}
}
- // On failure, schedule an error event to be sent to Omaha.
- CreatePendingErrorEvent(action, code);
+ if (code != ErrorCode::kNoUpdate) {
+ // On failure, schedule an error event to be sent to Omaha.
+ CreatePendingErrorEvent(action, code);
+ }
return;
}
// Find out which action completed (successfully).
@@ -1326,22 +1330,12 @@
void UpdateAttempter::CreatePendingErrorEvent(AbstractAction* action,
ErrorCode code) {
- if (error_event_.get()) {
+ if (error_event_.get() || status_ == UpdateStatus::REPORTING_ERROR_EVENT) {
// This shouldn't really happen.
LOG(WARNING) << "There's already an existing pending error event.";
return;
}
- // For now assume that a generic Omaha response action failure means that
- // there's no update so don't send an event. Also, double check that the
- // failure has not occurred while sending an error event -- in which case
- // don't schedule another. This shouldn't really happen but just in case...
- if ((action->Type() == OmahaResponseHandlerAction::StaticType() &&
- code == ErrorCode::kError) ||
- status_ == UpdateStatus::REPORTING_ERROR_EVENT) {
- return;
- }
-
// Classify the code to generate the appropriate result so that
// the Borgmon charts show up the results correctly.
// Do this before calling GetErrorCodeForAction which could potentially
diff --git a/update_attempter.h b/update_attempter.h
index 4b50072..4b51478 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -178,6 +178,8 @@
// Broadcasts the current status to all observers.
void BroadcastStatus();
+ ErrorCode GetAttemptErrorCode() const { return attempt_error_code_; }
+
// Returns the special flags to be added to ErrorCode values based on the
// parameters used in the current update attempt.
uint32_t GetErrorCodeFlags();
@@ -446,6 +448,9 @@
// HTTP server response code from the last HTTP request action.
int http_response_code_ = 0;
+ // The attempt error code when the update attempt finished.
+ ErrorCode attempt_error_code_ = ErrorCode::kSuccess;
+
// CPU limiter during the update.
CPULimiter cpu_limiter_;
diff --git a/update_manager/chromeos_policy.cc b/update_manager/chromeos_policy.cc
index c389d8b..655ec82 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -143,6 +143,7 @@
case ErrorCode::kUserCanceled:
case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kUpdatedButNotActive:
+ case ErrorCode::kNoUpdate:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";