Initial keytar commit. by thompsonja · Pull Request #2582 · vitessio/vitess

thompsonja · 2017-02-23T00:12:19Z

Wanted to get this out for some initial comments while I'm on vacation. There's still some minor cleanup that's needed (comments, docs, etc.), but generally the gist is there. I went with the name keytar, I'll have to backronym that later...

After this commit, we can add a dockerhub webhook that points to a running instance of keytar on GKE. Every new image will fire a json payload to the instance, triggering a cluster test run to fire using the GKE resources of the project that keytar is running under. This will result in tests running on actual cloud resources and can be considered as post-submit tests. We can discuss later how to react better to these tests.

This commit includes a small front-end that displays test results, dockerfile for creating the docker image, kubernetes templates, unit tests/webdriver tests, and some (admittedly light) documentation.

@michael-berlin
@dumbunny for python readability.

dumbunny · 2017-03-02T08:01:02Z

test/cluster/keytar/keytar.py

+  if keytar_args.config_file:
+    with open(keytar_args.config_file, 'r') as yaml_file:
+      yaml_config = yaml_file.read()
+  if not yaml_config:


If keytar_args.config_file is False, yaml_config is not initialized.

dumbunny · 2017-03-02T08:03:57Z

test/cluster/keytar/keytar_web_test.py

+
+  @classmethod
+  def tearDownClass(cls):
+    os.killpg(cls.flask_process.pid, signal.SIGKILL)


Is SIGKILL necessary? Is SIGTERM flaky?

dumbunny · 2017-03-02T08:08:00Z

test/cluster/keytar/test_runner.py

+          '-e', environment_config['application_type'], '-n', name]
+      if 'params' in test:
+        test_args += ['-t', ':'.join(
+            ['%s=%s' % (k, v) for (k, v) in test['params'].iteritems()])]


Extraneous inner brackets.

thompsonja-bot · 2017-03-02T21:24:07Z

Review status: 0 of 17 files reviewed at latest revision, 3 unresolved discussions.

test/cluster/keytar/keytar.py, line 201 at r1 (raw file):

Previously, dumbunny wrote…

If keytar_args.config_file is False, yaml_config is not initialized.

Fixed by making config_file a required flag.

test/cluster/keytar/keytar_web_test.py, line 51 at r1 (raw file):

Previously, dumbunny wrote…

Is SIGKILL necessary? Is SIGTERM flaky?

Swapped with SIGTERM, still works fine.

test/cluster/keytar/test_runner.py, line 67 at r1 (raw file):

Previously, dumbunny wrote…

Extraneous inner brackets.

Done.

Comments from Reviewable

mberlin-bot · 2017-03-02T23:57:50Z

First rounds of comments. (Sorry for the late review.)

Reviewed 17 of 17 files at r1, 2 of 3 files at r2.
Review status: 16 of 17 files reviewed at latest revision, 29 unresolved discussions.

test/cluster/keytar/Dockerfile, line 1 at r2 (raw file):

# This Dockerfile should be built from within the accompanying build.sh script.

The component "keytar" as a whole needs a documentation somewhere such that people easily understand what it's doing.

Once you have that, you can reference it here.

Otherwise, people will go to our Docker hub repo and wonder what the purpose of this image is.

test/cluster/keytar/Dockerfile, line 28 at r2 (raw file):

ENV CLOUDSDK_PYTHON_SITEPACKAGES $PYTHONPATH

RUN wget https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-138.0.0-linux-x86_64.tar.gz

They probably have a generic download URL where the version is not hardcoded?

In any case, we have the problem that we'll have to maintain this image in case they make any breaking changes. I think that's fine and given that I would prefer a generic download URL here (if possible).

test/cluster/keytar/dummy_test.py, line 2 at r2 (raw file):

#!/usr/bin/env python
"""Dummy no-op test."""

Mention here the purpose of this file.

test/cluster/keytar/keytar-controller-template.yaml, line 15 at r2 (raw file):

thompsonja

Please push this to our repo instead.

test/cluster/keytar/keytar-up.sh, line 10 at r2 (raw file):

vitess_p0

I find the "p0" in here misleading.

Is there a reason for this?

test/cluster/keytar/keytar-up.sh, line 33 at r2 (raw file):

$addr

"$addr" because it's a string.

Also: Please make this "$ip" because we usually use addr for ip:port (or host:port).

test/cluster/keytar/keytar-up.sh, line 34 at r2 (raw file):

echo Keytar address: http://${addr}:80

echo "Keytar address: http://${addr}:80"

test/cluster/keytar/keytar-up.sh, line 37 at r2 (raw file):

echo Waiting for keytar external IP

echo "Waiting for keytar external IP"

