553300 - Get release builders working with schedulerdb

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Chris AtLee [:catlee]]

Having multiple sets of release builders/schedulers for different branches won't work with schedulerdb, since the builders and schedulers for different branches all use the same name.

A quick fix is to have the scheduler and builder names include the branch name so we get uniqueness.

A better solution is to have the release builders be more data-driven...sort of related to bug 502876 and bug 505318.

Comment 1

[Aki Sasaki (not active)]

Taking to test out a hypothetical fix in my head (releaseBuilderSuffix)

Comment 2

[Aki Sasaki (not active)]

Attachment: releaseBuilder*Prefix* patch

Ok, went the releaseBuilderPrefix route so they sort together.
Changing the directories may involve some clobberer/purge_builds changes as well.

This checkconfigs ok; submitting for feedback before I go through a long test cycle.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 436589 [details] [diff] [review]
releaseBuilder*Prefix* patch

Most of the details here seem ok. A couple of things:
I'm going back and forth on whether 3.6.x makes sense or using the full, exact version. What do you guys think about that?

If we go with the former, I don't think it belongs in the configs -- it's easy enough to calculate in release_master.py.

Comment 4

[Aki Sasaki (not active)]

The reason I'd put it in the configs is so we can have things like Lorentz with special names.  Also to put mobile in the mobile name, if that's desired.  But not really feeling strongly either way.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> The reason I'd put it in the configs is so we can have things like Lorentz with
> special names.  Also to put mobile in the mobile name, if that's desired.  But
> not really feeling strongly either way.

Things like Lorentz also have special version numbers, so it's not necessary because of them IMHO. If names are preferred to version numbers, maybe we could use the last part of sourceRepoPath as the prefix.

As for Mobile, those builders are already prefixed with "mobile", so my suggestion of using 'version' would work without the possibility of mobile/desktop colliding.

Either way, I don't think there's any reason to introduce yet another variable here -- there's already at least 2 or 3 pieces of information in the configs that could be used for this.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 436589 [details] [diff] [review]
releaseBuilder*Prefix* patch

So, I'm marking this feedback- because I don't see any reason why we need to introduce yet another config variable. Any approach that creates the prefix based on existing data would be fine with me, in principle.

Comment 7

[Chris AtLee [:catlee]]

Comment on attachment 436589 [details] [diff] [review]
releaseBuilder*Prefix* patch

I like using the 'releaseBuilderPrefix', but I'd have it use some other variable to start with.

maybe releaseBuilderPrefix = 'release_%s' % sourceRepoName is a good starting point?

The release clobberer code will have to be updated as well.

Comment 9

[Syed Albiz [:salbiz]]

Attachment: [tested]buildbot-configs_release-080

Adding release_config skeletons and release portions of master.cfg to the mozilla bucket in buildbot-configs.

Comment 10

[Syed Albiz [:salbiz]]

Attachment: [tested]buildbotcustom-release-080

Necessary changes to make buildbot-080 branch work with release jobs. In order to make this work, the following couple of change sets from the default branch were rolled in: 889,907,910,940,941

Comment 11

[Syed Albiz [:salbiz]]

Attachment: [tested]upstream_buildbot_patch

Applied patch 6e717ac10e28059b0315 from upstream to fix issue with sourcestamp revision being dropped in buildbot-080

Comment 12

[Chris AtLee [:catlee]]

Comment on attachment 479479 [details] [diff] [review]
[tested]buildbotcustom-release-080

Looks good to me.  Needs some extra eyes though.

Comment 13

[Chris AtLee [:catlee]]

Comment on attachment 479473 [details] [diff] [review]
[tested]buildbot-configs_release-080

We need to get rid of the "if True:" clause.  I think putting some variable in master_localconfig.py which lets us turn the release stuff on/off per master would be the best way to go right now.

Comment 14

[Syed Albiz [:salbiz]]

Attachment: buildbot-configs_release-080

Something like this? Seems a bit clunky to put these in master_localconfig. Perhaps it might be more intuitive to put them in release_config, (but not in the releaseConfig dict), since 'staging' doesn't seem release-specific

Comment 15

[Chris AtLee [:catlee]]

Comment on attachment 479895 [details] [diff] [review]
buildbot-configs_release-080

Yeah, that's better...We can't put it in release_config, since we don't know if we _should_ be loading release_config yet.

Maybe just change the names to make it more clear that these are 'special' variables?

ENABLE_RELEASES, and ENABLE_STAGING_RELEASES (or maybe ENABLE_PRODUCTION_RELEASES instead?)

Initially all the localconfigs, except maybe the staging ones, should have releases disabled.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 479479 [details] [diff] [review]
[tested]buildbotcustom-release-080

Tagging is going to change really soon, when I land bug 508896, but I'll take care of porting that to 0.8.0.

Looks good though!

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 479895 [details] [diff] [review]
buildbot-configs_release-080

I see staging=True in a bunch of production localconfig's, definitely don't want that!

