Add a new buildbot-tool generator for master configs

Add a new buildbot-tool generator for master configs.

We eventually want to get to a point where we have a
single stock config for buildbot masters (and slaves,
and recipes) where as much as possible is configured
from a single data file plus client-side recipes. This
would allow us to change master configs by simply 
updating scripts and either noting that the data file
changed (and having buildbot reconfigure itself
automatically), or by typing 'make reconfig' to force
it to do so without aborting any existing state.

This tool is perhaps the first step on that path: it 
uses a single (json) data file and a set of templates,
and adds a generator utility called 'buildbot-tool'
that can generate a static, checked-in master config.

None of the 'make reconfig' stuff works yet, and we
only support one very simple kind of waterfall
that uses a single Gitiles poller.

R=iannucci@chromium.org, luqui@chromium.org
BUG=356974
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=293339

[Dirk Pranke, 2014-12-06 02:08:08]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2014-12-06 02:08:15]

Patchset #1 (id:20001) has been deleted

[Dirk Pranke, 2014-12-06 02:08:23]

Patchset #1 (id:40001) has been deleted

[Dirk Pranke, 2014-12-06 02:11:40]

Patchset #2 (id:80001) has been deleted

[Dirk Pranke, 2014-12-06 02:30:30]

dpranke@chromium.org changed reviewers:
+ iannucci@chromium.org, luqui@chromium.org

[Dirk Pranke, 2014-12-06 02:30:31]

Okay, here's a first pass at the 'buildbot-tool' generator and a set of
templates that, combined with a builders.json file, can generate a valid
buildbot master config.

You can see a generated config in

https://codereview.chromium.org/782043003

I've intentionally made this first pass as basic as possible:

- The generated config matches what we do today
- I do simple python format string expansion, rather than
  something fancier like Jinja
- We support one kind of buildbot master (a waterfall bot
  triggered off of a single git repo)

Obviously we'd want to extend this to support tryservers that can be triggered
from Rietveld, and perhaps other more complicated kinds of schedulers, and we'd
want to move much of the boilerplate logic into testable python code (probably
moving much of the logic out of buildbot-tool as well).

This patch does not address how one would automatically find ports to use for
the master (from mastermap), or automatically update tests/master_cfg_test.py,
or update gatekeeper-ng. It's not yet clear to me how much of those things can
be automated.

This patch also does not add any integration into annotated_run.py; I'd add that
in a follow-on patch.

This patch also needs unit tests, but apart from adding the Filesystem
abstraction to make that easy, I haven't written them yet; I will do so before
landing anything, if we agree that something like this should land.

Lastly, I think the 'buildbot-tool' wrapper concept will be useful for other
things. At the very least I'd like to extend it in separate patches to add
support for:

  % buildbot-tool run-local-test-master chromium.mojo
  % buildbot-tool run-local-test-slave 'Chromium Mojo Linux'

or some such thing to hide the voodoo you need to do with TESTING_LOCAL_HOST,
etc.

(ideally we can also add support for a local rietveld instance).

Thoughts?

[iannucci, 2014-12-08 10:20:52]

On 2014/12/06 02:30:31, Dirk Pranke wrote:
> Okay, here's a first pass at the 'buildbot-tool' generator and a set of
> templates that, combined with a builders.json file, can generate a valid
> buildbot master config.
> 
> You can see a generated config in
> 
> https://codereview.chromium.org/782043003
> 
> I've intentionally made this first pass as basic as possible:
> 
> - The generated config matches what we do today
> - I do simple python format string expansion, rather than
>   something fancier like Jinja
> - We support one kind of buildbot master (a waterfall bot
>   triggered off of a single git repo)
> 
> Obviously we'd want to extend this to support tryservers that can be triggered
> from Rietveld, and perhaps other more complicated kinds of schedulers, and
we'd
> want to move much of the boilerplate logic into testable python code (probably
> moving much of the logic out of buildbot-tool as well).

yep

> 
> This patch does not address how one would automatically find ports to use for
> the master (from mastermap), or automatically update tests/master_cfg_test.py,
> or update gatekeeper-ng. It's not yet clear to me how much of those things can
> be automated.
> 
> This patch also does not add any integration into annotated_run.py; I'd add
that
> in a follow-on patch.
> 
> This patch also needs unit tests, but apart from adding the Filesystem
> abstraction to make that easy, I haven't written them yet; I will do so before
> landing anything, if we agree that something like this should land.

I think something like this should definitely land

