update_engine: Update complete event with exclusions
When updates are complete, currently all the AppIDs within the request
parameter are considered to be updated. This however is not true with
exclusions as non-critical AppIDs (e.g. DLCs) can be excluded. This
change sends the correct event for |kTypeUpdateComplete| event type.
BUG=chromium:928805
TEST=FEATURES=test emerge-$B update_engine update_engine-client
Change-Id: I8c21721688fb8a6501316cb87bd0a6f8e005b7ae
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2247489
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Auto-Submit: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Andrew Lassalle <andrewlassalle@chromium.org>
diff --git a/common/error_code.h b/common/error_code.h
index 3dd7402..7acb3b6 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -83,6 +83,7 @@
kInternalLibCurlError = 57,
kUnresolvedHostError = 58,
kUnresolvedHostRecovered = 59,
+ kPackageExcludedFromUpdate = 60,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index 397cdf2..55d876f 100644
--- a/common/error_code_utils.cc
+++ b/common/error_code_utils.cc
@@ -167,6 +167,8 @@
return "ErrorCode::kUnresolvedHostError";
case ErrorCode::kUnresolvedHostRecovered:
return "ErrorCode::kUnresolvedHostRecovered";
+ case ErrorCode::kPackageExcludedFromUpdate:
+ return "ErrorCode::kPackageExcludedFromUpdate";
// Don't add a default case to let the compiler warn about newly added
// error codes which should be added here.
}
diff --git a/metrics_utils.cc b/metrics_utils.cc
index efbd067..0d333ca 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -122,6 +122,7 @@
case ErrorCode::kOmahaUpdateIgnoredOverCellular:
case ErrorCode::kNoUpdate:
case ErrorCode::kFirstActiveOmahaPingSentPersistenceError:
+ case ErrorCode::kPackageExcludedFromUpdate:
return metrics::AttemptResult::kInternalError;
// Special flags. These can't happen (we mask them out above) but
@@ -236,6 +237,7 @@
case ErrorCode::kRollbackNotPossible:
case ErrorCode::kFirstActiveOmahaPingSentPersistenceError:
case ErrorCode::kVerityCalculationError:
+ case ErrorCode::kPackageExcludedFromUpdate:
break;
// Special flags. These can't happen (we mask them out above) but
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index 3a0b91c..83ee5b2 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -582,6 +582,7 @@
LOG(INFO) << "Found package " << package.name;
OmahaResponse::Package out_package;
+ out_package.app_id = app->id;
out_package.can_exclude = can_exclude;
for (const string& codebase : app->url_codebase) {
if (codebase.empty()) {
@@ -631,6 +632,7 @@
// Removes the candidate URLs which are excluded within packages, if all the
// candidate URLs are excluded within a package, the package will be excluded.
void ProcessExclusions(OmahaResponse* output_object,
+ OmahaRequestParams* params,
ExcluderInterface* excluder) {
for (auto package_it = output_object->packages.begin();
package_it != output_object->packages.end();
@@ -657,6 +659,9 @@
// If there are no candidate payload URLs, remove the package.
if (package_it->payload_urls.empty()) {
LOG(INFO) << "Excluding payload hash=" << package_it->hash;
+ // Need to set DLC as not updated so correct metrics can be sent when an
+ // update is completed.
+ params->SetDlcNoUpdate(package_it->app_id);
package_it = output_object->packages.erase(package_it);
continue;
}
@@ -1023,6 +1028,7 @@
if (!ParseResponse(&parser_data, &output_object, &completer))
return;
ProcessExclusions(&output_object,
+ system_state_->request_params(),
system_state_->update_attempter()->GetExcluder());
output_object.update_exists = true;
SetOutputObject(output_object);
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 6a0c213..e608c07 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -2776,8 +2776,8 @@
}
TEST_F(OmahaRequestActionTest, UpdateWithPartiallyExcludedDlcTest) {
- request_params_.set_dlc_apps_params(
- {{request_params_.GetDlcAppId(kDlcId1), {.name = kDlcId1}}});
+ const string kDlcAppId = request_params_.GetDlcAppId(kDlcId1);
+ request_params_.set_dlc_apps_params({{kDlcAppId, {.name = kDlcId1}}});
fake_update_response_.dlc_app_update = true;
tuc_params_.http_response = fake_update_response_.GetUpdateResponse();
// The first DLC candidate URL is excluded.
@@ -2790,11 +2790,12 @@
// One candidate URL.
EXPECT_EQ(response.packages[1].payload_urls.size(), 1u);
EXPECT_TRUE(response.update_exists);
+ EXPECT_TRUE(request_params_.dlc_apps_params().at(kDlcAppId).updated);
}
TEST_F(OmahaRequestActionTest, UpdateWithExcludedDlcTest) {
- request_params_.set_dlc_apps_params(
- {{request_params_.GetDlcAppId(kDlcId1), {.name = kDlcId1}}});
+ const string kDlcAppId = request_params_.GetDlcAppId(kDlcId1);
+ request_params_.set_dlc_apps_params({{kDlcAppId, {.name = kDlcId1}}});
fake_update_response_.dlc_app_update = true;
tuc_params_.http_response = fake_update_response_.GetUpdateResponse();
// Both DLC candidate URLs are excluded.
@@ -2805,6 +2806,7 @@
EXPECT_EQ(response.packages.size(), 1u);
EXPECT_TRUE(response.update_exists);
+ EXPECT_FALSE(request_params_.dlc_apps_params().at(kDlcAppId).updated);
}
TEST_F(OmahaRequestActionTest, UpdateWithDeprecatedDlcTest) {
diff --git a/omaha_request_builder_xml.cc b/omaha_request_builder_xml.cc
index 097b9f1..2eb71bb 100644
--- a/omaha_request_builder_xml.cc
+++ b/omaha_request_builder_xml.cc
@@ -184,17 +184,26 @@
}
}
} else {
+ int event_result = event_->result;
// The error code is an optional attribute so append it only if the result
// is not success.
string error_code;
- if (event_->result != OmahaEvent::kResultSuccess) {
+ if (event_result != OmahaEvent::kResultSuccess) {
error_code = base::StringPrintf(" errorcode=\"%d\"",
static_cast<int>(event_->error_code));
+ } else if (app_data.is_dlc && !app_data.app_params.updated) {
+ // On a |OmahaEvent::kResultSuccess|, if the event is for an update
+ // completion and the App is a DLC, send error for excluded DLCs as they
+ // did not update.
+ event_result = OmahaEvent::Result::kResultError;
+ error_code = base::StringPrintf(
+ " errorcode=\"%d\"",
+ static_cast<int>(ErrorCode::kPackageExcludedFromUpdate));
}
app_body = base::StringPrintf(
" <event eventtype=\"%d\" eventresult=\"%d\"%s></event>\n",
event_->type,
- event_->result,
+ event_result,
error_code.c_str());
}
diff --git a/omaha_request_builder_xml_unittest.cc b/omaha_request_builder_xml_unittest.cc
index 017acec..291189d 100644
--- a/omaha_request_builder_xml_unittest.cc
+++ b/omaha_request_builder_xml_unittest.cc
@@ -148,10 +148,10 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
+ const string kRequestXml = omaha_request.GetRequest();
const string key = "requestid";
const string request_id =
- FindAttributeKeyValueInXml(request_xml, key, kGuidSize);
+ FindAttributeKeyValueInXml(kRequestXml, key, kGuidSize);
// A valid |request_id| is either a GUID version 4 or empty string.
if (!request_id.empty())
EXPECT_TRUE(base::IsValidGUID(request_id));
@@ -169,10 +169,10 @@
0,
fake_system_state_.prefs(),
gen_session_id};
- const string request_xml = omaha_request.GetRequest();
+ const string kRequestXml = omaha_request.GetRequest();
const string key = "sessionid";
const string session_id =
- FindAttributeKeyValueInXml(request_xml, key, kGuidSize);
+ FindAttributeKeyValueInXml(kRequestXml, key, kGuidSize);
// A valid |session_id| is either a GUID version 4 or empty string.
if (!session_id.empty()) {
EXPECT_TRUE(base::IsValidGUID(session_id));
@@ -191,9 +191,9 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
- EXPECT_EQ(1, CountSubstringInString(request_xml, "<updatecheck"))
- << request_xml;
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(1, CountSubstringInString(kRequestXml, "<updatecheck"))
+ << kRequestXml;
}
TEST_F(OmahaRequestBuilderXmlTest, GetRequestXmlPlatformUpdateWithDlcsTest) {
@@ -210,9 +210,9 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
- EXPECT_EQ(3, CountSubstringInString(request_xml, "<updatecheck"))
- << request_xml;
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(3, CountSubstringInString(kRequestXml, "<updatecheck"))
+ << kRequestXml;
}
TEST_F(OmahaRequestBuilderXmlTest, GetRequestXmlDlcInstallationTest) {
@@ -231,25 +231,25 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
- EXPECT_EQ(2, CountSubstringInString(request_xml, "<updatecheck"))
- << request_xml;
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(2, CountSubstringInString(kRequestXml, "<updatecheck"))
+ << kRequestXml;
- auto FindAppId = [request_xml](size_t pos) -> size_t {
- return request_xml.find("<app appid", pos);
+ auto FindAppId = [kRequestXml](size_t pos) -> size_t {
+ return kRequestXml.find("<app appid", pos);
};
// Skip over the Platform AppID, which is always first.
size_t pos = FindAppId(0);
for (auto&& _ : dlcs) {
(void)_;
- EXPECT_NE(string::npos, (pos = FindAppId(pos + 1))) << request_xml;
+ EXPECT_NE(string::npos, (pos = FindAppId(pos + 1))) << kRequestXml;
const string dlc_app_id_version = FindAttributeKeyValueInXml(
- request_xml.substr(pos), "version", string(kNoVersion).size());
+ kRequestXml.substr(pos), "version", string(kNoVersion).size());
EXPECT_EQ(kNoVersion, dlc_app_id_version);
const string false_str = "false";
const string dlc_app_id_delta_okay = FindAttributeKeyValueInXml(
- request_xml.substr(pos), "delta_okay", false_str.length());
+ kRequestXml.substr(pos), "delta_okay", false_str.length());
EXPECT_EQ(false_str, dlc_app_id_delta_okay);
}
}
@@ -267,8 +267,8 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
- EXPECT_EQ(0, CountSubstringInString(request_xml, "<ping")) << request_xml;
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(0, CountSubstringInString(kRequestXml, "<ping")) << kRequestXml;
}
TEST_F(OmahaRequestBuilderXmlTest, GetRequestXmlDlcPingRollCallNoActive) {
@@ -289,9 +289,9 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
- EXPECT_EQ(1, CountSubstringInString(request_xml, "<ping rd=\"36\""))
- << request_xml;
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(1, CountSubstringInString(kRequestXml, "<ping rd=\"36\""))
+ << kRequestXml;
}
TEST_F(OmahaRequestBuilderXmlTest, GetRequestXmlDlcPingRollCallAndActive) {
@@ -313,10 +313,93 @@
0,
fake_system_state_.prefs(),
""};
- const string request_xml = omaha_request.GetRequest();
+ const string kRequestXml = omaha_request.GetRequest();
EXPECT_EQ(1,
- CountSubstringInString(request_xml,
+ CountSubstringInString(kRequestXml,
"<ping active=\"1\" ad=\"25\" rd=\"36\""))
- << request_xml;
+ << kRequestXml;
+}
+
+TEST_F(OmahaRequestBuilderXmlTest, GetRequestXmlUpdateCompleteEvent) {
+ OmahaRequestParams omaha_request_params{&fake_system_state_};
+ OmahaEvent event(OmahaEvent::kTypeUpdateComplete);
+ OmahaRequestBuilderXml omaha_request{&event,
+ &omaha_request_params,
+ false,
+ false,
+ 0,
+ 0,
+ 0,
+ fake_system_state_.prefs(),
+ ""};
+ const string kRequestXml = omaha_request.GetRequest();
+ LOG(INFO) << kRequestXml;
+ EXPECT_EQ(
+ 1,
+ CountSubstringInString(
+ kRequestXml, "<event eventtype=\"3\" eventresult=\"1\"></event>"))
+ << kRequestXml;
+}
+
+TEST_F(OmahaRequestBuilderXmlTest,
+ GetRequestXmlUpdateCompleteEventSomeDlcsExcluded) {
+ OmahaRequestParams omaha_request_params{&fake_system_state_};
+ omaha_request_params.set_dlc_apps_params({
+ {omaha_request_params.GetDlcAppId("dlc_1"), {.updated = true}},
+ {omaha_request_params.GetDlcAppId("dlc_2"), {.updated = false}},
+ });
+ OmahaEvent event(OmahaEvent::kTypeUpdateComplete);
+ OmahaRequestBuilderXml omaha_request{&event,
+ &omaha_request_params,
+ false,
+ false,
+ 0,
+ 0,
+ 0,
+ fake_system_state_.prefs(),
+ ""};
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(
+ 2,
+ CountSubstringInString(
+ kRequestXml, "<event eventtype=\"3\" eventresult=\"1\"></event>"))
+ << kRequestXml;
+ EXPECT_EQ(
+ 1,
+ CountSubstringInString(
+ kRequestXml,
+ "<event eventtype=\"3\" eventresult=\"0\" errorcode=\"60\"></event>"))
+ << kRequestXml;
+}
+
+TEST_F(OmahaRequestBuilderXmlTest,
+ GetRequestXmlUpdateCompleteEventAllDlcsExcluded) {
+ OmahaRequestParams omaha_request_params{&fake_system_state_};
+ omaha_request_params.set_dlc_apps_params({
+ {omaha_request_params.GetDlcAppId("dlc_1"), {.updated = false}},
+ {omaha_request_params.GetDlcAppId("dlc_2"), {.updated = false}},
+ });
+ OmahaEvent event(OmahaEvent::kTypeUpdateComplete);
+ OmahaRequestBuilderXml omaha_request{&event,
+ &omaha_request_params,
+ false,
+ false,
+ 0,
+ 0,
+ 0,
+ fake_system_state_.prefs(),
+ ""};
+ const string kRequestXml = omaha_request.GetRequest();
+ EXPECT_EQ(
+ 1,
+ CountSubstringInString(
+ kRequestXml, "<event eventtype=\"3\" eventresult=\"1\"></event>"))
+ << kRequestXml;
+ EXPECT_EQ(
+ 2,
+ CountSubstringInString(
+ kRequestXml,
+ "<event eventtype=\"3\" eventresult=\"0\" errorcode=\"60\"></event>"))
+ << kRequestXml;
}
} // namespace chromeos_update_engine
diff --git a/omaha_response.h b/omaha_response.h
index 2b86fe7..77f9083 100644
--- a/omaha_response.h
+++ b/omaha_response.h
@@ -54,6 +54,8 @@
// True if the payload can be excluded from updating if consistently faulty.
// False if the payload is critical to update.
bool can_exclude = false;
+ // The App ID associated with the package.
+ std::string app_id;
};
std::vector<Package> packages;
diff --git a/payload_state.cc b/payload_state.cc
index cf3aab9..4b4120c 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -373,6 +373,7 @@
case ErrorCode::kInternalLibCurlError:
case ErrorCode::kUnresolvedHostError:
case ErrorCode::kUnresolvedHostRecovered:
+ case ErrorCode::kPackageExcludedFromUpdate:
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 dd6cc8d..cc10b57 100644
--- a/update_manager/chromeos_policy.cc
+++ b/update_manager/chromeos_policy.cc
@@ -154,6 +154,7 @@
case ErrorCode::kInternalLibCurlError:
case ErrorCode::kUnresolvedHostError:
case ErrorCode::kUnresolvedHostRecovered:
+ case ErrorCode::kPackageExcludedFromUpdate:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";