648114 - means of pushing sendchanges from amo to addons perf test pool

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Comment 1

[alice nodelman [:alice] [:anode]]

From offline conversation bwarner suggests that the way to fake a send change from a post is to write a new onchange recipient for buildbot.

We can also investigate creating a small webservice to pick up the amo request and then convert it to a sendchange.

Comment 2

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]correctly handle full path to addons

We currently use addonUrls of the form:

/en-US/firefox/downloads/latest/10900

For the on-demand system we'll need to handle full links:

http://ftp.mozilla.org/pub/mozilla.org/addons/92382/ie_tab_2__ff_3.6+_-2.12.21.1-fx.xpi

This patch does not break the current addonUrl parsing, but adds support for full links.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 548867 [details] [diff] [review]
[checked in]correctly handle full path to addons

lgtm.

Comment 4

[alice nodelman [:alice] [:anode]]

Attachment: version #1 of trigger.cgi

This is the initial cgi to trigger sendchanges for testing addons through a web device.  It will be checked in as part of the releng tools/buildfarm/utils directory.

Comment 5

[alice nodelman [:alice] [:anode]]

Comment on attachment 548867 [details] [diff] [review]
[checked in]correctly handle full path to addons

changeset:   1715:87aaab5cc5f5

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 550793 [details] [diff] [review]
version #1 of trigger.cgi

>MASTER = 'localhost'
>PORT = '9010'
>MASTERPATH = "/builds/buildbot/staging-addons"
>ENV = os.environ.copy()
>ENV['PYTHONPATH'] = "MASTERPATH/buildbotcustom:MASTERPATH/lib/python2.6/site-packages:MASTERPATH/lib/python2.6/site-packages/twisted:MASTERPATH/lib/python2.6/site-packages/zope".replace("MASTERPATH", MASTERPATH)
>ENV['PATH'] = "/opt/local/bin:MASTERPATH/buildbot/bin:MASTERPATH/tools/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/cltbld/bin".replace("MASTERPATH", MASTERPATH)

