1107571 - Mozharness should set blobber urls in a buildbot property directly instead of going through the manifest

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Andrew Halberstadt [:ahal]]

Currently there's a buildbot property that points to a manifest containing file names to blobber urls. I think it would be cool if the dictionary of file name to url was itself the buildbot property. This way consumers can skip the step of downloading an intermediate manifest.

The number of files being uploaded to blobber is small enough that I don't think this will have much of an impact on the size of buildbot properties, but maybe this is still a concern?

Comment 1

[Andrew Halberstadt [:ahal]]

Also if there are existing consumers of blobber urls via the pulse BuildConsumer they would need to be updated (I'm not aware of any). Blobber urls are not yet exposed via the NormalizedBuildConsumer so we don't need to worry about that.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: bz://1107571/ahal

Comment 3

[Andrew Halberstadt [:ahal]]

/r/1175 - Bug 1107571 - Mozharness should place blobber uploads directly in buildbot props instead of via external manifest, r=armenzg

Pull down this commit:

hg pull review -r a05daa6b76f17a5dcc2c1afa34e77535d770adf2

Comment 4

[Andrew Halberstadt [:ahal]]

This patch doesn't depend on any changes to blobber so can land right away, but we should probably also stop uploading the manifest in blobber. I'll upload another patch for that in a bit if this is ok with you.

Works with mozharness try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=45c45d443ab7

p.s mozharness try is awesome!

Comment 5

[Andrew Halberstadt [:ahal]]

Jcranmer, I'm needinfo'ing you because you commented in https://bugzilla.mozilla.org/show_bug.cgi?id=986112#c4

Are you using the blobber_upload_manifest buildbot property at all? This patch would rename it to 'blobber_files' and just contain the dictionary directly instead of a url to a manifest. I just want to make sure I'm not breaking something with this change.

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: [blobber] stop uploading the files manifest

Once the mozharness change is in production, there will no longer be a reason to upload the manifest. We still need to accept the --output-manifest-url parameter until the mozharness bits stop passing it in, which in turn depend on this change landing :p. So we'll need to do a few patches back and forth to turn it off properly.

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Jcranmer, I'm needinfo'ing you because you commented in
> https://bugzilla.mozilla.org/show_bug.cgi?id=986112#c4
> 
> Are you using the blobber_upload_manifest buildbot property at all? This
> patch would rename it to 'blobber_files' and just contain the dictionary
> directly instead of a url to a manifest. I just want to make sure I'm not
> breaking something with this change.

Right now, it looks like my script is just using the treeherder data output:
        # Find all of the gcda artifacts.
        data = loadJSON(job['resource_uri'])
        artifact = filter(lambda x: x['name'] == 'Job Info',
            data['artifacts'])
        if len(artifact) == 0:
            print("Can't find results for %s, try again later?" %
                tconfig['shortname'])
            return
        ajson = loadJSON(artifact[0]['resource_uri'])
        artifacts = dict()
        for a in ajson['blob']['job_details']:
            if a['title'] != 'artifact uploaded': continue
            artifacts[a['value']] = a['url']
        files = filter(lambda x: re.match('gcda.*?.zip', x), artifacts)

It looks like the answer is "no"

Comment 8

[Mihai Tabara [:mtabara]⌚️GMT]

@ahal: Not sure I'm writing to the correct bug but I've merged your PR[1] and released 1.2.3 version. I had some trouble with my environment so I haven't tested it. Though, should it be ready and does not break anything - you can ask rail and he will deploy it[2] to production following which a mozharness script to bump blobuploader version will be needed.

I have added you to needinfo to ack on this ;-)

[1]: https://github.com/MihaiTabara/blobuploader
[2]: https://pypi.python.org/pypi/blobuploader

Comment 9

[Andrew Halberstadt [:ahal]]

