1328301 - Autophone - support adb 1.0.36

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Autophone failed to run jobs successfully when the adb version on the Ubuntu hosts was updated to the most recent 1.0.36 version. The jobs were queued and treeherder was updated to show them pending. When the jobs were executed, the treeherder status was updated to running but the jobs ended without the tests being run and with the treeherder status remaining set to running.

We should support the most recent adb version regardless but this may also have an effect on the frequent unrecoverable disconnections were are experiencing with the Pixel devices.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: example adb push profile logcat

adb 1.0.32 allowed us to do adb push localdir remotedir and have remotedir contain the contents of localdir. adb 1.0.36 changes the semantics so that adb push localdir remotedir results in remotedir/localdir instead.

https://android.googlesource.com/platform/system/core/+/master/adb/file_sync_client.cpp#965

if the destination directory exists, the source directory will be copied *into* the destination directory otherwise it will be copied *onto* the destination directory.

Comment 2

[Bob Clary [:bc] (inactive)]

gbrown handled adb push for adb 1.0.36 in bug 1285040 for mozdevice's devicemanagerADB. I tried a patch for adb.py which detects the version of adb in use and if it is >= 1.0.36 it uses the parent directory as the destination for push *and* pull instead of the actual destination. I believe this works pretty well for s1s2 but fails with the unittests.

I noticed that bug 1285040 only dealt with push but not pull. See do_sync_push and do_sync_pull in https://android.googlesource.com/platform/system/core/+/master/adb/file_sync_client.cpp

I ran some tests on staging which you can see at https://treeherder.allizom.org/#/jobs?repo=autoland&filter-searchStr=autophone&exclusion_profile=false&fromchange=89753b9617c1277c382d761392a7a6787b357744&tochange=3ea611dc2979d4b377531d4032b72c8ec49d2062&selectedJob=64349015

The first run for the build at 21:03:38 was using adb 1.0.32 while the second run was with adb 1.0.36. The build at 22:49:42 was run twice with adb 1.0.36.

Note that the crash test failures which were detected in C1/C2 in the first build using adb 1.0.32 were not detected as failures using adb 1.0.36. It appears that the unittest is not detecting the dmp files which may be the result of not handling the adb pull case in devicemanagerADB. Additionally, the unittest does not detect if it terminates unexpectedly without emitting the test shutdown log line which is not related to the adb issues.

I'll attach my work in process patch for adb.py and will work on devicemanagerADB.py next.

gbrown: what do you think about my comments regarding devicemanagerADB?

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: wip bug-1328301-adb-v1.patch

Comment 4

[Geoff Brown [:gbrown]]

I think you are right! I was not aware that pull changed also; it is important for that to be addressed too. I like your wip patch / lgtm.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: autophone - bug-1328301-adb-v2.patch

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-1328301-adb-v1.patch

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: bug-1328301-devicemanagerADB-v1.patch

Comment 8

[Bob Clary [:bc] (inactive)]

I have been running a try job on production which actually runs production autophone using adb 1.0.32 and unittests with the updated devicemanagerADB.py and a try job here in mozvirginia which is using adb 1.0.36, the updated adb.py in autophone and the updated devicemanagerADB.py in the unittests:

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&exclusion_profile=false&group_state=expanded&tochange=58b91229611066c29ba7f205f9e15c3e2943f006&fromchange=58b91229611066c29ba7f205f9e15c3e2943f006&selectedJob=70267409

https://treeherder.allizom.org/#/jobs?repo=try&exclusion_profile=false&group_state=expanded&tochange=58b91229611066c29ba7f205f9e15c3e2943f006&fromchange=58b91229611066c29ba7f205f9e15c3e2943f006

Things look pretty good *so far*.

Comment 9

[Bob Clary [:bc] (inactive)]

gbrown: If you want to wait on these reviews while the tests complete running and while I look the patches over yet again, that would probably work best. I'm thinking about removing the self.dirExists(remoteDir) requirement at https://hg.mozilla.org/try/rev/c24adca728f05def207de87eb1d0b978acd017ff#l1.67 and similar conditionals in the other locations/patches.

In fact, lets just plan on doing that. I'll cancel the review requests.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: test-pushpull.py

I used this to test from the mozdevice dir. If you run it, you'll need to change the device and the directories.

Comment 11

[Geoff Brown [:gbrown]]