I am not a big fan of these paths hardcoded in a script.  Is this common in other related scripts?  I understand adding in some of the /builds/buildbot stuff to the existing, but we have /usr/* and /opt stuff hardcoded.


>
>#if var is a valid number returns a value other than None
>def checkNumber(var):
>    if var is None:
>      return 1
According to the comment, this should be returning None?  Also mixing 2 and 4 space indentation


>#if var is a valid string returns a value other than None
>def checkString(var):
>    if var is None:
>      return 1
According to the comment, this should be returning None?  Also mixing 2 and 4 space indentation

>    reString = re.compile('^[0-9A-Za-z._\-+/:]*$')
what about the '.' character?  I see it in there, but it isn't escaped.  Maybe we need to have a better definition of valid string in the comment


>def getBuildUrl(firefoxVersion, osVersion):
>    buildUrl = ''
>    try:
>        releaseUrl = "http://releases.mozilla.org/pub/mozilla.org/firefox/releases/latest-%s/" % (firefoxVersion,)
>        if osVersion == 'win32':
>            releaseUrl = releaseUrl + "win32/en-US/"
>            f = urllib2.urlopen(releaseUrl)
>            file = f.read()
>            match = re.search('"Firefox[^"]*.exe"',file)
>            if match:
>                buildPath = match.group(0).strip('"')
>                buildUrl = releaseUrl + buildPath

I would like to see some table where we have:
buildurl[osversion] = [path, binary_regex]

ex:
buildurl['win32'] = ['win32/en-US', '"Firefox[^"]*.exe"']

this is an example, but it could be implemented in various methods.  I see the different if/else conditions doing the same thing except for the path and regex.  It could even be as simple as:
if osversion == 'win32':
  path = 'win32/en-US/'
  binary_regex = '"Firefox[^"]*.exe"'
elif osversion == 'win64':
  ...

then do the urlopen and re.search on the path and binary_regex.


>def pingUrl(myUrl):
can you put a little comment describing this a bit more?

>        h.send(msg)
>        errcode, errmsg, headers = h.getreply()
>        if errcode == 200: 
>            return p[1] + p[2]
>        print "INFO: response from %s: %d %s %s" % (myUrl, errcode, errmsg, headers)
why do we have a print INFO here?  I assume anything but a 200 is an error?  Can we explain this a bit more?

># incoming trigger string has the following parameters:
># os, one of linux/linux64/macosx/macosx64/win32/win53

win53 doesn't look right here

>fields = ["os", "url", "firefox"]
>os = None
>url = None
>firefox = None
>#get form values and ensure that they are valid
>for field in fields:
>    if form.has_key(field):
>        form[field].value = urllib.quote_plus(form[field].value, '/:')
>        print "INFO: validating key " + field + " " + form[field].value
>        if checkString(form[field].value):
>          globals()[field] = form[field].value
mixing 2 and 4 space indendation here


>    cmdline = MASTERPATH + "/bin/buildbot sendchange --master=" + MASTER + ":" + PORT + " --branch=addontester-" + os + "-talos --username=addonWebDevice --property addonUrl:" + url + " " + buildUrl

can you use string interpolation here:
"%s/bin/buildbot sendchange --master=%s:%s --branch=addontester-%s -talos --username=addonWebDevice --property addonURL:%s %s" % (MASTERPATH, MASTER, PORT, os, url, buildUrl)



overall we should use string interpolation vs the + operator.  If I had to maintain this, I could hack through it, but I would immediately refactor stuff (as I mentioned above) and add a bunch of comments.  Also, why the print INFO/ERROR/SENDCHANGE stuff?  Is there a way to turn on/off INFO messages?  

r- so we can review this cleaned up a bit more.

Comment 7

[alice nodelman [:alice] [:anode]]

Attachment: version #2 of trigger.cgi

In this version:
- cleaned up indenting
- cleaned up string interpolation
- removed most of the env settings (turned out to be unnecessary)
- cleaned up how full urls to release builds are determined, removed a bunch of repetitive code

As to the INFO/ERROR/SENDCHANGE markers, this code is meant to be operated by another script on the AMO side of the world.  Those tokens are easy to pick out and read and still be able to debug from.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 551867 [details] [diff] [review]
version #2 of trigger.cgi

this looks a lot cleaner and better!

Comment 9

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]version #3 of trigger.cgi

Going to wait to request review for the latest version of cgi.  It's the result of an afternoon of testing with clouserw that shook out some new issues.

Comment 10

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]correctly handle full path to addons, now handle quoted urls

When doing a round of testing with clouserw, discovered some issues with how full urls are passed around.  We are going to have to use them encoded (ie, ':' becomes '%3A') so as to avoid how buildbot handles sendchange parameters (it always assumes that you can split on ':', which isn't the case for full and complete urls).  Using quoted urls means then having to unquote them later so that they are useful for downloading.  

This little patch does the unquoting.  Green on staging.

Comment 11

[alice nodelman [:alice] [:anode]]

Comment on attachment 552154 [details] [diff] [review]
[checked in]correctly handle full path to addons, now handle quoted urls

changeset:   1734:09769e8ae2ac

Comment 12

[alice nodelman [:alice] [:anode]]

Comment on attachment 552153 [details] [diff] [review]
[checked in]version #3 of trigger.cgi

Version #3 is what is currently being run and has been working fine.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 552153 [details] [diff] [review]
[checked in]version #3 of trigger.cgi

>mport cgitb; cgitb.enable()
>
accidentally included?


>	    print "ERROR: getBuildUrl: no buildInfo for given osVersion %s" % (osVersion,)
>    except Exception, e:
>        print "ERROR: buildUrl construction: ", e.__class__,  e, firefoxVersion, osVersion

nit: dangling , before the exception: '(osVersion,)'.  Also you are using string interpolation everywhere, but the exception error you are using a regular print with no interpolation.


>    except urllib2.URLError, e:
>        if hasattr(e, 'reason'):
>            print 'ERROR: pingUrl: %s' % (e.reason,)
>        elif hasattr(e, 'code'):
>            print 'ERROR: pingUrl, server error: %s' % (e.code,)
>    except Exception, e:
>        print "ERROR: pingUrl: ", e.__class__, e, myUrl
>    return ''
>
same nit here as previously.


>#check values for correctness
>if (None in (os, url, firefox)):
>    print "ERROR: didn't get all necessary fields"
>    sys.exit(1)
>#check os for correctness
>if (not (os in ('linux', 'linux64', 'macosx', 'macosx64', 'win32', 'win64'))):
>    print "ERROR: invalid os specified: %s" % (os,)
>    sys.exit(1)
>#check firefox version for correctness
>match = re.search('firefox(\d\.\d)', firefox)
>if match:
>    firefox = match.group(1) #reduce down to the version number
>else:
>    print "ERROR: invalid firefox specified: %s" % (firefox,)
>    sys.exit(1)
>#check addon url for correctness
>url = pingUrl(url)
>if not url:
>    print "ERROR: addon url no good"
>    sys.exit(1)

nit: I would prefer a space or two here to spread this out



r=me.  All feedback I have are nit's on print string styles and formatting.

Comment 14

[alice nodelman [:alice] [:anode]]

Comment on attachment 552153 [details] [diff] [review]
[checked in]version #3 of trigger.cgi

changeset:   1820:cf265ea8fb5e

Comment 15

[alice nodelman [:alice] [:anode]]

This is up and working.  Bugs in the system should now be filed separately.