[Telemetry] Refactor common Android cleanup functions.

build/android/pylib/android_commands.py

 # Copyright (c) 2012 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.
 
 """Provides an interface to communicate with the device via the adb command.
 
 Assumes adb binary is currently on system path.
 """
 
 import collections
@@ skipping 447 matching lines @@
         reboots_left -= 1
 
   def MakeSystemFolderWritable(self):
     """Remounts the /system folder rw."""
     out = self._adb.SendCommand('remount')
     if out.strip() != 'remount succeeded':
       raise errors.MsgException('Remount failed: %s' % out)
 
   def RestartAdbdOnDevice(self):
     logging.info('Killing adbd on the device...')
+    self._adb.SendCommand('wait-for-device')

[tonyg, 2013/11/25 15:46:33]

While I love the fix, it seems misplaced to me.

This method doesn't do anything special before this that would require
wait-for-device. So that tells me that the wait-for-device belongs elsewhere
(e.g. at the end of RestartAdbServer). Otherwise, you could foresee a
wait-for-device call at the beginning of every method in this file.

Also, does this fix http://crbug.com/322828 ?

[Primiano Tucci (use gerrit), 2013/11/25 17:26:51]

Right, I should have added more context, my bad.
The call stack involved here is.
android_commands.py::RunShellCommand
 -> adb_interface.py::SendShellCommand
   -> adb_interface.py::SendCommand
     -> run_command.py::RunCommand

Now, run_command.py::RunCommand is not robust enough to tolerate adb server
restarts. If you look to its code it has a retry_count. However the retry logic
kicks in only in case of a command timeout, and not in the case in which the
device temporary disappear (e.g., while adb server is respawning).

I think (but I might be wrong, I still miss some adb details) that the main
problem is that at the time RestartAdbdOnDevice is called, there is no guarantee
that adbd is already running, thus KillAll fails. I think that in my specific
case, this happens because of the call to RestartAdbServer() which precedes this
call in CleanupPendingProcesses.

Looking better at the code, it looks like waiting for the adb (host) daemon in
StartAdbServer with psgrep is not enough and does not guarantee that the device
will be visible in the next call to adb whatever.

Ideally we'd move wait-for-device in StartAdbServer.
However, the adb server (host side) is only one, while the devices can be many,
and there is no knowledge about them in StartAdbServer. That's why I placed the
wait-for-device here (does it make sense till this point?).
The only other place in which it could make sense IMHO is in
CleanupPendingProcesses() itself.
I suspect something similar is happening. But I miss a mental picture of the
current adb stop-start flow in the clank bots. Is it possible that, in that bug,
the adb server was restarted just before the failure point?

[bulach, 2013/11/27 12:10:53]

I agree this is misplaced and StartAdbServer's pgrep condition is insufficient
:)
I think it should be enough do "adb devices" in line 506 before declaring it has
succeeded?
at that point, pgrep would have said there's an adb running, so running any
command that would involve talking to the adb daemon in the host should be
enough? wdyt?

[Primiano Tucci (use gerrit), 2013/11/27 15:14:41]

Oh right, I got lost in the many layers. makes perfect sense.
Very funny that now I can't re-reproduce the issue locally anymore (but I'm sure
I've seen it failing yesterday exactly in that point).
Maybe let's add a note to the crbug.com/322828 and see if the flakiness goes
away back after this cl.

     adb_pids = self.KillAll('adbd', signal=signal.SIGTERM, with_su=True)
     assert adb_pids, 'Unable to obtain adbd pid'
     logging.info('Waiting for device to settle...')
     self._adb.SendCommand('wait-for-device')
 
   def RestartAdbServer(self):
     """Restart the adb server."""
     ret = self.KillAdbServer()
     if ret != 0:
       raise errors.MsgException('KillAdbServer: %d' % ret)
@@ skipping 1225 matching lines @@
   """
   def __init__(self, output):
     self._output = output
 
   def write(self, data):
     data = data.replace('\r\r\n', '\n')
     self._output.write(data)
 
   def flush(self):
     self._output.flush()