From b9992195e0d0cb90324a2eb74f8737edebf5b0ea Mon Sep 17 00:00:00 2001 From: Aneesh Agrawal Date: Wed, 6 Apr 2016 16:41:32 -0400 Subject: [PATCH 1/3] Refactor buildbot states Split the master and slave states into separate directories, use the standard files/ directory, and use more Jinja templating. --- buildbot/buildbot-master.conf | 10 --------- buildbot/buildbot-slave.conf | 10 --------- .../files}/buildbot-github-listener.conf | 4 ++-- buildbot/master/files/buildbot-master.conf | 10 +++++++++ .../master/{ => files/config}/buildbot.tac | 2 +- buildbot/master/{ => files/config}/master.cfg | 4 ++-- .../master/{ => files/config}/passwords.py | 0 .../config}/public_html/bg_gradient.jpg | Bin .../config}/public_html/default.css | 0 .../config}/public_html/favicon.ico | Bin .../{ => files/config}/public_html/robots.txt | 0 .../{ => files/config}/templates/README.txt | 0 .../{ => master/files}/github_buildbot.py | 0 buildbot/{master.sls => master/init.sls} | 21 +++++++++++------- buildbot/slave/files/buildbot-slave.conf | 10 +++++++++ .../slave/{ => files/config}/buildbot.tac | 6 +---- buildbot/slave/{ => files/config}/info/admin | 0 buildbot/slave/{ => files/config}/info/host | 0 .../files}/net.buildbot.buildslave.plist | 0 buildbot/{slave.sls => slave/init.sls} | 14 +++++++----- 20 files changed, 48 insertions(+), 43 deletions(-) delete mode 100644 buildbot/buildbot-master.conf delete mode 100644 buildbot/buildbot-slave.conf rename buildbot/{ => master/files}/buildbot-github-listener.conf (74%) create mode 100644 buildbot/master/files/buildbot-master.conf rename buildbot/master/{ => files/config}/buildbot.tac (95%) rename buildbot/master/{ => files/config}/master.cfg (99%) rename buildbot/master/{ => files/config}/passwords.py (100%) rename buildbot/master/{ => files/config}/public_html/bg_gradient.jpg (100%) rename buildbot/master/{ => files/config}/public_html/default.css (100%) rename buildbot/master/{ => files/config}/public_html/favicon.ico (100%) rename buildbot/master/{ => files/config}/public_html/robots.txt (100%) rename buildbot/master/{ => files/config}/templates/README.txt (100%) rename buildbot/{ => master/files}/github_buildbot.py (100%) rename buildbot/{master.sls => master/init.sls} (68%) create mode 100644 buildbot/slave/files/buildbot-slave.conf rename buildbot/slave/{ => files/config}/buildbot.tac (90%) rename buildbot/slave/{ => files/config}/info/admin (100%) rename buildbot/slave/{ => files/config}/info/host (100%) rename buildbot/{ => slave/files}/net.buildbot.buildslave.plist (100%) rename buildbot/{slave.sls => slave/init.sls} (78%) diff --git a/buildbot/buildbot-master.conf b/buildbot/buildbot-master.conf deleted file mode 100644 index 09d06837b..000000000 --- a/buildbot/buildbot-master.conf +++ /dev/null @@ -1,10 +0,0 @@ -exec /usr/local/bin/buildbot start --nodaemon /home/servo/buildbot/master - -setuid servo -setgid servo - -start on (local-filesystems and net-device-up IFACE!=lo) -stop on runlevel [016] - -env HOME=/home/servo - diff --git a/buildbot/buildbot-slave.conf b/buildbot/buildbot-slave.conf deleted file mode 100644 index 121da7474..000000000 --- a/buildbot/buildbot-slave.conf +++ /dev/null @@ -1,10 +0,0 @@ -exec /usr/local/bin/buildslave start --nodaemon /home/servo/buildbot/slave - -setuid servo -setgid servo - -start on (local-filesystems and net-device-up IFACE!=lo) -stop on runlevel [016] - -env HOME=/home/servo - diff --git a/buildbot/buildbot-github-listener.conf b/buildbot/master/files/buildbot-github-listener.conf similarity index 74% rename from buildbot/buildbot-github-listener.conf rename to buildbot/master/files/buildbot-github-listener.conf index 1f190d7bc..7d35d4546 100644 --- a/buildbot/buildbot-github-listener.conf +++ b/buildbot/master/files/buildbot-github-listener.conf @@ -1,4 +1,4 @@ -exec /usr/local/bin/github_buildbot.py -p 9010 -m localhost:9001 --auth=change:{{ pillar['buildbot']['credentials']['change-pass'] }} --secret={{ pillar['buildbot']['credentials']['gh-hook-secret'] }} -l /home/servo/buildbot/master/github-listener.log +exec /usr/local/bin/github_buildbot.py -p 9010 -m localhost:9001 --auth=change:{{ pillar['buildbot']['credentials']['change-pass'] }} --secret={{ pillar['buildbot']['credentials']['gh-hook-secret'] }} -l {{ common.servo_home }}/buildbot/master/github-listener.log setuid servo setgid servo @@ -6,5 +6,5 @@ setgid servo start on (local-filesystems and net-device-up IFACE!=lo) stop on runlevel [016] -env HOME=/home/servo +env HOME={{ common.servo_home }} diff --git a/buildbot/master/files/buildbot-master.conf b/buildbot/master/files/buildbot-master.conf new file mode 100644 index 000000000..414407c89 --- /dev/null +++ b/buildbot/master/files/buildbot-master.conf @@ -0,0 +1,10 @@ +exec /usr/local/bin/buildbot start --nodaemon {{ common.servo_home }}/buildbot/master + +setuid servo +setgid servo + +start on (local-filesystems and net-device-up IFACE!=lo) +stop on runlevel [016] + +env HOME={{ common.servo_home }} + diff --git a/buildbot/master/buildbot.tac b/buildbot/master/files/config/buildbot.tac similarity index 95% rename from buildbot/master/buildbot.tac rename to buildbot/master/files/config/buildbot.tac index 88c75d7a3..7980f36d3 100644 --- a/buildbot/master/buildbot.tac +++ b/buildbot/master/files/config/buildbot.tac @@ -3,7 +3,7 @@ import os from twisted.application import service from buildbot.master import BuildMaster -basedir = '/home/servo/buildbot/master' +basedir = '{{ common.servo_home }}/buildbot/master' rotateLength = 10000000 maxRotatedFiles = 10 configfile = 'master.cfg' diff --git a/buildbot/master/master.cfg b/buildbot/master/files/config/master.cfg similarity index 99% rename from buildbot/master/master.cfg rename to buildbot/master/files/config/master.cfg index 6fd2c07cd..60047a187 100644 --- a/buildbot/master/master.cfg +++ b/buildbot/master/files/config/master.cfg @@ -97,9 +97,9 @@ linux_test_env = dict({ linux_headless_env = dict({'SERVO_HEADLESS': '1'}, **linux_test_env) mac_test_env = dict({ - 'CARGO_HOME': '/Users/servo/.cargo', + 'CARGO_HOME': '{{ common.servo_home }}/.cargo', 'CCACHE': '/usr/local/bin/ccache', - 'SERVO_CACHE_DIR': '/Users/servo/.servo' + 'SERVO_CACHE_DIR': '{{ common.servo_home }}/.servo' }, **common_test_env) linux_dev_factory = create_servo_factory([ diff --git a/buildbot/master/passwords.py b/buildbot/master/files/config/passwords.py similarity index 100% rename from buildbot/master/passwords.py rename to buildbot/master/files/config/passwords.py diff --git a/buildbot/master/public_html/bg_gradient.jpg b/buildbot/master/files/config/public_html/bg_gradient.jpg similarity index 100% rename from buildbot/master/public_html/bg_gradient.jpg rename to buildbot/master/files/config/public_html/bg_gradient.jpg diff --git a/buildbot/master/public_html/default.css b/buildbot/master/files/config/public_html/default.css similarity index 100% rename from buildbot/master/public_html/default.css rename to buildbot/master/files/config/public_html/default.css diff --git a/buildbot/master/public_html/favicon.ico b/buildbot/master/files/config/public_html/favicon.ico similarity index 100% rename from buildbot/master/public_html/favicon.ico rename to buildbot/master/files/config/public_html/favicon.ico diff --git a/buildbot/master/public_html/robots.txt b/buildbot/master/files/config/public_html/robots.txt similarity index 100% rename from buildbot/master/public_html/robots.txt rename to buildbot/master/files/config/public_html/robots.txt diff --git a/buildbot/master/templates/README.txt b/buildbot/master/files/config/templates/README.txt similarity index 100% rename from buildbot/master/templates/README.txt rename to buildbot/master/files/config/templates/README.txt diff --git a/buildbot/github_buildbot.py b/buildbot/master/files/github_buildbot.py similarity index 100% rename from buildbot/github_buildbot.py rename to buildbot/master/files/github_buildbot.py diff --git a/buildbot/master.sls b/buildbot/master/init.sls similarity index 68% rename from buildbot/master.sls rename to buildbot/master/init.sls index d17e627f8..4bb0c1b40 100644 --- a/buildbot/master.sls +++ b/buildbot/master/init.sls @@ -13,12 +13,12 @@ buildbot-master: - enable: True - watch: - pip: buildbot-master - - file: /home/servo/buildbot/master + - file: {{ common.servo_home }}/buildbot/master - file: /etc/init/buildbot-master.conf -/home/servo/buildbot/master: +{{ common.servo_home }}/buildbot/master: file.recurse: - - source: salt://buildbot/master + - source: salt://{{ tpldir }}/files/config - user: servo - group: servo - dir_mode: 755 @@ -29,25 +29,30 @@ buildbot-master: /etc/init/buildbot-master.conf: file.managed: - - source: salt://buildbot/buildbot-master.conf + - source: salt://{{ tpldir }}/files/buildbot-master.conf - user: root - group: root - mode: 644 + - template: jinja + - context: + common: {{ common }} /usr/local/bin/github_buildbot.py: file.managed: - - source: salt://buildbot/github_buildbot.py + - source: salt://{{ tpldir }}/files/github_buildbot.py - user: root - group: root - mode: 755 /etc/init/buildbot-github-listener.conf: file.managed: - - source: salt://buildbot/buildbot-github-listener.conf - - template: jinja + - source: salt://{{ tpldir }}/files/buildbot-github-listener.conf - user: root - group: root - mode: 644 + - template: jinja + - context: + common: {{ common }} buildbot-github-listener: service.running: @@ -58,7 +63,7 @@ buildbot-github-listener: remove-old-build-logs: cron.present: - - name: 'find /home/servo/buildbot/master/*/*.bz2 -mtime +5 -delete' + - name: 'find {{ common.servo_home }}/buildbot/master/*/*.bz2 -mtime +5 -delete' - user: root - minute: 1 - hour: 0 diff --git a/buildbot/slave/files/buildbot-slave.conf b/buildbot/slave/files/buildbot-slave.conf new file mode 100644 index 000000000..6da026670 --- /dev/null +++ b/buildbot/slave/files/buildbot-slave.conf @@ -0,0 +1,10 @@ +exec /usr/local/bin/buildslave start --nodaemon {{ common.servo_home }}/buildbot/slave + +setuid servo +setgid servo + +start on (local-filesystems and net-device-up IFACE!=lo) +stop on runlevel [016] + +env HOME={{ common.servo_home }} + diff --git a/buildbot/slave/buildbot.tac b/buildbot/slave/files/config/buildbot.tac similarity index 90% rename from buildbot/slave/buildbot.tac rename to buildbot/slave/files/config/buildbot.tac index f72d59890..26f446afe 100644 --- a/buildbot/slave/buildbot.tac +++ b/buildbot/slave/files/config/buildbot.tac @@ -4,11 +4,7 @@ import os from buildslave.bot import BuildSlave from twisted.application import service -{% if grains["kernel"] != "Darwin" %} -basedir = '/home/servo/buildbot/slave' -{% else %} -basedir = '/Users/servo/buildbot/slave' -{% endif %} +basedir = '{{ common.servo_home }}/buildbot/slave' rotateLength = 10000000 maxRotatedFiles = 10 diff --git a/buildbot/slave/info/admin b/buildbot/slave/files/config/info/admin similarity index 100% rename from buildbot/slave/info/admin rename to buildbot/slave/files/config/info/admin diff --git a/buildbot/slave/info/host b/buildbot/slave/files/config/info/host similarity index 100% rename from buildbot/slave/info/host rename to buildbot/slave/files/config/info/host diff --git a/buildbot/net.buildbot.buildslave.plist b/buildbot/slave/files/net.buildbot.buildslave.plist similarity index 100% rename from buildbot/net.buildbot.buildslave.plist rename to buildbot/slave/files/net.buildbot.buildslave.plist diff --git a/buildbot/slave.sls b/buildbot/slave/init.sls similarity index 78% rename from buildbot/slave.sls rename to buildbot/slave/init.sls index 54a24a127..03575f0c6 100644 --- a/buildbot/slave.sls +++ b/buildbot/slave/init.sls @@ -9,8 +9,7 @@ buildbot-slave-dependencies: {{ common.servo_home }}/buildbot/slave: file.recurse: - - source: salt://buildbot/slave - - template: jinja + - source: salt://{{ tpldir }}/files/config - user: servo {% if grains['kernel'] == 'Darwin' %} - group: staff @@ -19,6 +18,9 @@ buildbot-slave-dependencies: {% endif %} - dir_mode: 755 - file_mode: 644 + - template: jinja + - context: + common: {{ common }} {% if grains['kernel'] != 'Darwin' %} - watch_in: - service: buildbot-slave @@ -28,7 +30,7 @@ buildbot-slave-dependencies: /Library/LaunchDaemons/net.buildbot.buildslave.plist: file.managed: - - source: salt://buildbot/net.buildbot.buildslave.plist + - source: salt://{{ tpldir }}/files/net.buildbot.buildslave.plist - user: root - group: wheel - mode: 644 @@ -43,10 +45,13 @@ launchctl load -w /Library/LaunchDaemons/net.buildbot.buildslave.plist: /etc/init/buildbot-slave.conf: file.managed: - - source: salt://buildbot/buildbot-slave.conf + - source: salt://{{ tpldir }}/files/buildbot-slave.conf - user: root - group: root - mode: 644 + - template: jinja + - context: + common: {{ common }} - watch_in: - service: buildbot-slave @@ -55,4 +60,3 @@ buildbot-slave: - enable: True {% endif %} - From c0221d171ca9b2bf7ca652cbf14067205232971e Mon Sep 17 00:00:00 2001 From: Aneesh Agrawal Date: Wed, 6 Apr 2016 19:25:25 -0400 Subject: [PATCH 2/3] Don't autorestart Buildbot master Automatically restarting Buildbot when there are outstanding builds causes them to get lost and is generally a bad idea. Until we have a more robust way to restart the Buildbot master on configuration change, disable the auto-restart behavior. --- buildbot/master/init.sls | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/buildbot/master/init.sls b/buildbot/master/init.sls index 4bb0c1b40..fe8010f41 100644 --- a/buildbot/master/init.sls +++ b/buildbot/master/init.sls @@ -11,7 +11,9 @@ buildbot-master: - pkg: pip service.running: - enable: True - - watch: + # Buildbot must be restarted manually! See 'Buildbot administration' on the + # wiki and https://github.com/servo/saltfs/issues/304. + - require: - pip: buildbot-master - file: {{ common.servo_home }}/buildbot/master - file: /etc/init/buildbot-master.conf From d5f50b076b835ff94ba2539baa49f7dfe1980ea6 Mon Sep 17 00:00:00 2001 From: Aneesh Agrawal Date: Wed, 6 Apr 2016 21:00:40 -0400 Subject: [PATCH 3/3] Add test to check Buildbot master config This test can be run via `./test.py sls.buildbot.master`. It will only work properly once the Buildbot master states have been run, and is meant to be run either in Vagrant or on Travis, not locally. Running `./test.py` without arguments will only run the linting tests, since those can be run anywhere with just the source code. For now, hard-code this test to be run on the servo-master1 node on Travis. This should be replaced with a more intelligent test dispatcher in `test.py` which uses the top file via Salt's state.show_top in the future, but hard coding this for now will reduce Buildbot's fragility. --- .travis.yml | 2 + .travis/dispatch.sh | 5 ++ test.py | 65 +++++++++++++++---------- tests/lint/__init__.py | 0 tests/{ => lint}/shebang.py | 2 +- tests/{ => lint}/trailing_whitespace.py | 2 +- tests/sls/__init__.py | 0 tests/sls/buildbot/__init__.py | 0 tests/sls/buildbot/master/__init__.py | 0 tests/sls/buildbot/master/config.py | 17 +++++++ 10 files changed, 65 insertions(+), 28 deletions(-) create mode 100644 tests/lint/__init__.py rename tests/{ => lint}/shebang.py (93%) rename tests/{ => lint}/trailing_whitespace.py (94%) create mode 100644 tests/sls/__init__.py create mode 100644 tests/sls/buildbot/__init__.py create mode 100644 tests/sls/buildbot/master/__init__.py create mode 100644 tests/sls/buildbot/master/config.py diff --git a/.travis.yml b/.travis.yml index 228278914..a15db459a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,6 +20,8 @@ matrix: os: linux sudo: required dist: trusty + language: python + python: 3.5 - env: SALT_NODE_ID=test # Not a Salt node, runs test suite instead os: linux sudo: required diff --git a/.travis/dispatch.sh b/.travis/dispatch.sh index 684c48ca8..164b0ad60 100755 --- a/.travis/dispatch.sh +++ b/.travis/dispatch.sh @@ -20,4 +20,9 @@ else sudo salt-call --id="${SALT_NODE_ID}" --retcode-passthrough state.show_highstate # Full on installation test sudo salt-call --id="${SALT_NODE_ID}" --retcode-passthrough --log-level=warning state.highstate + + # TODO: don't hard-code this + if [ "${SALT_NODE_ID}" = "servo-master1" ]; then + ./test.py sls.buildbot.master + fi fi diff --git a/test.py b/test.py index 9af2a787f..040917a26 100755 --- a/test.py +++ b/test.py @@ -7,35 +7,48 @@ from tests.util import color, GREEN, RED, Failure, project_path -def is_python_script(path): - return path.endswith('.py') +def is_python_script(dir_entry): + return dir_entry.name.endswith('.py') and dir_entry.is_file() + + +def run_tests(tests): + any_failures = False + + for test_spec in tests: + test_dir = os.path.join(project_path(), 'tests', *test_spec.split('.')) + + python_scripts = filter(is_python_script, os.scandir(test_dir)) + tests = sorted([entry.name for entry in python_scripts]) + + for test in tests: + test_mod_name = 'tests.{}.{}'.format(test_spec, test[:-3]) + test_mod = importlib.import_module(test_mod_name) + if not hasattr(test_mod, 'run'): # Not a test script + continue + + try: + result = test_mod.run() + except Exception as e: + message = 'Test \'{}\' raised an exception:'.format(test) + result = Failure(message, str(e)) + + if result.is_success(): + print('[ {} ] {}'.format(color(GREEN, 'PASS'), result.message)) + else: + any_failures = True + print('[ {} ] {}'.format(color(RED, 'FAIL'), result.message)) + for line in result.output.splitlines(): + print(' {}'.format(line)) + + return 1 if any_failures else 0 def main(): - ANY_FAILURES = False - - test_dir = os.path.join(project_path(), 'tests') - tests = sorted(filter(is_python_script, os.listdir(test_dir))) - for test in tests: - test_mod = importlib.import_module('tests.{}'.format(test[:-3])) - if not hasattr(test_mod, 'run'): # Not a test script - continue - - try: - result = test_mod.run() - except Exception as e: - result = Failure('Test \'{}\' raised an exception:'.format(test), - str(e)) - - if result.is_success(): - print('[ {} ] {}'.format(color(GREEN, 'PASS'), result.message)) - else: - ANY_FAILURES = True - print('[ {} ] {}'.format(color(RED, 'FAIL'), result.message)) - for line in result.output.splitlines(): - print(' {}'.format(line)) - - return 1 if ANY_FAILURES else 0 + tests = ['lint'] # Only tests that are always safe and meaningful to run + if len(sys.argv) > 1: + tests = sys.argv[1:] + + return run_tests(tests) if __name__ == '__main__': sys.exit(main()) diff --git a/tests/lint/__init__.py b/tests/lint/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/shebang.py b/tests/lint/shebang.py similarity index 93% rename from tests/shebang.py rename to tests/lint/shebang.py index f79ff9b9c..6746eaa02 100644 --- a/tests/shebang.py +++ b/tests/lint/shebang.py @@ -1,7 +1,7 @@ import os import stat -from .util import display_path, paths, Failure, Success +from tests.util import display_path, paths, Failure, Success SHEBANG = """\ diff --git a/tests/trailing_whitespace.py b/tests/lint/trailing_whitespace.py similarity index 94% rename from tests/trailing_whitespace.py rename to tests/lint/trailing_whitespace.py index 105f90468..05d52e4ce 100644 --- a/tests/trailing_whitespace.py +++ b/tests/lint/trailing_whitespace.py @@ -1,7 +1,7 @@ import itertools import re -from .util import colon, color, display_path, paths, Failure, Success +from tests.util import colon, color, display_path, paths, Failure, Success def display_trailing_whitespace(whitespace): diff --git a/tests/sls/__init__.py b/tests/sls/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/sls/buildbot/__init__.py b/tests/sls/buildbot/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/sls/buildbot/master/__init__.py b/tests/sls/buildbot/master/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/sls/buildbot/master/config.py b/tests/sls/buildbot/master/config.py new file mode 100644 index 000000000..f0ba05c47 --- /dev/null +++ b/tests/sls/buildbot/master/config.py @@ -0,0 +1,17 @@ +import subprocess + +from tests.util import Failure, Success + + +def run(): + command = ['sudo', # To get access to buildbot files owned by servo + 'buildbot', 'checkconfig', '/home/servo/buildbot/master'] + ret = subprocess.run(command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True) + + if ret.returncode == 0: + return Success("Buildbot master config passed checkconfig") + else: + return Failure("Buildbot master config check failed:", ret.stderr)