378226 - implement Push support for all Steps that need it

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Robert Helmer [:rhelmer]]

Attachment: make Build and Repack get the pushdir from the build log

Bootstrap supports an optional Push method for the Step class which runs after the Execute and Verify but before Announce.

At minimum the following steps need to use it:

Build/Repack: copy from dated dir to candidates dir on stage
Source: copy to candidates dir on stage
Updates: copy MARs to candidates dir on stage, copy AUS config to AUS staging

Build and Repack both need to know where Tinderbox pushed to builds to, I am attaching a patch which allows this. Add support for getting the buildID to Build step, fix a bug in TinderLogParse::GetLogFilename(), and make both Build and Repack announce the buildid (for en-US only) and pushdir (for both), while we're at it.

Comment 1

[J. Paul Reed [:preed]]

Comment on attachment 262311 [details] [diff] [review]
make Build and Repack get the pushdir from the build log

Looks good; some comments inline:

>+    if (! defined($logParser->GetBuildID)) {
>+        die("No buildID found in $buildLog");
>+    }
>+    if (! defined($logParser->GetPushDir)) {
>+        die("No pushDir found in $buildLog");
>+    }

Can you put parens around the Get*, so it's clear they're function calls and not attributes (one of my perl pet peeves, BTW)

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: parens around method calls :)

same as before, plus parens

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to comment #2)
> Created an attachment (id=262688) [details]
> parens around method calls :)
> 
> same as before, plus parens
> 

Landed this one:

Checking in Bootstrap/Step/Build.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Build.pm,v  <--  Build.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bootstrap/Step/Repack.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Repack.pm,v  <--  Repack.pm
new revision: 1.11; previous revision: 1.10
done
Checking in MozBuild/TinderLogParse.pm;
/cvsroot/mozilla/tools/release/MozBuild/TinderLogParse.pm,v  <--  TinderLogParse.pm
new revision: 1.2; previous revision: 1.1
done

Comment 4

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

needed for "release automation", hence marking as critical.

Comment 5

[Robert Helmer [:rhelmer]]

Going back to comment #1, I think that Push() method is fine for the Source and Update step. However, Build and Repack are a little murkier to me right now, since we make the assumption that each step is run on a single machine.

These are different because tinderbox *already does* do a push, it just pushes to a dated directory and not a candidates directory. Seems like we can:

a) make Build/Repack::Push() ssh to stage, then locally copy dated_dir -> candidates_dir
b) make Build/Repack::Push() re-upload local build to stage:candidates_dir
c) have a whole independent step that runs on stage, e.g. BuildPush and RepackPush

"b" is more like what Tinderbox does, but I don't like going around Tinderbox like this and making assumptions about where it's dropping it's build, so I'd say that's out.

"c" seems way to heavy handed for a single command.

That leaves us with "a", which seems ok to me. Any thoughts?

Comment 6

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

Attachment: Push support for the Source step [checked in, rev 1.6]

Comment 7

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

(In reply to comment #5)

Option a) sounds fine to me. For nightlies, tinderbox also uses ssh to do an rsync from the dated dir to the latest- dir.

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 269665 [details] [diff] [review]
Push support for the Source step [checked in, rev 1.6]

looks good; have you been able to test it?

Thanks for filling in the Verify method too, looks great.

Comment 9

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

(In reply to comment #8)
> looks good; have you been able to test it?

I did some testing while working on the patch, by setting up a /home/ftp/pub dir on my mac for Push, and against a source log from Firefox 2.0.0.4 for Verify. Would be worth checking as part of a full source run at some point.

Comment 10

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

Attachment: Push support for the Build & Repack steps

* I could go either way on whether we need to have a separate log for the pushes
* Repack::Verify should probably move elsewhere, since the flow is 
     Build -> Repack -> Sign -> l10n verification
  and it's a pain to go ./release -o Repack -e, ./release -o Repack -p
  Part of the great verification rewrite ?
* Do we care that the two Push steps are almost identical ? (2nd rsync and log location differ). Do you have any re-factoring suggestions ?
* What do you say to moving the checks on BuildID and PushDir to their Get* functions, so that we don't have to be so careful every time they're used ?

I've done local testing with Fx 2.0.0.4 logs, a second user account and a passphrase-less ssh key (ala cltbld, using -i argument to ssh - now removed).

Comment 11

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

Attachment: Push support for the Build & Repack steps - v2

Only saner use of include & exclude with rsync, and whitespace changes, from attachment 270032 [details] [diff] [review] (v1).

Comment 12

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

Attachment: Push support for Update step

The final part of push support. I'm enforcing that only partial mars are pushed up to the FTP server, but not that only appropriate channels in any dir are pushed to the AUS server. Mainly that's because I couldn't come with a nice way to do it without second guessing the patcher config. I think it's important though, especially if we are using a script to make update snippets live, and so do less checks then. Any suggestions ?

Used local testing again.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to comment #12)
> Created an attachment (id=270721) [details]
> Push support for Update step
> 
> The final part of push support. I'm enforcing that only partial mars are pushed
> up to the FTP server, but not that only appropriate channels in any dir are
> pushed to the AUS server. Mainly that's because I couldn't come with a nice way
> to do it without second guessing the patcher config. I think it's important
> though, especially if we are using a script to make update snippets live, and
> so do less checks then. Any suggestions ?

There's a TestAusCallback() function there that checks that there are only test dirs for a particular path; it currently checks the "aus2.test" directory (it checks that, if the dirname
contains "beta" or "release", then it also contains "test").

I think that we only want to push test snippets live automatically. It'd also useful to announce the location on the AUS server that the snippets were staged to.

What I'd really like is to be able to update AUS and have it be disabled or only enabled in some testing capacity until someone flipped the switch; this isn't the right bug to solve that though :) That is, separate the "update AUS config" from "enable/disable a particular set of updates"... there is not a good way to do that with the current server AFAIK.

Comment 14

[J. Paul Reed [:preed]]

Sorry for the bugspam; these are now P2 in the New View of the World (tm).

Comment 15

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

Comment on attachment 270721 [details] [diff] [review]
Push support for Update step

Clearing review request to address pre-review comments.

Comment 16

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

Attachment: Push support for Update step - v2

Since the last patch:

- Fixes up TestAusCallback, which would have missed a release or beta snippet slipping into a subdir of aus2.test (by matching on that test instead of something in the channel name). Also restricts file names to partial and complete.txt, and check aus2.beta and aus2 too. Not partner friendly, is that a problem at this stage ?

- I'm kinda torn over the callbacks, there are now 4 to handle different cases. We could collapse them by passing the regexp via a global, but which is the lesser evil ? I couldn't find a way to pass an argument without clobbering the defaults set by Find. Also, there is the global $snippetErrors to get all the failing files rather than just the first one. Would be glad of suggestions on how to make this better.

- I tested by commenting out all but the verification in Execute, and all of Verify - otherwise $ausDeliveryDir is undefined when calling only Announce. I think we should fix this by moving the logic in Verify out to a new step, or by changing release to allow the likes of ./release -o Update -e -p -a

- We're only pushing the snippets to AUS, not making them live. I think we should do more testing, and possibly add more verification before adding this, and then only for the test channels.

Comment 17

[Robert Helmer [:rhelmer]]

(In reply to comment #16)
> Created an attachment (id=271883) [details]
> Push support for Update step - v2
> 
> Since the last patch:
> 
> - Fixes up TestAusCallback, which would have missed a release or beta snippet
> slipping into a subdir of aus2.test (by matching on that test instead of
> something in the channel name). Also restricts file names to partial and
> complete.txt, and check aus2.beta and aus2 too. Not partner friendly, is that a
> problem at this stage ?


No, I think we've moved away from doing any kind of custom MARs at this point, anyway.

 
> - I'm kinda torn over the callbacks, there are now 4 to handle different cases.
> We could collapse them by passing the regexp via a global, but which is the
> lesser evil ? I couldn't find a way to pass an argument without clobbering the
> defaults set by Find. Also, there is the global $snippetErrors to get all the
> failing files rather than just the first one. Would be glad of suggestions on
> how to make this better.


I think that's fine. On a generally note, I find the Find callback stuff to be a pain, not sure how it helps that it's a callback (usually you use a callback so you can go on with your work and be interrupted when it comes back, e.g. for event-based stuff).

That aside, I also don't know of any better way to do what you're trying to do here, and I think gathering all the errors is best.


> - I tested by commenting out all but the verification in Execute, and all of
> Verify - otherwise $ausDeliveryDir is undefined when calling only Announce. I
> think we should fix this by moving the logic in Verify out to a new step, or by
> changing release to allow the likes of ./release -o Update -e -p -a


Yeah that'd be fine.


> - We're only pushing the snippets to AUS, not making them live. I think we
> should do more testing, and possibly add more verification before adding this,
> and then only for the test channels.


I agree. I'd be happy if we got an email when this was done and had to do the staging->live push manually for now, as it's way better than how we've done this in the past!

Comment 18

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

Thanks for the reviews and comments, both v2 patches landed.