Closed Bug 691177 Opened 8 years ago Closed 7 years ago

Need a way to exclude jobs from TryChooser's "all"

Categories

(Release Engineering :: General, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philor, Assigned: sfink)

References

Details

(Keywords: sheriffing-P1, trychooser, Whiteboard: [capacity])

Attachments

(5 files, 3 obsolete files)

I've mentioned this several times before while someone was adding a new test to Try, but never really been heard or understood.

We run things on Try which are permafailing, for two reasons: someone is trying to get a new test greened up (debug Android crashtests and reftests are more or less in that state, pushing a point Jetpack is too), or someone has totally forgotten about something that they sort of wanted to look at some time in the past (Windows reftest-no-accel is in that state, since it never got the annotations that are all it needs to be green).

We currently have no mechanism to keep those things from being a constant annoyance, making developer after developer say "hey, does anyone know why my Try push seems to have broken a bunch of Android debug crashtests and reftests, when it only changes Windows-specific code?" "hey, does anyone know why my Try push seems to have broken a bunch of Windows reftests when it only touches Android-specific code?" "hey, does anyone know why my Try push seems to have broken Windows debug Jetpack when it only touches Mac-specific code?"

The end result (for things which are being greened up rather than just totally forgotten), is that by the time a test has been greened up and appears on other trees, it is already hated, and thought of as something that's always broken. Anyone who pushes to Try very much already knows that Android debug tests are permaorange, and when they show up on mozilla-inbound or mozilla-central, will be likely to completely ignore them whether or not they were green on the previous push.

Somehow, somewhere, we need to be able to say of a particular job that it should never run unless someone has actually intentionally checked a box which never defaults to being checked, even when an "All" checkbox is checked, or intentionally added a "-exp android-debug-crashtest windows-reftest-no-accel".
Whiteboard: [trychooser]
Valid bug (and concern).  Adding this to the many requests for improvements to the syntax.
Severity: normal → enhancement
Priority: -- → P5
Blocks: 693015
Blocks: 750285
Pardon me.  I was wondering if the following is a good syntax for this
enhancement?

