803823 - Port buildbot master manifests to PuppetAgain

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Description

[Chris AtLee [:catlee]]

The existing manifests live in our puppet-manifests repo. They're mostly implemented by the buildmaster module:
http://hg.mozilla.org/build/puppet-manifests/file/default/buildmaster-production.pp#l34

We need to fix bug 777742 at the same time.

Dustin, do you think this requires changing the platform the build masters use, or is it likely we can centos 5.5 working?

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

We'll need to use CentOS 6.0.  I don't expect that to be difficult, though.

Comment 2

[Massimo Gervasini [:massimo]]

Attachment: changes to port old buildmaster to puppet-again

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 709049 [details] [diff] [review]
changes to port old buildmaster to puppet-again

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

This is nicely done!  Sorry about the huge review -- splinter seems to make that too easy.  It's really not that much!  Anyway, fb+ so far, but a bit of fixing to do before landing.

Minor, but global notes:
* All of the files generated internally need copyright headers.
* Please remove the toplevel::server::master class, since you've replaced it
* If you reference variables from another class, you should include that class

And notes for when this lands:
* Docs for all new secrets and config parameters
* Docs for the buildmaster module -- how to use it, what it does, limitations
* Add the new toplevel class to the toplevel module's docs
* Changes to the main PuppetAgain documentation if we decide to define resources in the node definitions (see below)

::: manifests/nodes.pp
@@ +11,5 @@
>  node "relabs06.build.mtv1.mozilla.com" {
>  }
>  
>  node "relabs07.build.mtv1.mozilla.com" {
> +    $num_masters = 2

It should be possible to calculate this now, rather than having to specify explicitly.

@@ +24,5 @@
> +        http_port => 8002,
> +        master_type => "build",
> +        basedir => "build2";
> +    }
> +    include toplevel::server::buildmaster

Specifying per-host configuration in the node definition is a change from how we've decided to do things.  From https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain:

"In anticipation of using an external node classifier (ENC), node definitions should only include classes - do not define any resources within nodes. In general, the included classes should be in the toplevel module. "

Nothing's carved in stone, of course, but we should think carefully before changing that.  As I understand it, this information is already redundant to information in production-masters.json, etc.  Could it be generated dynamically from that file somehow?  Perhaps with some Ruby DSL?

Alternately, the class invocations here could be moved to a case $::fqdn { .. } statement in the toplevel::server::buildmaster class.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +17,5 @@
> +    include nrpe::settings
> +    include nrpe::service
> +    $master_group = $users::builder::group
> +    $master_user = $users::builder::username
> +    $master_basedir = $buildmaster::settings::master_root

need to import users::builder and buildmaster::settings if you refer to them

@@ +22,5 @@
> +
> +    $master_name = $name
> +    $full_master_dir = "$master_basedir/$basedir"
> +
> +    #$virtualenv_dir = "${full_master_dir}/venv"

remove commented-out code

@@ +30,5 @@
> +        fail("you must set num_masters")
> +    }
> +
> +    # Different types of masters require different BuildSlaves.py files
> +    $buildslaves_template = "BuildSlaves-$master_type.py.erb"

${..} please (and several other places)

@@ +67,5 @@
> +            group => $master_group,
> +            mode => 600,
> +            content => template("buildmaster/postrun.cfg.erb");
> +
> +        "/etc/default/buildbot.d/$basedir":

shouldn't this be $master_name rather than $basedir?

