first pass - I haven't pushed this to staging puppet yet

Comment on attachment 596237 [details] [diff] [review] puppetize foopy Review of attachment 596237 [details] [diff] [review]: ----------------------------------------------------------------- This actually seems good. For the review, could you it from someone that is better at puppet than me? If you can't, I will try my best. ::: modules/foopy/manifests/init.pp @@ +53,5 @@ > + group => $foopy_group; > + $foopy_basedir: > + ensure => directory, > + owner => $foopy_user, > + group => $foopy_group; Are "/builds" and $foopy_basedir entries redundant? (since it resolves to "/builds" from foopy::settings)

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #2) > Comment on attachment 596237 [details] [diff] [review] > puppetize foopy > > Review of attachment 596237 [details] [diff] [review]: > ----------------------------------------------------------------- > > This actually seems good. > > For the review, could you it from someone that is better at puppet than me? > If you can't, I will try my best. sure - I wanted a feedback pass to make sure I was heading in the right direction. I will poke catlee or bhearsum for the review (I have a question for one of them already queued up.) > > ::: modules/foopy/manifests/init.pp > @@ +53,5 @@ > > + group => $foopy_group; > > + $foopy_basedir: > > + ensure => directory, > > + owner => $foopy_user, > > + group => $foopy_group; > > Are "/builds" and $foopy_basedir entries redundant? (since it resolves to > "/builds" from foopy::settings) it could be, I borrowed this pattern and I haven't seen it run yet - i'll keep an eye out for that.

Naive question: we're going to be targeting CentOS >= 6 as the base image for these foopies, correct?

we should - it's now our baseline os

Bear: can we hand this off to Callek? I'd like to get this done well before we have working pandaboards to worry about.

(In reply to Chris Cooper [:coop] from comment #6) > Bear: can we hand this off to Callek? I'd like to get this done well before > we have working pandaboards to worry about. Callek and I discussed this briefly before the holiday, so yes :) - it was on my todo list to bring up at our next 1x1.

a start_cp.sh change will be required for foopy-on-linux due to twistd/buildbot being in a different directory. I chose to use some magic here, by having facter run, and divert stderr to /dev/null so we don't pollute the mac foopy start_cp terminal with this. The mac foopies have this file copied over, not symlinked so these changes do not necessarily have to take affect unless we want them to, but they are mac-compatible unless there are other changes here. Also I explicitly chose not to allow outside-screen clientproxy invokes for now, until we are fully off the mac foopies, so we don't forget/mess up there.

Comment on attachment 635193 [details] [diff] [review] [tools] v1 should we make this into an include? so it can be placed into all of the .sh tools that will need the environment?

This is my current WIP, this does not yet have a working crontab, and requires some manual setup (tegra_stats.sh, create_dirs.sh stuff, etc.) but gets us 96% of the way to a working foopy. I know I have some general cleanup to do, and I did not check the dependancy tree for these changes yet, incase I need to do puppet before/after/require dances. But so far I think we're pretty good.

Comment on attachment 635223 [details] [diff] [review] [puppet] WIP 0 Review of attachment 635223 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/packages/manifests/editors.pp @@ -12,5 @@ > - default: { > - fail("cannot install on $operatingsystem") > - } > - } > -} Is this just a whitespace change? ::: modules/puppet/manifests/periodic.pp @@ -11,1 @@ > } +1 to this! Can you add the reverse and have ::atboot remove the cron? ::: modules/python/manifests/virtualenv/prerequisites.pp @@ -6,5 @@ > "virtualenv.py": ; > > # these two need to be in the same dir as virtualenv.py, or it will > # want to download them from pypi > "pip-0.8.2.tar.gz": ; This will be taken care of in bug 669977 ::: modules/toplevel/manifests/foopy.pp @@ +15,5 @@ > + "bind-utils": > + ensure => latest; > + "rsync": > + ensure => latest; > + } (sorry if this is too much for a fb?) these should be packages::bind-utils (or maybe a better name if there's one particular utility you're using?) and packages::rsync. @@ +32,5 @@ > + owner => $config::builder_username, > + group => $config::builder_username, > + ensure => directory, > + mode => 0755; > + } This should all require/include dirs::builds, and it may be worth adding some of these to the dirs package (I'll leave that to your discretion) @@ +50,5 @@ > + ], > + creates => "/builds/talos-data/talos-repo/.hg", > + command => "/tools/python27-mercurial/bin/hg clone -u $frozen_talos_rev http://hg.mozilla.org/build/talos /builds/talos-data/talos-repo", > + user => $config::builder_username; > + } Would it make sense to create a separate module/class to do hg checkouts? It'd be nice if it could ensure a particular revision, so that you can change $frozen_talos_rev, it automatically updates? This is probably a common need (slavealloc will need it, too). @@ +78,5 @@ > + "/builds/sut_tools/devicemanagerADB.py": > + owner => $config::builder_username, > + group => $config::builder_username, > + ensure => link, > + target => "/builds/talos-data/talos-repo/talos/devicemanagerADB.py"; This would probably be clearer if it were broken out into a separate define that you invoke repeatedly: whatever_module::sut_tools_link { "devicemanagerADB.py": target => "/builds/talos-data/talos-repo/talos/devicemanagerADB.py"; } @@ +98,5 @@ > + "/builds/tegra_stats.log": > + owner => $config::builder_username, > + group => $config::builder_username, > + ensure => file, > + mode => 0644; These are all identical, and are essentially just "touch", so could probably be done in one stanza with [ .. ] @@ +130,5 @@ > + > + # apply tweaks > + include tweaks::dev-ptmx > +} > + Overall, this is a lot of specific stuff to put in a toplevel::xxx class. I'd rather see toplevel::foopy include the 'foopy' module (which could include others). ::: modules/users/files/screenrc @@ +111,5 @@ > +#bindkey -k k8 select 7 # F8 = screen 7 > +#bindkey -k F2 next # F12 = next > +#bindkey -k F1 prev # F11 = previous > +######################### > + A lot of this looks user-specific, unless mutt is part of the foopy process :/