> 
> Lastly, I think the 'buildbot-tool' wrapper concept will be useful for other
> things. At the very least I'd like to extend it in separate patches to add
> support for:
> 
>   % buildbot-tool run-local-test-master chromium.mojo
>   % buildbot-tool run-local-test-slave 'Chromium Mojo Linux'
> 
> or some such thing to hide the voodoo you need to do with TESTING_LOCAL_HOST,
> etc.
> 
> (ideally we can also add support for a local rietveld instance).
> 

Woohoo!

> Thoughts?

So presumably this tool would be changed so that once the code to dynamically
load the content of builders.json exists, both master.cfg and slaves.cfg would
just have a single function call `c = readTheJSONandGetTheMasterStuff()` and
`slaves = readTheJSONandGetTheSlavesStuff()` (for backwards-compatibility with
things that read slaves.cfg)?

I would also suggest that the new-master flow go like:

mkdir masters/master.cool
edit masters/master.cool/builders.json
./buildbot-tool gen master.cool

And then the update flow would go like:

edit masters/master.cool/builders.json
./buildbot-tool gen master.cool

Instead of having the path-to-json be a parameter.  We could also maybe have a
mode `gen --dry` which just returns 1 if the files don't match their json
(basically, re-generates them and does a diff). We could then add that to
PRESUBMIT so that we don't end up getting drift.

[iannucci, 2014-12-08 10:23:46]

On 2014/12/08 10:20:52, iannucci wrote:
> On 2014/12/06 02:30:31, Dirk Pranke wrote:
> > Okay, here's a first pass at the 'buildbot-tool' generator and a set of
> > templates that, combined with a builders.json file, can generate a valid
> > buildbot master config.
> > 
> > You can see a generated config in
> > 
> > https://codereview.chromium.org/782043003
> > 
> > I've intentionally made this first pass as basic as possible:
> > 
> > - The generated config matches what we do today
> > - I do simple python format string expansion, rather than
> >   something fancier like Jinja
> > - We support one kind of buildbot master (a waterfall bot
> >   triggered off of a single git repo)
> > 
> > Obviously we'd want to extend this to support tryservers that can be
triggered
> > from Rietveld, and perhaps other more complicated kinds of schedulers, and
> we'd
> > want to move much of the boilerplate logic into testable python code
(probably
> > moving much of the logic out of buildbot-tool as well).
> 
> yep
> 
> > 
> > This patch does not address how one would automatically find ports to use
for
> > the master (from mastermap), or automatically update
tests/master_cfg_test.py,
> > or update gatekeeper-ng. It's not yet clear to me how much of those things
can
> > be automated.
> > 
> > This patch also does not add any integration into annotated_run.py; I'd add
> that
> > in a follow-on patch.
> > 
> > This patch also needs unit tests, but apart from adding the Filesystem
> > abstraction to make that easy, I haven't written them yet; I will do so
before
> > landing anything, if we agree that something like this should land.
> 
> I think something like this should definitely land
> 
> > 
> > Lastly, I think the 'buildbot-tool' wrapper concept will be useful for other
> > things. At the very least I'd like to extend it in separate patches to add
> > support for:
> > 
> >   % buildbot-tool run-local-test-master chromium.mojo
> >   % buildbot-tool run-local-test-slave 'Chromium Mojo Linux'
> > 
> > or some such thing to hide the voodoo you need to do with
TESTING_LOCAL_HOST,
> > etc.
> > 
> > (ideally we can also add support for a local rietveld instance).
> > 
> 
> Woohoo!
> 
> > Thoughts?
> 
> So presumably this tool would be changed so that once the code to dynamically
> load the content of builders.json exists, both master.cfg and slaves.cfg would
> just have a single function call `c = readTheJSONandGetTheMasterStuff()` and
> `slaves = readTheJSONandGetTheSlavesStuff()` (for backwards-compatibility with
> things that read slaves.cfg)?
> 
> I would also suggest that the new-master flow go like:
> 
> mkdir masters/master.cool
> edit masters/master.cool/builders.json
> ./buildbot-tool gen master.cool
> 
> And then the update flow would go like:
> 
> edit masters/master.cool/builders.json
> ./buildbot-tool gen master.cool
> 
> Instead of having the path-to-json be a parameter.  We could also maybe have a
> mode `gen --dry` which just returns 1 if the files don't match their json
> (basically, re-generates them and does a diff). We could then add that to
> PRESUBMIT so that we don't end up getting drift.

Actually if we did that PRESUBMIT thing, we could get rid of masters_test for
these auto-generated masters. The only thing that test does (besides flaking and
taking a long time to pass) is verify that the master /could/ start, which we
can now statically assert by having the generated master code be correct (so
maybe we can have a masters_test for /one/ of the generated masters).

