Upgrading generate_yamls.py for easier configuration automation.#506
Upgrading generate_yamls.py for easier configuration automation.#506syberkitten wants to merge 4 commits intoNetflix:devfrom
Conversation
read_consitency -rc secure_server_option -sso updated readme.md
syberkitten
changed the title
syberkitten
changed the title
hashbrowncipher
left a comment
hashbrowncipher
left a comment
There was a problem hiding this comment.
Thanks for making this script better! This change overlaps a lot with my own first contribution to Dynomite, which was the cluster generation functionality in #494 and #495. We'll want to merge the two pieces of functionality eventually, and I'd love to hear any input you have on how the testing use-case from #494 is similar to or different from the automation use-case here.
If you want to, you're welcome to do the merging yourself. It would involve factoring DynoSpec out of cluster_generator.py to be used by both scripts.
|
|
||
|
|
||
| parser.add_argument( | ||
| '-cp', |
There was a problem hiding this comment.
Can we go with two dashes for multi-character arguments?
| 'dyn_port': args.peer_port, | ||
| 'dyn_listen': '0.0.0.0:' + args.peer_port, | ||
| 'datacenter': ip_dc[2], | ||
| 'rack': """{}""".format(ip_dc[1]), |
There was a problem hiding this comment.
Any reason not to do just 'rack': rack?
|
|
||
| output_path = args.output_dir | ||
|
|
||
| if os.path.isabs(dir): |
There was a problem hiding this comment.
This error message doesn't match the test condition, which checks whether the path is absolute.
| if os.path.isabs(dir): | ||
| print "path exists:{}".format(dir) | ||
| else: | ||
| output_path = os.path.join(dir, args.output_dir) |
There was a problem hiding this comment.
To me, if I specify generate_yamls.py ... -o foobar, I expect foobar to created in my current working directory. It would be surprising to me if it were instead put next to the script itself.
| key = y.split(':') | ||
| dyn_seeds.append(key[0] + ':' + str(args.peer_port) + ':' + key[1] + ':' + key[2] + ':' + str(z)) | ||
|
|
||
| ip_dc = k.split(':') |
There was a problem hiding this comment.
I think this would work better as ip, rack, datacenter = k.split(':')
| default=DEFAULT_CLIENT_PORT, | ||
| help=""" | ||
| Client port to use (The client port Dynomite provides instead of directly accessing redis or memcache) | ||
| Default is: {} |
There was a problem hiding this comment.
Instead of formatting the string ourselves, let's do %(default)s here, and let argparse do the work.
| parser.add_argument( | ||
| '-sp', | ||
| dest='server_port', | ||
| type=str, |
There was a problem hiding this comment.
should this be an int type?
| ) | ||
|
|
||
|
|
||
| parser.add_argument( |
There was a problem hiding this comment.
This and --mem would be a good use of action="store_const" and ArgumentParser.add_mutually_exclusive_group
|
|
||
| More commands: | ||
| ``` | ||
| python generate_yamls.py --help |
There was a problem hiding this comment.
Let's just leave a reference to the fact that help is available, and not include the whole help text. Something like:
For full help output, do `scripts/dynomite/generate_yamls.py --help`
| output_path = os.path.join(dir, args.output_dir) | ||
| if (not os.path.exists(output_path)): | ||
| os.makedirs(output_path) | ||
| print "creating output path %s" % output_path |
There was a problem hiding this comment.
Following the Python convention of EAFP, I would do (in Python 3):
try:
os.makedirs(output_path)
print("Created output path {}".format(output_path), file=sys.stderr)
except FileExistsError:
pass
| """) | ||
|
|
||
| parser.add_argument('nodes', type=str, nargs='+', | ||
| help=""" |
There was a problem hiding this comment.
Could we get this help text down to a single line describing how to correctly format a node description? I think some extraneous stuff snuck in here.
|
Hi Guys 👍 |
|
@syberkitten Do you think we can move forward with this and merge it once the requested changes are in place? What is your expected time frame? |
|
@ipapapa Hey, I'm not exactly continuing with this right now because we are not using Dynomite. If someone else can take it over would be great, if not |
3 participants
To make creating clusters simpler, faster, easier and error prone
i've extended generate_yamls.py so it is now more automated
and covers more options that can be supplied via the command line.
for usage:
generate_yamls.py --helpalso updated usage in readme.md.
let me know what you think and any questions...
and thanks again for this great project. 👍
I've actually managed to set up my first
two datacenter setup after finishing this upgrade,
so it works quite well.
in the future, it would be possible to add some tests
so the creation of clusters can be made sure to not break.
but for now, it should help quickly start a new setup
without having to drown in many yml files that need to be updated
manually.
If you think something else should be added please let know...