(In reply to Justin Wood (:Callek) from comment #12) > http://hg.mozilla.org/build/tools/rev/09bec04b91d5 deployed to all foopies

I still have to refresh my mind and skim all these f- comments from dustin, but I wanted to toss my current take up here, so we can both take another pass through this.

Comment on attachment 640120 [details] [diff] [review] [puppet] v0.9 Review of attachment 640120 [details] [diff] [review]: ----------------------------------------------------------------- I've not repeated notes from comment 11 here: * screenrc is probably unnecessarily user-specific * toplevel::server::foopy should refer to a foopy module, not do stuff itself * hg checkout class * require => Class['dirs::builds'] Also, given that you're "tossing" this, and that it doesn't look much changed from the last patch, I'll limit myself to just these comments. ::: modules/puppet/manifests/periodic.pp @@ +7,5 @@ > # Mozilla IT. There is enough splay here to avoid killing the master > # (configured in puppet.conf) > "/etc/cron.d/puppetcheck.cron": > + #content => template("puppet/puppetcheck.cron.erb"); > + ensure => absent; What's all this, then? ::: modules/toplevel/files/foopy.crontab @@ +1,4 @@ > +# m h dom mth dow user > +*/5 * * * * cltbld /builds/tegra_stats.sh > /builds/tegra_stats.log 2>&1 > +42 * * * * cltbld /builds/check.sh -c > /builds/check.log 2>&1 > +24 * * * * cltbld /builds/check.sh -r > /builds/check2.log 2>&1 'cltbld' here should be parameterized ::: modules/toplevel/manifests/server.pp @@ +5,5 @@ > include puppet::periodic > include ntp::daemon > include smarthost > include disableservices::server > + include packages::screen screen should probably be included in the same place as users::builder, which I note is *not* in toplevel::server.

(In reply to Dustin J. Mitchell [:dustin] from comment #11) > Comment on attachment 635223 [details] [diff] [review] > [puppet] WIP 0 > ::: modules/packages/manifests/editors.pp > Is this just a whitespace change? Yes, fixed. > ::: modules/puppet/manifests/periodic.pp > @@ -11,1 @@ > > } > > +1 to this! > > Can you add the reverse and have ::atboot remove the cron? This was actually just for local testing simplicity, I need to back out this change for real landing, if you want a reverse/magic similar to this done, I'd like to spin that to another bug. > ::: modules/python/manifests/virtualenv/prerequisites.pp > @@ -6,5 @@ > > "virtualenv.py": ; > > > > # these two need to be in the same dir as virtualenv.py, or it will > > # want to download them from pypi > > "pip-0.8.2.tar.gz": ; > > This will be taken care of in bug 669977 > > ::: modules/toplevel/manifests/foopy.pp > @@ +15,5 @@ > > + "bind-utils": > > + ensure => latest; > > + "rsync": > > + ensure => latest; > > + } > > (sorry if this is too much for a fb?) > > these should be packages::bind-utils (or maybe a better name if there's one > particular utility you're using?) and packages::rsync. Done as nslookup and rsync respectively. > @@ +32,5 @@ > > + owner => $config::builder_username, > > + group => $config::builder_username, > > + ensure => directory, > > + mode => 0755; > > + } > > This should all require/include dirs::builds, and it may be worth adding > some of these to the dirs package (I'll leave that to your discretion) They already do, automatically by puppet. I don't think adding them to the dirs package is as helpful. > @@ +50,5 @@ > > + ], > > + creates => "/builds/talos-data/talos-repo/.hg", > > + command => "/tools/python27-mercurial/bin/hg clone -u $frozen_talos_rev http://hg.mozilla.org/build/talos /builds/talos-data/talos-repo", > > + user => $config::builder_username; > > + } > > Would it make sense to create a separate module/class to do hg checkouts? > It'd be nice if it could ensure a particular revision, so that you can > change $frozen_talos_rev, it automatically updates? This is probably a > common need (slavealloc will need it, too). I like the concept, but could you concede its a followup, nicety? This is explicitly done as a frozen-rev because talos on foopies _needs_ this rev for devicemanager, and won't be changing anytime soon without a basic rework of how this is setup here. > @@ +78,5 @@ > > + "/builds/sut_tools/devicemanagerADB.py": > > + owner => $config::builder_username, > > + group => $config::builder_username, > > + ensure => link, > > + target => "/builds/talos-data/talos-repo/talos/devicemanagerADB.py"; > > This would probably be clearer if it were broken out into a separate define > that you invoke repeatedly: For how picky the foopy setup is (currently), and how unlikely another thing is to use it, I'm perfectly happy keeping it confined to one space (here), and the comments I added should help alleviate confusion, I suspect. But if you insist I can do a modules/foopy/manifests/sut_tools_link etc. and abstract much of this stuff to that module. > whatever_module::sut_tools_link { > "devicemanagerADB.py": > target => "/builds/talos-data/talos-repo/talos/devicemanagerADB.py"; > } > > @@ +98,5 @@ > > + "/builds/tegra_stats.log": > > + owner => $config::builder_username, > > + group => $config::builder_username, > > + ensure => file, > > + mode => 0644; > > These are all identical, and are essentially just "touch", so could probably > be done in one stanza with [ .. ] Done > @@ +130,5 @@ > > + > > + # apply tweaks > > + include tweaks::dev-ptmx > > +} > > + > > Overall, this is a lot of specific stuff to put in a toplevel::xxx class. > I'd rather see toplevel::foopy include the 'foopy' module (which could > include others). If you insist, I'll do so. > ::: modules/users/files/screenrc > @@ +111,5 @@ > > +#bindkey -k k8 select 7 # F8 = screen 7 > > +#bindkey -k F2 next # F12 = next > > +#bindkey -k F1 prev # F11 = previous > > +######################### > > + > > A lot of this looks user-specific, unless mutt is part of the foopy process > :/ We have user-specific configs tied to cltbld for vim as well, and this screenrc is what we already have present on foopies (mac) which actually helps aleviate confusion in my experience, and I figured its better to have a similar screenrc between mac foopies and linux foopies than to have a confusing difference. I don't know all the screenrc commands/args so I copied the mac one, and everything works fine for me in testing. If you want this changed, can you suggest a way/how/whatever to specifically do so :-) (In reply to Dustin J. Mitchell [:dustin] from comment #15) > Comment on attachment 640120 [details] [diff] [review] > [puppet] v0.9 > > Review of attachment 640120 [details] [diff] [review]: > ----------------------------------------------------------------- > > I've not repeated notes from comment 11 here: > * screenrc is probably unnecessarily user-specific > * toplevel::server::foopy should refer to a foopy module, not do stuff > itself > * hg checkout class > * require => Class['dirs::builds'] ** All addressed above, in my own thoughts, none of which will be changed unless you insist :-) > Also, given that you're "tossing" this, and that it doesn't look much > changed from the last patch, I'll limit myself to just these comments. > > ::: modules/puppet/manifests/periodic.pp > @@ +7,5 @@ > > # Mozilla IT. There is enough splay here to avoid killing the master > > # (configured in puppet.conf) > > "/etc/cron.d/puppetcheck.cron": > > + #content => template("puppet/puppetcheck.cron.erb"); > > + ensure => absent; > > What's all this, then? This was me, just disabling it locally (to avoid having puppet run, generating errors/email, if I have to go with my user repo in an error state -- I find it easier to manually trigger and watch output when deving than having a cron potentially do it for me, and me miss output) > ::: modules/toplevel/files/foopy.crontab > @@ +1,4 @@ > > +# m h dom mth dow user > > +*/5 * * * * cltbld /builds/tegra_stats.sh > /builds/tegra_stats.log 2>&1 > > +42 * * * * cltbld /builds/check.sh -c > /builds/check.log 2>&1 > > +24 * * * * cltbld /builds/check.sh -r > /builds/check2.log 2>&1 > > 'cltbld' here should be parameterized Good catch, will do. > ::: modules/toplevel/manifests/server.pp > @@ +5,5 @@ > > include puppet::periodic > > include ntp::daemon > > include smarthost > > include disableservices::server > > + include packages::screen > > screen should probably be included in the same place as users::builder, > which I note is *not* in toplevel::server. IMO, we want screen on all servers (or at maybe toplevel::base) even if we don't have a .screenrc like what I added for the builder user, and screenrc present for builder, even on builders that don't need it, won't hurt. Are you ok with moving the package to toplevel::base ?

So as far as insisting, a foopy module, rather than lots of concrete resources in a toplevel class, is important. The rest I can live with. I had a closer look at the .screenrc, and realized that all of the "weird" stuff is commented out. So if you can edit that down to remove the outcommented stuff, that'd be great. And it's probably easiest to ensure that the screenrc is installed in exactly the places screen is installed is to include packages::screen *from* users::builder.

This should do it for the linux foopy, I think I addressed everything you needed. I did not run this through the dot dependency verifier, but we can fix up outstanding issues there, in the future.

Comment on attachment 641321 [details] [diff] [review] [puppet] v1.0 Review of attachment 641321 [details] [diff] [review]: ----------------------------------------------------------------- Yay!

http://hg.mozilla.org/build/puppet/rev/debe5ef4f173 Still need to setup/install ganglia (which I'll do in a seperate bug) and begin actually testing on this VM and might have minor tweaks to do in other bugs for potential dependancy issues. But we're a gooood way of the way there.

This patch uses a new hostutils binary, the Darwin one is a symlink to the existing, Linux is a new one, and the build property is set from the new step I'm adding in the buildbotcustom patch

This patch adds the Buildbot property for if the foopy is linux or mac (to support different Hostutils) It also changes the python path to what is on the linux foopies, (we manually created symlinks on the mac foopies for this, and it does have py2.6 on mac) This does *not* change the breakpad dump-syms path to be for linux though, primarily because the only crash here that would help us is xpcshell which is being rewritten for a python-based http server instead (Fennec crashes won't go through this) We did discuss symlinking the "Linux" breakpad symbols thing to the mac location on the mac foopies, but after evaluating that option it was not feasable, since we do rm -rf of the tools checkout, and then re-check it out. There are ways around that but imo it is not worth doing it in a hacky way here.

Comment on attachment 650811 [details] [diff] [review] [buildbotcustom] v1 Review of attachment 650811 [details] [diff] [review]: ----------------------------------------------------------------- ::: process/factory.py @@ +5518,4 @@ > return "C:\\mozilla-build\\python25\\python.exe" > elif (platform.find("android") > -1): > # path in the foopies > + return "/usr/local/bin/python2.6" Hrmmm, I'm not a big fan of tying us to an older version of python. At least it's not 2.5. Is there a reason we need to be explicit here?

http://hg.mozilla.org/build/buildbot-configs/rev/bcc110581f8a (In reply to Chris Cooper [:coop] from comment #23) > Comment on attachment 650811 [details] [diff] [review] > [buildbotcustom] v1 > > + return "/usr/local/bin/python2.6" > > Hrmmm, I'm not a big fan of tying us to an older version of python. At least > it's not 2.5. > > Is there a reason we need to be explicit here? Just that I'd rather not have a mismatch of py versions, we already have 2.6 installed on the mac Foopies, and |/usr/local/bin/python| is not created by the moz-rpm on the linux one, so I figured I'd symlink to the known-good, latest-version-everywhere. Once we drop mac foopies, we can give a shot at the linux python2.7

The foopy module still needs docs. I can open a new bug for that if you'd prefer.

In production since yesterday.

reopen for docs and new patch to finish all this up.

Final touchups here

Comment on attachment 652310 [details] [diff] [review] [puppet] Looks good, except why the include of disableservices::slave on a non-slave? Are there things we should move to disableservices? Or maybe include from both *::slave and *::server? r+ with adding a comment for the reason to modules/foopy/manifests/init.pp, but if there's a way to straighten that out so it doesn't require explanation, that'd be better.

This adjusts disableservices as discussed on IRC.

Comment on attachment 652472 [details] [diff] [review] [puppet] Part 2 v1.1 whops didn't qref after my last change

http://hg.mozilla.org/build/puppet/rev/6eda6e0cef80 We should be done *here* now.