Remote fetch device status (attributes and logs) command

Remote fetch device status (attributes and logs) command

This CL introduces a remote command that enables device administrators to remotely fetch device status (attributes and logs). Attributes were being uploaded every 3 hours and logs every 12 hours from the device side, and there was no way to trigger a remote refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked and rewired to decode into this command and then on the server side (CPanel admin) I clicked on "REBOOT NOW" and I observed the device attributes and logs being updated.

P.S. I've also fixed up some linter errors.

BUG=686389
TEST=Manually tested new behaviour.

Review-Url: https://codereview.chromium.org/2765463002
Cr-Commit-Position: refs/heads/master@{#459037}
Committed: https://chromium.googlesource.com/chromium/src/+/35b9ec40290254e484cc17e76ee5205b618e23c9

[Ivan Šandrk, 2017-03-20 14:34:39]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 14:34:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-20 15:09:12]

isandrk@chromium.org changed reviewers:
+ atwilson@chromium.org

[Ivan Šandrk, 2017-03-20 15:09:13]

Hey Drew, please take a look!

I'm unsure about how I handled the code around
ScheduleNextStatusUploadImmediately, I could use your input there.

[Ivan Šandrk, 2017-03-20 15:11:48]

Description was changed from

==========
Remote fetch device status command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes). Status was being uploaded from the
device side, every 3 hours by default, and there was no way to trigger a remote
refresh.

More context can be found by reading the design doc go/remote-audio-management

BUG=686389
==========

to

==========
Remote fetch device status command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes). Status was being uploaded from the
device side, every 3 hours by default, and there was no way to trigger a remote
refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked
and rewired to decode into this command and then on the server side (CPanel
admin) I clicked on "REBOOT NOW" and I observed the device attributes being
updated.

BUG=686389
TEST=Manually tested new behaviour.
==========

[commit-bot: I haz the power, 2017-03-20 15:56:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 15:56:16]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2017-03-20 16:33:11]

https://codereview.chromium.org/2765463002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/status_uploader.cc:96:
upload_callback_.Reset(base::Bind(&StatusUploader::UploadStatus,
My main concern here is that we could have a status upload in progress already
(i.e. what happens if this is called after an existing call to UploadStatus()
has already been invoked, but before OnStatusReceived() is invoked)? Does the
status collector gracefully handle two calls to GetDeviceAndSessionStatusAsync()
in parallel?

https://codereview.chromium.org/2765463002/diff/1/components/policy/proto/dev...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2765463002/diff/1/components/policy/proto/dev...
components/policy/proto/device_management_backend.proto:1083: //
DEVICE_SET_VOLUME: {"volume" : 30}
This seems unrelated to this CL? maybe drive this comment in a separate CL?

[Ivan Šandrk, 2017-03-20 17:43:04]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:43:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-20 17:48:53]

ptal

https://codereview.chromium.org/2765463002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/status_uploader.cc:96:
upload_callback_.Reset(base::Bind(&StatusUploader::UploadStatus,
On 2017/03/20 16:33:10, Andrew T Wilson (Slow) wrote:
> My main concern here is that we could have a status upload in progress already
> (i.e. what happens if this is called after an existing call to UploadStatus()
> has already been invoked, but before OnStatusReceived() is invoked)? Does the
> status collector gracefully handle two calls to
GetDeviceAndSessionStatusAsync()
> in parallel? 

I think it should actually be safe, but I haven't really looked. I've added a
bool flag to guard against this case. WDYT?

[commit-bot: I haz the power, 2017-03-20 18:39:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 18:39:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Šandrk, 2017-03-21 14:07:49]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 14:08:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 15:08:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 15:08:35]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2017-03-21 17:08:49]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:09:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-21 17:10:17]

The CQ bit was unchecked by isandrk@chromium.org

[Ivan Šandrk, 2017-03-21 17:12:15]

Patchset #4 (id:60001) has been deleted

[Ivan Šandrk, 2017-03-21 17:12:52]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:13:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-21 17:16:00]

Description was changed from

==========
Remote fetch device status command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes). Status was being uploaded from the
device side, every 3 hours by default, and there was no way to trigger a remote
refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked
and rewired to decode into this command and then on the server side (CPanel
admin) I clicked on "REBOOT NOW" and I observed the device attributes being
updated.

BUG=686389
TEST=Manually tested new behaviour.
==========

to

==========
Remote fetch device status (attributes and logs) command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes and logs). Attributes were being
uploaded every 3 hours and logs every 12 hours from the device side, and there
was no way to trigger a remote refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked
and rewired to decode into this command and then on the server side (CPanel
admin) I clicked on "REBOOT NOW" and I observed the device attributes and logs
being updated.

P.S. I've also fixed up some linter errors.

BUG=686389
TEST=Manually tested new behaviour.
==========

[Ivan Šandrk, 2017-03-21 17:17:02]

Hey Drew ptal! Now it's fetching both attrs & logs.

[Andrew T Wilson (Slow), 2017-03-21 18:03:42]