[Dirk Pranke, 2014-12-08 18:49:38]

On 2014/12/08 10:23:46, iannucci wrote:
> On 2014/12/08 10:20:52, iannucci wrote:
> > On 2014/12/06 02:30:31, Dirk Pranke wrote:
> > > Thoughts?
> > 
> > So presumably this tool would be changed so that once the code to
dynamically
> > load the content of builders.json exists, both master.cfg and slaves.cfg
would
> > just have a single function call `c = readTheJSONandGetTheMasterStuff()` and
> > `slaves = readTheJSONandGetTheSlavesStuff()` (for backwards-compatibility
with
> > things that read slaves.cfg)?

Yes, exactly.

> > 
> > I would also suggest that the new-master flow go like:
> > 
> > mkdir masters/master.cool
> > edit masters/master.cool/builders.json
> > ./buildbot-tool gen master.cool
> > 
> > And then the update flow would go like:
> > 
> > edit masters/master.cool/builders.json
> > ./buildbot-tool gen master.cool
> > 
> > Instead of having the path-to-json be a parameter.

Yeah, that's probably the right thing to do. I've been finding that path-to-json

felt a little silly and path-to-directory-containing-json probably makes more
sense (and is more consistent w/ GN, I suppose, not that that matters much).

> >   We could also maybe have a
> > mode `gen --dry` which just returns 1 if the files don't match their json
> > (basically, re-generates them and does a diff). We could then add that to
> > PRESUBMIT so that we don't end up getting drift.

Yeah.

> 
> Actually if we did that PRESUBMIT thing, we could get rid of masters_test for
> these auto-generated masters. The only thing that test does (besides flaking
and
> taking a long time to pass) is verify that the master /could/ start, which we
> can now statically assert by having the generated master code be correct (so
> maybe we can have a masters_test for /one/ of the generated masters).

You'd probably want some sort of presubmit that checked the JSON file for
validity if you did that.

-- Dirk

[iannucci, 2014-12-08 19:11:15]

On 2014/12/08 18:49:38, Dirk Pranke wrote:
> On 2014/12/08 10:23:46, iannucci wrote:
> > On 2014/12/08 10:20:52, iannucci wrote:
> > > On 2014/12/06 02:30:31, Dirk Pranke wrote:
> > > > Thoughts?
> > > 
> > > So presumably this tool would be changed so that once the code to
> dynamically
> > > load the content of builders.json exists, both master.cfg and slaves.cfg
> would
> > > just have a single function call `c = readTheJSONandGetTheMasterStuff()`
and
> > > `slaves = readTheJSONandGetTheSlavesStuff()` (for backwards-compatibility
> with
> > > things that read slaves.cfg)?
> 
> Yes, exactly.
> 
> > > 
> > > I would also suggest that the new-master flow go like:
> > > 
> > > mkdir masters/master.cool
> > > edit masters/master.cool/builders.json
> > > ./buildbot-tool gen master.cool
> > > 
> > > And then the update flow would go like:
> > > 
> > > edit masters/master.cool/builders.json
> > > ./buildbot-tool gen master.cool
> > > 
> > > Instead of having the path-to-json be a parameter.
> 
> Yeah, that's probably the right thing to do. I've been finding that
path-to-json
> 
> felt a little silly and path-to-directory-containing-json probably makes more
> sense (and is more consistent w/ GN, I suppose, not that that matters much).
> 
> > >   We could also maybe have a
> > > mode `gen --dry` which just returns 1 if the files don't match their json
> > > (basically, re-generates them and does a diff). We could then add that to
> > > PRESUBMIT so that we don't end up getting drift.
> 
> Yeah.
> 
> > 
> > Actually if we did that PRESUBMIT thing, we could get rid of masters_test
for
> > these auto-generated masters. The only thing that test does (besides flaking
> and
> > taking a long time to pass) is verify that the master /could/ start, which
we
> > can now statically assert by having the generated master code be correct (so
> > maybe we can have a masters_test for /one/ of the generated masters).
> 
> You'd probably want some sort of presubmit that checked the JSON file for
> validity if you did that.
> 

Yes, especially when we generate code that just reads the json file at runtime.

> -- Dirk

[agable, 2014-12-09 21:13:23]

agable@chromium.org changed reviewers:
+ agable@chromium.org

