buffet: Report command instance parsing error to cloud server
When a command comes from GCD server and fails to parse, we used to
just skip the command without reporting anything to the server.
This lead to the same command coming down in the next fetch cycle and
be retried over and over again.
Now if buffet cannot process a command, we mark it as "aborted" on the
server and provide actual failure reason - in the "error" property of
the command resource.
BUG=brillo:952
TEST=`FEATURES=test emerge-buffet`
Tested manually on device through GCD dev site.
Change-Id: Idcda5ca296696a01d7e008f148f125c1a4ed1072
Reviewed-on: https://chromium-review.googlesource.com/268442
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/buffet/commands/command_instance.cc b/buffet/commands/command_instance.cc
index 24c7869..a2e098f 100644
--- a/buffet/commands/command_instance.cc
+++ b/buffet/commands/command_instance.cc
@@ -96,17 +96,27 @@
const base::Value* value,
const std::string& origin,
const CommandDictionary& dictionary,
+ std::string* command_id,
chromeos::ErrorPtr* error) {
std::unique_ptr<CommandInstance> instance;
+ std::string command_id_buffer; // used if |command_id| was nullptr.
+ if (!command_id)
+ command_id = &command_id_buffer;
+
// Get the command JSON object from the value.
const base::DictionaryValue* json = nullptr;
if (!value->GetAsDictionary(&json)) {
chromeos::Error::AddTo(error, FROM_HERE, chromeos::errors::json::kDomain,
chromeos::errors::json::kObjectExpected,
"Command instance is not a JSON object");
+ command_id->clear();
return instance;
}
+ // Get the command ID from 'id' property.
+ if (!json->GetString(commands::attributes::kCommand_Id, command_id))
+ command_id->clear();
+
// Get the command name from 'name' property.
std::string command_name;
if (!json->GetString(commands::attributes::kCommand_Name, &command_name)) {
@@ -136,10 +146,9 @@
instance.reset(
new CommandInstance{command_name, origin, command_def, parameters});
- std::string command_id;
- if (json->GetString(commands::attributes::kCommand_Id, &command_id)) {
- instance->SetID(command_id);
- }
+
+ if (!command_id->empty())
+ instance->SetID(*command_id);
return instance;
}
diff --git a/buffet/commands/command_instance.h b/buffet/commands/command_instance.h
index f67b30b..6ce7036 100644
--- a/buffet/commands/command_instance.h
+++ b/buffet/commands/command_instance.h
@@ -63,10 +63,15 @@
// object, checking the JSON |value| against the command definition schema
// found in command |dictionary|. On error, returns null unique_ptr and
// fills in error details in |error|.
+ // |command_id| is the ID of the command returned, as parsed from the |value|.
+ // The command ID extracted (if present in the JSON object) even if other
+ // parsing/validation error occurs and command instance is not constructed.
+ // This is used to report parse failures back to the server.
static std::unique_ptr<CommandInstance> FromJson(
const base::Value* value,
const std::string& origin,
const CommandDictionary& dictionary,
+ std::string* command_id,
chromeos::ErrorPtr* error);
// Returns JSON representation of the command.
diff --git a/buffet/commands/command_instance_unittest.cc b/buffet/commands/command_instance_unittest.cc
index 8d7d0ec..f7744b0 100644
--- a/buffet/commands/command_instance_unittest.cc
+++ b/buffet/commands/command_instance_unittest.cc
@@ -108,14 +108,18 @@
TEST_F(CommandInstanceTest, FromJson) {
auto json = CreateDictionaryValue(R"({
'name': 'robot.jump',
+ 'id': 'abcd',
'parameters': {
'height': 53,
'_jumpType': '_withKick'
},
'results': {}
})");
+ std::string id;
auto instance =
- CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr);
+ CommandInstance::FromJson(json.get(), "cloud", dict_, &id, nullptr);
+ EXPECT_EQ("abcd", id);
+ EXPECT_EQ("abcd", instance->GetID());
EXPECT_EQ("robot.jump", instance->GetName());
EXPECT_EQ("robotd", instance->GetCategory());
EXPECT_EQ(53, instance->FindParameter("height")->GetInt()->GetValue());
@@ -126,7 +130,7 @@
TEST_F(CommandInstanceTest, FromJson_ParamsOmitted) {
auto json = CreateDictionaryValue("{'name': 'base.reboot'}");
auto instance =
- CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr);
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, nullptr);
EXPECT_EQ("base.reboot", instance->GetName());
EXPECT_EQ("robotd", instance->GetCategory());
EXPECT_TRUE(instance->GetParameters().empty());
@@ -135,7 +139,8 @@
TEST_F(CommandInstanceTest, FromJson_NotObject) {
auto json = CreateValue("'string'");
chromeos::ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), "cloud", dict_, &error);
+ auto instance =
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, &error);
EXPECT_EQ(nullptr, instance.get());
EXPECT_EQ("json_object_expected", error->GetCode());
EXPECT_EQ("Command instance is not a JSON object", error->GetMessage());
@@ -144,7 +149,8 @@
TEST_F(CommandInstanceTest, FromJson_NameMissing) {
auto json = CreateDictionaryValue("{'param': 'value'}");
chromeos::ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), "cloud", dict_, &error);
+ auto instance =
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, &error);
EXPECT_EQ(nullptr, instance.get());
EXPECT_EQ("parameter_missing", error->GetCode());
EXPECT_EQ("Command name is missing", error->GetMessage());
@@ -153,7 +159,8 @@
TEST_F(CommandInstanceTest, FromJson_UnknownCommand) {
auto json = CreateDictionaryValue("{'name': 'robot.scream'}");
chromeos::ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), "cloud", dict_, &error);
+ auto instance =
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, &error);
EXPECT_EQ(nullptr, instance.get());
EXPECT_EQ("invalid_command_name", error->GetCode());
EXPECT_EQ("Unknown command received: robot.scream", error->GetMessage());
@@ -165,7 +172,8 @@
'parameters': 'hello'
})");
chromeos::ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), "cloud", dict_, &error);
+ auto instance =
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, &error);
EXPECT_EQ(nullptr, instance.get());
auto inner = error->GetInnerError();
EXPECT_EQ("json_object_expected", inner->GetCode());
@@ -183,7 +191,8 @@
}
})");
chromeos::ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), "cloud", dict_, &error);
+ auto instance =
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, &error);
EXPECT_EQ(nullptr, instance.get());
auto first = error->GetFirstError();
EXPECT_EQ("out_of_range", first->GetCode());
@@ -207,7 +216,7 @@
'results': {}
})");
auto instance =
- CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr);
+ CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, nullptr);
instance->SetProgress(
native_types::Object{{"progress", unittests::make_int_prop_value(15)}});
instance->SetProgress(
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc
index ffd4394..262b85d 100644
--- a/buffet/commands/dbus_command_dispatcher_unittest.cc
+++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -106,7 +106,7 @@
void AddNewCommand(const std::string& json, const std::string& id) {
auto command_instance = CommandInstance::FromJson(
CreateDictionaryValue(json.c_str()).get(), "cloud", dictionary_,
- nullptr);
+ nullptr, nullptr);
command_instance->SetID(id);
// Two interfaces are added - Command and Properties.
EXPECT_CALL(*mock_exported_object_manager_, SendSignal(_)).Times(2);
diff --git a/buffet/commands/dbus_command_proxy_unittest.cc b/buffet/commands/dbus_command_proxy_unittest.cc
index e0cad8f..c2ca7d8 100644
--- a/buffet/commands/dbus_command_proxy_unittest.cc
+++ b/buffet/commands/dbus_command_proxy_unittest.cc
@@ -93,7 +93,7 @@
}
})");
command_instance_ =
- CommandInstance::FromJson(json.get(), "local", dict_, nullptr);
+ CommandInstance::FromJson(json.get(), "local", dict_, nullptr, nullptr);
command_instance_->SetID(kTestCommandId);
// Set up a mock ExportedObject to be used with the DBus command proxy.
diff --git a/buffet/commands/schema_constants.cc b/buffet/commands/schema_constants.cc
index 2868a06..6aaf780 100644
--- a/buffet/commands/schema_constants.cc
+++ b/buffet/commands/schema_constants.cc
@@ -52,6 +52,8 @@
const char kCommand_Results[] = "results";
const char kCommand_State[] = "state";
const char kCommand_Progress[] = "progress";
+const char kCommand_ErrorCode[] = "error.code";
+const char kCommand_ErrorMessage[] = "error.message";
const char kCommand_Visibility[] = "visibility";
const char kCommand_Visibility_None[] = "none";
diff --git a/buffet/commands/schema_constants.h b/buffet/commands/schema_constants.h
index 9740303..fbbb3e3 100644
--- a/buffet/commands/schema_constants.h
+++ b/buffet/commands/schema_constants.h
@@ -56,6 +56,8 @@
extern const char kCommand_Results[];
extern const char kCommand_State[];
extern const char kCommand_Progress[];
+extern const char kCommand_ErrorCode[];
+extern const char kCommand_ErrorMessage[];
extern const char kCommand_Visibility[];
extern const char kCommand_Visibility_None[];
diff --git a/buffet/device_registration_info.cc b/buffet/device_registration_info.cc
index a56a15a..eec9985 100644
--- a/buffet/device_registration_info.cc
+++ b/buffet/device_registration_info.cc
@@ -53,6 +53,7 @@
const int kMaxStartDeviceRetryDelayMinutes{1};
const int64_t kMinStartDeviceRetryDelaySeconds{5};
+const int64_t kAbortCommandRetryDelaySeconds{5};
std::pair<std::string, std::string> BuildAuthHeader(
const std::string& access_token_type,
@@ -789,6 +790,45 @@
base::Bind(&IgnoreCloudErrorWithCallback, on_error));
}
+void DeviceRegistrationInfo::NotifyCommandAborted(
+ const std::string& command_id,
+ chromeos::ErrorPtr error) {
+ base::DictionaryValue command_patch;
+ command_patch.SetString(commands::attributes::kCommand_State,
+ CommandInstance::kStatusAborted);
+ if (error) {
+ command_patch.SetString(commands::attributes::kCommand_ErrorCode,
+ chromeos::string_utils::Join(":",
+ error->GetDomain(),
+ error->GetCode()));
+ std::vector<std::string> messages;
+ const chromeos::Error* current_error = error.get();
+ while (current_error) {
+ messages.push_back(current_error->GetMessage());
+ current_error = current_error->GetInnerError();
+ }
+ command_patch.SetString(commands::attributes::kCommand_ErrorMessage,
+ chromeos::string_utils::Join(";", messages));
+ }
+ UpdateCommand(command_id,
+ command_patch,
+ base::Bind(&base::DoNothing),
+ base::Bind(&DeviceRegistrationInfo::RetryNotifyCommandAborted,
+ weak_factory_.GetWeakPtr(),
+ command_id, base::Passed(std::move(error))));
+}
+
+void DeviceRegistrationInfo::RetryNotifyCommandAborted(
+ const std::string& command_id,
+ chromeos::ErrorPtr error) {
+ base::MessageLoop::current()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&DeviceRegistrationInfo::NotifyCommandAborted,
+ weak_factory_.GetWeakPtr(),
+ command_id, base::Passed(std::move(error))),
+ base::TimeDelta::FromSeconds(kAbortCommandRetryDelaySeconds));
+}
+
void DeviceRegistrationInfo::UpdateDeviceResource(
const base::Closure& on_success,
const CloudRequestErrorCallback& on_failure) {
@@ -903,11 +943,15 @@
continue;
}
+ std::string command_id;
+ chromeos::ErrorPtr error;
auto command_instance = CommandInstance::FromJson(
command, commands::attributes::kCommand_Visibility_Cloud,
- command_dictionary, nullptr);
+ command_dictionary, &command_id, &error);
if (!command_instance) {
- LOG(WARNING) << "Failed to parse a command";
+ LOG(WARNING) << "Failed to parse a command with ID: " << command_id;
+ if (!command_id.empty())
+ NotifyCommandAborted(command_id, std::move(error));
continue;
}
diff --git a/buffet/device_registration_info.h b/buffet/device_registration_info.h
index 4fa4c47..8f74d2a 100644
--- a/buffet/device_registration_info.h
+++ b/buffet/device_registration_info.h
@@ -214,6 +214,16 @@
void PublishStateUpdates();
+ // If unrecoverable error occurred (e.g. error parsing command instance),
+ // notify the server that the command is aborted by the device.
+ void NotifyCommandAborted(const std::string& command_id,
+ chromeos::ErrorPtr error);
+
+ // When NotifyCommandAborted() fails, RetryNotifyCommandAborted() schedules
+ // a retry attempt.
+ void RetryNotifyCommandAborted(const std::string& command_id,
+ chromeos::ErrorPtr error);
+
// Builds Cloud API devices collection REST resource which matches
// current state of the device including command definitions
// for all supported commands and current device state.
diff --git a/buffet/manager.cc b/buffet/manager.cc
index 7b710f1..82ee7a8 100644
--- a/buffet/manager.cc
+++ b/buffet/manager.cc
@@ -184,7 +184,7 @@
chromeos::ErrorPtr error;
auto command_instance = buffet::CommandInstance::FromJson(
value.get(), commands::attributes::kCommand_Visibility_Local,
- command_manager_->GetCommandDictionary(), &error);
+ command_manager_->GetCommandDictionary(), nullptr, &error);
if (!command_instance) {
response->ReplyWithError(error.get());
return;
