455578 - create a post-upload script on stage to do release-to-dated, etc

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[(not currently active) Ted Mielczarek]

bhearsum and I were discussing this the other day. I thought it would be much nicer to have a script that lives on stage that our upload process could call to do all of the release-to-dated linking and other muckery. This would mean that the upload script could be very simple, just scp'ing files, and calling this post-upload script, which would handle all of our (very specific) details of arranging the files on disk. justdave indicated that he was ok with the concept via IRC.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: a proposed skeleton of implementation

Here's a skeleton for a proposed implementation. You pass some relevant info in the args. I've implemented a --release-to-latest (although I'm not sure it does exactly what it ought to). You'll have to pass --product=foo --branch=whatever to the script, but you should be able to set that all in the environment, like:
export POST_UPLOAD_CMD="/home/luser/post_upload.py --product=firefox --branch=mozilla-central --release-to-latest"

Comment 2

[(not currently active) Ted Mielczarek]

Note that you'll probably need attachment 347973 [details] [diff] [review] to land before being able to usefully test this.

Comment 3

[(not currently active) Ted Mielczarek]

Over to bhearsum for follow-on work.

Comment 4

[[e-mail] (:bhearsum)]

Attachment: post-upload script - WIP

This is a WIP but if either of you has time for high-level feedback it'd be great. I've implemented all of our types of uploads (latest, dated, tbox, candidates) and tested them in a manual fashion.

With some more testing and bugfixing I think this will do the job.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: fixes & enhancments for upload.py

Two things here:
1) Use '-oIdentityFile' instead of -i (because ~/ style paths don't work with -i).
2) Add support for uploading files to a temporary directory.

I think this is all pretty straightforward. MakeRemoteTempDir isn't great - it assumes the mktemp on the other side supports '-d' - which is true on Linux buy may not be on other unixes. I thought about doing something with a PID but that'll be generated client-side and could conflict with other clients trying to upload things.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 352605 [details] [diff] [review]
fixes & enhancments for upload.py

Can you use "temp" instead of "tmp" in all the variable names, etc? I find it a little easier to read.

+        ssh_key = WindowsPathToMsysPath(ssh_key)
+        cmdline.extend(["-o", "IdentityFile=%s" % ssh_key])

I'd just use WindowsPathToMsysPath() directly in the string substitution and skip the intermediate variable.

+    if (not path and not upload_to_tmp_dir) or (path and upload_to_tmp_dir):
+        print "One (and only one of UPLOAD_PATH or UPLOAD_TO_TMP must be " + \
+              "defined."

Surely this should error out?

+def MakeRemoteTmpDir(user, host, port=None, ssh_key=None):

