Closed Bug 1203085 Opened 9 years ago Closed 9 years ago

Mozharness tries to grab information from Buildbot's changes which is unavailable for BBB triggered jobs

Categories

(Release Engineering :: General, defect)

Product:
Release Engineering
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed, firefox44 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: armenzg, Assigned: armenzg)

References

Details

Attachments

(2 files, 9 obsolete files)

find_artifacts.py
1.34 KB, text/plain
Details
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD
40 bytes, text/x-review-board-request
jlund
: review+
Details 
      No description provided.
Can you add some more detail about the problem you're trying to solve?
My apologies.

I've recently started tinkering with the Buildbot Bridge to schedule
graphs of Buildbot builders.

A blocker I've found is that the test jobs cannot determine the
installer and tests urls that the parent builder has uploaded [1].

The way that this currently works for Buildbot is:
* On the parent builder a buildbot sendchange step pointing to the
uploaded files is executed
* The Buildbot schedulers create a Changes object with the two files
* When test job executes, it can find the Changes object inside of
buildprops.json

The BBB does not listen to sendchanges, hence, not being able to set a
Changes object.

I know that we could upload the files with well known names, however, I
fear that many other downstream systems rely on the current naming.
Besides, I can't make the Buildbot builders to use --installer-url and --test-url.

Instead, what I propose is that we modify Mozharness to be able to
inquire which is its parent task (the taskId is already there for the
current job) and find the artifacts it needs from the parent task.

We currently read buildprops.json inside of the action
read-buildbot-config. I will still keep this action, however, I would
like to add a taskcluster function to determine the missing
installer_url and test_url.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1194264#c12
Depends on: 1203552
Attached file find_artifacts.py — 
We don't need bug 1203552 because every Buildbot job triggered through the BBB has a task which uploads all properties set by the Buildbot job [1].

I've attached a python script which finds the parent task, finds the properties.json file and determines the installer, test package and symbols URLs.

dustin is also working on bug 1203573 which will create well known file paths that do not change.

This will be useful for TaskCluster tasks defined to run Buildbot jobs where you can specify --installer-url and --test-url.
For now, most Buildbot builders do *not* use those flags, hence, we still need to hijack read-buildbot-config to fetch the missing pieces of data in the Changes object associated to the test job triggered by BBB.

See bug 1197204 for more context.

