Closed Bug 1328301 Opened 3 years ago Closed 3 years ago

Autophone - support adb 1.0.36

Categories

(Testing :: Autophone, defect)

defect
Not set

Tracking

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
Blocks: 1328302
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.
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?
Flags: needinfo?(gbrown)
See Also: → 1285040
Attached patch wip bug-1328301-adb-v1.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(gbrown)
Attachment #8827927 - Attachment is obsolete: true
Attachment #8828368 - Flags: review?(gbrown)
Attachment #8828367 - Flags: review?(gbrown)
Attachment #8828369 - Flags: review?(gbrown)
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=bclary@mozilla.com&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*.
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.
Attachment #8828367 - Flags: review?(gbrown)
Attachment #8828368 - Flags: review?(gbrown)
Attachment #8828369 - Flags: review?(gbrown)
Attached file 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.
Attachment #8828367 - Flags: review?(gbrown)
Attachment #8828368 - Flags: review?(gbrown)
Attachment #8828369 - Flags: review?(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)?
Attachment #8828367 - Flags: review?(gbrown) → review+
Attachment #8828368 - Flags: review?(gbrown) → review+
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!!
Attachment #8828369 - Flags: review?(gbrown) → review+
(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.
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)
Flags: needinfo?(gbrown)
lgtm
Flags: needinfo?(gbrown)
Pushed by bclary@mozilla.com:
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
Leave open while we are waiting for this to be merged to autoland and mozilla-central before continuing with the deployment.
https://hg.mozilla.org/mozilla-central/rev/e78066796efe
https://hg.mozilla.org/mozilla-central/rev/92f215ed4df3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1340004
See Also: → 1346083
See Also: → 1410723
You need to log in before you can comment on or make changes to this bug.