This command pretty much duplicates all of DoSSHCommand. How about instead, you change DoSSHCommand to always return the command output, and then you could just call DoSSHCommand("mktemp -d",...)? Also, you're using os.popen2 here, but we're already importing subprocess in this file (via util), so you might as well use that. (See http://docs.python.org/library/subprocess.html#replacing-os-popen-os-popen2-os-popen3 )

+        if upload_to_tmp_dir:
+            path = MakeRemoteTmpDir(user, host, port=port, ssh_key=key)

In the interest of future usefulness, can you add another keyword arg to UploadFiles, "upload_to_temp_dir"? Then in UploadFiles, at the very beginning, you can check that and run your SSH command to set path. That way if we want to use this script as a module in the future, we don't have to duplicate the temp dir logic.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 352604 [details] [diff] [review]
post-upload script - WIP

This looks great to me, my only style comment is that I would write this:

+    buildidDict = {}
+    try:
+        # strptime is no good here because it strips leading zeros
+        buildidDict['year']   = buildid[0:4]
+        buildidDict['month']  = buildid[4:6]
+        buildidDict['day']    = buildid[6:8]
+        buildidDict['hour']   = buildid[8:10]
+        buildidDict['minute'] = buildid[10:12]
+        buildidDict['second'] = buildid[12:14]
+    except:

like:
try:
  buildidDict = {'year': buildid[0:4],
                'month': buildid[4:6],
  ...
  }
except:

Comment 8

[[e-mail] (:bhearsum)]

Attachment: post-upload script - fini

This isn't too much different than the first version. The candidates dir target now supports win32 builds properly and I fixed a couple of small bugs. You can see this in action on staging-master right now. (http://staging-master.build.mozilla.org:8010/waterfall?category=release uploading to http://staging-stage.build.mozilla.org/pub/mozilla.org/firefox/nightly/3.1b2-candidates/build1/)

Note that it's using the yet-to-be-posted l10n-upload-% target - but that is very much akin to the en-US upload one.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 353870 [details] [diff] [review]
post-upload script - fini

Review cat iz sad, bugzilla eated cheesepatch.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: again, using subprocess

I've addressed all of your comments, Ted. The ssh_key one didn't really matter anymore, though, because I had to special case paths starting with tilde. I also added a 'rm -rf' at the end of UploadFiles when using a temp directory. As it turns out, uploading builds to temp directories and not cleaning them up ends up eating quite a bit of disk space!

Comment 11

[[e-mail] (:bhearsum)]

Attachment: post upload script, for realz

see comment #8 for details.

Comment 12

[[e-mail] (:bhearsum)]

Attachment: upload.py fixes, last round

My comments from the last patch apply here, plus I've moved the temp dir cleanup to UploadFiles and fixed a bug that caused UPLOAD_TO_TEMP to fail (because 'path' is None now that the temp dir is generated in UploadFiles)

Comment 13

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

Comment on attachment 354171 [details] [diff] [review]
post upload script, for realz

Ship it!

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 354315 [details] [diff] [review]
upload.py fixes, last round

Looks good. Just one nit: can you fixup the comment block at the top of the file to mention that one and only one of UPLOAD_PATH or UPLOAD_TO_TEMP_DIR are required?

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 354171 [details] [diff] [review]
post upload script, for realz

You didn't take my style suggestion from comment 7, I am saddened. But really, I have nothing else to add.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 354171 [details] [diff] [review]
post upload script, for realz

changeset:   32:c769ab9edf8f

Comment 17

[[e-mail] (:bhearsum)]

I created a checkout of hg.m.o/build/tools on stage.m.o and dropped post_upload.py into /home/ffxbld/bin to be compatible with the Buildbot patches over in 464154.

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 354315 [details] [diff] [review]
upload.py fixes, last round

Pushed to mozilla-central & mozilla-1.9.1 (tracemonkey isn't doing l10n, so we don't need it there yet - hopefully they'll pick it up before we do).

m-c: changeset:   22996:36d425e44cff
m-1.9.1: changeset:   22471:8cfb060b7567

Comment 19

[[e-mail] (:bhearsum)]

This stuff is all in.

Comment 20

[Axel Hecht [:Pike]]

Reopening. This script is breaking l10n uploads at 

http://hg.mozilla.org/build/tools/annotate/c769ab9edf8f/stage/post_upload.py#l76

with a malformed if.

Comment 21

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

changeset:   33:ae9225931907

I've updated the checkout on stage.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

I set u+x on upload.py for ffxbld so this can execute, we were getting
  bash: /home/ffxbld/bin/post_upload.py: Permission denied

Also, is it expected that stage:/tmp is filling up with dirs, or just a symptom of the failure ?

Comment 23

[Dave Miller [:justdave]]

I wasn't paying close enough attention to this bug, but on the mythical new staging server that someday will finish getting set up, there will be no /home/ffxbld available to you, and the script will probably be put in /usr/bin.

Comment 24

[(not currently active) Ted Mielczarek]

(In reply to comment #23)
> I wasn't paying close enough attention to this bug, but on the mythical new
> staging server that someday will finish getting set up, there will be no
> /home/ffxbld available to you, and the script will probably be put in /usr/bin.

That's the sort of place I was expecting it to wind up. It's generic enough that that should be fine. (takes commandline params for all the path info it needs) I think bhearsum just put it in /home/ffxbld because it was easy and he had write permission there.

Comment 25

[[e-mail] (:bhearsum)]

(In reply to comment #22)
> I set u+x on upload.py for ffxbld so this can execute, we were getting
>   bash: /home/ffxbld/bin/post_upload.py: Permission denied
> 
> Also, is it expected that stage:/tmp is filling up with dirs, or just a symptom
> of the failure ?

That definitely should not be happening. I'll look into this.

Comment 26

[[e-mail] (:bhearsum)]

(In reply to comment #24)
> (In reply to comment #23)
> > I wasn't paying close enough attention to this bug, but on the mythical new
> > staging server that someday will finish getting set up, there will be no
> > /home/ffxbld available to you, and the script will probably be put in /usr/bin.
> 
> That's the sort of place I was expecting it to wind up. It's generic enough
> that that should be fine. (takes commandline params for all the path info it
> needs) I think bhearsum just put it in /home/ffxbld because it was easy and he
> had write permission there.

Yeah, that's exactly what happened.

Comment 27

[[e-mail] (:bhearsum)]

Attachment: make sure upload.py always cleans up after itself

Nick, it turns out that filling up /tmp only happened because the upload script was bailing. Looks like I failed to encase the cleanup portion in a finally: block, so it would only execute on success. This patch should remedy it.

Comment 28

[[e-mail] (:bhearsum)]

Oh, I also checked in a patch to build/tools which added the executable bit to post_upload.py

Comment 29

[[e-mail] (:bhearsum)]

Comment on attachment 355744 [details] [diff] [review]
make sure upload.py always cleans up after itself

m-c: changeset:   23409:e30ccbc29e3c
m-1.9.1: changeset:   22725:ac02c42d1484

Comment 30

[Axel Hecht [:Pike]]

Sob, reopening again.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/ only gets l10n xpis now.

It seems to me that we're not running post_upload.py to upload to latest for anything but l10n builds, is that right? If so, can we change that one way or the other?

Comment 31

[[e-mail] (:bhearsum)]

I checked in a bustage fix for this (http://hg.mozilla.org/build/tools/rev/cf6614a8d619) - to make sure we get some proper nightlies out this morning.

I just filed bug 474297 which I will get to this week or early next week.

Comment 32

[[e-mail] (:bhearsum)]

Okay, I forgot to update the script on staging until this morning. I just watched 'ar' be uploaded successfully though, so this should be fine now.