[1] https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/KrVnqlYAR7msaUtyxHTWWg/0/public/properties.json
Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (We can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on that 'taskID' we can determine the parent task and the artifacts it uploaded.

* We add a get() function to ScriptMixin to return the contents of a URL
* We add in TaskClusterArtifactsFinderMixin to taskcluster_helper module
* The TaskClusterArtifactsFinderMixin allows any inheriting class to
find a the artifacts of the associated build of a test job
* In testbase.py we add the functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
Attachment #8660666 - Flags: review?(jlund)
Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes
Attachment #8660667 - Flags: review?(cmanchester)
Since both reviews came this way I will dupe bug 1204077 to this one.

Bug 1204077 - TryToolsMixin does not work with Buildbot jobs triggered through BBB
##################################################################################
The problem is the same as to what I am facing for read-buildbot-config (bug 1203085).
The changes object is empty, hence, we can't find the commit message.

Unlike in bug 1203085, I could find the info I needed from properties.json.

Nevertheless, I can grab the info from here:
https://hg.mozilla.org/try/json-pushes?changeset=71e69424094d&tipsonly=1&version=2&full=1

I can use the new get() method to grab the info:
https://hg.mozilla.org/try/rev/57c3bbe8cbcd

[1] https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/KrVnqlYAR7msaUtyxHTWWg/0/public/properties.json

Failing code:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/try_tools.py#31

You can see the jobs in here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e69424094d&filter-searchStr=Ubuntu%20VM%2012.04%20x64%20try%20opt%20test%20mochitest-1

The error:
0:41:45    ERROR - Exception during post-action for download-and-extract: Traceback (most recent call last):
10:41:45    ERROR -   File "/builds/slave/test/scripts/mozharness/base/script.py", line 1651, in run_action
10:41:45    ERROR -     method(action, success=success and self.return_code == 0)
10:41:45    ERROR -   File "/builds/slave/test/scripts/mozharness/mozilla/testing/try_tools.py", line 63, in _set_extra_try_arguments
10:41:45    ERROR -     msg = self._extract_try_message()
10:41:45    ERROR -   File "/builds/slave/test/scripts/mozharness/mozilla/testing/try_tools.py", line 31, in _extract_try_message
10:41:45    ERROR -     msg = self.buildbot_config['sourcestamp']['changes'][-1]['comments']
10:41:45    ERROR - IndexError: list index out of range
Summary: Help buildbot test jobs scheduled through BBB to find the installer and test packages it needs → Mozharness tries to grab information from Buildbot's changes which is unavailable for BBB triggered jobs
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

https://reviewboard.mozilla.org/r/19157/#review17079

::: testing/mozharness/mozharness/mozilla/testing/try_tools.py:32
(Diff revision 1)
> +        if len(self.buildbot_config['sourcestamp']['changes']) != 0:

nit: This can just be 

    if self.buildbot_config['sourcestamp']['changes']:
Attachment #8660667 - Flags: review?(cmanchester) → review+
I will look at this tomorrow
Attachment #8660666 - Flags: review?(jlund)
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

https://reviewboard.mozilla.org/r/19037/#review17309

this looks sweet! has it been tested anywhere? Apology for the delay.

::: testing/mozharness/mozharness/base/script.py:703
(Diff revision 1)
> +    def get(self, url):

how about instead of downloading the url contents to file, reading that file, and then loading json, why not just skip the file altogether:

return json.load(urllib2.urlopen(url)


if you wanted, to get the benefit of retry and urllib2 error handling, we could probably re-use retry_download_file and make it more generic (warning, awful untested code inbound): http://people.mozilla.org/~jlund/download_json_mh_func


s/retry_

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:121
(Diff revision 1)
> +    QUEUE_URL = 'https://queue.taskcluster.net/v1'

is there staging urls for this? I'm wondering if this should be configured?

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:126
(Diff revision 1)
> +            (self.QUEUE_URL + '/task/{}').format(task_id)))

looks like every use of QUEUE_URL immediately follows with + '/task/'

likewise SCHEDULER_URL follows with + '/task-graph/'


maybe we should just include that in the initial init of those class attrs. Also, urlparse.urljoin might help in these methods by cleaning up the string concatenation and '/' chars

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:139
(Diff revision 1)
> +    def find_parent_task_id(self, task_id):

this and find_artifacts at the very least can probably use some json try/catching

::: testing/mozharness/mozharness/mozilla/testing/testbase.py:286
(Diff revision 1)
> +    def find_artifacts_from_taskcluster(self):

I don't think this method has any side-effects. iow - I don't think it is mutating self.installer_url or self.test_url bc you are ignoring the return val from find_parent_artifacts
https://reviewboard.mozilla.org/r/19037/#review17309

Yes, it has been.
You can see that in this push I scheduled jobs through the BBB since the try syntax indicates the "fake" platform, hence, nothing should have been scheduled.

The jobs are running green in here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a76ca511bdea

Without this change the test jobs turn red.