try -b od -p all~macosx64,win32,linux-debug
(In reply to Edmund Wong (:ewong) from comment #2)
> Pardon me.  I was wondering if the following is a good syntax for this
> enhancement?
> 
> try -b od -p all~macosx64,win32,linux-debug

Forgot to clarify:

   "all~macosx64,win32,linux-debug"

means 

   all platforms except macosx64, win32 and linux-debug.
That might be a good different enhancement, but this bug is about allowing releng to say that, for instance, "-p all" does not include Android XUL, while still having "-p android-xul" (and perhaps "-p all,android-xul") produce it.
(In reply to Phil Ringnalda (:philor) from comment #4)
> That might be a good different enhancement, but this bug is about allowing
> releng to say that, for instance, "-p all" does not include Android XUL,
> while still having "-p android-xul" (and perhaps "-p all,android-xul")
> produce it.

Sorry for the misunderstanding.  Which platforms would be defaulted to 
being not included in "-p all"?
From the dependencies, looks like android-xul, macosx (the 10.5 flavor), and possibly debug android if anyone's interested in resurrecting it, though in general this is more likely to be useful for unittests and talos, to start making it possible to run one in development on Try before it's actually ready to give strange and terrible results to people who didn't specifically ask for it.
Keywords: trychooser
Whiteboard: [trychooser]
Keywords: sheriffing-P1
This is one way to do it. Requesting feedback.

I rewrote a chunk of try_parser, which I really should do as a separate refactoring patch to apply first. Let me know if you'd prefer that.

Platforms are marked independently for builds and tests. This implements just one possible way of marking them. For tests, in particular, it might make sense to in addition (or instead) mark the slave platforms individually, so you could eg disable OSX 10.6 and 10.7 tests by default, but keep the 10.8 ones.

We might also want to consider disabling linux32 by default.

Usage of this, since I didn't document it anywhere: -p all gives all default platforms. -p all,win64 would add in a hypothetically non-default win64 platform. -p win64 is unaffected. There's a supersekrit option for getting all known platforms.

This is currently missing tests. I want to get feedback on whether this is a viable approach first.
Assignee: nobody → sphink
Attachment #695026 - Flags: feedback?(bhearsum)
(In reply to Steve Fink [:sfink] from comment #7)
> Created attachment 695026 [details] [diff] [review]
> Allow marking build or test platforms as not being part of 'all' on try
> pushes
> 
> This is one way to do it. Requesting feedback.
> 
> I rewrote a chunk of try_parser, which I really should do as a separate
> refactoring patch to apply first. Let me know if you'd prefer that.

Looks simple enough, don't worry about it.

> Platforms are marked independently for builds and tests. This implements
> just one possible way of marking them. For tests, in particular, it might
> make sense to in addition (or instead) mark the slave platforms
> individually, so you could eg disable OSX 10.6 and 10.7 tests by default,
> but keep the 10.8 ones.

I think slave_platform is the right place for tests. I can easily see a preference for Win7-only on 10.8-only by default.

> We might also want to consider disabling linux32 by default.

Heh. Don't worry about that part. It will be taken care of in bug 822924.

> Usage of this, since I didn't document it anywhere: -p all gives all default
> platforms. -p all,win64 would add in a hypothetically non-default win64
> platform. -p win64 is unaffected.

> There's a supersekrit option for getting
> all known platforms.

I'm torn on whether or not we should have this. I'm worried about people simply s/all/full/g and this not improving things. Though, even if only 10% of the jobs used all instead of full, it would be better.....probably best to leave this ability in, for now.

> 
> This is currently missing tests. I want to get feedback on whether this is a
> viable approach first.

The approach seems fine. I have a few small things that will be in the next comment.
Comment on attachment 695026 [details] [diff] [review]
Allow marking build or test platforms as not being part of 'all' on try pushes

Review of attachment 695026 [details] [diff] [review]:
-----------------------------------------------------------------

::: misc.py
@@ +702,5 @@
>      for platform in enabled_platforms:
>          pf = config['platforms'][platform]
>          base_name = pf['base_name']
> +        try_status = ''
> +        if pf.get('default-off-on-try'):

This reads a bit like a double negative to me. Can you invert it to something like "try_by_default"? Also, this makes me realize that we can have branch-specific "-p all" behaviour, which is great!

@@ +704,5 @@
>          base_name = pf['base_name']
> +        try_status = ''
> +        if pf.get('default-off-on-try'):
> +            try_status = 'try-nondefault '
> +        pretty_name = PRETTY_NAME % (base_name, try_status)

Any particular reason you're putting this in the name? It might be confusing to have the builder name here different than the equivalent on other trees.

@@ +1910,5 @@
>              talos_pgo_builders = {}
>  
> +            try_status = ''
> +            if branch_config['platforms'][platform].get('default-off-on-try'):
> +                try_status = ' try-nondefault'

Leading space here -- intended?
Attachment #695026 - Flags: feedback?(bhearsum) → feedback+
(In reply to Ben Hearsum [:bhearsum] from comment #9)
> (In reply to Steve Fink [:sfink] from comment #7)
> > We might also want to consider disabling linux32 by default.
> 
> Heh. Don't worry about that part. It will be taken care of in bug 822924.

Those builds will be disabled in that bug, which presumably will remove them from try as well, but it would be nice to have them explicitly requestable for special cases like trying to check whether something is dependent on 32- vs 64-bit or whatever. Which would mean adding another possibility "off on production, default off on try, but selectable." I think that would mean turning them off normally for non-try, leaving them on but off by default for try.

> > Usage of this, since I didn't document it anywhere: -p all gives all default
> > platforms. -p all,win64 would add in a hypothetically non-default win64
> > platform. -p win64 is unaffected.
> 
> > There's a supersekrit option for getting
> > all known platforms.
> 
> I'm torn on whether or not we should have this. I'm worried about people
> simply s/all/full/g and this not improving things. Though, even if only 10%
> of the jobs used all instead of full, it would be better.....probably best
> to leave this ability in, for now.

I don't really see the concern, since the new meaning of 'all' is what people want anyway unless they happen to be messing around with tier > 1 architectures. So I don't see why you'd expect 90% of users to switch away from 'all'.

> @@ +702,5 @@
> >      for platform in enabled_platforms:
> >          pf = config['platforms'][platform]
> >          base_name = pf['base_name']
> > +        try_status = ''
> > +        if pf.get('default-off-on-try'):
> 
> This reads a bit like a double negative to me. Can you invert it to
> something like "try_by_default"? Also, this makes me realize that we can
> have branch-specific "-p all" behaviour, which is great!

It is a double-negative, which drives me crazy, but (1) I wanted a nonexistent key to mean False, not True; and (2) I was avoiding updating all existing config sections, especially since the common case would be to have active branches part of 'all' on try.

If you think that either of the above is not a problem, then 'try_by_default' is fine. Otherwise, I could switch to 'suppress_on_try', though it's really only suppressing something from the default set, not suppressing it entirely.

> @@ +704,5 @@
> >          base_name = pf['base_name']
> > +        try_status = ''
> > +        if pf.get('default-off-on-try'):
> > +            try_status = 'try-nondefault '
> > +        pretty_name = PRETTY_NAME % (base_name, try_status)
> 
> Any particular reason you're putting this in the name? It might be confusing
> to have the builder name here different than the equivalent on other trees.

Ack! You're right, this is changing the builder name, not just the trychooser-only prettyName. No, I did not intend to do that. I really do not want this to change any builder names.

> @@ +1910,5 @@
> >              talos_pgo_builders = {}
> >  
> > +            try_status = ''
> > +            if branch_config['platforms'][platform].get('default-off-on-try'):
> > +                try_status = ' try-nondefault'
> 
> Leading space here -- intended?

Yes. I want "Linux try leak test build try-nondefault", not "Linux try leak test buildtry-nondefault".

prettyNames *are* only used for trychooser, right? How would you feel about changing them to either a sequence of space-separated key=value pairs, or a full-on JSON string? Substring matching seems unnecessarily brittle.
(In reply to Steve Fink [:sfink] from comment #11)
> (In reply to Ben Hearsum [:bhearsum] from comment #9)
> > (In reply to Steve Fink [:sfink] from comment #7)
> > > We might also want to consider disabling linux32 by default.
> > 
> > Heh. Don't worry about that part. It will be taken care of in bug 822924.
> 
> Those builds will be disabled in that bug, which presumably will remove them
> from try as well, but it would be nice to have them explicitly requestable
> for special cases like trying to check whether something is dependent on 32-
> vs 64-bit or whatever. Which would mean adding another possibility "off on
> production, default off on try, but selectable." I think that would mean
> turning them off normally for non-try, leaving them on but off by default
> for try.

I'm not 100% sure what our plans are here, but I don't think you need to worry about it. That bug is taking care of 32-bit Linux, so we can deal with it there.

> > > Usage of this, since I didn't document it anywhere: -p all gives all default
> > > platforms. -p all,win64 would add in a hypothetically non-default win64
> > > platform. -p win64 is unaffected.
> > 
> > > There's a supersekrit option for getting
> > > all known platforms.
> > 
> > I'm torn on whether or not we should have this. I'm worried about people
> > simply s/all/full/g and this not improving things. Though, even if only 10%
> > of the jobs used all instead of full, it would be better.....probably best
> > to leave this ability in, for now.
> 
> I don't really see the concern, since the new meaning of 'all' is what
> people want anyway unless they happen to be messing around with tier > 1
> architectures. So I don't see why you'd expect 90% of users to switch away
> from 'all'.

I was musing that everyone may just stop using "all" and start using "full", which would result in a net-zero change in load. I can't imagine that would happen to that degree, so let's keep all and full for now and see what happens...

> > @@ +702,5 @@
> > >      for platform in enabled_platforms:
> > >          pf = config['platforms'][platform]
> > >          base_name = pf['base_name']
> > > +        try_status = ''
> > > +        if pf.get('default-off-on-try'):
> > 
> > This reads a bit like a double negative to me. Can you invert it to
> > something like "try_by_default"? Also, this makes me realize that we can
> > have branch-specific "-p all" behaviour, which is great!
> 
> It is a double-negative, which drives me crazy, but (1) I wanted a
> nonexistent key to mean False, not True; and (2) I was avoiding updating all
> existing config sections, especially since the common case would be to have
> active branches part of 'all' on try.

You can use pf.get('try-by-default', True), which returns "True" if try-by-default doesn't exist in pf. Then you only need to set try-by-default to False for the platforms we don't want in "all".

> > @@ +704,5 @@
> > >          base_name = pf['base_name']
> > > +        try_status = ''
> > > +        if pf.get('default-off-on-try'):
> > > +            try_status = 'try-nondefault '
> > > +        pretty_name = PRETTY_NAME % (base_name, try_status)
> > 
> > Any particular reason you're putting this in the name? It might be confusing
> > to have the builder name here different than the equivalent on other trees.
> 
> Ack! You're right, this is changing the builder name, not just the
> trychooser-only prettyName. No, I did not intend to do that. I really do not
> want this to change any builder names.
> 
> > @@ +1910,5 @@
> > >              talos_pgo_builders = {}
> > >  
> > > +            try_status = ''
> > > +            if branch_config['platforms'][platform].get('default-off-on-try'):
> > > +                try_status = ' try-nondefault'
> > 
> > Leading space here -- intended?
> 
> Yes. I want "Linux try leak test build try-nondefault", not "Linux try leak
> test buildtry-nondefault".
> 
> prettyNames *are* only used for trychooser, right?

Confusingly, this seems to be the case...I need to look deeper to verify, though.

> How would you feel about
> changing them to either a sequence of space-separated key=value pairs, or a
> full-on JSON string? Substring matching seems unnecessarily brittle.

Assuming the above is correct, this seems fine. I'm going to ask for a rename on the variable too though. I'll get back to you hopefully later today when I can verify that the only consumer is try chooser.
OK, I verified that PRETTY_NAME doesn't affect any builder/scheduler stuff, so f+ on that part too.
(In reply to Ben Hearsum [:bhearsum] from comment #12)
> (In reply to Steve Fink [:sfink] from comment #11)
> > (In reply to Ben Hearsum [:bhearsum] from comment #9)
> > > (In reply to Steve Fink [:sfink] from comment #7)
> > > > There's a supersekrit option for getting
> > > > all known platforms.
> > > 
> > > I'm torn on whether or not we should have this. I'm worried about people
> > > simply s/all/full/g and this not improving things. Though, even if only 10%
> > > of the jobs used all instead of full, it would be better.....probably best
> > > to leave this ability in, for now.
> > 
> > I don't really see the concern, since the new meaning of 'all' is what
> > people want anyway unless they happen to be messing around with tier > 1
> > architectures. So I don't see why you'd expect 90% of users to switch away
> > from 'all'.
> 
> I was musing that everyone may just stop using "all" and start using "full",
> which would result in a net-zero change in load. I can't imagine that would
> happen to that degree, so let's keep all and full for now and see what
> happens...

I understand what you were musing; I was challenging the psychology behind it. :-) But I agree, nothing more to discuss for now.

> > > @@ +702,5 @@
> > > >      for platform in enabled_platforms:
> > > >          pf = config['platforms'][platform]
> > > >          base_name = pf['base_name']
> > > > +        try_status = ''
> > > > +        if pf.get('default-off-on-try'):
> > > 
> > > This reads a bit like a double negative to me. Can you invert it to
> > > something like "try_by_default"? Also, this makes me realize that we can
> > > have branch-specific "-p all" behaviour, which is great!
> > 
> > It is a double-negative, which drives me crazy, but (1) I wanted a
> > nonexistent key to mean False, not True; and (2) I was avoiding updating all
> > existing config sections, especially since the common case would be to have
> > active branches part of 'all' on try.
> 
> You can use pf.get('try-by-default', True), which returns "True" if
> try-by-default doesn't exist in pf. Then you only need to set try-by-default
> to False for the platforms we don't want in "all".

Yeah, I know how to code it. I guess you're just saying that my #1 doesn't bother you. Ok, works for me.

> > How would you feel about
> > changing them to either a sequence of space-separated key=value pairs, or a
> > full-on JSON string? Substring matching seems unnecessarily brittle.
> 
> Assuming the above is correct, this seems fine. I'm going to ask for a
> rename on the variable too though. I'll get back to you hopefully later
> today when I can verify that the only consumer is try chooser.

I'll put a complete set of patches up for review so you can request it there.
Updated to 'try-by-default'
Attachment #696174 - Flags: review?(bhearsum)
Attachment #695026 - Attachment is obsolete: true
Although hardcoding is simpler, it makes it more difficult to update the tests. Plus, I may be changing the prettyName format dramatically soonish, and that would just be a pain with the current setup.
Attachment #696175 - Flags: review?(bhearsum)
These are pretty minimal. I guess I ought to do better.
Attachment #696178 - Flags: review?(bhearsum)
Updated to allow removing things from the default set in more places. In particular, this now allows disabling eg the OSX 10.6 (snowleopard) tests separately from the other macosx64 builds with something like this:

-PLATFORMS['macosx64']['snowleopard'] = {'name': "Rev4 MacOSX Snow Leopard 10.6"}
+PLATFORMS['macosx64']['snowleopard'] = {'name': "Rev4 MacOSX Snow Leopard 10.6", 'try-by-default': False}

It also makes my previously posted config change actually work. The config schema is complicated, man.
Attachment #696182 - Flags: review?(bhearsum)
Attachment #696174 - Attachment is obsolete: true
Attachment #696174 - Flags: review?(bhearsum)
(In reply to Steve Fink [:sfink] from comment #18)
> Created attachment 696182 [details] [diff] [review]
> Allow marking build or test platforms as not being part of 'all' on try
> pushes
> 
> Updated to allow removing things from the default set in more places. In
> particular, this now allows disabling eg the OSX 10.6 (snowleopard) tests
> separately from the other macosx64 builds with something like this:
> 
> -PLATFORMS['macosx64']['snowleopard'] = {'name': "Rev4 MacOSX Snow Leopard
> 10.6"}
> +PLATFORMS['macosx64']['snowleopard'] = {'name': "Rev4 MacOSX Snow Leopard
> 10.6", 'try-by-default': False}
> 
> It also makes my previously posted config change actually work. The config
> schema is complicated, man.

Yeah :(. This the worst file we have in our repositories - and that's saying something!
Attachment #696182 - Flags: review?(bhearsum) → review+
Attachment #696175 - Flags: review?(bhearsum) → review+
Attachment #696178 - Flags: review?(bhearsum) → review+
I freely admit this is gross. But it makes "try-nondefault" work for specific slave platforms. I realized partway through that the intended semantics are a little fuzzy. Normally, |-p all| would only give the default platforms, |-p win64| would give win64 whether or not it was a default, and this selection would carry over to tests. So the former would not run any win64 tests. But if we want to mark specific test slave platforms as nondefault, then it can't be governed by the -p flag. The -t flag, however, is requesting *tests*, not slave platforms, so there's no way to request a nondefault slave platform (eg if you made OSX 10.5 nondefault).

This patch borrows the filter notation I implemented earlier. So now |-p all -t all| will run all default tests on all default platforms. To get hypothetically nondefault OSX 10.5 tests, you could use |-p all -t all[10.5]| (ok, you probably wouldn't want those other platforms' builds, so it'd really be |-p macosx64 -t all[10.5]|). If you want everything, you could do |-p full -t all[-Waldo]| to run all tests that don't have "Waldo" in them, namely all of them.

Though really the simplest filter that would work is |-p full -t all[]|, which runs any test that includes the empty string (again, that's all of them.)

A little strange, maybe, but it seems somewhat sensible. And in the normal case, where you *don't* want anything that's nondefault, everything just works as expected.

It would probably be better to clean up the prettyName naming and semantics first, but this works and I need to switch away from tormenting buildbot for a while.
Attachment #696400 - Flags: review?(bhearsum)
Comment on attachment 696400 [details] [diff] [review]
Make the try-nondefault marker on slave platform names work

Review of attachment 696400 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is so bad from a usability standpoint, the syntax seems intuitive enough to me. Or maybe I'm just far enough down the rabbit hole?

::: try_parser.py
@@ +49,5 @@
>  
> +def basePlatform(platform):
> +    '''Platform name without any 'try-nondefault' markers, whether at the
> +    beginning or in the middle of the string'''
> +    return platform.replace(' try-nondefault', '').replace('try-nondefault ', '')

We've done this before in other places, and always regretted overloading "platform". I don't see any other place that this could be attached though, considering that the interface between the syntax and the schedulers is only builder names :(.
Attachment #696400 - Flags: review?(bhearsum) → review+
Attachment #696175 - Flags: checked-in+
Attachment #696178 - Flags: checked-in+
Attachment #696182 - Flags: checked-in+
Attachment #696400 - Flags: checked-in+
in production
Attachment #696175 - Flags: checked-in+ → checked-in-
Attachment #696178 - Flags: checked-in+ → checked-in-
Attachment #696182 - Flags: checked-in+ → checked-in-
Attachment #696400 - Flags: checked-in+ → checked-in-
This [we think] broke things yesterday, and has been backed out on default and production branches.
(In reply to Justin Wood (:Callek) from comment #24)
> This [we think] broke things yesterday, and has been backed out on default
> and production branches.

Sorry about that. Can you provide a little more detail? What was broken? What were the symptoms?
Inadequate details in bug 825717 where I foolishly didn't put links to affected pushes on tbpl and self-serve. I suspect what was broken was objdir naming - debug builds were burning not finding something in the objdir, talos was being run but no other tests were, and although self-serve knew builds were being run, none were showing on tbpl.
https://secure.pub.build.mozilla.org/buildapi/self-serve//mozilla-inbound/rev/b3993179ea52 though it's not very useful since things got retriggered post-backout, so new green logs overwrote the busted ones. https://tbpl.mozilla.org/php/getParsedLog.php?id=18378568&tree=Mozilla-Inbound from the push before may be one of the things that was busted, where package-tests was trying

make.py[0]: Entering directory 'e:\builds\moz2_slave\WINNT-6.1-x86-64-m-in-bld-w32-dbg\build\obj-firefox'
No makefile found

and failing since there's no WINNT-6.1-x86-64-m-in-bld-w32-dbg, only m-in-w32-dbg.
That seems like it could be related to this, though I'm not exactly sure how...
It also broke post_upload with things like "no directory /.../ffxbld/6.1" due to |-p WINNT 6.1 x86-64 linux64| and no I'm not joking, it did list linux64 in the same command as a WINNT thing. So part of the problem was quoting, other part related to a platform mixup.
Good:

http://buildbot-master12.build.scl1.mozilla.com:8001/builders/WINNT%205.2%20mozilla-inbound%20leak%20test%20build/builds/4759/steps/make_pkg_tests/logs/stdio

Bad:

http://buildbot-master12.build.scl1.mozilla.com:8001/builders/WINNT%205.2%20mozilla-inbound%20leak%20test%20build/builds/4764/steps/make_pkg_tests/logs/stdio

The only difference is the "in dir e:\builds\moz2_slave\WINNT-6.1-x86-64-m-in-bld-w32-dbg\build/obj-firefox" part. (It should be "e:\builds\moz2_slave\m-in-w32-dbg\build/obj-firefox").

This is in the make_pkg_tests step. If I dump out its configuration with and without the patches in this bug, it's the same. In particular, 'workdir' is just "build/obj-firefox" in both cases.

/me is puzzled

I haven't looked at the post_upload step Callek mentioned yet.
Comment on attachment 696182 [details] [diff] [review]
Allow marking build or test platforms as not being part of 'all' on try pushes

Review of attachment 696182 [details] [diff] [review]:
-----------------------------------------------------------------

So this patch, by itself caused most of the problems afaict:

Android jamun build NightlyBuildFactory
...
-    SendChangeStep {'branch': 'jamun-android-opt-unittest',
+    SendChangeStep {'branch': 'WINNT 6.1 x86-64 jamun build-android-opt-unittest',
..
- ... 'POST_UPLOAD_CMD': <WithProperties "post_upload.py --tinderbox-builds-dir jamun-android-debug -p mobile -i %(buildid)s --revision %(got_revision)s --release-to-tinderbox-dated-builds">, ...
+ ... 'POST_UPLOAD_CMD': <WithProperties "post_upload.py --tinderbox-builds-dir WINNT 6.1 x86-64 jamun build-android-debug -p mobile -i %(buildid)s --revision %(got_revision)s --release-to-tinderbox-dated-builds">, 

And then on the scheduler side, things like:

 <buildbotcustom.scheduler.Scheduler-props> {'builderNames': ['Android jaegermonkey build',
                   'Android Armv6 jaegermonkey build',
                   'Android Debug jaegermonkey build',
                   'Android X86 jaegermonkey build'],
  'change_filter': <ChangeFilter on branch == projects/jaegermonkey>,
  'fileIsImportant': <function isImportant>,
- 'name': 'jaegermonkey-mobile',
- 'properties': {'scheduler': 'jaegermonkey-mobile'},
+ 'name': 'WINNT 6.1 x86-64 jaegermonkey build-mobile',
+ 'properties': {'scheduler': 'WINNT 6.1 x86-64 jaegermonkey build-mobile'},
  'propfuncs': [<function buildIDSchedFunc>,
                <function buildUIDSchedFunc>],
  'treeStableTimer': None}
Attachment #696182 - Flags: review-
Thanks Callek, that made it easy to track down. The problem is a major name collision -- I split out 'pretty_name' into 'pretty_name' (used for trychooser) and 'name' (used for scheduler name, among other things.) 'name' was unfortunately already being used -- it is passed in as a parameter. So I totally messed up the naming of a bunch of things, in a way that didn't bother checkconfig.

This updated patch instead splits 'pretty_name' into 'pretty_name' and 'builder_id'. 'buildername' is already taken.

I dumped out the universal_master_sqlite configuration before and after this patch, and the update removes all discrepancies.
Attachment #697682 - Flags: review?(bhearsum)
Attachment #696182 - Attachment is obsolete: true
Comment on attachment 697682 [details] [diff] [review]
Allow marking build or test platforms as not being part of 'all' on try pushes.

I can attest that this patch (by itself) does not change any buildernames/scheduler names/etc. so should be safe if it lives up to naming/style muster of ben's review.
Attachment #697682 - Flags: feedback+
Comment on attachment 697682 [details] [diff] [review]
Allow marking build or test platforms as not being part of 'all' on try pushes.

Review of attachment 697682 [details] [diff] [review]:
-----------------------------------------------------------------

So, by adding try-by-default: False to the platform def in mozilla/config.py for linux[32] and win64 the *only* difference I see is in the Scheduler, specifically:

======================
 <buildbotcustom.scheduler.BuilderChooserScheduler-props> {'buildbotBranch': 'try',
  'builderNames': ['Android try build',
                   'Android Armv6 try build',
                   'Android Debug try build',
                   'Android no-ionmonkey try build',
                   'Android X86 try build',
                   'Linux try build',
                   'Linux try pgo-build',
                   'Linux try leak test build',
                   'Linux x86-64 try build',
                   'Linux x86-64 try pgo-build',
                   'Linux x86-64 try leak test build',
                   'OS X 10.7 try build',
                   'OS X 10.7 64-bit try leak test build',
                   'WINNT 5.2 try build',
                   'WINNT 5.2 try pgo-build',
                   'WINNT 5.2 try leak test build',
                   'WINNT 6.1 x86-64 try build',
                   'WINNT 6.1 x86-64 try pgo-build'],
  'change_filter': <ChangeFilter on branch == try>,
  'chooserFunc': <function tryChooser>,
  'fileIsImportant': <function <lambda>>,
  'name': 'try-firefox',
  'prettyNames': {'android': 'Android try build',
                  'android-armv6': 'Android Armv6 try build',
                  'android-debug': 'Android Debug try build',
                  'android-noion': 'Android no-ionmonkey try build',
                  'android-x86': 'Android X86 try build',
-                 'linux': 'Linux try build',
+                 'linux': 'Linux try try-nondefault build',
                  'linux-debug': 'Linux try leak test build',
                  'linux64': 'Linux x86-64 try build',
                  'linux64-debug': 'Linux x86-64 try leak test build',
                  'macosx64': 'OS X 10.7 try build',
                  'macosx64-debug': 'OS X 10.7 64-bit try leak test build',
                  'win32': 'WINNT 5.2 try build',
                  'win32-debug': 'WINNT 5.2 try leak test build',
-                 'win64': 'WINNT 6.1 x86-64 try build'},
+                 'win64': 'WINNT 6.1 x86-64 try try-nondefault build'},
  'properties': {'scheduler': 'try-firefox'},
  'propfuncs': [<function buildUIDSchedFunc>],
  'talosSuites': None,
  'treeStableTimer': None,
  'unittestPrettyNames': None,
  'unittestSuites': None}
===============================

This means there is indeed no change in rest of the builders, or in other schedulers.

::: misc.py
@@ +718,5 @@
>                  nightlyBuilders.append(buildername)
>              continue
>  
> +        values = { 'basename': base_name,
> +                   'trystatus': '' if pf.get('try-by-default', True) else 'try-nondefault ',

bikeshed-nit: given our other uses in buildbot-configs for platform keys, we use underscores instead of hyphens, I'd love to do that here (for the key; actual text for buildername is fine)
Blocks: 772458
Whiteboard: [capacity]
Comment on attachment 697682 [details] [diff] [review]
Allow marking build or test platforms as not being part of 'all' on try pushes.

Review of attachment 697682 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for running this through dump master, callek. I'm not too concerned about the hyphen vs. underscore.
Attachment #697682 - Flags: review?(bhearsum) → review+
Attachment #697682 - Flags: checked-in+
This is in production - Steve, can you do some tests to make sure it's working?
(In reply to Ben Hearsum [:bhearsum] from comment #37)
> This is in production - Steve, can you do some tests to make sure it's
> working?

The main test to see if it's doing any harm is just checking if any try jobs have stopped running. They shouldn't. I quickly eyeballed tbpl, and nothing seems clearly amiss.

But to test anything beyond that, we'll need to pick something to turn off by default -- either something that's currently on, or for which a configuration exists but is disabled. win64 is an obvious example, except I think the config may have been removed entirely (?). We could also turn off some of the OSX tests (10.6, 10.7, or 10.8). But all of these require a buildbot-configs change, review, landing, and reconfig, so they won't help in testing this immediately.

Let me know if there's something you can think of turning off. Ed Morley or philor might have opinions too.
Flags: needinfo?
NoIon seems like a good candidate for making non-default on Try imo
Flags: needinfo?
Any additions to the nondefault set should be done in separate bugs now. The ASan (Address Sanitizer) builds are currently using this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.