Just to make sure I understand correctly: the schedulers and change sources for a release will live on builder masters -- not scheduler masters, so that the release stays on a single master?

Why do the scheduler master localconfigs need to be touched at all? It doesn't look like the scheduler masters are going to be aware at all of releases.

Comment 18

[Syed Albiz [:salbiz]]

Attachment: [tested]buildbot-configs_release-080

Comment 19

[Syed Albiz [:salbiz]]

Attachment: [tested]buildbot-configs_release-080

(In reply to comment #17)
> Comment on attachment 479895 [details] [diff] [review]
> buildbot-configs_release-080
> 
> I see staging=True in a bunch of production localconfig's, definitely don't
> want that!
> 
yup, removed. ENABLE_RELEASES and ENABLE_STAGING_RELEASES which control that functionality default to false in all localconfigs now. I assume we don't want release builders to be on by default in staging?

> Just to make sure I understand correctly: the schedulers and change sources for
> a release will live on builder masters -- not scheduler masters, so that the
> release stays on a single master?
> 
> Why do the scheduler master localconfigs need to be touched at all? It doesn't
> look like the scheduler masters are going to be aware at all of releases.
I didn't look too closely at that before, since having those declarations in there doesn't do anything by themselves, hence can't break anything. You're right though, no need for it to be there if it isn't needed.

Comment 20

[[e-mail] (:bhearsum)]

(In reply to comment #19)
> Created attachment 480624 [details] [diff] [review]
> [tested]buildbot-configs_release-080
> 
> (In reply to comment #17)
> > Comment on attachment 479895 [details] [diff] [review] [details]
> > buildbot-configs_release-080
> > 
> > I see staging=True in a bunch of production localconfig's, definitely don't
> > want that!
> > 
> yup, removed. ENABLE_RELEASES and ENABLE_STAGING_RELEASES which control that
> functionality default to false in all localconfigs now. I assume we don't want
> release builders to be on by default in staging?

Actually, I think we do.

> > Just to make sure I understand correctly: the schedulers and change sources for
> > a release will live on builder masters -- not scheduler masters, so that the
> > release stays on a single master?
> > 
> > Why do the scheduler master localconfigs need to be touched at all? It doesn't
> > look like the scheduler masters are going to be aware at all of releases.
> I didn't look too closely at that before, since having those declarations in
> there doesn't do anything by themselves, hence can't break anything. You're
> right though, no need for it to be there if it isn't needed.

Alright, let's remove them, then.

Comment 21

[[e-mail] (:bhearsum)]

Comment on attachment 480624 [details] [diff] [review]
[tested]buildbot-configs_release-080

We definitely want releases enabled on all of the builder masters, here's what I think you should do:
- drop ENABLE_RELEASES altogether and always add the release stuff
- rename ENABLE_STAGING_RELEASES to 'STAGING'

Does that make sense?

Comment 22

[Syed Albiz [:salbiz]]

Attachment: [tested]buildbot-configs_release-080

Ok, replaced the ENABLE_STAGING_RELEASES with just STAGING, and added release-configs for production builders, and enabled production releases, and enabled staging releases on the staging masters.

I didn't drop ENABLE_RELEASES, since it needs to be off on try

Comment 23

[[e-mail] (:bhearsum)]

I'm going to try a staging run today.

Comment 24

[Syed Albiz [:salbiz]]

Attachment: fixing bitrot in buildbotcustom patch

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 481942 [details] [diff] [review]
[tested]buildbot-configs_release-080

Hmmm, I just noticed that the release configs aren't named properly. release_config_[spb]m0?.py should all be symlinks that point to the real configs, which should be named according to branch like in mozilla2/. Gotta switch this to r-.

Comment 26

[Syed Albiz [:salbiz]]

Attachment: reshuffle release_configs

Hmm, good point. I didn't notice the branch-specific configs before, since I those symlinks were not built in setup-master. I've added the per master symlinks to be tracked in hg as well,

Comment 27

[Syed Albiz [:salbiz]]

Attachment: disable releases on production amsters for now

Adding a version of the buildbot-configs patch which has releases disabled on production masters by default. Staging releases are still enabled on staging masters in this patch

Comment 28

[[e-mail] (:bhearsum)]

(Going to start dropping notes from my staging runs)

Can't hardcode "'slavenames': branchConfig['platforms']['macosx']['slaves']" for l10n verify anymore. Probably best to change it to macosx + macosx64

'updates' builder doesn't have release prefix

Comment 29

[[e-mail] (:bhearsum)]

(In reply to comment #28)
> 
> 'updates' builder doesn't have release prefix

Oops, this was my fault.

Comment 30

[[e-mail] (:bhearsum)]

Comment on attachment 483569 [details] [diff] [review]
reshuffle release_configs

Per IRC, we may flip off ENABLE_RELEASES in production for now.
The release configs are out of date, too, but I guess we can fix that at check-in time -- I don't think it's worth the effort trying to keep them up to date before that.

Comment 31

[[e-mail] (:bhearsum)]

Comment on attachment 483481 [details] [diff] [review]
fixing bitrot in buildbotcustom patch

Need to fix the slave lists for l10n verify and partner repacks -- I think both should be using macosx + macosx64 slaves.

Comment 32

[Syed Albiz [:salbiz]]

Attachment: extend slave lists to include 64-bit slaves for l10n-verify and partner-repack

Fixed l10n_verify and partner-repacks now get slavenames for macosx + macosx64.

Comment 33

[Chris AtLee [:catlee]]

Comment on attachment 483569 [details] [diff] [review]
reshuffle release_configs

Yeah, looks good as long as ENABLE_RELEASES are off in production for now.

Comment 34

[[e-mail] (:bhearsum)]

Comment on attachment 484498 [details] [diff] [review]
extend slave lists to include 64-bit slaves for l10n-verify and partner-repack

changeset:   1088:1501598455c8

toot toot, here comes the 0.8.0 release train!

Comment 35

[[e-mail] (:bhearsum)]

Comment on attachment 483968 [details] [diff] [review]
disable releases on production amsters for now

changeset:   3198:420ddf8f7adc

Comment 36

[Syed Albiz [:salbiz]]

Attachment: use reserved slaves for release builds

Switch to using reserved slaves for release-builds, reading the number of slaves to reserve from `reserved_slaves_${master_name}`.

Comment 37

[Syed Albiz [:salbiz]]

Attachment: set up reservedSlaves files for release masters

Comment 38

[Syed Albiz [:salbiz]]

Attachment: set up reservedSlaves files for release masters

Moving watchReservedFile() call into the config file, based on IRC conversation with catlee.

Comment 39

[Syed Albiz [:salbiz]]

Attachment: use reserved slaves for release builds

Comment 40

[[e-mail] (:bhearsum)]

Comment on attachment 485797 [details] [diff] [review]
set up reservedSlaves files for release masters

I think you need to protect the watchReservedFile call behind an 'if RESERVED_SLAVES:'. Looks fine to me otherwise.

Comment 41

[Syed Albiz [:salbiz]]

Attachment: guard watchReservedFile with RESERVED_SLAVE

Good point. I was letting this slide since _readReservedFile just returns 0 if the name doesn't exist, but it's a waste of time to call it every minute if the filename isn't even set.

Comment 42

[Chris AtLee [:catlee]]

I think watchReservedFile may need some work to be reconfig-safe (or testing at least)

Comment 43

[Syed Albiz [:salbiz]]

Attachment: make readReservedSlaves reconfig-safe

The global LoopingCall was getting overwritten on reconfig, but reactor still had a reference to it and kept it running. This version of the patch forgoes a looping call and opportunistically reads the file when _nextReservedSlaves is called if the last access time is > 60 seconds ago.

Comment 44

[Syed Albiz [:salbiz]]

Attachment: use reconfig-safe method of polling reserved-slaves file

Comment 45

[Chris AtLee [:catlee]]

Comment on attachment 486184 [details] [diff] [review]
make readReservedSlaves reconfig-safe

What are all the changes on ftppoller from?

Comment 46

[Chris AtLee [:catlee]]

Comment on attachment 486185 [details] [diff] [review]
use reconfig-safe method of polling reserved-slaves file

unrelated changes in this patch too?

Comment 47

[Syed Albiz [:salbiz]]

Attachment: un-mangle configs patch

sorry for the error, previous patch got mangled with other work

Comment 48

[Syed Albiz [:salbiz]]

Attachment: un-mangle buildbotcustom patch to add reserved slaves

sorry, accidentally mangled this patch with other work

Comment 49

[Chris AtLee [:catlee]]

Comment on attachment 486347 [details] [diff] [review]
un-mangle buildbotcustom patch to add reserved slaves

I think we need some code to handle the case here _reservedFileName is None.  Inside _readReservedFile should work.

Comment 50

[[e-mail] (:bhearsum)]

Comment on attachment 486347 [details] [diff] [review]
un-mangle buildbotcustom patch to add reserved slaves

Waiting on new patch, removing review.

Comment 51

[Syed Albiz [:salbiz]]

Attachment: guard _readReservedFile for case where a Nonetype is passed

Right, I mistakenly assumed the os.path.exists() call would catch that case, and that the guard to set RESERVED_SLAVES would otherwise.

Comment 52

[Syed Albiz [:salbiz]]

landing the reserved-slaves patch should only require a reconfig.

Comment 53

[[e-mail] (:bhearsum)]

Comment on attachment 486478 [details] [diff] [review]
guard _readReservedFile for case where a Nonetype is passed

changeset:   1197:996956fdbe83

Comment 54

[[e-mail] (:bhearsum)]

Comment on attachment 486344 [details] [diff] [review]
un-mangle configs patch

changeset:   3333:2bc23626e94e

Comment 55

[[e-mail] (:bhearsum)]

All done here. Turning on in production will be tracked elsewhere.