> how about instead of downloading the url contents to file, reading that file, and then loading json, why not just skip the file altogether:
> 
> return json.load(urllib2.urlopen(url)
> 
> 
> if you wanted, to get the benefit of retry and urllib2 error handling, we could probably re-use retry_download_file and make it more generic (warning, awful untested code inbound): http://people.mozilla.org/~jlund/download_json_mh_func
> 
> 
> s/retry_

I will work on this.

> is there staging urls for this? I'm wondering if this should be configured?

There is no staging version.

> looks like every use of QUEUE_URL immediately follows with + '/task/'
> 
> likewise SCHEDULER_URL follows with + '/task-graph/'
> 
> 
> maybe we should just include that in the initial init of those class attrs. Also, urlparse.urljoin might help in these methods by cleaning up the string concatenation and '/' chars

I will fix this.

> this and find_artifacts at the very least can probably use some json try/catching

What do you have in mind? I have not yet hit any issues.

In any case if this fails, it's because the json downloaded was bad at source to begin with.

> I don't think this method has any side-effects. iow - I don't think it is mutating self.installer_url or self.test_url bc you are ignoring the return val from find_parent_artifacts

I will fix this.
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (We can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on that 'taskID' we can determine the parent task and the artifacts it uploaded.

* We add a get() function to ScriptMixin to return the contents of a URL
* We add in TaskClusterArtifactsFinderMixin to taskcluster_helper module
* The TaskClusterArtifactsFinderMixin allows any inheriting class to
find a the artifacts of the associated build of a test job
* In testbase.py we add the functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
Attachment #8660666 - Flags: review?(jlund)
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes
Small nit from chmanchester
Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

* Refactor _retry_download_file to _retry_download
* If no file is specified for _retry_download() we call _urlopen() instead
* Add load_json_url to fetch the contents of a json file without writing to disk
* Use urljoin and load_json_url in TaskClusterArtifactFinderMixin
* Add better docs to TaskClusterArtifactFinderMixin
* Fix issues in TaskClusterArtifactFinderMixin raised by jlund
Comment on attachment 8661801 [details]
MozReview Request: Small nit from chmanchester

Small nit from chmanchester
Attachment #8661801 - Flags: review?(armenzg)
Comment on attachment 8661802 [details]
MozReview Request: Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

* Refactor _retry_download_file to _retry_download
* If no file is specified for _retry_download() we call _urlopen() instead
* Add load_json_url to fetch the contents of a json file without writing to disk
* Use urljoin and load_json_url in TaskClusterArtifactFinderMixin
* Add better docs to TaskClusterArtifactFinderMixin
* Fix issues in TaskClusterArtifactFinderMixin raised by jlund
Attachment #8661802 - Flags: review?(jlund)
Comment on attachment 8661801 [details]
MozReview Request: Small nit from chmanchester

https://reviewboard.mozilla.org/r/19457/#review17415
Attachment #8661801 - Flags: review?(armenzg) → review+
https://reviewboard.mozilla.org/r/19037/#review17309

> What do you have in mind? I have not yet hit any issues.
> 
> In any case if this fails, it's because the json downloaded was bad at source to begin with.

Hi Jordan, can you please let me know if we can drop this issue? or give me more info please?
(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #19)
> https://reviewboard.mozilla.org/r/19037/#review17309
> 
> > What do you have in mind? I have not yet hit any issues.
> > 
> > In any case if this fails, it's because the json downloaded was bad at source to begin with.
> 
> Hi Jordan, can you please let me know if we can drop this issue? or give me
> more info please?

I agree with you, bad json should at least show that in traceback. I was thinking of catching, logging correctly in mh and letting the script cleanly finish but it's probably not worth the added complexity in conditions and try/catch lines.
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (We can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on that 'taskID' we can determine the parent task and the artifacts it uploaded.

* We add a get() function to ScriptMixin to return the contents of a URL
* We add in TaskClusterArtifactsFinderMixin to taskcluster_helper module
* The TaskClusterArtifactsFinderMixin allows any inheriting class to
find a the artifacts of the associated build of a test job
* In testbase.py we add the functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes
Comment on attachment 8661801 [details]
MozReview Request: Small nit from chmanchester

Small nit from chmanchester
Comment on attachment 8661802 [details]
MozReview Request: Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

* Refactor _retry_download_file to _retry_download
* If no file is specified for _retry_download() we call _urlopen() instead
* Add load_json_url to fetch the contents of a json file without writing to disk
* Use urljoin and load_json_url in TaskClusterArtifactFinderMixin
* Add better docs to TaskClusterArtifactFinderMixin
* Fix issues in TaskClusterArtifactFinderMixin raised by jlund
* Use kwargs when calling _retry_download() to avoid positional issues
* First check for files in Buildbot's changes
* Raise exception if we can't set the files
https://reviewboard.mozilla.org/r/19037/#review17309

> Hi Jordan, can you please let me know if we can drop this issue? or give me more info please?

From jlund's comment on Bugzilla, I'm dropping this requirement.
Comment on attachment 8661970 [details]
MozReview Request: * Use kwargs when calling _retry_download() to avoid positional issues

* Use kwargs when calling _retry_download() to avoid positional issues
* First check for files in Buildbot's changes
* Raise exception if we can't set the files
Attachment #8661970 - Flags: review?(jlund)
Attached file MozReview Request: Fix small issue (obsolete) — 
Fix small issue
Attachment #8662019 - Flags: review?(jlund)
All changes are in MozReview.
I'm doing one last run after the small fix I just landed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9417c6151f2c
Comment on attachment 8661802 [details]
MozReview Request: Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

https://reviewboard.mozilla.org/r/19459/#review17507

thanks for doing this. I mention some small nits about urljoin but other than that, looks great to me! :)

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:130
(Diff revision 2)
> +                                          '{}'.format(task_id)))

nit: isn't '{}'.format(task_id) == task_id?

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:143
(Diff revision 2)
> +        return urljoin(self.QUEUE_URL, '{}/artifacts/{}'.format(task_id, full_path))

same with here: I thought the benefit of urljoin is you can do things like:


urljoin(self.QUEUE_URL, task_id, 'artifacts', full_path)

::: testing/mozharness/scripts/desktop_l10n.py:700
(Diff revision 2)
> -            return self._retry_download_file(binary_file, dst_filename, error_level=FATAL)
> +            return self.download_file(url=binary_file, file_name=dst_filename,

huh, interesting how other scripts called _retry_download_file directly. thanks for smoking these out
Attachment #8661802 - Flags: review?(jlund) → review+
Comment on attachment 8662019 [details]
MozReview Request: Fix small issue

https://reviewboard.mozilla.org/r/19477/#review17511
Attachment #8662019 - Flags: review?(jlund) → review+
Comment on attachment 8661970 [details]
MozReview Request: * Use kwargs when calling _retry_download() to avoid positional issues

https://reviewboard.mozilla.org/r/19467/#review17509

::: testing/mozharness/mozharness/mozilla/testing/testbase.py:311
(Diff revision 1)
> -                self.find_artifacts_from_buildbot_changes()
> +                raise Exception(

good call. I think log.exception would be more mh like but whateves.
Attachment #8661970 - Flags: review?(jlund) → review+
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (We can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on that 'taskID' we can determine the parent task and the artifacts it uploaded.

* We add a get() function to ScriptMixin to return the contents of a URL
* We add in TaskClusterArtifactsFinderMixin to taskcluster_helper module
* The TaskClusterArtifactsFinderMixin allows any inheriting class to
find a the artifacts of the associated build of a test job
* In testbase.py we add the functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes
Comment on attachment 8661801 [details]
MozReview Request: Small nit from chmanchester

Small nit from chmanchester
Comment on attachment 8661802 [details]
MozReview Request: Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

* Refactor _retry_download_file to _retry_download
* If no file is specified for _retry_download() we call _urlopen() instead
* Add load_json_url to fetch the contents of a json file without writing to disk
* Use urljoin and load_json_url in TaskClusterArtifactFinderMixin
* Add better docs to TaskClusterArtifactFinderMixin
* Fix issues in TaskClusterArtifactFinderMixin raised by jlund
Comment on attachment 8661970 [details]
MozReview Request: * Use kwargs when calling _retry_download() to avoid positional issues

* Use kwargs when calling _retry_download() to avoid positional issues
* First check for files in Buildbot's changes
* Raise exception if we can't set the files
Comment on attachment 8662019 [details]
MozReview Request: Fix small issue

Fix small issue
Proper usage of urljoin and remove unused json
Attachment #8662363 - Flags: review?(armenzg)
Comment on attachment 8662363 [details]
MozReview Request: Proper usage of urljoin and remove unused json

https://reviewboard.mozilla.org/r/19587/#review17563
Attachment #8662363 - Flags: review?(armenzg) → review+
Call self.exception instead of raising exceptions + minor fixes
Attachment #8662366 - Flags: review?(jlund)
jlund: one minor changes for the exceptions.

Re-testing the last few fixes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9fec0c1c020

I reviewed only the changes that were addressing comments by jlund.
Different UX makes me do unusual things.
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

This patch had received an r+ earlier. The flag got reset.
Attachment #8660666 - Flags: review?(jlund) → review+
https://reviewboard.mozilla.org/r/19037/#review17595

Putting back the original r+ that was lost.
Comment on attachment 8662366 [details]
MozReview Request: Call self.exception instead of raising exceptions + minor fixes

https://reviewboard.mozilla.org/r/19589/#review17597

This is a follow up change so I'm carrying forward the review.
Attachment #8662366 - Flags: review+
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (We can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on that 'taskID' we can determine the parent task and the artifacts it uploaded.

* We add a get() function to ScriptMixin to return the contents of a URL
* We add in TaskClusterArtifactsFinderMixin to taskcluster_helper module
* The TaskClusterArtifactsFinderMixin allows any inheriting class to
find a the artifacts of the associated build of a test job
* In testbase.py we add the functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
Attachment #8660666 - Flags: review?(jlund)
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes
Comment on attachment 8661801 [details]
MozReview Request: Small nit from chmanchester

Small nit from chmanchester
Comment on attachment 8661802 [details]
MozReview Request: Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

Bug 1203085 - Add load_json_url and use it in TaskClusterArtifactFinderMixin

* Refactor _retry_download_file to _retry_download
* If no file is specified for _retry_download() we call _urlopen() instead
* Add load_json_url to fetch the contents of a json file without writing to disk
* Use urljoin and load_json_url in TaskClusterArtifactFinderMixin
* Add better docs to TaskClusterArtifactFinderMixin
* Fix issues in TaskClusterArtifactFinderMixin raised by jlund
Comment on attachment 8661970 [details]
MozReview Request: * Use kwargs when calling _retry_download() to avoid positional issues

* Use kwargs when calling _retry_download() to avoid positional issues
* First check for files in Buildbot's changes
* Raise exception if we can't set the files
Comment on attachment 8662019 [details]
MozReview Request: Fix small issue

Fix small issue
Comment on attachment 8662363 [details]
MozReview Request: Proper usage of urljoin and remove unused json

Proper usage of urljoin and remove unused json
Comment on attachment 8662366 [details]
MozReview Request: Call self.exception instead of raising exceptions + minor fixes

Call self.exception instead of raising exceptions + minor fixes
Fix urljoin() usage. r=bustage
Comment on attachment 8662537 [details]
MozReview Request: Fix urljoin() usage. r=bustage

Fix urljoin() usage. r=bustage
Attachment #8662537 - Flags: review?(armenzg)
Comment on attachment 8662537 [details]
MozReview Request: Fix urljoin() usage. r=bustage

https://reviewboard.mozilla.org/r/19625/#review17599

::: testing/mozharness/mozharness/mozilla/taskcluster_helper.py:136
(Diff revision 1)
> -        return self.load_json_url(urljoin(self.QUEUE_URL, task_id, 'artifacts'))
> +        return self.load_json_url(urljoin(self.QUEUE_URL, '{}/artifacts'.format(task_id)))

my bad. sorry about that
Attachment #8662537 - Flags: review+
Attachment #8662537 - Flags: review?(armenzg)
Attachment #8662366 - Flags: review?(jlund)
Attachment #8660666 - Flags: review?(jlund)
https://hg.mozilla.org/mozilla-central/rev/c9dbc1119342
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Fun story: I merged mozilla-central to b2g-inbound (a mistake which I will not make again), with my merge being the cset above the cset which made b2g actually *use* in-tree mozharness for tests. They all failed, trying to download the tests zip with the filename "fatal" and then not liking the result of that. This seems like a pretty good candidate for the cause of that, so I backed it out in https://hg.mozilla.org/integration/b2g-inbound/rev/a5b1224e416a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks philor.
I'm still puzzled as to what is different with Gaia tests on b2g-inbound compared to other trees.

For reference:
* Back out: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=a5b1224e416a
* The merge: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=07711f754650
* The m-i commit: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c9dbc1119342

It seems like all the gaia jobs failed (b2g desktop and Mulet):
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=07711f754650&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-searchStr=gaia

The difference between the two branches is when they download the test_packages.json file [1] [2]
We're probably calling _download_file without being explicit.


[1]
18:15:07     INFO - retry: Calling _download_file with args: ('https://queue.taskcluster.net/v1/task/BW64VCMNRqOOpLwwU5Q1yg/artifacts/public/build/test_packages.json', '/home/worker/build/test_packages.json'), kwargs: {}, attempt #1

[2]
06:35:35     INFO - retry: Calling _download_file with args: (), kwargs: {'url': 'https://queue.taskcluster.net/v1/task/DZTE0-ApQOKK3mzcTgW8JA/artifacts/public/build/test_packages.json', 'file_name': 'fatal'}, attempt #1
My impression is that what was different at the time that I merged to it was that the tip of it was https://hg.mozilla.org/integration/b2g-inbound/rev/7f2c1a5e6771, causing your patch to actually affect the tests. That has now been merged around, so I would now expect your patch to fail against any tree.
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Support fetching installer and test url from TaskCluster. r=jlund

If a Buildbot test job is scheduled through TaskCluster (The Buildbot Bridge supports this),
then the generated Buildbot Change associated to a test job does not have the installer and
test url necessary to Mozharness to run the test job.

Since we can't modify how a test job is called on Buildbot (we can't switch from
--read-builbot-config to --installer-url and --test-url), we have to detect that there is
a 'taskId' defined for the test job (this indicates that the job was scheduled through the BBB)
and based on suc 'taskID' we can determine the parent task and the artifacts it uploaded.

Changes to ScriptMixin:
* Refactor _retry_download_file() to _retry_download()
* If no file is specified when calling_retry_download() we call _urlopen() instead of _download_file()
* Add load_json_url() method to fetch the contents of a json file without writing to disk

Changes to TestingMixin:
* If the job is triggered through Buildbot we look for the Changes object, otherwise, we look
for artifacts of the parent task
* Added functions find_artifacts_from_buildbot_changes (original behaviour)
and find_artifacts_from_taskcluster (functionality via TaskClusterArtifactsFinderMixin)
* Call self.exception() instead of raising exceptions + minor fixes

New TaskClusterArtifactsFinderMixin:
* It allows any inheriting class to find the artifacts of the build job which triggers this test job
Attachment #8660666 - Attachment description: MozReview Request: Bug 1203085 - Teach Mozharness to query for build artifacts through TaskCluster → MozReview Request: Bug 1203085 - Support fetching installer and test url from TaskCluster. r=jlund
Attachment #8660666 - Flags: review?(jlund)
Attachment #8660667 - Attachment description: MozReview Request: Bug 1204077 - Grab try syntax from pushlog if not available in Buildbot Changes → MozReview Request: Call _retry_download() with kwargs
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

Call _retry_download() with kwargs
Attachment #8661801 - Attachment is obsolete: true
Attachment #8661802 - Attachment is obsolete: true
Attachment #8661970 - Attachment is obsolete: true
Attachment #8662019 - Attachment is obsolete: true
Attachment #8662363 - Attachment is obsolete: true
Attachment #8662366 - Attachment is obsolete: true
Attachment #8662537 - Attachment is obsolete: true
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Mozreview is the worse.
Attachment #8660666 - Flags: review?(jlund)
Somewhere in this mozreview madness, I lost a patch (or I think I do).
For some reason, it seems to think that I had already asked chmanchester to review 7 days ago (which I don't think so).

Here's a push that fails in the same way as b2g-inbound:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5e907c3288

Here's a push which has the patch that I thought it landed with everything else:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e7558818c27
Comment on attachment 8660667 [details]
MozReview Request: Call _retry_download() with kwargs

I'm unsure if anyone reviewed this code or if it got lost in my queue of patches.

This is all it is needed to get everything back to normal.

I don't how it would have worked without it.
Attachment #8660667 - Flags: review+ → review?(jlund)
(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #66)
> Comment on attachment 8660667 [details]
> MozReview Request: Call _retry_download() with kwargs
> 
> I'm unsure if anyone reviewed this code or if it got lost in my queue of
> patches.
> 
> This is all it is needed to get everything back to normal.
> 
> I don't how it would have worked without it.

sorry, I'm confusing myself. I went to review this in mozreview and it says r+ from chmanchester so I didn't bother. However I still have a r? in bugzilla. If you still need a reviewer. r+ from me on the last rev in mozreview
Attachment #8660667 - Flags: review?(jlund) → review+
https://hg.mozilla.org/mozilla-central/rev/7a8fcc716362
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
From bug 1205694:

We are highlighting "We have not been able to determine which artifacts to use in order to run the tests."[1] as an error. That should just be a red herring as that code is trying to determine the installer and test url location. But that is not expected to succeed yet. IIUC, we rely on the cli args of the mozharness call for those artifacts still:

" 08:24:56     INFO - Run as ./mozharness/scripts/b2g_emulator_unittest.py ... more cli ... --installer-url https://queue.taskcluster.net/v1/task/UBZa_3zOT5W3wtX2nY9eyA/artifacts/public/build/emulator.tar.gz --test-packages-url https://queue.taskcluster.net/v1/task/UBZa_3zOT5W3wtX2nY9eyA/artifacts/public/build/test_packages.json
"

Armen, I believe the intention for Bug 1203085 is for BBB. maybe we should silence the error raised in [1] so this doesn't look like it is a reason for failures? Or am I missing something?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8660666 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

When we added support for finding artifacts from TaskCluster BBB tasks
we added an error for not finding the artifacts either via buildbot properties
or through the TC APIs.

Unfortunately, this error did not take into consideration when mozharness sets
the artifacts through the options --installer-url and --test-url.

We're removing the error as few lines below the error logging is sufficient
to handle all three cases and not raise misleading errors.
Attachment #8660666 - Attachment description: MozReview Request: Bug 1203085 - Support fetching installer and test url from TaskCluster. r=jlund → MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD
Attachment #8660666 - Flags: review?(jlund)
Attachment #8660667 - Attachment is obsolete: true
Attachment #8660666 - Attachment is obsolete: true
Attachment #8660666 - Flags: review?(jlund)
Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

When we added support for finding artifacts from TaskCluster BBB tasks
we added an error for not finding the artifacts either via buildbot properties
or through the TC APIs.

Unfortunately, this error did not take into consideration when mozharness sets
the artifacts through the options --installer-url and --test-url.

We're removing the error as few lines below the error logging is sufficient
to handle all three cases and not raise misleading errors.
Attachment #8667865 - Flags: review?(jlund)
Comment on attachment 8667865 [details]
MozReview Request: Bug 1203085 - Silence unnecessary error about not finding artifacts. DONTBUILD

https://reviewboard.mozilla.org/r/20847/#review19145
Attachment #8667865 - Flags: review?(jlund) → review+
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
No longer depends on: 1203552
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: