508403 - release automation should do directory cleanup on slave before starting release builds

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[[e-mail] (:bhearsum)]

Just like we have a clobberer for regular dep/leak test/unit tests builds, we should have one for the release builders, too. I suggest we use:
https://build.mozilla.org/clobberer/release and https://build.mozilla.org/stage-clobberer/release.

It should just be simple enough to set up the new clobberers, but we also need to get IT to adjust the permissions for these directories to only allow RelEng folk to use them.

Comment 1

[alice nodelman [:alice] [:anode]]

Futuring till appropriate bugs against IT are filed to let this move forward.

Comment 2

[[e-mail] (:bhearsum)]

I'm morphing this to be something more generic.

Comment 3

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

For example, in FF3.5.3 release, we had to do the following cleanup work manually. 

    *  Linux
          o /builds/slave/{tag,source,linux_build,linux_repack,updates,linux_update_verify,final_verification}/* 
    * Mac
          o /builds/slave/{macosx_build,macosx_repack,l10n_verification,macosx_update_verify}/* 
    * Win32
          o /e/builds/moz2_slave/{win32_build,win32_repack,win32_update_verify}/* 


To avoid logging into every slave in the pool, we do this cleanup manually on whatever slave was used for the *previous* release. This assumes that everyone doing a release has been good at doing that, else there is a theoretical risk release builds could still hit some old prev-prev-release files and fail out.

Adding this logic into release automation means:
- the cleanup would always happen on the slave which will be doing the release. This is good because it closes the theoretical risk.
- its one thing less to have to do manually during a release.

Comment 4

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

(In reply to comment #3)
> Adding this logic into release automation means:
> - the cleanup would always happen on the slave which will be doing the release.
> This is good because it closes the theoretical risk.
> - its one thing less to have to do manually during a release.

Tweaking summary to match.

Comment 5

[Chris AtLee [:catlee]]

So the idea here is to have a 2nd version of clobberer that gets run on *every* build (even non-release builds), and checks a web service for clobber instructions.

It would be nice if the slaves doing a release contacted this same web service somehow to indicate which release and task they were doing so clobbers could be more targeted.  This is to avoid the risk that somebody clobbers release Y while somebody else is working on release Z.

Comment 6

[Chris AtLee [:catlee]]

Ok, what I'm thinking of is to have a table on the web side that records:
- unique id
- username
- date
- master
- release (e.g. "Firefox 3.5.5")

On each build, the slaves will check a web service by sending its slave name, name of the release, and buildbot master name.

The server will respond with a "server clobber time".

The slave will then look for a file /builds/slave/last_release_clobber.txt that contains their last clobber time.  If this file doesn't exist, then default to use the "server clobber time" (i.e., skip the next step)

The slave will compare its "last clobber time" with the "server clobber time".  If the server's time is newer, the slave will delete all the release related directories.

The slave will then update its local clobber time to be the server's clobber time.

Comment 7

[Chris AtLee [:catlee]]

Attachment: Updates to how clobber is called

Add additional parameters to clobberer.py call, and handle new output format.

Comment 8

[Chris AtLee [:catlee]]

Attachment: Clobber tools updates

Updates index.php to allow special people to trigger clobbers on release builders.  The DB schema is changed, so we'll need to wipe out the old db file first.

Updates clobberer.py to send both the builder name and build directory, and handle clobbers for multiple build directories at once.

Adds a test harness to make sure everything works!

Comment 9

[Chris AtLee [:catlee]]

Attachment: Set clobberURL on release builders

When clobberURL is set, the slaves will run the clobber step.  We don't set clobberTime since we don't want periodic clobbers on release builds.

Comment 10

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

Comment on attachment 415880 [details] [diff] [review]
Updates to how clobber is called

>     def __init__(self, branch, clobber_url, clobberer_path, clobberTime=None,
>-                 timeout=3600, workdir='.', **kwargs):
>+            timeout=3600, workdir='..', **kwargs):

Intended whitespace change?

Comment 11

[Chris AtLee [:catlee]]

(In reply to comment #10)
> (From update of attachment 415880 [details] [diff] [review])
> >     def __init__(self, branch, clobber_url, clobberer_path, clobberTime=None,
> >-                 timeout=3600, workdir='.', **kwargs):
> >+            timeout=3600, workdir='..', **kwargs):
> 
> Intended whitespace change?

I guess vim did that when I changed the workdir...I restore the indentation in the patch

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 415881 [details] [diff] [review]
Clobber tools updates

rename 'time' in the builds table to 'last_build_time' and r=me

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 415880 [details] [diff] [review]
Updates to how clobber is called

>+    def setBuild(self, build):
>+        self.super_class.setBuild(self, build)
>+        # Set the "master" property
>+        master = build.builder.botmaster.parent.buildbotURL
>+        self.setProperty('master', master)

It would be nice if Buildbot set the buildbotURL as a property on every build. Diving up the chain like this is unfortunate, but it seems necessary here.

Comment 14

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

Comment on attachment 415881 [details] [diff] [review]
Clobber tools updates

>-          else:              
>+          else:

Whitespace?

>+  #print "Got"
>+  #print data

Safe to remove debugging code (or toggle based on -v)?

One thing I'd like to see in clobberer at some point is the ability to collapse/expand the rows in the branch-builder-slave table (with everything collapsed by default), but the drop-down list for the release builds is a step in the right direction.

Comment 15

[Chris AtLee [:catlee]]

(In reply to comment #14)
> (From update of attachment 415881 [details] [diff] [review])
> >-          else:              
> >+          else:
> 
> Whitespace?

Yeah, deleted some trailing whitespace

> >+  #print "Got"
> >+  #print data
> 
> Safe to remove debugging code (or toggle based on -v)?

I'll get rid of it.

> One thing I'd like to see in clobberer at some point is the ability to
> collapse/expand the rows in the branch-builder-slave table (with everything
> collapsed by default), but the drop-down list for the release builds is a step
> in the right direction.

Yeah, it's probably more common that you need to clobber a builder in general, rather than on just some slaves.

Comment 16

[Chris AtLee [:catlee]]

Attachment: follow-up to the above to make sure that release builds from other branches don't get clobbered

Comment 17

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

Comment on attachment 415881 [details] [diff] [review]
Clobber tools updates

>diff --git a/clobberer/clobberer.py b/clobberer/clobberer.py
> from datetime import datetime, timedelta

This may be removable.
 
>+  #root_dir = os.path.dirname(os.path.dirname(os.getcwd()))
>+  root_dir = os.getcwd()

Stray comment ?
 
>+  for builddir, (server_clobber_date, who) in server_clobber_dates.items():
>+    builder_dir = os.path.join(root_dir, builddir)
...
>+    os.chdir(builder_dir)
...
>+        do_clobber(options.dir, options.dryrun, options.skip)

where options.dir defaults to '.' (but could be different) seems strange to me. Could we pass the absolute directory builder_dir to do_clobber() for clarity ? Not sure what the use case for options.dir is really.

diff --git a/clobberer/index.php b/clobberer/index.php
>+$RELEASE_BUILDERS = array(
...
>+  'wince_build',

Thanks for fixing this omission. I'd add wince_repack too, it's coming soon.

>+                   .'buildername VARCHAR(50),'

"OS X 10.5.2 mozilla-central opt test mochitests-5/5" is 51 chars. Even though this clobbers on every build we probably should use a bigger field to capture the full builder name. 

I skipped the rest of the php.

>diff --git a/clobberer/test_clobberer.py b/clobberer/test_clobberer.py
>+    def testSetSlaveClobber(self):
...
>+        # Add a build on another builder, to make sure it's not getting clobbered
>+        updateBuild("branch1", "My Builder 2", "mybuilder2", "slave01", "master01")

This pattern appears in the first three tests but doesn't appear to actually get checked. Should it be removed, or the comment corrected ?

I wondered if it would be sensible to use unique builder names for each test to avoid "cross-talk" between the separate tests.

Comment 18

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

Comment on attachment 416151 [details] [diff] [review]
follow-up to the above to make sure that release builds from other branches don't get clobbered

>diff --git a/clobberer/test_clobberer.py b/clobberer/test_clobberer.py
>     def testSlaveClobberRelease(self):
...
>         setClobber('branch2','linux_build', None, None, now+10)

Why is the release clobbers set in the future ?
 
>+    def testSlaveClobberReleaseOtherBranchOtherBuilder(self):
>+        # Test that clobbers on release builders don't clobber builds from
>+        # other branches when run from another builder
...
>         data = runClobberer('branch1', 'My Builder', 'mybuilder', 'slave01', 'master01')
>         self.assert_('linux_build:Server is forcing' not in data, data)

Don't really follow this test. Are you saying that clobbers set on release builders should not be performed until we do that release build again ? Or that clobbers set on release builders shouldn't affect mybuilder ? If the latter then the assert is incorrect.
 
>+    def testSlaveClobberRelease4(self):

I would put this test first, it seems to be the simplest release test.

Comment 19

[Chris AtLee [:catlee]]

(In reply to comment #17)
> (From update of attachment 415881 [details] [diff] [review])
> >diff --git a/clobberer/clobberer.py b/clobberer/clobberer.py
> > from datetime import datetime, timedelta
> 
> This may be removable.

Removed.

> 
> >+  #root_dir = os.path.dirname(os.path.dirname(os.getcwd()))
> >+  root_dir = os.getcwd()
> 
> Stray comment ?

Replaced with root_dir = os.path.abspath(options.dir)

> >+  for builddir, (server_clobber_date, who) in server_clobber_dates.items():
> >+    builder_dir = os.path.join(root_dir, builddir)
> ...
> >+    os.chdir(builder_dir)
> ...
> >+        do_clobber(options.dir, options.dryrun, options.skip)
> 
> where options.dir defaults to '.' (but could be different) seems strange to me.
> Could we pass the absolute directory builder_dir to do_clobber() for clarity ?
> Not sure what the use case for options.dir is really.
> 
> diff --git a/clobberer/index.php b/clobberer/index.php
> >+$RELEASE_BUILDERS = array(
> ...
> >+  'wince_build',
> 
> Thanks for fixing this omission. I'd add wince_repack too, it's coming soon.
> 
> >+                   .'buildername VARCHAR(50),'
> 
> "OS X 10.5.2 mozilla-central opt test mochitests-5/5" is 51 chars. Even though
> this clobbers on every build we probably should use a bigger field to capture
> the full builder name. 

Fixed this, and made the builddir columns bigger.

> I skipped the rest of the php.
> 
> >diff --git a/clobberer/test_clobberer.py b/clobberer/test_clobberer.py
> >+    def testSetSlaveClobber(self):
> ...
> >+        # Add a build on another builder, to make sure it's not getting clobbered
> >+        updateBuild("branch1", "My Builder 2", "mybuilder2", "slave01", "master01")
> 
> This pattern appears in the first three tests but doesn't appear to actually
> get checked. Should it be removed, or the comment corrected ?

Added checks for these tests.

> I wondered if it would be sensible to use unique builder names for each test to
> avoid "cross-talk" between the separate tests.

The database gets wiped before each run in the setUp() method.  Is that what you mean?  Or more for clarity when reading the tests?


New patch coming with all of the requested changes.

Comment 20

[Chris AtLee [:catlee]]

(In reply to comment #18)
> (From update of attachment 416151 [details] [diff] [review])
> >diff --git a/clobberer/test_clobberer.py b/clobberer/test_clobberer.py
> >     def testSlaveClobberRelease(self):
> ...
> >         setClobber('branch2','linux_build', None, None, now+10)
> 
> Why is the release clobbers set in the future ?

Just to make sure they're after the build.  I've updated the tests so that builds happen in the past.

> 
> >+    def testSlaveClobberReleaseOtherBranchOtherBuilder(self):
> >+        # Test that clobbers on release builders don't clobber builds from
> >+        # other branches when run from another builder
> ...
> >         data = runClobberer('branch1', 'My Builder', 'mybuilder', 'slave01', 'master01')
> >         self.assert_('linux_build:Server is forcing' not in data, data)
> 
> Don't really follow this test. Are you saying that clobbers set on release
> builders should not be performed until we do that release build again ? Or that
> clobbers set on release builders shouldn't affect mybuilder ? If the latter
> then the assert is incorrect.

So, this is testing the branch-specific clobbers.  The slave does a build in linux_build on branch1 first, then branch2, then a clobber is set on branch1.  Because branch2 is the latest build in that directory, it shouldn't be clobbered.  This checks that doesn't happen when the slave is doing some other build.  testSlaveClobberReleaseNotOtherBranch tests that it doesn't get clobbered if the slave builds linux_build next.

However, in clarifying the test case for this, I came across some other problems which will be fixed in the followup patch.

> >+    def testSlaveClobberRelease4(self):
> 
> I would put this test first, it seems to be the simplest release test.

Sure.

Comment 21

[Chris AtLee [:catlee]]

Attachment: Clobber tools updates, v3

This patch is freshly made against build-tools.  It addresses your previous comments, and adds some additional tests.

Comment 22

[Chris AtLee [:catlee]]

Attachment: differences between previous patches and v3 patch

makes it easier to see what's changed from the last patch.

Comment 23

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

Comment on attachment 418170 [details] [diff] [review]
Clobber tools updates, v3

Improved testing looks great to me.

Comment 24

[Chris AtLee [:catlee]]

Comment on attachment 415880 [details] [diff] [review]
Updates to how clobber is called

changeset:   555:486bad773ea5

Comment 25

[Chris AtLee [:catlee]]

Comment on attachment 415883 [details] [diff] [review]
Set clobberURL on release builders

changeset:   1872:e078dd8f0802

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 418170 [details] [diff] [review]
Clobber tools updates, v3

changeset:   452:5e2d9cb03e90

Comment 27

[Chris AtLee [:catlee]]

Worked great on 3.6rc2!