671450 - change dated dirs on ftp.m.o to use new longer BuildID (part2)

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P2)

Description

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

After talking with coop, filing follow on from bug#449607 to do the same for build-on-checkin builds. 

Currently, we use epoch seconds for the directory name for build-on-checkin, but would change this to use longer buildID, like we already use for nightlies. For example, this:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1307987800/

...would look like:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/2011-06-13-10-56-40/

Comment 1

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

I would (continue) to say that the revision is what matters here for people/scripts parsing directory listings, not the build time. Using buildtime-revision gets you human-friendly sorting.

Comment 2

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

(In reply to comment #1)
> I would (continue) to say that the revision is what matters here for
> people/scripts parsing directory listings, not the build time. Using
> buildtime-revision gets you human-friendly sorting.

Do we particularly care whether buildtime is expressed in seconds-since-epoch or human-friendly?

Comment 3

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

Attachment: Use buildid and rev to create tinderbox-builds path.

(In reply to Nick Thomas [:nthomas] (out sick) from comment #1)
> I would (continue) to say that the revision is what matters here for
> people/scripts parsing directory listings, not the build time. Using
> buildtime-revision gets you human-friendly sorting.

This patch implements this. I use the buildid (which is human-readable and sortable) and the rev to facilitate scripting. The resulting path will look like this:

firefox/tinderbox-builds/mozilla-central-macosx64/20110926221552-cf6245609f48/

This is running on dev-stage01 now.

Comment 4

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

Comment on attachment 563406 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path.

Withdrawing review request until I can make sure revision isn't going to end up as None.

Comment 5

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

Attachment: Pass revision to post_upload.py

(In reply to Chris Cooper [:coop] from comment #4)
> Comment on attachment 563406 [details] [diff] [review] [diff] [details] [review]
> Use buildid and rev to create tinderbox-builds path.
> 
> Withdrawing review request until I can make sure revision isn't going to end
> up as None.

Found the two spots where we need to pass the revision through to post_upload.py.

Here's an example of how this looks in staging:
http://dev-stage01.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/20110930113204-743ed92f9332/

Comment 6

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

Comment on attachment 563828 [details] [diff] [review]
Pass revision to post_upload.py

In the unlikely event you missed a spot, will it be obvious to buildduty ?

Comment 7

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

(In reply to Nick Thomas [:nthomas] from comment #6)
> Comment on attachment 563828 [details] [diff] [review] [diff] [details] [review]
> Pass revision to post_upload.py
> 
> In the unlikely event you missed a spot, will it be obvious to buildduty ?

Shouldn't affect nightlies or releases at all. The fallout would be what I saw in staging: directories under tinderbox-builds that look like YYYYMMDDHHmmss-None/.

Comment 8

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

(In reply to Chris Cooper [:coop] from comment #7)
> Shouldn't affect nightlies or releases at all. The fallout would be what I
> saw in staging: directories under tinderbox-builds that look like
> YYYYMMDDHHmmss-None/.

Note that there are some directories like this appearing on dev-stage, but these are from other masters running without the buildbotcustom patches.

Comment 9

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

Comment on attachment 563406 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path.

https://hg.mozilla.org/build/tools/rev/39067fcf57d2

Comment 10

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

Comment on attachment 563828 [details] [diff] [review]
Pass revision to post_upload.py

https://hg.mozilla.org/build/buildbotcustom/rev/cbb385844136

Comment 11

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

Comment on attachment 563406 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path.

stage.m.o updated (from rev ddf6da21f86f to 0f564985fdfc, 1526)

Comment 12

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

Master reconfig'd with this.

Comment 13

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

I had to revert the log_uploader.py part of this 
  http://hg.mozilla.org/build/buildbotcustom/rev/60814e5bb5bb

got_revision isn't a property on test jobs, so we get:
Running [u'/builds/buildbot/tests1-windows/bin/python', u'/builds/buildbot/tests1-windows/buildbotcustom/bin/log_uploader.py', u'stage.mozilla.org', u'-u', u'ffxbld', u'-i', u'/home/cltbld/.ssh/ffxbld_dsa', u'-b', u'mozilla-central', u'-p', u'win32', u'--product', u'firefox', u'/builds/buildbot/tests1-windows/master/mozilla-central_win7-debug_test-xpcshell', u'308']
Traceback (most recent call last):
  File "/builds/buildbot/tests1-windows/buildbotcustom/bin/log_uploader.py", line 300, in <module>
    revision=build.getProperty('got_revision'),
  File "/builds/buildbot/tests1-windows/lib/python2.6/site-packages/buildbot-0.8.2_hg_aeaa057e9df6_production_0.8-py2.6.egg/buildbot/status/builder.py", line 1196, in getProperty
    return self.properties[propname]
  File "/builds/buildbot/tests1-windows/lib/python2.6/site-packages/buildbot-0.8.2_hg_aeaa057e9df6_production_0.8-py2.6.egg/buildbot/process/properties.py", line 50, in __getitem__
    rv = self.properties[name][0]
KeyError: 'got_revision'

Comment 14

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

Also had some load upload failures for l10n dep, and release major updates.

Comment 15

[Phil Ringnalda (:philor)]

Yeah, yeah, I should have realized this before it happened, but tbpl reverse-engineers the location of logs in http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/2ac6361a98bd/dataimport/import-buildbot-data.py#l29 so this completely busted it - it can tell you that a job was green or orange, but other than that you're on your own to figure out how many tests failed, what they were, and what bugs might have caused them.

Going back to that nightmare dark-ages state is unthinkable, so I'm closing trees.

Comment 16

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

Comment on attachment 563406 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path.

Backed out
  http://hg.mozilla.org/build/tools/rev/d6c393ca33ff
and stage.m.o updated to that.

Comment 17

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

(In reply to Phil Ringnalda (:philor) from comment #15)
> Yeah, yeah, I should have realized this before it happened, but tbpl
> reverse-engineers the location of logs in
> http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/
> 2ac6361a98bd/dataimport/import-buildbot-data.py#l29 so this completely
> busted it - it can tell you that a job was green or orange, but other than
> that you're on your own to figure out how many tests failed, what they were,
> and what bugs might have caused them.
> 
> Going back to that nightmare dark-ages state is unthinkable, so I'm closing
> trees.

Not expecting you to catch everything, philor. What this points to more is a need for us (releng) to have our own project branch so that changes that break TBPL can be caught *without* philor-related intervention.

I'll try to come up with a better patch tomorrow. Sorry for the trouble, nthomas.

Comment 18

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

(In reply to Chris Cooper [:coop] from comment #17)
> I'll try to come up with a better patch tomorrow.

Here's my proposal for the correct way to fix this, given the dependency in TBPL:

* Change post_upload.py to put tinderbox-builds into the the new directory structure (YYYYMMDDHHmmss-rev), but *also* create a symlink to the old seconds-since-epoch dir. That should allow TBPL to still find logs for now.
* Message about the change so that people have time to change other scripts that may be relying on the seconds-since-epoch dir structure.
* After a month, change TBPL to look for logs under the new dir structure. We expire logs after a month anyway, so all logs linked by TBPL will be available under the new format by then.
* Stop making the symlink in post_upload.py.

Sound reasonable?

(In reply to Nick Thomas [:nthomas] from comment #14)
> Also had some load upload failures for l10n dep, and release major updates.

Nick: do you have links to logs for these failures? I'd like to make sure they get run in staging this time.

Comment 19

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

(In reply to Chris Cooper [:coop] from comment #18)
> Here's my proposal for the correct way to fix this, given the dependency in
> TBPL:

Sounds good

> Nick: do you have links to logs for these failures? I'd like to make sure
> they get run in staging this time.

I can look some up for the generic case:
 http://buildbot-master08.build.mozilla.org:8001/builders/release-mozilla-1.9.2-major_update/builds/9
 http://buildbot-master08.build.mozilla.org:8001/builders/Firefox%20mozilla-central%20linux64%20l10n%20dep/builds/175

Perhaps we have some sort of fallback scheme, eg try for got_revision, then revision, otherwise use None.

Comment 20

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

Attachment: Try different sources for revision in log_uploader

(In reply to Nick Thomas [:nthomas] from comment #19) 
> Perhaps we have some sort of fallback scheme, eg try for got_revision, then
> revision, otherwise use None.

Done.

Comment 21

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

Comment on attachment 566948 [details] [diff] [review]
Try different sources for revision in log_uploader

Yay buildbot for having a 
  build.getProperty('foo')
which fails with a key error if foo doesn't exist, and 
  props = build.getProperties()
  props.getProperty('foo')
which happily returns None.

Comment 22

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

Attachment: Use buildid and rev to create tinderbox-builds path, v2

(In reply to Chris Cooper [:coop] from comment #18)
> * Change post_upload.py to put tinderbox-builds into the the new directory
> structure (YYYYMMDDHHmmss-rev), but *also* create a symlink to the old

This patch also creates the symlink. See examples in staging:

http://dev-stage01.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/
http://dev-stage01.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/

We'll need to examine the cronjobs on dev-stage01 and stage.m.o to make sure we're still cleaning up properly.

Comment 23

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

Comment on attachment 569819 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path, v2

>diff --git a/stage/post_upload.py b/stage/post_upload.py
>+        buildid_dir = "%s-%s" % (str(options.buildid),options.revision)

Nit, add a space before options.revision

>+    # Bug 671450 - see above fore removal criteria

Nit: fore -> for, tbpl doesn't like it if you play golf with log files and swat them round the place.

>+    if tinderboxBuildsPathOld:
>+        os.symlink(tinderboxBuildsPath, tinderboxBuildsPathOld)

For something long-lived I'd prefer a relative symlink like 
 1318571412 -> 20111013225012-9545b88eed82
rather than
 1318571412 -> /home/ftp/pub/firefox/tinderbox-builds/mozilla-central-linux/20111013225012-9545b88eed82
but it's not worth it here.

> We'll need to examine the cronjobs on dev-stage01 and stage.m.o to make sure
> we're still cleaning up properly.

Yeah, they're looking for -name 1????... at the moment. Something like '-type d -maxdepth N' might be better, with a dash of 'symlinks -d' thrown in to clean up anything that's dangling afterwards.

Comment 24

[Aki Sasaki (not active)]

Attachment: 2nd make upload breaks on dev-stage01

Coop: It looks like your patch on dev-stage01 is breaking multilocale uploads?

1) make upload # for en-US
2) multilocale steps
3) make upload AB_CD=multi # this breaks

Comment 25

[Aki Sasaki (not active)]

Attachment: en-US upload step

Comment 26

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

(In reply to Aki Sasaki [:aki] from comment #24)
> Created attachment 573957 [details]
> 2nd make upload breaks on dev-stage01
> 
> Coop: It looks like your patch on dev-stage01 is breaking multilocale
> uploads?

How quickly do you need the fix? Feel free to revert the local change to post_upload.py if it's preventing you from testing.

Comment 27

[Aki Sasaki (not active)]

(In reply to Chris Cooper [:coop] from comment #26)
> How quickly do you need the fix? Feel free to revert the local change to
> post_upload.py if it's preventing you from testing.

Awesome, I'll do that when I'm testing android native+xul builds.
But I was mainly pointing out that if we roll this change as-is to production, we're probably going to error out on all multilocale uploads.
No real urgency if it's fixed before it goes to production.

Comment 28

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

Attachment: Use buildid and rev to create tinderbox-builds path, v3

Only change from attachment 569819 [details] [diff] [review] is to check whether the symlinked dir already exists before trying to create it.

Here's the interdiff:

4c4
< @@ -154,36 +154,45 @@ def ReleaseToLatest(options, upload_dir,
---
> @@ -154,36 +154,47 @@ def ReleaseToLatest(options, upload_dir,
42c42,44
< +    if tinderboxBuildsPathOld:
---
> +    # Must also check whether the old directory structure has already been
> +    # created. Mobile repacks upload in two stages.
> +    if tinderboxBuildsPathOld and not os.path.exists(tinderboxBuildsPathOld):


This is running on dev-stage01 again.

Comment 29

[Aki Sasaki (not active)]

Comment on attachment 575509 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path, v3

I think this'll fix it.
I can try running a multilocale build to dev-stage01 this afternoon if you'd like.

Comment 30

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #29)
> Comment on attachment 575509 [details] [diff] [review] [diff] [details] [review]
> Use buildid and rev to create tinderbox-builds path, v3
> 
> I think this'll fix it.
> I can try running a multilocale build to dev-stage01 this afternoon if you'd
> like.

I just ran a multilocale android build and the upload ran fine against dev-stage01.
Thanks Coop!

Comment 31

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

Comment on attachment 566948 [details] [diff] [review]
Try different sources for revision in log_uploader

https://hg.mozilla.org/build/buildbotcustom/rev/c6f4f75908cb

Comment 32

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

Comment on attachment 575509 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path, v3

https://hg.mozilla.org/build/tools/rev/21c4e03dac14

The tools checkout on stage (/data/ffxbld/checkouts/hg-build-tools) can be safely updated with this after the buildbot masters get reconfig-ed to pick-up https://hg.mozilla.org/build/buildbotcustom/rev/c6f4f75908cb (attachment 566948 [details] [diff] [review]).

We'll worry about getting the TBPL log parsing updated sometime in the month following that. 

Do we care to do that though? Maybe it's enough to have the new symlinks so you can access the logs in either location, with the date-revision syntax being easier to search on.

Comment 33

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

I've updated post_upload.py on stage. I'll adjust the cleanup crontabs on Monday once I'm sure the new directories are being created/symlinked properly.

Comment 34

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

Comment on attachment 575509 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path, v3

Sorry, have had to back this out again, due to bug 709488. We missed that os.symlink is getting two absolute paths, so it creates absolute symlinks, and we don't necessarily have all the same mount arrangements on ftp as we do on stage.

http://hg.mozilla.org/build/tools/rev/f5e323d6be60

Comment 35

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

There's an issue with Shark builds too, although this some of that might be a buildbot config problem.

[ffxbld@surf tinderbox-builds]$ find mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/firefox-11.0a1.en-US.mac-shark.dmg
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/firefox-11.0a1.en-US.langpack.xpi
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/firefox-11.0a1.en-US.mac-shark.txt
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/jsshell-mac-shark.zip
mozilla-central-None/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/firefox-11.0a1.en-US.mac-shark.checksums
[ffxbld@surf tinderbox-builds]$ find mozilla-central-macosx64/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5
mozilla-central-macosx64/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5
mozilla-central-macosx64/20111210031150-63bff373cb94613e93c2795eb3b46e9a23ab3ee5/mozilla-central-macosx64-shark-build102.txt.gz

* the rev isn't truncated to 12 chars
* platform is None for the upload of Shark binaries, and (maybe?) correct for the log

Comment 36

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

(In reply to Nick Thomas [:nthomas] (away until Jan 3rd) from comment #35) 
> * the rev isn't truncated to 12 chars
> * platform is None for the upload of Shark binaries, and (maybe?) correct
> for the log

Tackling this part in bug 712008.

Comment 37

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

Attachment: Use buildid and rev to create tinderbox-builds path, v4

Uses the same pattern as elsewhere in the script to create a relative symlink in the same dir. 

This is running on dev-stage now.

Comment 38

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

Comment on attachment 584087 [details] [diff] [review]
Use buildid and rev to create tinderbox-builds path, v4

>diff --git a/scripts/release/tag-release.py b/scripts/release/tag-release.py
> REQUIRED_CONFIG = ('version', 'appVersion', 'appName', 'productName',
>-                   'milestone', 'buildNumber', 'hgUsername', 'hgSshKey',
>+                   'milestone', 'buildNumber', 'hgUsernameRW', 'hgSshKeyRW',

Is this file from another bug ?

>diff --git a/stage/post_upload.py b/stage/post_upload.py

>+        pathExtra = "%s-%s" % (str(options.buildid),options.revision) 

nit, add a space after the comma.

>     if options.builddir:
>-        tinderboxBuildsPath = os.path.join(tinderboxBuildsPath, options.builddir)
>-        tinderboxUrl = os.path.join(tinderboxUrl, options.builddir)
>+        if pathExtra == "":
>+            pathExtra = options.builddir
>+        else:
>+            pathExtra = os.path.join(pathExtra, options.builddir)
>+
>+    if pathExtra != "":
>+        tinderboxUrl = os.path.join(tinderboxUrl, pathExtra)
>+        dest_dir = os.path.join(tinderboxBuildsPath, pathExtra)
>+    else :
>+        dest_dir = tinderboxBuildsPath

To improve the flow here, can we write this ?
     if options.builddir:
         pathExtra = os.path.join(pathExtra, options.builddir)

     tinderboxUrl = os.path.join(tinderboxUrl, pathExtra)
     dest_dir = os.path.join(tinderboxBuildsPath, pathExtra)

As far as I can see (even on the ancient python 2.4.3 on stage) os.path.join handles empty strings sanely; at worst you get a trailing / doing join("foo","").
 
>+    # Bug 671450 - see above fore removal criteria

s/fore/for/

>+    if pathExtra != "" and pathOldExtra != "":

What does this catch that a check like |if pathExtra and pathOldExtra:| doesn't ? This should also be guarded by a check on dated, since it'll do the wrong thing if there's an options.builddir in pathExtra. Unit tests anyone ?

>+        try:
>+            cwd = os.getcwd()
>+            os.chdir(tinderboxBuildsPath)
>+            if not os.path.exists(pathOldExtra):
>+                os.symlink(pathExtra, pathOldExtra)
>+        finally:
>+            os.chdir(cwd)

I think you can boil this down to 
   os.symlink(pathExtra, tinderboxBuildsPath)
so that a relative link is created at an absolute location without a manual chdir. What sort of failures need a try block ? Doesn't this sort of pattern swallow them without alerting us ?

Comment 39

[Justin Wood (:Callek)]

(In reply to Nick Thomas [:nthomas] from comment #38)
> >+        try:
> >+            cwd = os.getcwd()
> >+            os.chdir(tinderboxBuildsPath)
> >+            if not os.path.exists(pathOldExtra):
> >+                os.symlink(pathExtra, pathOldExtra)
> >+        finally:
> >+            os.chdir(cwd)
> 
> I think you can boil this down to 
>    os.symlink(pathExtra, tinderboxBuildsPath)
> so that a relative link is created at an absolute location without a manual
> chdir. What sort of failures need a try block ? Doesn't this sort of pattern
> swallow them without alerting us ?

try...finally without an except won't swallow the error, BUT it will make sure we chdir back out if we fail in there at any point. Which is probably a good thing. finally: is executed on failure and success!

Comment 40

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

So I reconsidered a bit on my review. If the final state, where we don't need to do the symlinks any more, is clean then it's OK to have less-clean for now. Assuming of course that tbpl moves over to the new style sharpish.

Comment 41

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

If someone else wants to pick this up, they are free to. I might suggest tackling this at the same time as we move away from putting the dep builds under a directory named "tinderbox-builds."

Given that this is strictly cleanup, and that there would be required follow-up changes to both TBPL and some QA (MozMill) tools, I'm going to WONTFIX.