(In reply to Mihai Tabara [:mtabara] from comment #8)
> @ahal: Not sure I'm writing to the correct bug but I've merged your PR[1]
> and released 1.2.3 version. I had some trouble with my environment so I
> haven't tested it. Though, should it be ready and does not break anything -
> you can ask rail and he will deploy it[2] to production following which a
> mozharness script to bump blobuploader version will be needed.
> 
> I have added you to needinfo to ack on this ;-)

Sorry, I guess I should have commented on the PR.. but don't merge that until the mozharness change here is landed or you'll probably break every job :p. Also Armen hadn't reviewed it yet, but I guess if he finds anything wrong with it I can do a follow up PR.

For future reference, should I not use the PR -> bugzilla review -> merge model for blobber?

Comment 10

[Mihai Tabara [:mtabara]⌚️GMT]

Ah, apologies for that.

Still, we should be okay for the moment. As long as nobody bumps up the blobuploader version in mozharness we're safe with this merge till it's ready to go into production.

Re: merge model for blobber - yes, it's the correct approach. Sorry again, it's my fault I rushed a bit the things this time :)

Comment 11

[Armen [:armenzg]]

https://reviewboard.mozilla.org/r/1173/#review729

The code is in the right direction, however, I want to not use an internal filename.

My apologies for the delay in the review. I had a backlog from the work week.