PTAL if you address my comments below (esp logic around when to fail the remote
command)

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:121:
SystemLogUploader* GetSysLogUploader() const {
If the class is called SystemLogUploader then call this API
GetSystemLogUploader().

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_fetch_status_job.cc
(right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_fetch_status_job.cc:68:
FROM_HERE, base::Bind(failed_callback, nullptr));
OK, you no longer fail if status uploader and system log uploader are null. Is
that intentional? Why do you fail in once case but not the other?

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:91: if
(status_upload_in_progress_)
Log something here, as this is a weird exceptional case I think.

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/system_log_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader.cc:324:
weak_factory_.InvalidateWeakPtrs();
I thought there was a race condition in this code, but there isn't because this
call to InvalidateWeakPtrs() will invalidate any pending calls to
StartLogUpload(). It took me a few minutes to convince myself this is race-free,
so perhaps you should add a comment to this effect (why we can't get two log
uploads even if there's one queued up on the current thread).

[commit-bot: I haz the power, 2017-03-21 18:15:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 18:15:18]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2017-03-22 20:15:30]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 20:16:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2017-03-22 20:20:02]

Ptal :)

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:121:
SystemLogUploader* GetSysLogUploader() const {
On 2017/03/21 18:03:42, Andrew T Wilson (Slow) wrote:
> If the class is called SystemLogUploader then call this API
> GetSystemLogUploader().

Done.

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_fetch_status_job.cc
(right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_fetch_status_job.cc:68:
FROM_HERE, base::Bind(failed_callback, nullptr));
On 2017/03/21 18:03:42, Andrew T Wilson (Slow) wrote:
> OK, you no longer fail if status uploader and system log uploader are null. Is
> that intentional? Why do you fail in once case but not the other?

It took me some time to clean up my thoughts on this and the reasoning behind
this. I had a strong feeling (not a good thing, I know) that the lifetimes of
all three (manager & StatusUploader & SystemLogUploader) were linked together,
and I still think it's true but I haven't really proved it.

What I did prove now (after some digging thru codesearch) was that
StatusUploader and SystemLogUploader have linked lifetimes, and this simplifies
the code much (if they weren't I would have to somehow signal the XOR state,
making the command much more complex both client and server side).

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:91: if
(status_upload_in_progress_)
On 2017/03/21 18:03:42, Andrew T Wilson (Slow) wrote:
> Log something here, as this is a weird exceptional case I think.

Done.

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/system_log_uploader.cc (right):

https://codereview.chromium.org/2765463002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader.cc:324:
weak_factory_.InvalidateWeakPtrs();
On 2017/03/21 18:03:42, Andrew T Wilson (Slow) wrote:
> I thought there was a race condition in this code, but there isn't because
this
> call to InvalidateWeakPtrs() will invalidate any pending calls to
> StartLogUpload(). It took me a few minutes to convince myself this is
race-free,
> so perhaps you should add a comment to this effect (why we can't get two log
> uploads even if there's one queued up on the current thread).

Done.

[commit-bot: I haz the power, 2017-03-22 21:24:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 21:24:19]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2017-03-23 10:03:57]

lgtm

[Ivan Šandrk, 2017-03-23 10:07:00]

The CQ bit was checked by isandrk@chromium.org

[commit-bot: I haz the power, 2017-03-23 10:07:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 10:11:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490263620782520,
"parent_rev": "ca0f6132d435767127a2b287cce8912d81296678", "commit_rev":
"35b9ec40290254e484cc17e76ee5205b618e23c9"}

[commit-bot: I haz the power, 2017-03-23 10:11:53]

Description was changed from

==========
Remote fetch device status (attributes and logs) command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes and logs). Attributes were being
uploaded every 3 hours and logs every 12 hours from the device side, and there
was no way to trigger a remote refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked
and rewired to decode into this command and then on the server side (CPanel
admin) I clicked on "REBOOT NOW" and I observed the device attributes and logs
being updated.

P.S. I've also fixed up some linter errors.

BUG=686389
TEST=Manually tested new behaviour.
==========

to

==========
Remote fetch device status (attributes and logs) command

This CL introduces a remote command that enables device administrators to
remotely fetch device status (attributes and logs). Attributes were being
uploaded every 3 hours and logs every 12 hours from the device side, and there
was no way to trigger a remote refresh.

More context can be found by reading the design doc go/remote-audio-management

The new functionality was manually tested; remote reboot command was hijacked
and rewired to decode into this command and then on the server side (CPanel
admin) I clicked on "REBOOT NOW" and I observed the device attributes and logs
being updated.

P.S. I've also fixed up some linter errors.

BUG=686389
TEST=Manually tested new behaviour.

Review-Url: https://codereview.chromium.org/2765463002
Cr-Commit-Position: refs/heads/master@{#459037}
Committed:
https://chromium.googlesource.com/chromium/src/+/35b9ec40290254e484cc17e76ee5...
==========

[commit-bot: I haz the power, 2017-03-23 10:11:55]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/35b9ec40290254e484cc17e76ee5...