[agable, 2014-12-09 21:13:24]

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/common/f...
File scripts/common/filesystem.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/common/f...
scripts/common/filesystem.py:8: class Filesystem(object):
Seems like this should be in a tests/ directory somewhere. Maybe there's not any
other good place for it =/

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
File scripts/tools/buildbot_tool.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
scripts/tools/buildbot_tool.py:58: help='path to builders.json file')
Yeah, path-to-master-directory makes more sense to me.

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
File scripts/tools/buildbot_tool_templates/master_source_cfg.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
scripts/tools/buildbot_tool_templates/master_source_cfg.py:5: # This file is
used by scripts/tools/buildbot-tool to generate master configs.
I'm not convinced of the need for a master_source_config.py. I think it makes it
harder to see what's going on all in one place. I'd just put it directly in
master.cfg.

[Dirk Pranke, 2014-12-09 21:32:01]

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/common/f...
File scripts/common/filesystem.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/common/f...
scripts/common/filesystem.py:8: class Filesystem(object):
On 2014/12/09 21:13:24, agable wrote:
> Seems like this should be in a tests/ directory somewhere. Maybe there's not
any
> other good place for it =/

It's used by buildbot-tool directly, not just by test code, so I wouldn't be
inclined to put it in a test directory. Besides, a test (fake) implementation
will actually live in a separate file anyway.

We could put this in scripts/tools, next to buildbot-tool, but I'm not sure if
that's any better.

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
File scripts/tools/buildbot_tool.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
scripts/tools/buildbot_tool.py:58: help='path to builders.json file')
On 2014/12/09 21:13:24, agable wrote:
> Yeah, path-to-master-directory makes more sense to me.

Acknowledged.

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
File scripts/tools/buildbot_tool_templates/master_source_cfg.py (right):

https://chromiumcodereview.appspot.com/761253003/diff/100001/scripts/tools/bu...
scripts/tools/buildbot_tool_templates/master_source_cfg.py:5: # This file is
used by scripts/tools/buildbot-tool to generate master configs.
On 2014/12/09 21:13:24, agable wrote:
> I'm not convinced of the need for a master_source_config.py. I think it makes
it
> harder to see what's going on all in one place. I'd just put it directly in
> master.cfg.

OK. I tend to agree, but I wasn't sure if it would be more or less confusing to
hack on the templates too much here.

[Dirk Pranke, 2014-12-10 01:21:20]

Okay, I believe I've addressed all of the review feedback, and I've added unit
tests.

Please take another look?

[luqui, 2014-12-10 07:46:58]

People seem to be fairly heavily in favor of painting the bikeshed python ast
instead of json, so let's go with that.  Otherwise lgtm.  Some random comments
which I don't expect to be addressed in this CL:

As iannucci@ said, we should move this into a proper module instead of
generating boilerplate.  But this is a good first step, and will at least give a
standard new master template so there's no new divergence to worry about.  
There should also be a lot more validation of the json, er, pyast, as mentioned
in the doc.

[Dirk Pranke, 2014-12-10 17:55:00]

On 2014/12/10 07:46:58, luqui wrote:
> People seem to be fairly heavily in favor of painting the bikeshed python ast
> instead of json, so let's go with that.

I think python ast is probably a better solution as well, so I'm fine w/ that.

We should still be able to do automated refactoring just fine using 2to3.

(even in this patch I found the lack of support for trailing commas annoying,
and
using strings as comments is at best awkward).

>  Otherwise lgtm.  Some random comments
> which I don't expect to be addressed in this CL:
> 
> As iannucci@ said, we should move this into a proper module instead of
> generating boilerplate.  But this is a good first step, and will at least give
a
> standard new master template so there's no new divergence to worry about.  
> There should also be a lot more validation of the json, er, pyast, as
mentioned
> in the doc.

Right. I will add such things in followup CLs.

[Dirk Pranke, 2014-12-10 21:31:43]

I think this patch is good to land. Please take a look?

I've updated the chromium.mojo CL to match this code, for reference:

https://codereview.chromium.org/782043003/

I had to tell the PRESUBMIT to skip pylint'ing builders.py because otherwise it
complains about a useless expression. This seems unfortunate, but suppressing
that warning globally would be worse. Perhaps we should have a separate,
dedicated  lint check just for masters/*/builders.py once there are more of
them.

[chromium-reviews, 2014-12-10 21:54:01]

Having a dedicated lint check for builders.py sounds right.

This patch lgtm.

On Wed Dec 10 2014 at 1:31:44 PM <dpranke@chromium.org> wrote:

> I think this patch is good to land. Please take a look?
>
> I've updated the chromium.mojo CL to match this code, for reference:
>
> https://codereview.chromium.org/782043003/
>
> I had to tell the PRESUBMIT to skip pylint'ing builders.py because
> otherwise it
> complains about a useless expression. This seems unfortunate, but
> suppressing
> that warning globally would be worse. Perhaps we should have a separate,
> dedicated  lint check just for masters/*/builders.py once there are more of
> them.
>
> https://codereview.chromium.org/761253003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2014-12-10 21:59:54]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-12-10 22:00:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/761253003/200001