to be consistent

test/cluster/keytar/keytar.py, line 23 at r2 (raw file):

keytar_config = None
results = {}
q = Queue.Queue()

Comment missing. What are you storing here? When do elements get added/removed?

Do some methods use these and can they be moved closer?

Can this be moved into an object and the methods be part of that object? This way things would be more self-documenting as well.

test/cluster/keytar/keytar.py, line 28 at r2 (raw file):

def worker():
  while True:

This needs a comment.

test/cluster/keytar/keytar.py, line 38 at r2 (raw file):

def _add_new_result(timestamp):

I would like to see methods like this further down.

I want to be able to read the file from top to bottom, going from the high level implementation to the low level.

(I'm not familiar with the Python style guide, but I assume there's no rule which would disallow this?)

test/cluster/keytar/keytar.py, line 114 at r2 (raw file):

def test_log():
  log = '%s.log' % flask.request.values['log_name']
  return (flask.send_from_directory('/tmp/testlogs', log), 200,

What if log = "../../var/log/auth"?

I could escape out of the testlogs dir and download any file within the container?

Keep it simple and restrict it to files only e.g. call os.path.basename on it and then use that?

test/cluster/keytar/README.md, line 2 at r2 (raw file):

docker hub

nit: Docker Hub
(Let's use the official casing for proper names.)

test/cluster/keytar/README.md, line 2 at r2 (raw file):

# Keytar
Keytar is a system for monitoring docker images on [docker hub](https://hub.docker.com). When a new image is created, Keytar can start a cluster on Google Compute Engine (GKE) and run Kubernetes applications for the purpose of executing cluster tests. It exposes a simple web status page showing test results.

Start the first sentence with describing the purpose and then go into the details?

I would expect something like this:

Keytar is an internal Vitess system which continuously runs cluster tests to test the Vitess integration with Kubernetes.
It is run when ...
It does by ...

test/cluster/keytar/README.md, line 2 at r2 (raw file):

When a new image is created

The relationships between the different components is not clear for me from this.

Let's discuss this offline.

test/cluster/keytar/README.md, line 4 at r2 (raw file):

keytar

inconsistent spelling within this file. Let's always use "Keytar" unless you talk about specific binaries?

test/cluster/keytar/README.md, line 6 at r2 (raw file):

$VTTOP/test/cluster/keytar/config

Put these in `` here and throughout the file.

test/cluster/keytar/README.md, line 18 at r2 (raw file):

KEYTAR_KEY=

Are the terms "keyfile" and "key" the same?

It doesn't look like that?

What about renaming "KEY" to "PASSWORD" because that's the authentication between the Docker web hook and keytar?

test/cluster/keytar/README.md, line 20 at r2 (raw file):

Docker hub

nit: Docker Hub

test/cluster/keytar/README.md, line 24 at r2 (raw file):

Viewing Status

Dashboard?

test/cluster/keytar/README.md, line 31 at r2 (raw file):

## Limitations
Currently, Keytar has the following limitations:
* Only one configuration file allowed at a time.

FYI: I'm surprised that you don't need a newline between the previous paragraph and this enumeration.

This may work with GitHub but I think it doesn't work with the Markdown parser for your website. The same probably applies to the headlines and the following text.

This is just an FYI.

test/cluster/keytar/config/vitess_p0_config.yaml, line 31 at r2 (raw file):

  extra:
    - apt-key adv --recv-keys --keyserver keyserver.ubuntu.com 0xcbcb082a1bb943db
    - add-apt-repository 'deb [arch=amd64,i386,ppc64el] http://sfo1.mirrors.digitalocean.com/mariadb/repo/10.1/debian jessie main'

What's the purpose behind these things?

Can we not reuse our bootstrap images?

Let's discuss this offline.

test/cluster/keytar/config/vitess_p0_config.yaml, line 49 at r2 (raw file):

  cluster_setup:
    - type: gke
      project_name: e-sunlight-726

Why this name?

Add a comment here.

test/cluster/keytar/config/vitess_p0_config.yaml, line 63 at r2 (raw file):

    before_test:
      - ./bootstrap.sh
      - if [[ $? -gt 0 ]]; then ./bootstrap.sh; fi

Let's fix this in the script and not here.

I would be fine with adding a single retry there.

test/cluster/keytar/static/script.js, line 6 at r2 (raw file):

var html = "
";
This looks complicated.
You could write this as a multi-line string in JS:
var html = "<table align='center' id='results' border='1'>
  <thead>
    <tr>
      <th>Time</th>
      <th>Docker Image</th>
      <th>Sandbox Name</th>
      <th>Status</th>
      <th>Tests</th>
      <th>Results</th>
    </thead>
    <tbody>";
test/cluster/keytar/static/script.js, line 10 at r2 (raw file):

key

What's a key here? Can you use a more descriptive variable name? Or is this actually a term in your framework?
Comments from Reviewable
Time Docker Image Sandbox Name Status Tests Results

thompsonja-bot · 2017-03-07T06:19:14Z

Review status: 16 of 17 files reviewed at latest revision, 29 unresolved discussions.

test/cluster/keytar/Dockerfile, line 1 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

The component "keytar" as a whole needs a documentation somewhere such that people easily understand what it's doing.

Once you have that, you can reference it here.

Otherwise, people will go to our Docker hub repo and wonder what the purpose of this image is.

Referenced the README.md file.

test/cluster/keytar/Dockerfile, line 28 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

They probably have a generic download URL where the version is not hardcoded?

In any case, we have the problem that we'll have to maintain this image in case they make any breaking changes. I think that's fine and given that I would prefer a generic download URL here (if possible).

Done.

test/cluster/keytar/dummy_test.py, line 2 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Mention here the purpose of this file.

Done.

test/cluster/keytar/keytar-controller-template.yaml, line 15 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

thompsonja

Please push this to our repo instead.

Done.

test/cluster/keytar/keytar-up.sh, line 10 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

vitess_p0

I find the "p0" in here misleading.

Is there a reason for this?

It's to parallel the idea of p0 use cases, i.e. primary use cases. I'll just call it vitess_config.yaml

test/cluster/keytar/keytar-up.sh, line 33 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

$addr

"$addr" because it's a string.

Also: Please make this "$ip" because we usually use addr for ip:port (or host:port).

Done.

test/cluster/keytar/keytar-up.sh, line 34 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

echo Keytar address: http://${addr}:80

echo "Keytar address: http://${addr}:80"

Done.

test/cluster/keytar/keytar-up.sh, line 37 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

echo Waiting for keytar external IP

echo "Waiting for keytar external IP"

to be consistent

Done.

test/cluster/keytar/keytar.py, line 23 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Comment missing. What are you storing here? When do elements get added/removed?

Do some methods use these and can they be moved closer?

Can this be moved into an object and the methods be part of that object? This way things would be more self-documenting as well.

Done.

test/cluster/keytar/keytar.py, line 28 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

This needs a comment.

Done.

test/cluster/keytar/keytar.py, line 38 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

I would like to see methods like this further down.

I want to be able to read the file from top to bottom, going from the high level implementation to the low level.

(I'm not familiar with the Python style guide, but I assume there's no rule which would disallow this?)

Sure I don't mind reorganizing.

test/cluster/keytar/keytar.py, line 114 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

What if log = "../../var/log/auth"?

I could escape out of the testlogs dir and download any file within the container?

Keep it simple and restrict it to files only e.g. call os.path.basename on it and then use that?

Done.

test/cluster/keytar/README.md, line 2 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

docker hub

nit: Docker Hub
(Let's use the official casing for proper names.)

Done.

test/cluster/keytar/README.md, line 2 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Start the first sentence with describing the purpose and then go into the details?

I would expect something like this:

Keytar is an internal Vitess system which continuously runs cluster tests to test the Vitess integration with Kubernetes.
It is run when ...
It does by ...

Done.

test/cluster/keytar/README.md, line 4 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

keytar

inconsistent spelling within this file. Let's always use "Keytar" unless you talk about specific binaries?

Done.

test/cluster/keytar/README.md, line 6 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

$VTTOP/test/cluster/keytar/config

Put these in `` here and throughout the file.

Done.

test/cluster/keytar/README.md, line 18 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

KEYTAR_KEY=

Are the terms "keyfile" and "key" the same?

It doesn't look like that?

What about renaming "KEY" to "PASSWORD" because that's the authentication between the Docker web hook and keytar?

keyfile refers to the cloud credential files, whereas key refers to the password encoded in the docker web hook. I'll change it to password.

test/cluster/keytar/README.md, line 20 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Docker hub

nit: Docker Hub

Done.

test/cluster/keytar/README.md, line 24 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Viewing Status

Dashboard?

Done.

test/cluster/keytar/README.md, line 31 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

FYI: I'm surprised that you don't need a newline between the previous paragraph and this enumeration.

This may work with GitHub but I think it doesn't work with the Markdown parser for your website. The same probably applies to the headlines and the following text.

This is just an FYI.

Fixed.

test/cluster/keytar/config/vitess_p0_config.yaml, line 31 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

What's the purpose behind these things?

Can we not reuse our bootstrap images?

Let's discuss this offline.

Discussed offline, will work on using the bootstrap image.

test/cluster/keytar/config/vitess_p0_config.yaml, line 49 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Why this name?

Add a comment here.

I don't think I actually need this if it's running in the same project that will be used to run tests, so I just removed it.

test/cluster/keytar/config/vitess_p0_config.yaml, line 63 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

Let's fix this in the script and not here.

I would be fine with adding a single retry there.

Agreed, I'll do some digging as to why it kept failing for me (plus, this will go away when I use the bootstrap image anyway).

test/cluster/keytar/static/script.js, line 6 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

var html = "
";
This looks complicated.
You could write this as a multi-line string in JS:
var html = "<table align='center' id='results' border='1'>
  <thead>
    <tr>
      <th>Time</th>
      <th>Docker Image</th>
      <th>Sandbox Name</th>
      <th>Status</th>
      <th>Tests</th>
      <th>Results</th>
    </thead>
    <tbody>";
Done.
test/cluster/keytar/static/script.js, line 10 at r2 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

key

What's a key here? Can you use a more descriptive variable name? Or is this actually a term in your framework?

Just generic iterating over a dict.
Comments from Reviewable
Time Docker Image Sandbox Name Status Tests Results

mberlin-bot · 2017-03-08T01:02:32Z

Thanks for addressing the comments. Looks good so far.

Reviewed 10 of 10 files at r3.
Review status: 16 of 17 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

test/cluster/keytar/keytar.py, line 189 at r3 (raw file):

  def worker_loop(self):
    # Run forever, executing tests as they are added to the queue

nit: Missing period :)

test/cluster/keytar/README.md, line 23 at r3 (raw file):

default vitess_p0_config.yaml

Needs to be updated.

Comments from Reviewable

thompsonja-bot · 2017-03-09T17:44:58Z

Review status: 13 of 17 files reviewed at latest revision, 15 unresolved discussions.

test/cluster/keytar/keytar.py, line 189 at r3 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

nit: Missing period :)

Done.

test/cluster/keytar/README.md, line 2 at r2 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

When a new image is created

The relationships between the different components is not clear for me from this.

Let's discuss this offline.

Reworded (and discussed offline).

test/cluster/keytar/README.md, line 23 at r3 (raw file):

Previously, mberlin-bot (Michael Berlin) wrote…

default vitess_p0_config.yaml

Needs to be updated.

Done.

Comments from Reviewable

thompsonja · 2017-03-21T19:29:23Z

LGTM

Initial keytar commit.

0fc2280

googlebot added the cla: yes label Feb 23, 2017

thompsonja requested review from dumbunny and michael-berlin March 2, 2017 00:39

dumbunny reviewed Mar 2, 2017

View reviewed changes

Address comments

901ff3b

Address comments.

f0cfb9b

thompsonja added 3 commits March 8, 2017 09:50

Address comments.

36cfeba

Simplify vitess_config.yaml to minimum steps needed to run tests.

baeac3f

Add check for default cloud project.

db54b8c

thompsonja added 4 commits March 17, 2017 11:58

Add comments and other minor refactors.

e3caf2f

Address readability comments.

65c10cd

Address readability comments

2cf506d

Address readability comments.

fe50f2c

thompsonja merged commit 14a8263 into vitessio:master Mar 21, 2017

thompsonja deleted the cluster_test branch March 21, 2017 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial keytar commit.#2582

Initial keytar commit.#2582
thompsonja merged 10 commits intovitessio:masterfrom
thompsonja:cluster_test

thompsonja commented Feb 23, 2017

Uh oh!

dumbunny Mar 2, 2017

Uh oh!

dumbunny Mar 2, 2017

Uh oh!

dumbunny Mar 2, 2017

Uh oh!

thompsonja-bot commented Mar 2, 2017

Uh oh!

mberlin-bot commented Mar 2, 2017

Uh oh!

thompsonja-bot commented Mar 7, 2017

Uh oh!

mberlin-bot commented Mar 8, 2017

Uh oh!

thompsonja-bot commented Mar 9, 2017

Uh oh!

thompsonja commented Mar 21, 2017 •

edited by alainjobart

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

thompsonja commented Feb 23, 2017

Uh oh!

dumbunny Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

dumbunny Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

dumbunny Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

thompsonja-bot commented Mar 2, 2017

Uh oh!

mberlin-bot commented Mar 2, 2017

Uh oh!

thompsonja-bot commented Mar 7, 2017

Uh oh!

mberlin-bot commented Mar 8, 2017

Uh oh!

thompsonja-bot commented Mar 9, 2017

Uh oh!

thompsonja commented Mar 21, 2017 • edited by alainjobart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thompsonja commented Mar 21, 2017 •

edited by alainjobart

Loading