@@ +70,5 @@
> +
> +        "/etc/default/buildbot.d/$basedir":
> +            content => $full_master_dir,
> +            require => [
> +                File["/etc/default/buildbot.d"],

This will be required automatically - no need to add it here

@@ +98,5 @@
> +            environment => [
> +                "VIRTUALENV=/tools/python27-virtualenv/bin/virtualenv",
> +                "PYTHON=/tools/python27/bin/python2.7",
> +                "HG=/tools/python27-mercurial/bin/hg",
> +                "MASTERS_JSON=https://raw.github.com/gerva/build-tools/803823/buildfarm/maintenance/production-masters.json",

I assume this is a temporary source URL?  This should become a config option.

::: modules/buildmaster/manifests/init.pp
@@ +7,5 @@
> +# buildmaster requires that $num_masters be set on the node prior to including this class
> +#
> +# TODO: move $libdir stuff into template?
> +# TODO: you still have to set up ssh keys!
> +# TODO: determine num_masters from json (bug 647374)

..or from the number of files in /etc/default/buildbot.d/

@@ +11,5 @@
> +# TODO: determine num_masters from json (bug 647374)
> +class buildmaster {
> +    # TODO: port releng module
> +    #include releng::master
> +    #include secrets

remove commented-out code

@@ +19,5 @@
> +    $master_basedir = $buildmaster::settings::master_basedir
> +    $clone_config_dir = $buildmaster::settings::master_basedir
> +    if $num_masters == '' {
> +        fail("you must set num_masters")
> +    }

This check is redundant to the one in buildbot::buildbot_master

@@ +24,5 @@
> +    service {
> +        "buildbot":
> +            require => File["/etc/init.d/buildbot"],
> +            enable => true;
> +    }

Will this successfully start a second buildmaster if one is added?  Or stop one if num_masters goes from 3 to 2?  It might make sense to split up the initscript to handle those cases.  Or maybe just not worry about it.

::: modules/buildmaster/manifests/queue.pp
@@ +27,5 @@
> +            require => Class["buildmaster::tools"],
> +            content => template("buildmaster/passwords.py.erb"),
> +            mode => 600,
> +            owner => $users::builder::username,
> +            group => $users::builder::username;

include users::builder

@@ +31,5 @@
> +            group => $users::builder::username;
> +        "${nrpe::settings::nrpe_etcdir}/nrpe.d/pulse_publisher.cfg":
> +            content => template("buildmaster/pulse_publisher.cfg.erb"),
> +            require => Package["nrpe"],
> +            notify => Class["nrpe::service"],

include nrpe

@@ +45,5 @@
> +            hasstatus => true,
> +            require => [
> +                File["/etc/init.d/command_runner"],
> +                File["${buildmaster::settings::queue_dir}/run_command_runner.sh"],
> +                Exec["install-tools"],

This should probably depend on Class['buildmaster::tools'] instead; see that class below for more comments.

::: modules/buildmaster/manifests/repos.pp
@@ +1,5 @@
> +# buildmaster repo class
> +# creates the souce code repoitories
> +define buildmaster::repos($repo_name, $dst_dir) {
> +
> +    $hg_repo = "http://hg.mozilla.org/build/$repo_name"

This should probably be a config option, since external users may want to use their own forks of these repos.

@@ +8,5 @@
> +        # make will take care of checking out
> +        # buildbotcustom and tools
> +        "clone-$repo_name-$dst_dir":
> +            require => [
> +                Class['packages::mozilla::py27_mercurial'],

include this class here, too

@@ +12,5 @@
> +                Class['packages::mozilla::py27_mercurial'],
> +                File[$dst_dir],
> +            ],
> +            command => "/tools/python27-mercurial/bin/hg clone $hg_repo $dst_dir",
> +            user => "$users::builder::username";

and include users::builder as well

::: modules/buildmaster/manifests/tools.pp
@@ +1,1 @@
> +class buildmaster::tools {

This should use anchor classes to make sure the whole thing can be required.

Also, this class is not a general "tools" install - it's specifically an install of a virtualenv for the command queue.  So it should probably be renamed.

@@ +4,5 @@
> +
> +    $python_packages_url = "http://puppetagain.pub.build.mozilla.org/data/python/packages/"
> +    python::virtualenv {
> +        "$buildmaster::settings::queue_dir":
> +            python => "/tools/python27/bin/python2.7",

With rail's recent landing, I think this should use $packages::mozilla::python27::python instead.

@@ +7,5 @@
> +        "$buildmaster::settings::queue_dir":
> +            python => "/tools/python27/bin/python2.7",
> +            require => Class['packages::mozilla::python27'],
> +            user => $users::builder::username,
> +            group => $users::builder::group,

include users::builder

@@ +15,5 @@
> +                "${python_packages_url}/carrot-0.10.7.tar.gz",
> +                "${python_packages_url}/amqplib-0.6.1.tgz",
> +                "${python_packages_url}/anyjson-0.3.tar.gz",
> +                "${python_packages_url}/pytz-2011d.tar.gz",
> +            ];

python::virtualenv takes package specs in the format seen in 'pip freeze', e.g., "buildbot==0.8.4-pre-moz2".

This is the wrong URL to use anyway, but even if it was the right URL ("http://repos/.."), that wouldn't be good enough - the package spec allows pip to consult several mirrors in the case that its nearest mirror is down.

@@ +23,5 @@
> +            require => [ File["${buildmaster::settings::queue_dir}"],
> +                         Python::Virtualenv["$buildmaster::settings::queue_dir"],
> +                         ],
> +            creates => "${buildmaster::settings::queue_dir}/tools",
> +            command => "/tools/python27-mercurial/bin/hg clone http://hg.mozilla.org/build/tools",

This URL should probably be the same config option as in buildmaster::repos

@@ +30,5 @@
> +            cwd => "${buildmaster::settings::queue_dir}";
> +        "install-tools":
> +            require => Exec["clone-tools"],
> +            creates => "${buildmaster::settings::queue_dir}/lib/python2.7/site-packages/buildtools.egg-link",
> +            command => "${buildmaster::settings::queue_dir}/bin/python setup.py develop",

It might be easier to run `$queue_dir/bin/pip install $queue_dir/tools`

::: modules/buildmaster/templates/BuildSlaves-build.py.erb
@@ +23,5 @@
> +}
> +tuxedoUsername = '<%=scope.lookupvar('secrets::tuxedo_user')%>'
> +tuxedoPassword = '<%=scope.lookupvar('secrets::tuxedo_password')%>'
> +balrog_username = '<%=scope.lookupvar('secrets::balrog_username')%>'
> +balrog_password = '<%=scope.lookupvar('secrets::balrog_password')%>'

I don't see any of these added to `secrets.pp`

::: modules/buildmaster/templates/buildmaster-cron.erb
@@ +1,3 @@
> +MAILTO=[e-mail]
> +@hourly cltbld <%=full_master_dir%>/bin/python <%=full_master_dir%>/tools/buildfarm/maintenance/watch_twistd_log.py -t .<%=master_name%>-last-time.txt -n <%=master_name%> -f cltbld@<%=fqdn%> -e [e-mail] <%=full_master_dir%>/master
> +15 * * * * cltbld lockfile -60 -r 3 $HOME/lockfile.<%=master_name%>_cleanup 2>/dev/null && (nice -n 19 <%=full_master_dir%>/bin/python <%=full_master_dir%>/tools/buildfarm/maintenance/master_cleanup.py -t4 <%=full_master_dir%>/master ; rm -f $HOME/lockfile.<%=master_name%>_cleanup) >> cleanup.log 2>&1

Could these lockfiles go somewhere other than $HOME?  Perhaps under /var/state or something?

::: modules/buildmaster/templates/run_pulse_publisher.sh.erb
@@ +1,3 @@
> +#!/bin/bash
> +####  This file under configuration management control
> +####  DO NOT EDIT MANUALLY

In general I think these headers are redundant.  Everything on the host is under configuration management control, and shouldn't be edited manually.  The comments have snuck in all over the place anyway, so I just mention this as a point for the future.

::: modules/dirs/manifests/builds/buildmaster.pp
@@ +1,1 @@
> +class dirs::builds::buildmaster {

The class name should mirror the path, so this class would create /builds/buildmaster, which isn't on the list :)

These dirs should get created in a buildmaster::dirs class, instead, most likely.  Or in modules/buildmaster/manifests/init.pp.  The dirs::* hierarchy is for directories that are shared among multiple classes that might appear on the same system (e.g., /builds or /tools).

@@ +6,5 @@
> +
> +    file {
> +        "/etc/default/buildbot.d/":
> +            ensure => directory,
> +            mode => 755;

This should have recurse => true and force => true so that it cleans out old master entries

::: modules/nrpe/manifests/settings.pp
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  class nrpe::settings {
>      include ::shared
>      $plugins_dir = "/usr/${::shared::lib_arch_dir}/nagios/plugins"
> +    $nrpe_etcdir = '/etc/nagios/'

Good change!

::: modules/ssh/manifests/keys.pp
@@ -18,5 @@
>          'catlee' => "ssh-dss AAAAB3NzaC1kc3MAAACBAMwVSpIUT8pAdFF/tKsGED//1oLJruGIhhUyMTLw7f5OTZb6GKQIY6D3RrnJJjaqwFqTIk1V1V92i0FSDUx/FZLY8Thjw3Q4oV+jcL4oi9V2JwFFsBZcAGHIGxU0oMXPoZYtikRo+955HgwCGvIdHdMx2cOSy6wDlecSzYF7xnFRAAAAFQD00WmYN7H+Le9rAnrLf3+Y9vbAdQAAAIAvE4xkVSC2bqOfnuGvH8pyQfBZtkO/d59/rT92VcHwID5axsuSoIFqJZiuic+pyvtiBtPPjEqmBlHiPydan3bjbNeO+lnWP4ZRFpzuk8UY70nAXVD02gZcbAIAcdfmZPk4GqBb++9Ht5AMvXIFsp4PqS3Ejfcs6Nc8yUaqCrWtSwAAAIAaOIdG/vJ/N/wli2632gwpokr2hcW7lTCcq3saV1jib+4LAy1y7J0IyLDcZyrijGhJolbPe8dTiaNCPZV0YjhHU/tn7AiRPFlG4RVkkS3mRWhkuyWqj5NBN/rxBCrwWO7/SaAL+02i7IFqtBbw2WiA0896MmbvODvNttwK0Oh+0w==",
>          'cltbld' => "ssh-dss AAAAB3NzaC1kc3MAAACBAOP7BCvqIQ2oaE8fYzkcMoSzZrLn/1KafC/LlylEOleTqjpSWIV/Ks3XtyzpN3p9Wa99piLS8xOQdFojr66PGl2mmqabFmPvz8CxnLmhGMmmnTcYdHCRJBU7NqQThubghoUPY/v5t9B1CCBwczKBFgeixB0eRi07VwS698QwDsqjAAAAFQC2Hja0EsjLUS/IrP0I0mkEQMtJVQAAAIBShl4tGC1ScIWT88AhQI1qLn3mpU/sKpYu8MPf+98oaK0m0FGrTbaph92R+rSaYw2FJY0kEKmDejD+ISRaHVTtypPNh0ljxEox6duNmqkY+58hWjoIk0CMWHRHzBLOWnecrTtevIjbt2tV0esJk9bvDC57xr2/ynXJY3+DBANWKwAAAIEAqc1Q2wPH6t2x7kK53Cpy49wh9jG68NX1DKcsN+9RSf7QLOmH4GYU57Tx/jSVu35NC9AcuuGsky7LfzR3UglciGb5SwzD8O1VO8BpSbius3YDexB+0S88EQvn8yW5tXapQGHjQEMmuzG1Km66w5faeo2YvdcziDGlNmAGxNTq1nY=",
>          'coop' => "ssh-dss AAAAB3NzaC1kc3MAAACBAPYrPziETSHpwbgjHfwhFxFM1sJuap+tow2by3AY7WdHIDgteDiXCidoaQvFmCK+IsTGF2szr/PcJlutkjGm0mvSSDnwzHQFVWPKWWHZfeyou+EZ+yzvqbZA2oRzCCMFPNBFnpeWR4iLrupRxnj0NcvK2UAmtGnngOzC7JW7Y7TtAAAAFQDt1rjod0gGbOfF1JZf/oJrKYDFYwAAAIBVoCj2fy1QCp3teHYNcGdpQibO8QTRi3D1Z4haTKmMz8CakFUTqBXfFFatGT/SRtl208sqHJ6JtS8eO16rnn/XCHlPrvqecSQTWsfQAe4sYZ4P92RuZDt2x5DfTnbEcmI919t4rb4ljzQCO/cw2Z4rRXu1L//6TY9vsj4LCnTuhAAAAIEAqnedL3n5JYy1oXvoVcnUKMAzc+3Hwhmo4smfb9kCjds8ZTlEtEZHi8LBY7SsroFgko1J4CIr5VnOveucoKbn9PF1OGAvwDIJvA2O1S/jw8WaaKBfTtGZz9FBGzO8X6SWZso4DlZS9KjNrpWSW0/53XHCQMPAqA7mDRZkVznVGUw=",
>          'dustin' => "ssh-dss AAAAB3NzaC1kc3MAAAIBAPAziraZXSA8rw3/PvqaM7unqGslOGGgjSfmTRNoMao3/YsTXTCJmrD/1fnMQ7TLRyaiPoqA/mO249qlyol/sc0ThsUokckj/0/oIms1UZPzrULt9qvmTbUpqbQg7Wr/aZNnDmVSP9k/SkbMpn7OAjw75XfhEBzoUFq9L5Tk8rBSraX8Q+CJQh9Z4fg6MH0oWQB60RpsEujEO3Z0oXtmOxAzo+oSnPovGHgR2ekL8EJG0/YZuXQIlnM8kmrIVGC65ynqYQ3+XXi+qKD57cQ31/Xh2tXmjTw2E75uKMseHE1Gvq6Q/JOogKC5wZ3cqVNOWnYDQyFCL5EmAbHhTLnSxmuJDcS4AmtvzIdsYURTcKjf0QAxby95oJVsiTBuEOLLvh2byg4FPk+2kmkwSLpmjKEg/vySieqZeOuoM5wKJEET1rRLpWiXiyUzZd297uUChBBLpipVu8LQusN7J98eb7iawBCrDOoZi5WEEQNTGe9jnGEnYxrxjmwUvEombu1FdDH7SyEgj+z8UGXjGDwwykD54pOVzUWZBbH4pYhEthyCihyrYH43NwiJqoNdJmLEdGW5Iyq747QSrevHQe3VDw6izTjwyPAv5ysphwzvpm2s4k9FHDDt3cIOYQ98SLqhsonBFsGVVoCsBovaXKJdwOoJxXIyEuc9WosbNJcfokw9AAAAFQD+xvq1OhlNI5lWKbJ6DnCmqXafzwAAAgEA14G7mF+rl2oxUN4lYHre8J5PakX0zS9qEWAEBKDayxiJpaxpWUvocKja5Oxopa1jWJR3ZzNbGNh0t0lj4gl3lmGi9ORxIc6ww0VovywJqkKxoAXmguqSC7pK05cqggMA8uYmVCgufI6C7CUHYSrtfneaq3+N0vlcWehL+dCNqPTScJy5dbejvXnyl1Ob53zGZ+8zHkvg9NjCHvDR7g/cL3DjKXuLYYeqrHzBMo4aRAiEa8+uema8XrS0wRzsBzfvfLwb47nCGeIW8w0mKiz70YUrkqCUNo3Rn+zbCWgjLiizBCNse8dlo8vgrIgeRs7elsUzj1P9YkxtV4YVZ1R+ar8E+D9//huYYWGkiUWT2ETAz2pZaFoed1sbdR1m9wRPrkXSIrulonUtwZ2UEOWIV8T+XZ9u9jQeKIINFKvcPOtHsYOuZciyG1sMcv45e/14VJdRTZBEBWD6x3BQHeue2aE1zN0WFSgQqLd1zqGlNc23lv0SEQHqelpYoRSIMrq35h912ECf5ky7/pbeEni21hOAy1nm16K1cMoJ1A1j7jfMOvvEjxo1hb/gKyQJCUQG104AajJMOsvU0qA3aX7FI4l60faNFN+DMn/Cfp3Y02XaK+JCoaEJccV7wMd2Jmkubufx5F6g6QYe0JFz86swcrtaqzxnQFO4oAQ78rkktrMAAAIAEvYx401EuEUKxsEzvuemOZs8WrpJ3Rp5jyQ2uCcTu4EEznH1ZqrtbS75Cexyg1ZUooFAx7ipeJ82mTQqYqKbUQw50yI+/INqJG63E4HRmv3CCwDFYddELBJ2j1ASldWkSBq0j6aC+RjlVZfhb+dmAFEM78tDkwrQ6LGArBxJ5OYjYJymZuHLgxmqVCPKevkh9zJneSeoL55nBX4UJ4zbNM8gWCqMcuygk11PZ3ob98DDZuUNQhOVwZzx9MDlWZy+xrW3vDnyfKzL990uAm+X8SITMyPcTA11VmOJslpUjSukgKZRg7DcBYjSolbU/9CZjdNtKcSZGFVIbeBE6yZS5tINUIjdnqrEr7eTFZwoMjx+jrSFflUiHbufRI0kSrsvr4Q76f0E6lrz2Rk0KBpWvgxaTilS6YxokpIQ01uSHaELjYaFtp4rdEj3nAI1wkdrscPUkRQJ4bhh1VqKu6p/lknVOOwyUlvSgqopf2/449uaneyWr04uFTRRG7UbdHRjuCalh9ridfK5zf+DqeFMKqtWZFbWBQ/NaxoMX4MYo3cJsPiDmfiXM0ImVo7x01WINGNrpSIkEMLbLS2N71wlQ64Q6Q9F7VPVPJwDeBwYccSvbiafAjgOxtwi3FOpRZ6FeA2Q75H2Lb+38/UGJu8HrKs8SbupHCYT4clJxwYqPGw=",
> -        "jhopkins" => "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAgEApAGT+xJUFMIon4l45N+KLs9XzC8srs8q2O57c4L8tidbA0myL0UrkLFrg9ByFVak2zHW25eDee1zGPMKpFFn/SaOJW6lULtZLzgG+Qyy90Y3yshwT9WEXz8qHGSqtk1QJNV0ZNerKeZzZjt/Wjn6p36Jv2HJe+nxk1liKkQY4hfPLXGQEysDDlqC/kjc0Aw6WyLcspPp7NMoXRzGpQ5lGOjDDBdWWUx6F/JZdqU/hkX7Mbg4uhXj9UvOuGNgxvZXbyPFr9/pxhAbFR/nJVy4ywrnQv9EGBREsih9pfGHUARkSett7cCuFHv6miEg0/f7U0LBYtGj9+39uOUhvLM6uFpew5IA/+ENhYkMNlKCE/aJkHx03ZzpYHHdhDPAhkeMI+QEseWh2xYzSJDZx210c8ffVvN8PHJ/ZSpx0G2uHWfB3kSOQbGHlgY0k8ooJKXDXCxszpSolTfD5+X6wiaslSWl+i2nVCmo+LkEonYYIYmks919U7WP7xR5ajtDCSIUS3Y0s/h2hQ7PY8qKLwzTPFZbSTe2eI2Jq+RPao37bhNsGsBaR05zidfM9laLVD5gPnXnbplw6tfzAa+AiBH9akCvllCsDaTNsssr23IpGws6sNbdSTjHHcsZggfAzSOSlSeKDYS2McK/IDfOjAwBy2HnEKCrcPMSGxwcRT1DL4M=",

What's changing here?

::: modules/toplevel/manifests/server/buildmaster.pp
@@ +11,5 @@
> +    include packages::mysql-devel
> +    include packages::mozilla::git
> +    include packages::mozilla::python27
> +    include packages::mozilla::py27_virtualenv
> +    include packages::mozilla::py27_mercurial

Most of these should be in the 'buildmaster' class (some of them already are)

Comment 4

[Rail Aliiev [:rail]]

Comment on attachment 709049 [details] [diff] [review]
changes to port old buildmaster to puppet-again

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

Since Dustin did a great job reviewing the patch I tried to concentrate on the functionality being ported.

So far I detected just minor issues.

* why do you need 2 mercurial packages? (packages::mercurial and packages::mozilla::py27_mercurial)? I don't mind to have 2 :), just curious.
* can you also add packages::strace (and maybe packages::tmux? ;) )

I'd be happy to review the final patch.

::: manifests/nodes.pp
@@ +24,5 @@
> +        http_port => 8002,
> +        master_type => "build",
> +        basedir => "build2";
> +    }
> +    include toplevel::server::buildmaster

I agree, we need something here what would prevent us duplicating information. We can use "case $::fqdn..." as an intermediate step here before we switch to using production-masters.json (not trivial, deserves a separate bug).

Comment 5

[Rail Aliiev [:rail]]

Comment on attachment 709049 [details] [diff] [review]
changes to port old buildmaster to puppet-again

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

One more thing.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +97,5 @@
> +            logoutput => on_failure,
> +            environment => [
> +                "VIRTUALENV=/tools/python27-virtualenv/bin/virtualenv",
> +                "PYTHON=/tools/python27/bin/python2.7",
> +                "HG=/tools/python27-mercurial/bin/hg",

Can you use $::packages::mozilla::python27::python, $::packages::mozilla::py27_mercurial::mercurial for PYTHON and HG; and add the same thing for VIRTUALENV in packages::mozilla::py27_virtualenv?

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

strace and tmux should probably be added to toplevel::server - they're useful everywhere!

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

You'll want to have a look at the patch I'm landing for bug 817762 - it does a better job of setting the TCP connection keepalives.

Comment 8

[Massimo Gervasini [:massimo]]

Attachment: additional fixes

fixes:
* added missing includes
* removed commented-out code
* merged tools.pp is queue.pp
* fixed python dependencies url (using "buildbot==0.8.4-pre-moz2" format for packages)
* added missing entries in secrets.pp
* removed redundant headers ("do not edit manually...")
* added MPL license headers
* renamed dirs::builds::buildmaster to dirs::builds::buildbot
* added recurse => true and force => true for /etc/default/buildbot.d/
* replaced hardcoded path for hg, python and virtualenv with "$::packages::mozilla::..."
* added tweaks and strace to server module
* master_json file, tools and buildbot-configs repositories are now configurable
* hardcoded $num_masters replaced by a facter (counts files in /etc/default/buildbot.d/) - I don't know if this is robust enough

still open:
* could not add tmux because a dependency error installing it on a aws box
* per-host configuration in nodes.pp

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 716570 [details] [diff] [review]
additional fixes

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

Looks great overall.  Just some minor changes.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +28,5 @@
> +    $master_group = $users::builder::group
> +    $master_user = $users::builder::username
> +    $master_basedir = $buildmaster::settings::master_root
> +    $master_name = $name
> +    $full_master_dir = "$master_basedir/${basedir}"

Add {..}

@@ +34,5 @@
> +
> +    # Different types of masters require different BuildSlaves.py files
> +    $buildslaves_template = "BuildSlaves-${master_type}.py.erb"
> +
> +    if ($buildslaves_template) {

This conditional will never be false

@@ +85,5 @@
> +    exec {
> +        "setup-${basedir}":
> +            require => Buildmaster::Repos["clone-buildbot-${master_name}"],
> +            command => "/usr/bin/make -f Makefile.setup all BASEDIR=${full_master_dir} MASTER_NAME=${master_name}",
> +            creates => "${full_master_dir}/master",

I assume this directory is created early in the make invocation.  If something else fails, this directory will still exist, and the exec won't run on the next puppet run.  Is there a file that's created nearer the end?  Or does this make invocation always work?

::: modules/buildmaster/manifests/init.pp
@@ +10,5 @@
> +#
> +# TODO: move $libdir stuff into template?
> +# TODO: you still have to set up ssh keys!
> +class buildmaster {
> +    # TODO: port releng module

There's a few TODO items here.  Are those still required?  If so, please file bugs to track them, and otherwise remove the comments.  Ideally puppet would not succeed unless any required SSH keys are configured correctly, and would fail with a message telling the user what to do.

@@ +29,5 @@
> +            group => $users::builder::username;
> +        "/etc/nagios/nrpe.d/buildbot.cfg":
> +            content => template("buildmaster/buildbot.cfg.erb"),
> +            notify => Class["nrpe::service"],
> +            require => Package["nrpe"];

This should be done via the nrpe module, rather than reaching into its internals.  Maybe something like

nrpe::custom {
    "buildbot.cfg":
        content => template("buildmaster/buildbot.cfg.erb");
}

::: modules/buildmaster/manifests/queue.pp
@@ +44,5 @@
> +        "${nrpe::settings::nrpe_etcdir}/nrpe.d/command_runner.cfg":
> +            content => template("buildmaster/command_runner.cfg.erb"),
> +            require => Package["nrpe"],
> +            notify => Class["nrpe::service"],
> +            mode => 644;

Similarly to buildbot.cfg, these should be done via the nrpe module

::: modules/buildmaster/manifests/repos.pp
@@ +15,5 @@
> +            require => [
> +                Class['packages::mozilla::py27_mercurial'],
> +            ],
> +            command => "$::packages::mozilla::py27_mercurial::mercurial clone $hg_repo $dst_dir",
> +            user => "$users::builder::username";

Shouldn't this have a creates => ?  From what I can tell it would run 'hg clone' on every puppet run.

::: modules/dirs/manifests/builds/buildbot.pp
@@ +5,5 @@
> +    include users::builder
> +    include config
> +
> +    file {
> +            "/builds/buildbot" :

indentation

Comment 10

[Massimo Gervasini [:massimo]]

Attachment: additional fixes

Thanks Dustin and Rail for the feedback.

here is another patch addressing:

* added missing ${...}
* removed always true conditional
* removed TODOs
* added 'creates =>...' in repos.pp
* created nrpe::custom module 
* fixed indentation

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 719570 [details] [diff] [review]
additional fixes

FWIW Just searching this patch for cltbld I see a lot of lines we need to fix (to use the puppet config var) I'll let dustin decide if thats blocking though.

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 719570 [details] [diff] [review]
additional fixes

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

A few small details, but nothing blocking committing this.  I agree with Callek regarding removing explicit mention of 'cltbld'.  There will probably be a few other things to make configurable so that seamonkey can use this.  But fixing those after the initial landing is fine by me if Callek's OK with it.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +40,5 @@
> +            content => template("buildmaster/${buildslaves_template}");
> +    }
> +
> +    file {["${full_master_dir}",
> +        #"${buildbot_configs_dir}",

why's this commented out?

::: modules/buildmaster/manifests/queue.pp
@@ +78,5 @@
> +        "pulse_publisher.cfg":
> +            content => template("buildmaster/pulse_publisher.cfg.erb"),
> +    }
> +
> +    nrpe::custom {

these two nrpe::custom clauses could be collapsed

::: modules/buildmaster/manifests/repos.pp
@@ +14,5 @@
> +        "clone-${dst_dir}":
> +            require => [
> +                Class['packages::mozilla::py27_mercurial'],
> +            ],
> +            command => "$::packages::mozilla::py27_mercurial::mercurial clone $hg_repo $dst_dir",

${..}

@@ +16,5 @@
> +                Class['packages::mozilla::py27_mercurial'],
> +            ],
> +            command => "$::packages::mozilla::py27_mercurial::mercurial clone $hg_repo $dst_dir",
> +            creates => "$dst_dir",
> +            user => "$users::builder::username";

${..}

::: modules/buildmaster/templates/buildmaster-cron.erb
@@ +1,2 @@
> +MAILTO=[e-mail]
> +hourly cltbld <%=full_master_dir%>/bin/python <%=full_master_dir%>/tools/buildfarm/maintenance/watch_twistd_log.py -t .<%=master_name%>-last-time.txt -n <%=master_name%> -f cltbld@<%=fqdn%> -e [e-mail] <%=full_master_dir%>/master

You'll probably want to change this email - it should be a configurable setting.

Comment 13

[Massimo Gervasini [:massimo]]

Attachment: more fixes

Thanks for the review Dustin.

some more fixes:
* added missing ${..}
* collapsed nrpe::custom clause
* email is now configurable
* moved buildbot.initd from files/ to templates/
* removed any reference of 'cltbld' user from templates.

Comment 15

[Rail Aliiev [:rail]]

I tried to puppetize a master in AWS and it failed with the following error:

[10.134.49.182] out: err: Could not retrieve catalog from remote server: Error 400 on SERVER: Failed to parse template buildmaster/buildbot.cfg.erb:
[10.134.49.182] out:   Filepath: /usr/lib/ruby/site_ruby/1.8/puppet/parser/templatewrapper.rb
[10.134.49.182] out:   Line: 65
[10.134.49.182] out:   Detail: Could not find value for 'num_masters' at 5:/etc/puppet/production/modules/buildmaster/templates/buildbot.cfg.erb
[10.134.49.182] out:  at /etc/puppet/production/modules/buildmaster/manifests/init.pp:41 on node buildbot-master51.srv.releng.use1.mozilla.com
[10.134.49.182] out: notice: Using cached catalog
[10.134.49.182] out: err: Could not retrieve catalog; skipping run


Looks like http://hg.mozilla.org/build/puppet/file/a5267cdc4944/modules/buildmaster/lib/facter/num_masters.rb#l4 doesn't set num_masters because 

ls: cannot access /etc/default/buildbot.d: No such file or directory

Comment 16

[Rail Aliiev [:rail]]

I'm testing the following fix, which makes the catalog compile stage pass:

https://gist.github.com/rail/5200780

num_master isn't set (undefined) yet.

Now it fails with the following error:

[10.134.48.4] out: err: /Stage[main]/Packages::Mysql-devel/Package[mysql-devel]/ensure: change from absent to latest failed: Could not update: Execution of '/
usr/bin/yum -d 0 -e 0 -y install mysql-devel' returned 1: Error: Package: glibc-2.12-1.47.el6_2.5.i686 (updates)
[10.134.48.4] out:            Requires: glibc-common = 2.12-1.47.el6_2.5
[10.134.48.4] out:            Installed: glibc-common-2.12-1.47.el6_2.12.x86_64 (@updates/$releasever)
[10.134.48.4] out:                glibc-common = 2.12-1.47.el6_2.12
[10.134.48.4] out:            Available: glibc-common-2.12-1.47.el6.x86_64 (base)
[10.134.48.4] out:                glibc-common = 2.12-1.47.el6
[10.134.48.4] out:            Available: glibc-common-2.12-1.47.el6_2.5.x86_64 (updates)
[10.134.48.4] out:                glibc-common = 2.12-1.47.el6_2.5
[10.134.48.4] out:  You could try using --skip-broken to work around the problem
[10.134.48.4] out:  You could try running: rpm -Va --nofiles --nodigest
[10.134.48.4] out:  at /etc/puppet/production/modules/packages/manifests/mysql-devel.pp:7

AFAIK, it's related to the repo we use... I don't recall how to fix this.

Comment 17

[Rail Aliiev [:rail]]

The issue above was caused by packages installed in AMI - we used the official mirrors because http://puppetagain.pub.build.mozilla.org/data/repos/yum/mirrors/centos/6/latest/os/x86_64/ is kind of broken - "yum groupinstall Base" doesn't even install rpm.

I recreated the AMIs using http://puppetagain.pub.build.mozilla.org/data/repos/yum/mirrors/centos/6/latest/os/x86_64-for-ks/ which looks much better.

Comment 18

[Rail Aliiev [:rail]]

Attachment: make it work v1

Comment 19

[Rail Aliiev [:rail]]

Comment on attachment 727505 [details] [diff] [review]
make it work v1

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

This patch works with 2 issues:

1) num_master (from a custom fact) is not set during the first run. The fact is evaluated during catalog compilation and not set because /etc/default/buildbot.d directory doesn't exist that time. Using  scope.lookupvar('::num_masters') allows set the value to "undefined" instead of catalog compilation error in case of <%= num_masters %>. In other words nagios check for buildbot master is unavailable until second run (in 30 min).

2) One of the Python modules is being installed on each puppet run:

notice: /Stage[main]/Buildmaster::Queue/Python::Virtualenv[/builds/buildbot/queue]/Python::Virtualenv::Package[/builds/buildbot/queue||mozillapulse==a2d95569a089e]/Exec[pip /builds/buildbot/queue||mozillapulse==ad95569a089e]/returns: executed successfully

Sounds like a bug in python::virtualenv::package, probably related to changeset-as-version.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +36,5 @@
>              require => Exec["setup-$basedir"],
>              owner => $master_user,
>              group => $master_group,
>              mode => 600,
> +            content => file("/etc/puppet/production/manifests/extlookup/${buildslaves_py}");

Instead of using secrets.csv -> class.pp -> template.erb with tons of CSV entries (hard to find, hard to deal with dupes) I used raw files stored on puppet master.

@@ +46,5 @@
>              require => Exec["setup-${basedir}"],
>              owner => $master_user,
>              group => $master_group,
>              mode => 600,
> +            content => file("/etc/puppet/production/manifests/extlookup/passwords.py");

the same here.

@@ +80,5 @@
> +                          VIRTUALENV=${::packages::mozilla::py27_virtualenv::virtualenv} \
> +                          PYTHON=${::packages::mozilla::python27::python} \
> +                          HG=${::packages::mozilla::py27_mercurial::mercurial} \
> +                          MASTERS_JSON=${buildmaster::settings::master_json} all",
> +            creates => "${full_master_dir}/master",

environment didn't work, I blame make. :)

::: modules/buildmaster/manifests/queue.pp
@@ +30,5 @@
>              notify => Service["pulse_publisher"],
>              mode => 755;
>          "${buildmaster::settings::queue_dir}/passwords.py":
>              require => Python::Virtualenv["${buildmaster::settings::queue_dir}"],
> +            content => file("/etc/puppet/production/manifests/extlookup/passwords.py"),

file() FTW! :)

::: modules/buildmaster/templates/my.cnf.erb
@@ +6,5 @@
>  [client]
> +host=<%= scope.lookupvar('config::secrets::buildbot_schedulerdb_hostname') %>
> +user=<%= scope.lookupvar('config::secrets::buildbot_schedulerdb_username') %>
> +password=<%= scope.lookupvar('config::secrets::buildbot_schedulerdb_password') %>
> +database=<%= scope.lookupvar('config::secrets::buildbot_schedulerdb_database') %>

typos...

::: modules/nrpe/manifests/check/mysql.pp
@@ +7,5 @@
>  
>      # TODO: the secrets here don't exist yet.  To fix when we set up masters.
>      nrpe::check {
>          'check_mysql':
> +            cfg => "/usr/<%=libdir%>/nagios/plugins/check_mysql -H <%= scope.lookupvar('secrets::buildbot_schedulerdb_hostname') %> -u <%= scope.lookupvar('secrets::buildbot_schedulerdb_user') %> -p <%= scope.lookupvar('secrets::buildbot_schedulerdb_password') %>";

I haven't checked this yet.

Comment 20

[Rail Aliiev [:rail]]

(In reply to Rail Aliiev [:rail] from comment #19)
> 
> 2) One of the Python modules is being installed on each puppet run:
> 
> notice:
> /Stage[main]/Buildmaster::Queue/Python::Virtualenv[/builds/buildbot/queue]/
> Python::Virtualenv::Package[/builds/buildbot/
> queue||mozillapulse==a2d95569a089e]/Exec[pip
> /builds/buildbot/queue||mozillapulse==ad95569a089e]/returns: executed
> successfully
> 
> Sounds like a bug in python::virtualenv::package, probably related to
> changeset-as-version.


http://puppetagain.pub.build.mozilla.org/data/python/packages/mozillapulse-ad95569a089e.tar.bz2 reports itself as "MozillaPulse .4" - version mismatch :/ 

We can fix ugly ".4" version and generate another tarball (may require adding a branch to http://hg.mozilla.org/automation/mozillapulse/ (even anon)) or grab the latest 0.61 version.

Comment 21

[Justin Wood (:Callek)]

Comment on attachment 727505 [details] [diff] [review]
make it work v1

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

f- for the issues I have with it. (I'm trying to avoid the corners we drove ourselves into with the old puppet, the secret files we had in puppet that were not exposed elsewhere was ALWAYS a pain for me when I was community, and is only *slightly* better now that I'm an employee, but is still a constant source of frustration and pain)

::: .hgignore
@@ +3,5 @@
>  ^manifests/extlookup/secrets.csv
>  # ignore the local config file/symlinks if present
>  ^manifests/extlookup/local-config.csv
>  ^manifests/extlookup/supplied-config.csv
> +^manifests/extlookup/.*.py

I'm against any secrets-as-raw-file in extlookup. I'd rather a seperate dir if we must do the raw-file thing, and I'd *require* a .tpl/.erb template stored in actual repo for every type of secret file we need, with guidance on how to manually generate it for other puppet consumers.

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +36,5 @@
>              require => Exec["setup-$basedir"],
>              owner => $master_user,
>              group => $master_group,
>              mode => 600,
> +            content => file("/etc/puppet/production/manifests/extlookup/${buildslaves_py}");

I'm not a fan of flat-file at all as a secrets means, I'd personally rather crufty csv stuff than this, but I won't block on it provided my req's in .hgignore's comments are solved.

Also note that using file() you're eliminating the possibility for the puppet user env to specify these, since you're going with abs path, so that if I create a puppetmaster for environ=jwood I'll always be using the production secret.

::: modules/buildmaster/manifests/queue.pp
@@ +30,5 @@
>              notify => Service["pulse_publisher"],
>              mode => 755;
>          "${buildmaster::settings::queue_dir}/passwords.py":
>              require => Python::Virtualenv["${buildmaster::settings::queue_dir}"],
> +            content => file("/etc/puppet/production/manifests/extlookup/passwords.py"),

as said elsewhere, it forces production secrets into puppet user env's. :(

Comment 22

[Rail Aliiev [:rail]]

Attachment: make it work v2

* I haven't tested this version yet
* I still need to update with new values to make sure none of the puppetmasters fails
* I have no good idea how to store signing servers from passwords.pp, thus leaving hardcoded :/
* I still need to review buildmaster::settings and probably remove duplicates. I see no reason why we should duplicate config::secrets entries there...

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 729770 [details] [diff] [review]
make it work v2

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

Looks good at a skim, can you re-request once you've tested so I can give it a more thorough skim (or a real review once you tested is fine as well)

::: modules/buildmaster/templates/passwords.py.erb
@@ +32,5 @@
> +        ('mac-signing2.srv.releng.scl3.mozilla.com:9120', '<%= scope.lookupvar('config::secrets::signing_server_username') %>', '<%= scope.lookupvar('config::secrets::signing_server_release_password') %>'),
> +    ],
> +}
> +
> +<% else %>

nit: can we tighten up these signing server checks so they are only used for moco systems [e.g. scl3 alone isn't good enough imo] and always have a domain set when being evaluated (e.g. if we open up a new DC 10 months from now, we won't use the scl1 servers by accident)

Comment 24

[Rail Aliiev [:rail]]

Attachment: make it work (for realz)

OK, this one has been tested by a builder, tester and buildbot master. Worked fine. I visually checked files I touched and the ones that should be generated. They look good. 10.134.49.119 is still can be used to test things (I disabled puppet in /etc/cron.d/puppetcheck.cron manually).

Before we land this patch we'll need to deploy new secrets file. I recorded all changes I made in a local hg repo 10.134.48.39:/etc/puppet/environments/secrets (hg diff -r 0).

* all secrets are handled by the config::secrets module
* manifests/extlookup/secrets-template.csv has been populated with missing dummy entries
* stricter fqdn matching in modules/buildmaster/templates/passwords.py.erb

The manifests still have the following non blocker issues:

* num_masters works only on second run (undefined for the first one)
* mozillapulse is being reinstalled on every run. Using ".4" as version should fix the problem.
* tweaks::tcp_keepalive works on the second run as well

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 730210 [details] [diff] [review]
make it work (for realz)

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

>  .../templates/BuildSlaves-scheduler.py.erb | 7 +--

huh? (Splinter is showing this real weird)

---

r+ with very minor nits below.

::: manifests/extlookup/secrets-template.csv
@@ +1,2 @@
> +android_tests_password,""
> +balrog_password,""

The sorting here made this diff harder to parse at a glance, I'm just going to assume "all is well"

::: modules/buildmaster/manifests/buildbot_master.pp
@@ +57,5 @@
>              content => template("buildmaster/postrun.cfg.erb");
>          "/etc/default/buildbot.d/${master_name}":
>              content => "${full_master_dir}",
> +            require => Exec["setup-${basedir}"],
> +            before => Nrpe::Custom["buildbot.cfg"];

I'm not a fan of using both directions of association in the same puppet manifests, easier to create a circular dep error this way. -- That said I don't think this case warrants a block so carry on.

::: modules/buildmaster/manifests/settings.pp
@@ +8,1 @@
>      $lock_dir = "/var/lock/${users::builder::username}"

I'd prefer to keep include users::builder in this class if we reference it in a var here.

@@ +8,5 @@
>      $lock_dir = "/var/lock/${users::builder::username}"
> +    $master_json = extlookup("master_json")
> +    $buildbot_tools_hg_repo = extlookup("buildbot_tools_hg_repo")
> +    $buildbot_configs_hg_repo = extlookup("buildbot_configs_hg_repo")
> +    $buildbot_mail_to = extlookup("buildbot_mail_to")

Please do these extlookups in http://mxr.mozilla.org/build/source/puppet/modules/config/manifests/init.pp instead of here. and then include config here and reference it that way.

::: modules/buildmaster/templates/passwords.py.erb
@@ +34,5 @@
> +        ('mac-signing3.build.scl1.mozilla.com:9120', '<%= scope.lookupvar('config::secrets::signing_server_username') %>', '<%= scope.lookupvar('config::secrets::signing_server_release_password') %>'),
> +        ('mac-signing4.build.scl1.mozilla.com:9120', '<%= scope.lookupvar('config::secrets::signing_server_username') %>', '<%= scope.lookupvar('config::secrets::signing_server_release_password') %>'),
> +    ],
> +}
> +<% elsif @fqdn.match "mozilla.com" %>

not quite strict enough, but lets go with this and adjust later. Its not like *I* don't know how to write up a proper fix for SeaMonkey needs here (once I need it :) )

(e.g.: sea-master1.community.scl3.mozilla.com has address 63.245.223.11 )

::: modules/nrpe/manifests/check/mysql.pp
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  class nrpe::check::ganglia {
>      include nrpe::settings
>      $plugins_dir = $nrpe::settings::plugins_dir

please include config::secrets

Comment 26

[Rail Aliiev [:rail]]

Comment on attachment 730210 [details] [diff] [review]
make it work (for realz)

I applied the following interdiff to the initial patch, tested it and landed.

https://gist.github.com/rail/5262808

http://hg.mozilla.org/build/puppet/rev/54790ccf8dfb

(In reply to Justin Wood (:Callek) from comment #25)
> >          "/etc/default/buildbot.d/${master_name}":
> >              content => "${full_master_dir}",
> > +            require => Exec["setup-${basedir}"],
> > +            before => Nrpe::Custom["buildbot.cfg"];
> 
> I'm not a fan of using both directions of association in the same puppet
> manifests, easier to create a circular dep error this way. -- That said I
> don't think this case warrants a block so carry on.

In this case it is (almost?) impossible to know the number of init files to use require in nrpe::custom. This is why I had to use this trick.

> ::: modules/buildmaster/manifests/settings.pp
> @@ +8,1 @@
> >      $lock_dir = "/var/lock/${users::builder::username}"
> 
> I'd prefer to keep include users::builder in this class if we reference it
> in a var here.

Sure.

> > +    $buildbot_mail_to = extlookup("buildbot_mail_to")
> 
> Please do these extlookups in
> http://mxr.mozilla.org/build/source/puppet/modules/config/manifests/init.pp
> instead of here. and then include config here and reference it that way.

I removed the variables from this module and moved them to ::config.

> > +<% elsif @fqdn.match "mozilla.com" %>
> 
> not quite strict enough, but lets go with this and adjust later. Its not
> like *I* don't know how to write up a proper fix for SeaMonkey needs here
> (once I need it :) )
> 
> (e.g.: sea-master1.community.scl3.mozilla.com has address 63.245.223.11 )

match "community" maybe?

> ::: modules/nrpe/manifests/check/mysql.pp
>  ...
> please include config::secrets

Done.


I adjusted secrets.csv before I landed the patch and will remove redundant entries once the patch is deployed.

Comment 27

[Dustin J. Mitchell [:dustin] (he/him)]

I'm not too comfortable with the signing-server handling either - it'd be better to include that in some kind of puppet data structure that's subsequently written out as Python code by the .erb.  That data structure can be conditionalized on $::config::org.  This looks great otherwise :)

Comment 28

[Rail Aliiev [:rail]]

Yeah... It looks so ugly. :/

Any idea how we can externalize/implement singing type -> dc -> servers -> user/pass relationships?

Comment 29

[Rail Aliiev [:rail]]

Attachment: add missing dep

I haven't hit this issue before, but it looks like 

notice: /Stage[main]/Packages::Mozilla::Py27_virtualenv/Package[mozilla-python27-virtualenv]/ensure: created

sometimes is installed after we call "make ...".

Comment 30

[Rail Aliiev [:rail]]

Comment on attachment 730730 [details] [diff] [review]
add missing dep

http://hg.mozilla.org/build/puppet/rev/10d291342426 (including missing symlink from the previous patch)

Comment 31

[Rail Aliiev [:rail]]

Attachment: use production branch for buildbot-configs checkout

It turns out that we need to use production branch for buildbot-configs checkout. To be tested.

Comment 32

[Justin Wood (:Callek)]

Comment on attachment 735225 [details] [diff] [review]
use production branch for buildbot-configs checkout

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

::: modules/buildmaster/manifests/repos.pp
@@ +14,5 @@
>          "clone-${dst_dir}":
>              require => [
>                  Class['packages::mozilla::py27_mercurial'],
>              ],
> +            command => "${::packages::mozilla::py27_mercurial::mercurial} clone -b ${branch} ${hg_repo} ${dst_dir}",

I would suggest/request (in a followup) that if we want a specific branch here we instead do a --no-update clone and then a seperate exec to update to the branch we want. It just feels like a better solution here for us.

Comment 33

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 735225 [details] [diff] [review]
use production branch for buildbot-configs checkout

Remember to doc the new config parameter.  Otherwise, it looks good.

Comment 34

[Rail Aliiev [:rail]]

(In reply to Justin Wood (:Callek) from comment #32)
> I would suggest/request (in a followup) that if we want a specific branch
> here we instead do a --no-update clone and then a seperate exec to update to
> the branch we want. It just feels like a better solution here for us.

Why it feels better? It adds another if/else branch. 1 command is more readable than even then "cmd && cmd" to me.

Comment 35

[Rail Aliiev [:rail]]

Comment on attachment 735225 [details] [diff] [review]
use production branch for buildbot-configs checkout

http://hg.mozilla.org/build/puppet/rev/5e8fe2ad8e97

Comment 36

[Rail Aliiev [:rail]]

(In reply to Dustin J. Mitchell [:dustin] from comment #33)
> Remember to doc the new config parameter.  Otherwise, it looks good.

Can you point me to the docs? Did you mean inline docs?

Comment 37

[Dustin J. Mitchell [:dustin] (he/him)]

https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/Modules/config

Comment 38

[Rail Aliiev [:rail]]

(In reply to Dustin J. Mitchell [:dustin] from comment #37)
> https://wiki.mozilla.org/ReleaseEngineering/PuppetAgain/Modules/config

Ah, thanks. Updated.

Comment 39

[Rail Aliiev [:rail]]

So far Massimo's great work (!) worked just fine for 30 masters. The only untested master type is scheduler master.

Any objections if I declare a victory here and file a follow up bug to improve the format of secrets.py?

Comment 40

[Rail Aliiev [:rail]]

Comment on attachment 735225 [details] [diff] [review]
use production branch for buildbot-configs checkout

Fix: https://hg.mozilla.org/build/puppet/rev/7983bf7fada6

Comment 41

[[e-mail] (:bhearsum)]

Is this FIXED?