Initial RemoteCommandService

components/policy/core/common/remote_commands/remote_commands_service.h

Index: components/policy/core/common/remote_commands/remote_commands_service.h
diff --git a/components/policy/core/common/remote_commands/remote_commands_service.h b/components/policy/core/common/remote_commands/remote_commands_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..b29f4eff007648a231f140b622e3af306903008d
--- /dev/null
+++ b/components/policy/core/common/remote_commands/remote_commands_service.h
@@ -0,0 +1,89 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_REMOTE_COMMANDS_SERVICE_H_
+#define COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_REMOTE_COMMANDS_SERVICE_H_
+
+#include <set>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "components/policy/core/common/remote_commands/remote_command_job.h"
+#include "components/policy/core/common/remote_commands/remote_commands_queue.h"
+#include "components/policy/policy_export.h"
+#include "policy/proto/device_management_backend.pb.h"
+
+namespace policy {
+
+class CloudPolicyClient;
+class RemoteCommandsFactory;
+
+// Service class which will connect to a CloudPolicyClient in order to fetch
+// remote commands from DMServer and send results for executed commands
+// back to server.
+class POLICY_EXPORT RemoteCommandsService
+    : public RemoteCommandsQueue::Observer {
+ public:
+  RemoteCommandsService(scoped_ptr<RemoteCommandsFactory> factory,
+                        CloudPolicyClient* client);
+  ~RemoteCommandsService() override;
+
+  // Attempts to fetch remote commands, mainly supposed to be called by
+  // invalidation service. Note that there will be at most one ongoing fetch
+  // request and all other fetch request will be enqueued if another fetch
+  // request is in-progress. And in such case, a following up request will be
+  // made immediately after current ongoing requests finishes. Returns true if a
+  // new command fetch request is created.
+  bool FetchRemoteCommands();
+
+  // Returns whether a command fetch request is in progress or not.
+  bool IsCommandFetchInProgressForTesting() const {
+    return command_fetch_in_progress_;
+  }
+
+ private:
+  // Helper function to add an command which we get from server.
+  void AddCommand(const enterprise_management::RemoteCommand& command);
+
+  // RemoteCommandsQueue::Observer:
+  void OnJobStarted(RemoteCommandJob* command) override;
+  void OnJobFinished(RemoteCommandJob* command) override;
+
+  // Callback to handle commands we get from server.
+  void OnRemoteCommandsFetched(
+      const std::vector<enterprise_management::RemoteCommand>& commands);
+
+  // Whether there is a command fetch on going or not.
+  bool command_fetch_in_progress_ = false;
+
+  // Whether there is an enqueued fetch request, which indicates there are
+  // additional FetchRemoteCommands() calls while a fetch request is ongoing.
+  bool has_enqueued_fetch_request_ = false;
+
+  // Commands results have not been sent back to server yet.
+  std::vector<enterprise_management::RemoteCommandResult> unsent_results_;
+
+  // ID for the latest command which has started execution. We will acknowledge

[bartfab (slow), 2015/02/23 13:13:57]

This will not work for the reboot command. Currently, the reboot command is
implemented as follows:

1) RemoteCommandService receives a reboot command and places it in the
RemoteCommandsQueue.
2) The command starts execution and reboots the device.
3) After the reboot, the RemoteCommandService talks to the server, receives the
same reboot command and places it in the RemoteCommandsQueue again.
4) The command starts execution. It notices that the device has been rebooted
already since the command was issued and reports success to the server, without
rebooting again.

If you acknowledge commands the moment they start execution, you will remove the
command in (2) and the device will not see it again in (3).

[binjin, 2015/02/24 05:29:49]

One way to handle this, as we discussed offline, is to add special field to
RemoteCommandJob (just like what we did for command timeout time), and allowing
certain types of RemoteCommands to force RemoteCommandsService to delay
acknowledging of its execution. It's not very hard to implement but it also have
a lot of corner cases to consider. I would like to implement this in separated
CL since this CL already goes way bigger than what I expected it to be.

[bartfab (slow), 2015/02/27 03:59:51]

I have no problem with separating this piece out into a separate CL, if we have
a solid idea how it will work. What ID will we persist in this case? What ID
will we send up to the server?

Speaking of which, do you want to add persistence in this CL or would you prefer
to do that in a follow-up as well?

[binjin, 2015/02/27 18:23:04]

I want to add persistence in separated CL. 

I think the choice between last fetched ID and last executed ID is clear, it
only matters if a command is fetched but doesn't get a chance to start
execution, so I will write last executed ID to local state, and acknowledging
the server with last executed ID as well (We need to remove duplicated remote
commands later).

There are subtle differences regarding the time we sync the last executed ID to
local disk and server, OnJobStarted or OnJobFinished. These two approach only
matters if the current running command will crash the system before finishing.
In such case, "OnJobStarted" approach will make the commands like reboot command
run only once (we discussed and agreed that running reboot command twice is much
simpler to implement if we want to report success back to server after reboot),
"OnJobFinished" will potentially lead to unintended commands loop. I currently
have no better idea on this, one approach is leave this decision to commands
implementations, "OnJobStarted", "OnJobFinished" or even "postpone reporting and
blocking all commands in the queue".

+  // the server with this ID so that we can re-fetch some commands not
+  // executed yet after a crash or browser restart.
+  RemoteCommandJob::UniqueIDType last_executed_command_id_ = 0;
+
+  // Collects all IDs for fetched commands. We need this since the command id
+  // is not guaranteed to be increasing.

[bartfab (slow), 2015/02/23 13:13:57]

Nit: s/not guaranteed to be increasing/opaque/

It is not just about decreasing/increasing. The ID is simply 100% opaque and we
must never try to reason about it.

[binjin, 2015/02/24 05:29:49]

Done.

+  std::set<RemoteCommandJob::UniqueIDType> fetched_command_ids_;
+
+  RemoteCommandsQueue queue_;
+  scoped_ptr<RemoteCommandsFactory> factory_;
+  CloudPolicyClient* client_;
+
+  base::WeakPtrFactory<RemoteCommandsService> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoteCommandsService);
+};
+
+}  // namespace policy
+
+#endif  // COMPONENTS_POLICY_CORE_COMMON_REMOTE_COMMANDS_REMOTE_COMMANDS_SERVICE_H_