-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement ES daemon-mode in process launcher #701
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I left some high-level comments that I think help simplifying the overall approach.
return p | ||
|
||
|
||
class ProcessLauncherTests(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think testing this gets much simpler once we only rely on the PID. Then we can just mock process startup and mock the check for a PID file. Once we expose a provision
and launch
subcommand this should also become way simpler to add this as an integration test in integration-test.sh
. I'd imagine an integration test along those lines:
PROVISIONER_ID=$(esrally provision --distribution-version=6.8.0 --car=4gheap)
esrally start --provisioner-id=${PROVISIONER_ID}
# maybe do a check here that the node is actually reachable via HTTP
esrally stop --provisioner-id=${PROVISIONER_ID}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating! I left a few comments but the overall direction looks good to me.
esrally/mechanic/launcher.py
Outdated
stdout=subprocess.PIPE, stderr=subprocess.STDOUT, stdin=subprocess.DEVNULL), node_name) | ||
def _start_process(self, cmd): | ||
subprocess.Popen(shlex.split(cmd), close_fds=True) | ||
#TODO: wait_for_docker_pidfile? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dliappis can we assume that if docker-compose
returns (with exit code 0
), Elasticsearch is about to startup? (i.e. no need to wait for the PID file)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid it's a "it depends" answer :)
With docker-compose it's possible to have healthchecks and depending on how they are defined we can safely assume ES is actually listening; this is done for example in
rally/docker/docker-compose-tests.yml
Lines 26 to 30 in 69cb36d
healthcheck: | |
test: curl -f http://localhost:9200 | |
interval: 5s | |
timeout: 2s | |
retries: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dliappis I think this is entirely in our control as the only way to start the Docker image is via https://github.com/elastic/rally/blob/master/esrally/resources/docker-compose.yml - So we cannot assume it is already listening (which is ok) but I wonder whether we can assume that the Elasticsearch process has been started already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments around _start_process
. Also, we usually try to avoid force-pushing during the review process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments on making the Docker Launcher more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating! I left some comments, mainly to discuss whether we can rid our selves from the Docker library dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating! I spent some time also testing various scenarios and this is looking good.
I left a comment for a bug, I think, that prevents DockerLauncher.stop()
from working correctly.
All comments addressed, but Daniel is unavailable to approve.
This needs to be re-opened (or a new one opened instead) as nightlies fail with:
|
Launch Elasticsearch as a daemon.
Relates to #697
Closes #718