::: mozharness/mozilla/blob_upload.py
(Diff revision 1)
> -                self.set_buildbot_property(prop_name="blobber_manifest_url",
> +                with open('uploaded_files.json', 'r') as f:

You're using an internal filename to blobuploader.
Please use a flag to tell blobberc.py where to stored the list of uploaded files.

Comment 12

[Armen [:armenzg]]

Comment on attachment 8532560 [details]
MozReview Request: bz://1107571/ahal

r- but in the right direction.

Comment 13

[Armen [:armenzg]]

Comment on attachment 8532646 [details] [review]
[blobber] stop uploading the files manifest

We need a flad to specify the path to uploaded_files.json.
A follow up pull request is needed to address the merge.

I'm glad you're fixing this. It's a cleaner approach!

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: [blobber] accept an --output-manifest argument for writing uploaded file contents

I was trying to land these patches in a way that doesn't require a simultaneous change to both mozharness and blobber. That's ok though, this will work too. Except now, this PR will need to be used in production before the mozharness change (that I will update soon) can land. Once this is in production, there will be a brief period of time where the uploaded files won't be set anywhere in buildbot properties.

Comment 15

[Armen [:armenzg]]

rail, do we run deploying newer versions of blobuploader through you? or buildduty?

Comment 16

[Armen [:armenzg]]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> Created attachment 8533762 [details] [review]
> [blobber] accept an --output-manifest argument for writing uploaded file
> contents
> 
> I was trying to land these patches in a way that doesn't require a
> simultaneous change to both mozharness and blobber. That's ok though, this
> will work too. Except now, this PR will need to be used in production before
> the mozharness change (that I will update soon) can land. Once this is in
> production, there will be a brief period of time where the uploaded files
> won't be set anywhere in buildbot properties.

It should be OK since nobody is currently using it.

Could you please give your mozharness changes a run through try once a newer version of blobber is released?
Also note the version bump of blobber in blob_upload.py

Comment 17

[Rail Aliiev [:rail]]

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #15)
> rail, do we run deploying newer versions of blobuploader through you? or
> buildduty?

If I'm around I can help with that, otherwise poking buildduty is OK.

Comment 18

[Andrew Halberstadt [:ahal]]

/r/1175 - Bug 1107571 - Mozharness should place blobber uploads directly in buildbot props instead of via external manifest, r=armenzg
/r/1363 - Updated for use with --output-manifest; stop passing in --output-manifest-url to blobber.

Pull down these commits:

hg pull review -r 8fce454f2c6adb8c3af3b42447a000503715316b

Comment 19

[Andrew Halberstadt [:ahal]]

/r/1175 - Bug 1107571 - Mozharness should place blobber uploads directly in buildbot props instead of via external manifest, r=armenzg
/r/1363 - Updated for use with --output-manifest; stop passing in --output-manifest-url to blobber.
/r/1365 - Bump required blobber version in blob_upload.py (it isn't released yet)

Pull down these commits:

hg pull review -r 00f267fb5beffedb01c6af3e51081c54bb0adf69

Comment 20

[Armen [:armenzg]]

coop, could you please deploy a new version of blobber?

That is for https://github.com/MihaiTabara/blobuploader/pull/18
I wonder if we also need a version bump for blobuploader.

Comment 21

[Armen [:armenzg]]

https://reviewboard.mozilla.org/r/1173/#review741

This should work. Feel free to land after you see the right results on your Try push.

Comment 22

[Armen [:armenzg]]

> I wonder if we also need a version bump for blobuploader.

As per ahal, the version has already been bumped by mtabara.

Comment 23

[Andrew Halberstadt [:ahal]]

Oh, I think I wasn't clear. :mtabara bumped after the first PR, and then I landed another PR after that. So if we want to be proper, we should probably bump it again.

Comment 24

[Andrew Halberstadt [:ahal]]

Armen just bumped it for me, so all I need help with now is getting version 1.2.4 uploaded to pypi as well as http://pypi.pub.build.mozilla.org/pub/

I'll take care of updating the version in blob_uploader.py as I want to push to try first.

Comment 25

[Andrew Halberstadt [:ahal]]

Rail, could you help get version 1.2.4 uploaded to http://pypi.pub.build.mozilla.org/pub/ (and possibly also pypi, but I don't technically need that). Thanks!

Comment 26

[Rail Aliiev [:rail]]

http://pypi.pub.build.mozilla.org/pub/blobuploader-1.2.4.tar.gz

I'm also moving the original repo to https://github.com/mozilla/build-blobuploader

Comment 27

[Andrew Halberstadt [:ahal]]

Comment on attachment 8532560 [details]
MozReview Request: bz://1107571/ahal

Here is a working try run (in the logs I see the blobber_files buildbot prop getting set):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9c326086efb9

Landed:
https://hg.mozilla.org/build/mozharness/rev/95d3f73dcefc

Comment 28

[Andrew Halberstadt [:ahal]]

Attachment: [mozharness] fix test_mozilla_blob_upload.py

My push broke a test, here's a follow-up fix that wfm locally.

Comment 29

[Andrew Halberstadt [:ahal]]

Comment on attachment 8535008 [details] [diff] [review]
[mozharness] fix test_mozilla_blob_upload.py

https://hg.mozilla.org/build/mozharness/rev/f08381a5cd10

Comment 30

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/95d3f73dcefc

Comment 31

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/f08381a5cd10

Comment 32

[Andrew Halberstadt [:ahal]]

I see this working on treeherder and via pulse \o/

Comment 33

[Andrew Halberstadt [:ahal]]

Hrm, I noticed that the blobber_files property is a string, even though I did a json.loads() before calling set_buildbot_property.

Armen (or anyone) do you know if it's possible to set a dict instead of a string? I see other properties that are dicts in pulse inspector, but they aren't being set via mozharness.

Comment 34

[Armen [:armenzg]]

(In reply to Andrew Halberstadt [:ahal] from comment #33)
> Hrm, I noticed that the blobber_files property is a string, even though I
> did a json.loads() before calling set_buildbot_property.
> 
> Armen (or anyone) do you know if it's possible to set a dict instead of a
> string? I see other properties that are dicts in pulse inspector, but they
> aren't being set via mozharness.

I was surprised that we could set a dict as such.
We could iterate for each value but we will have to give each a different name.
We can prepend a known string (e.g. uploaded_artifact_ ) and then the file name.

The way that the system works is this:
* Set a buildbot property
** "Writing buildbot properties ['blobber_files'] to /builds/slave/test/properties/blobber_files"
** This creates a file under the "properties" directory
* For every file written to the "properties" directory we can the contents and buildbot sets them as properties

Buildbot takes them as this:
key: string

Buildbot as-is cannot turn a string that represents a dictionary into a property.

I hope I'm answering this in a clear manner!

In here:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014/12/2014-12-14-03-02-05-mozilla-central-android-api-9/mozilla-central_ubuntu64_vm_mobile_test-mochitest-4-bm117-tests1-linux64-build18.txt.gz

I see:
04:38:53     INFO - Setting buildbot property blobber_files to {u'7901e38d-df32-bd0d-71cf92f7-746f8776.dmp': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/f7cd5e7fe55e2b9dd2ad2808e6d3cde5183233cd056dc58dcbe9082af1c76a9f9bb7ef519524a86a622ba99be19acce84a756e58120c8bd441fd0d36e5ea5b2e', u'logcat-emulator-5554.log': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/747036289685cdef732ebb1fd29d56cdf50c733e7b8b7bfc4e3683e4a044f084c313834bf01eec5fa29f9f3f675a8ad4feaffbdb5def2e2f53e2f00d9fa9cbbd', u'raw_structured_logs.log': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/ab8af11afc06b7469040788ec72a13de9b2f4d0d7e73de2fce324231c02066727771be982452c79c0513d5c5ad54d93a29dd424204d16fdbb60d460408b4c95e', u'7901e38d-df32-bd0d-71cf92f7-746f8776.extra': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/f918e08d21bd50464d4bddce70288e966ff7cbcac3e99d0c2294205c4c80a5020b18592544454e6ec003a3b485bff46b5ed2e619158a2e9e97ededb83befea6b'}
04:38:53     INFO - Writing buildbot properties ['blobber_files'] to /builds/slave/test/properties/blobber_files

and then:

blobber_files:{u'7901e38d-df32-bd0d-71cf92f7-746f8776.dmp': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/f7cd5e7fe55e2b9dd2ad2808e6d3cde5183233cd056dc58dcbe9082af1c76a9f9bb7ef519524a86a622ba99be19acce84a756e58120c8bd441fd0d36e5ea5b2e', u'logcat-emulator-5554.log': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/747036289685cdef732ebb1fd29d56cdf50c733e7b8b7bfc4e3683e4a044f084c313834bf01eec5fa29f9f3f675a8ad4feaffbdb5def2e2f53e2f00d9fa9cbbd', u'raw_structured_logs.log': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/ab8af11afc06b7469040788ec72a13de9b2f4d0d7e73de2fce324231c02066727771be982452c79c0513d5c5ad54d93a29dd424204d16fdbb60d460408b4c95e', u'7901e38d-df32-bd0d-71cf92f7-746f8776.extra': u'http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/f918e08d21bd50464d4bddce70288e966ff7cbcac3e99d0c2294205c4c80a5020b18592544454e6ec003a3b485bff46b5ed2e619158a2e9e97ededb83befea6b'}
build_url:https://ftp-ssl.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android-api-9/1418554925/en-US/fennec-37.0a1.en-US.android-arm.apk

Comment 35

[Andrew Halberstadt [:ahal]]

Ok, if you look at pulseinspector though, you'll notice that some buildbot properties (e.g 'request_ids' and 'request_times') actually show up as lists/dicts. Maybe this is happening at a later step though, like in the BuildPublisher.

I guess I can just get pulse translator to serialize it.

Comment 36

[Armen [:armenzg]]

(In reply to Andrew Halberstadt [:ahal] from comment #35)
> Ok, if you look at pulseinspector though, you'll notice that some buildbot
> properties (e.g 'request_ids' and 'request_times') actually show up as
> lists/dicts. Maybe this is happening at a later step though, like in the
> BuildPublisher.
> 
> I guess I can just get pulse translator to serialize it.

IIUC, those are set outside of the job, in the actual schedulers, hence a different process.
We can probably ask rail to help us figure this out if you believe we need to fix it properly.