[commit-bot: I haz the power, 2014-12-10 22:00:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-10 22:00:58]

Failed to apply patch for
build/scripts/tools/buildbot_tool_templates/master.cfg:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         build/scripts/tools/buildbot_tool_templates
  Created missing directory build/scripts/tools/buildbot_tool_templates.
  A         build/scripts/tools/buildbot_tool_templates/master.cfg
  Copied build/masters/master.client.mojo/master.cfg ->
build/scripts/tools/buildbot_tool_templates/master.cfg
  patching file build/scripts/tools/buildbot_tool_templates/master.cfg
  Hunk #3 FAILED at 24.
  1 out of 4 hunks FAILED -- saving rejects to file
build/scripts/tools/buildbot_tool_templates/master.cfg.rej

Patch:   NR 
build/masters/master.client.mojo/master.cfg->build/scripts/tools/buildbot_tool_templates/master.cfg
Index: scripts/tools/buildbot_tool_templates/master.cfg
diff --git build/masters/master.client.mojo/master.cfg
build/scripts/tools/buildbot_tool_templates/master.cfg
similarity index 77%
copy from build/masters/master.client.mojo/master.cfg
copy to build/scripts/tools/buildbot_tool_templates/master.cfg
index
f6913c94c93af5d4d7a03dcf9277d04383703d91..de1fd56a6e48964c867b48a9924903f69ae90680
100644
--- a/build/masters/master.client.mojo/master.cfg
+++ b/build/scripts/tools/buildbot_tool_templates/master.cfg
@@ -2,7 +2,10 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+# This file is used by scripts/tools/buildbot-tool to generate master configs.
+
 # These modules come from scripts/master, which must be in the PYTHONPATH.
+from master import gitiles_poller
 from master import master_utils
 from master import slaves_list
 from master.factory import annotator_factory
@@ -11,7 +14,7 @@ from buildbot.status.mail import MailNotifier
 
 import config
 import master_site_config
-ActiveMaster = master_site_config.Mojo
+ActiveMaster = master_site_config.%(master_classname)s
 
 m_annotator = annotator_factory.AnnotatorFactory()
 
@@ -21,34 +24,18 @@ c['schedulers'] = []
 c['builders'] = []
 c['status'] = []
 
-builders = ['Mojo Linux',
-            'Mojo Linux (dbg)',
-            'Mojo Android Builder',
-            'Mojo Android Builder (dbg)',
-            'Mojo Android Builder Tests (dbg)',
-            'Mojo ChromeOS Builder',
-            'Mojo ChromeOS Builder (dbg)',
-            'Mojo Win (dbg)',
-            'Mojo Linux Perf']
-
-for builder_name in builders:
-  c['builders'].append({
-    'name': builder_name,
-    'factory': m_annotator.BaseFactory('mojo'),
-    'slavebuilddir': 'mojo',
-  })
+%(builders_block)s
 
 config.DatabaseSetup(c, require_dbconfig=ActiveMaster.is_production_host)
 
-import master_source_cfg
-
-master_source_cfg.Update(config, ActiveMaster, c)
+c['change_source'].append(gitiles_poller.GitilesPoller(
+    '%(git_repo_url)s'))
 
 c['schedulers'].extend([
     SingleBranchScheduler(name='source',
                           branch='master',
                           treeStableTimer=60,
-                          builderNames=builders)
+                          builderNames=[b['name'] for b in c['builders']])
     ])
 
 c['logCompressionLimit'] = False
@@ -59,7 +46,7 @@ c['buildbotURL'] = ActiveMaster.buildbot_url
 
 # Associate the slaves to the manual builders. The configuration is in
 # slaves.cfg.
-slaves = slaves_list.SlavesList('slaves.cfg', 'Mojo')
+slaves = slaves_list.SlavesList('slaves.cfg', '%(master_classname)s')
 for builder in c['builders']:
   builder['slavenames'] = slaves.GetSlavesName(builder=builder['name'])

[Dirk Pranke, 2014-12-10 22:31:37]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-12-10 22:32:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/761253003/220001

[commit-bot: I haz the power, 2014-12-10 22:34:19]

Committed patchset #8 (id:220001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=293339