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

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: ted, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

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.
Assignee: nobody → ted.mielczarek
Created attachment 347975 [details]
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"
Note that you'll probably need attachment 347973 [details] [diff] [review] to land before being able to usefully test this.
Over to bhearsum for follow-on work.
Assignee: ted.mielczarek → bhearsum
(Assignee)

Updated

10 years ago
Priority: -- → P3
(Assignee)

Updated

10 years ago
Priority: P3 → P2
Created attachment 352604 [details] [diff] [review]
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.
Attachment #352604 - Flags: review?(nthomas)
Created attachment 352605 [details] [diff] [review]
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.
Attachment #347975 - Attachment is obsolete: true
Attachment #352605 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #352605 - Flags: review? → review?(ted.mielczarek)
(Assignee)

Updated

10 years ago
Attachment #352604 - Flags: review?(ted.mielczarek)
Attachment #352605 - Flags: review?(ted.mielczarek) → review-
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.
Attachment #352604 - Flags: review?(ted.mielczarek) → review+
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:
Created attachment 353870 [details] [diff] [review]
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.
Attachment #352604 - Attachment is obsolete: true
Attachment #353870 - Flags: review?(nthomas)
Attachment #352604 - Flags: review?(nthomas)
Comment on attachment 353870 [details] [diff] [review]
post-upload script - fini

Review cat iz sad, bugzilla eated cheesepatch.
Attachment #353870 - Flags: review?(nthomas)
Created attachment 354170 [details] [diff] [review]
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!
Attachment #352605 - Attachment is obsolete: true
Attachment #354170 - Flags: review?(ted.mielczarek)
Created attachment 354171 [details] [diff] [review]
post upload script, for realz

see comment #8 for details.
Attachment #353870 - Attachment is obsolete: true
Attachment #354171 - Flags: review?(nthomas)
Created attachment 354315 [details] [diff] [review]
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)
Attachment #354170 - Attachment is obsolete: true
Attachment #354315 - Flags: review?(ted.mielczarek)
Attachment #354170 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

10 years ago
Attachment #354171 - Flags: review?(ted.mielczarek)
Attachment #354171 - Flags: review?(nthomas)
Attachment #354171 - Flags: review?(ccooper)
Attachment #354171 - Flags: review?(ccooper) → review+
Comment on attachment 354171 [details] [diff] [review]
post upload script, for realz

Ship it!
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?
Attachment #354315 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #354171 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 354171 [details] [diff] [review]
post upload script, for realz

changeset:   32:c769ab9edf8f
Attachment #354171 - Flags: review+
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.
(Assignee)

Updated

10 years ago
Attachment #354171 - Flags: review+ → checked‑in+
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
Attachment #354315 - Flags: checked‑in+
This stuff is all in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bhearsum → ccooper
Status: REOPENED → ASSIGNED
changeset:   33:ae9225931907

I've updated the checkout on stage.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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 ?
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.
(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.
(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.
(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.
Created attachment 355744 [details] [diff] [review]
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.
Attachment #355744 - Flags: review?(ted.mielczarek)
Attachment #355744 - Flags: review?(ted.mielczarek) → review+
Oh, I also checked in a patch to build/tools which added the executable bit to post_upload.py
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
Attachment #355744 - Flags: checked‑in+
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: ccooper → bhearsum
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.