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

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: ted, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Assignee: nobody → ted.mielczarek
(Reporter)

Comment 1

9 years ago
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"
(Reporter)

Comment 2

9 years ago
Note that you'll probably need attachment 347973 [details] [diff] [review] to land before being able to usefully test this.
(Reporter)

Comment 3

9 years ago
Over to bhearsum for follow-on work.
Assignee: ted.mielczarek → bhearsum
(Assignee)

Updated

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

Updated

9 years ago
Priority: P3 → P2
(Assignee)

Comment 4

9 years ago
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)
(Assignee)

Comment 5

9 years ago
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

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

Updated

9 years ago
Attachment #352604 - Flags: review?(ted.mielczarek)
(Reporter)

Updated

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

Comment 6

9 years ago
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.
(Reporter)

Updated

9 years ago
Attachment #352604 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Comment 7

9 years ago
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:
(Assignee)

Comment 8

9 years ago
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)
(Assignee)

Comment 10

9 years ago
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)
(Assignee)

Comment 11

9 years ago
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)
(Assignee)

Comment 12

9 years ago
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

9 years ago
Attachment #354171 - Flags: review?(ted.mielczarek)
Attachment #354171 - Flags: review?(nthomas)
Attachment #354171 - Flags: review?(ccooper)

Updated

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

Ship it!
(Reporter)

Comment 14

9 years ago
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+
(Reporter)

Comment 15

9 years ago
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+
(Assignee)

Comment 16

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

changeset:   32:c769ab9edf8f
Attachment #354171 - Flags: review+
(Assignee)

Comment 17

9 years ago
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

9 years ago
Attachment #354171 - Flags: review+ → checked‑in+
(Assignee)

Comment 18

9 years ago
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+
(Assignee)

Comment 19

9 years ago
This stuff is all in.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 20

9 years ago
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 → ---

Updated

9 years ago
Assignee: bhearsum → ccooper
Status: REOPENED → ASSIGNED
changeset:   33:ae9225931907

I've updated the checkout on stage.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 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.
(Reporter)

Comment 24

9 years ago
(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.
(Assignee)

Comment 25

9 years ago
(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.
(Assignee)

Comment 26

9 years ago
(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.
(Assignee)

Comment 27

9 years ago
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)
(Reporter)

Updated

9 years ago
Attachment #355744 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 28

9 years ago
Oh, I also checked in a patch to build/tools which added the executable bit to post_upload.py
(Assignee)

Comment 29

9 years ago
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+

Comment 30

9 years ago
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 → ---
(Assignee)

Comment 31

9 years ago
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

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

9 years ago
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.