Chris Cooper [:coop] (he/him) Reporter
	Description • 8 years ago

The instructions for setting up a new personal development master are stale and probably incomplete:

https://wiki.mozilla.org/ReleaseEngineering/How_To/Setup_Personal_Development_Master

It references a perl script that performs some of the setup tasks for you, but still leaves other undone.

We should have a new, unified script written in python that can take a few simple command-line variables and does the whole thing:

* sets up venv along with any module reqs
* clones the required repos
* sets up queue dirs
* stubs out files you'll need to update like passwords.py and BuildSlaves.py (we use standard passwords in staging, so it could probably just fill them in)
* runs 'make checkconfig' for you to see if it all works
* sets up the environment to run pre-landing checks: same tox tests from travis, test-masters.sh, builder lists, etc.

	Iris Hsiao [:ihsiao]
	Comment 1 • 8 years ago

Please help to give us some feedback for the script, Thanks.

	Kim Moir [:kmoir] ET
	Comment 2 • 8 years ago

Comment on attachment 8759006 [details] [diff] [review]
bug1275135.patch

You could have the config files as templates that are consumed instead of including them in the script itself (
build_config_json and test_config_json)

You can use os.mkdir instead of a exec_cmd('mkdir etc

use the built in libraries to copy and delete files ie. shutil.copy, os.remove, os.rmdir


you might want to separate main out into smaller functions just call them from main instead

	Chris Cooper [:coop] (he/him) Reporter
	Comment 3 • 8 years ago

Comment on attachment 8759006 [details] [diff] [review]
bug1275135.patch

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

(In reply to Kim Moir [:kmoir] from comment #2) 
> you might want to separate main out into smaller functions just call them
> from main instead

I agree. I like my __main__ to read like a list of steps rather than details. After you handle the command-line input, just call a series of functions to handle each step, e.g.:

create master_dir()
write_master_config()
create_pulse_queue_dir()
copy_credential_files()
create_symlinks()
run_checkconfig()
run_tox_tests()
start_master()
stop_master()

Note: this may not be the complete or the correct functional breakdown. I'm just skimming your logic. Pass whatever params you'd need.

One of the benefits of this approach is that it's easier to see where there are opportunities for abstraction.

So, overall, a good first pass. What I'm mostly looking for here are some idiomatic python changes, and encapsulating the logical blocks into named functions.

::: buildbot-related/create-staging-master.py
@@ +7,5 @@
> +import logging
> +import fileinput
> +import time
> +
> +build_config_json = """

I agree with Kim that these should be separate template json files that could be read in.

@@ +88,5 @@
> +        logging.error(e)
> +        sys.exit(0)
> +
> +if __name__ == '__main__':
> +    logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')

I like the logging!

It common to create a named logger to make logging functions easier to call (and keep named logs separate for libs), e.g.:

import logging
log = logging.getLogger(__name__)
logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')

If you add a verbose flag, you can then change the logging level easily, e.g.:

if args.verbose:
    log.setLevel(logging.DEBUG)
else:
    log.setLevel(logging.INFO)

It's then easy to adjust the amount of logging with each invocation. This is especially useful if you want to add debug messages that will only appear when the flag is set: e.g.

log.debug("Debug message goes are only shown when -v is set.")
log.info("Info messages appear all the time."

@@ +100,5 @@
> +    parser.add_argument('--basedir', nargs='?', default='/builds/buildbot',
> +                    help='where to create master')
> +    parser.add_argument('--masterdir', nargs='?', default='',
> +                    help='master folder')
> +    parser.add_argument('--httpport', nargs='?', default=8044, type=int,

I don't think we should specify a default here, and I'm not just saying that because the default is the port I use. ;) This should be a required arg.

@@ +123,5 @@
> +    username = args.username
> +    base_dir = args.basedir
> +    http_port = args.httpport
> +    pb_port = http_port + 1000
> +    ssh_port = http_port - 1000

These port calculations need some bounds checking to make sure they're valid, i.e. 1024<port<65535

@@ +138,5 @@
> +        logging.error('Directory ' + out_dir + ' already exists. Specify a different master-dir?')
> +        sys.exit(0)
> +    
> +    logging.info('mkdir ' + out_dir)
> +    exec_cmd('mkdir %s' % out_dir)

As Kim mentioned, you should try to avoid invoking a subprocess for operations the os library can do (in this case, os.mkdir).

	Iris Hsiao [:ihsiao]
	Comment 4 • 8 years ago

Hi Coop and Kim,
I have modified the script based on your comments, please help to review it, and let me know if any problems that I should fix. 
Thank you.

	Kim Moir [:kmoir] ET
	Comment 5 • 8 years ago

Hi Iris

Great improvements!  Did everything work out okay when you tested it to create a new dev master for both test and build masters?

where you are creating this line here
filename = base_dir + '/' + username + '/' + master_dir + '/master/master.cfg'

you could use os.pathsep instead of "/"

import os

I don't know if 
def replace_all(text, dic):
+    for i, j in dic.iteritems():
+        text = text.replace(i, j)
+    return text
+
+def write_file(config):
+    f = open('config.json', 'w')
+    f.write(config)
+    f.close()

are needed to be split out as functions since they are just called once and are just a few lines of code. But that's really a personal preference.

	Kim Moir [:kmoir] ET
	Comment 6 • 8 years ago

Comment on attachment 8810772 [details] [diff] [review]
create-staging-master.patch

removing since I commented on the bug, please ask for review again

	Kim Moir [:kmoir] ET
	Comment 7 • 8 years ago

in my above comment is said to use os.pathsep, it should have been os.path.join()

sorry for the confusion

	Chris Cooper [:coop] (he/him) Reporter
	Comment 8 • 8 years ago

Comment on attachment 8810772 [details] [diff] [review]
create-staging-master.patch

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

Looks like good progress. There is some errant whitespace sprinkled throughout that should be removed.

What I'd like to see in addition to the code is the log output of the code actually running and successfully creating a new master.

::: buildbot-related/create-staging-master.py
@@ +125,5 @@
> +    logger.debug('start buildbot master')
> +    proc = subprocess.Popen('make start', stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
> +    proc.communicate()
> +    logger.info('wait twistd.log %s seconds to write start logs...' % (makestart_wait))
> +    time.sleep(makestart_wait)

Hrmmm, what happens if we blow through the timeout and we're still not done? We should be looping here on a clear exit criteria with an overall timeout. This applies to any where you're using time.sleep().

@@ +139,5 @@
> +    logger.debug('stop buildbot master')
> +    proc = subprocess.Popen('make stop', stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
> +    proc.communicate()
> +    logger.info('wait twistd.log %s seconds to write stop logs...' % (makestop_wait))
> +    time.sleep(makestop_wait)

See previous note about time.sleep()

@@ +200,5 @@
> +    args = parser.parse_args()
> +    
> +    if args.masterkind == 'build':
> +        master_dir = args.masterdir or 'build1'
> +        config_json = open('build_config.json').read()

I would prefer these files were named in a way that makes it clear they are templates, e.g. build_config.json.template.

@@ +205,5 @@
> +        makestart_wait = 5
> +        makestop_wait = 2
> +    elif args.masterkind == 'test':
> +        master_dir = args.masterdir or 'test1'
> +        config_json = open('test_config.json').read()

I would prefer these files were named in a way that makes it clear they are templates, e.g. test_config.json.template.

@@ +237,5 @@
> +    rst_srt = 'configuration update complete'
> +    rst_stop = 'Server Shut Down'
> +    braindump_dir = base_dir + '/' + username + '/braindump'
> +    
> +    ##create_master_dir()##

These comments don't really add anything. Your functions are clearly named, so I would prefer to see proper docstrings in the functions themselves.

	Iris Hsiao [:ihsiao]
	Comment 9 • 7 years ago

Hi Coop and Kim,
I have updated the script, please help to review and give some advice.
Thank you for your time.

	Iris Hsiao [:ihsiao]
	Comment 10 • 7 years ago

Kim,
> Did everything work out okay when you tested it to create a new dev master for both test and build masters?
I have tested the script on dev-master2 for both test and build master, the results are working fine. But I'm not sure if anything I missed, do you have any advice for testing the script?

	Chris Cooper [:coop] (he/him) Reporter
	Comment 11 • 7 years ago

Comment on attachment 8820129 [details] [diff] [review]
create-staging-master-v3.patch

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

I think it should be ready to land if you change the function header comments into proper docstrings. Please make those changes and then submit for final review.

::: buildbot-related/create-staging-master.py
@@ +50,5 @@
> +    logger.debug(cmd)
> +    exec_cmd(cmd)
> +
> +def create_queue_dir(queue_dir, base_dir, master_dir, username):
> +    ##setup user's queue folder under /dev/shm/<name>

Please change these function comments to docstrings.

@@ +80,5 @@
> +    ##or bhearsum's version is generally up-to-date and available at:
> +    ##/builds/buildbot/bhearsum/build1/master/passwords.py on dev-master1
> +    ##And download the BuildSlaves.py template files from puppet:
> +    ##puppet/modules/buildmaster/templates
> +    ##there are 3 templates, depending on the type of slave that is going to connect to that master (build, test, try)

docstring

@@ +99,5 @@
> +    else:
> +        logger.warning('NOTE: You may need to populate master/BuildSlaves.py so the download_token step does not fail.')
> +
> +def create_symlinks(args, base_dir, username, master_dir, out_dir):
> +    ##link master.cfg for production schedulers to run

docstring

@@ +115,5 @@
> +        logger.error('Invalid --masterkind ' + args.masterkind)
> +        sys.exit(0)
> +
> +def check_config(out_dir):
> +    ##make sure the default configs are in place and good

docstring

@@ +128,5 @@
> +        logger.error(stderr)
> +
> +def test_buildbot(retry_times, retry_wait, rst_srt, rst_stop, out_dir):
> +    ##make sure you master starts
> +    ##start buildbot master, check the process and logs

docstring

@@ +230,5 @@
> +    logger.debug('Retry: %s' % (retries))
> +    return retries, flag
> +
> +def test_master(braindump_dir, base_dir, out_dir, username):
> +    ##list builds/tests

docstring

	Iris Hsiao [:ihsiao]
	Comment 12 • 7 years ago

Hi Coop and Kim,
I have changed the function header comments into docstring, please help to review the script, thanks for your time.

	Iris Hsiao [:ihsiao]
	Comment 13 • 7 years ago

https://hg.mozilla.org/build/braindump/rev/4c7d9e96b2bd