Comment on attachment 8828367 [details] [diff] [review]
autophone - bug-1328301-adb-v2.patch

Review of attachment 8828367 [details] [diff] [review]:
-----------------------------------------------------------------

::: adb.py
@@ +162,5 @@
> +            output = subprocess.Popen([adb, 'version'],
> +                                      stdout=subprocess.PIPE,
> +                                      stderr=subprocess.PIPE).communicate()
> +            re_version = re.compile(r'Android Debug Bridge version (.*)')
> +            self._adb_version = re_version.match(output[0]).group(1)

I verified this works with my oldest adb, 1.0.31, from android sdk 18.

@@ +1736,5 @@
> +            # the remote destination directory exists, adb push will
> +            # copy the source directory *into* the destination
> +            # directory otherwise it will copy the source directory
> +            # *onto* the destination directory.
> +            #

Great explanation - thanks.

@@ +1754,5 @@
> +                local = new_local
> +            remote = '/'.join(remote.rstrip('/').split('/')[:-1])
> +        self.command_output(["push", local, remote], timeout=timeout)
> +        if copy_required:
> +            shutil.rmtree(temp_parent)

I dislike this copy/push/delete strategy. I feel like we do that elsewhere, or tried that before? If it breaks, you end up looking at an error message like 'failed command 'adb push /tmp/@#$#@% /lib' and you wonder, what are those files, and why are we pushing from /tmp? Or the command succeeds, but some files are missing, and then you need to figure out, were they lost on the local copy, or the push?

But I don't have a better idea!

@@ +1759,1 @@
>  

Are you comfortable with failure handling here wrt temporary file cleanup? Can command_output() raise, and if it does, then I suppose the temp files are not cleaned up (which might be okay)?

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 8828369 [details] [diff] [review]
bug-1328301-devicemanagerADB-v1.patch

Review of attachment 8828369 [details] [diff] [review]:
-----------------------------------------------------------------

Very glad you found and fixed the pull problem - thanks!!

Comment 13

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #11)

> 
> @@ +1754,5 @@
> > +                local = new_local
> > +            remote = '/'.join(remote.rstrip('/').split('/')[:-1])
> > +        self.command_output(["push", local, remote], timeout=timeout)
> > +        if copy_required:
> > +            shutil.rmtree(temp_parent)
> 
> I dislike this copy/push/delete strategy. I feel like we do that elsewhere,
> or tried that before? If it breaks, you end up looking at an error message
> like 'failed command 'adb push /tmp/@#$#@% /lib' and you wonder, what areco
> those files, and why are we pushing from /tmp? Or the command succeeds, but
> some files are missing, and then you need to figure out, were they lost on
> the local copy, or the push?
> 
> But I don't have a better idea!
> 

The inspiration was your original patch. It just makes things much simpler by making the directory leaf names identical.

> @@ +1759,1 @@
> >  
> 
> Are you comfortable with failure handling here wrt temporary file cleanup?
> Can command_output() raise, and if it does, then I suppose the temp files
> are not cleaned up (which might be okay)?

Good catch. command_output can raise. I could wrap it in a try and delete the temp file and reraise the exception. I'll add that before pushing. Thanks.

Comment 14

[Bob Clary [:bc] (inactive)]

gbrown: Is this ok you think?

        try:
            self.command_output(["push", local, remote], timeout=timeout)
        except:
            raise
        finally:
            if copy_required:
                shutil.rmtree(temp_parent)

Comment 15

[Geoff Brown [:gbrown]]

lgtm

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78066796efe
handle push/pull directory semantics changes in adb 1.0.36 for adb.py, r=gbrown.
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f215ed4df3
handle push/pull directory semantics changes in adb 1.0.36 for devicemanagerADB, r=gbrown

Comment 17

[Bob Clary [:bc] (inactive)]

Leave open while we are waiting for this to be merged to autoland and mozilla-central before continuing with the deployment.

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e78066796efe
https://hg.mozilla.org/mozilla-central/rev/92f215ed4df3

Comment 19

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/0ddd2b6c518dfb94dc571cb0673d9540c2f7196a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0b9bce076847
https://hg.mozilla.org/releases/mozilla-beta/rev/cd87cd1ee14e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/488bf5cc5ea1
https://hg.mozilla.org/releases/mozilla-release/rev/ea32d54d5b20