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

Remove unnecessary links to xdock test_driver #507

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
4 participants
@pavolloffay
Member

pavolloffay commented Nov 2, 2017

  • fixed mixed indentation
xdock remove unnecessary links to test_driver
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e24a4fd on pavolloffay:xdock-remove-links-to-test-driver into d3df7c8 on jaegertracing:master.

coveralls commented Nov 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e24a4fd on pavolloffay:xdock-remove-links-to-test-driver into d3df7c8 on jaegertracing:master.

@yurishkuro

This comment has been minimized.

Show comment
Hide comment
@yurishkuro

yurishkuro Nov 2, 2017

Member

I think making Compose take care of dependency order is great, but one concern I have is the visibility into that process, i.e. of something gets stuck and blocks the other things, do we see it in the logs?

I kicked the builds again

Member

yurishkuro commented Nov 2, 2017

I think making Compose take care of dependency order is great, but one concern I have is the visibility into that process, i.e. of something gets stuck and blocks the other things, do we see it in the logs?

I kicked the builds again

@yurishkuro yurishkuro changed the title from xdock remove unnecessary links to test_driver to Remove unnecessary links to xdock test_driver Nov 2, 2017

@yurishkuro yurishkuro merged commit 4c80d57 into jaegertracing:master Nov 2, 2017

3 checks passed

DCO All commits have a DCO sign-off from the author
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@wafflebot wafflebot bot removed the review label Nov 2, 2017

@yurishkuro

This comment has been minimized.

Show comment
Hide comment
@yurishkuro

yurishkuro Nov 2, 2017

Member

@pavolloffay please try making your PR titles look like good commit messages, it makes it easier to merge. Right now I have to edit the message every time I merge.

Member

yurishkuro commented Nov 2, 2017

@pavolloffay please try making your PR titles look like good commit messages, it makes it easier to merge. Right now I have to edit the message every time I merge.

@pavolloffay

This comment has been minimized.

Show comment
Hide comment
@pavolloffay

pavolloffay Nov 2, 2017

Member

I think making Compose take care of dependency order is great, but one concern I have is the visibility into that process, i.e. of something gets stuck and blocks the other things, do we see it in the logs?

I am not sure what you mean exactly? Depends_on just ensure that certain container starts before something so for instance DNS resolution will work. it also ensures that docker-compose run foo will start or dependent containers. We were using links before which does the same + something more.

Links https://docs.docker.com/compose/compose-file/#links
Links also express dependency between services in the same way as depends_on, so they determine the order of service startup.

What didn't work? I noticed that there are problems with java xdock, UDP sender sometimes fails to resolve agent's name.

Member

pavolloffay commented Nov 2, 2017

I think making Compose take care of dependency order is great, but one concern I have is the visibility into that process, i.e. of something gets stuck and blocks the other things, do we see it in the logs?

I am not sure what you mean exactly? Depends_on just ensure that certain container starts before something so for instance DNS resolution will work. it also ensures that docker-compose run foo will start or dependent containers. We were using links before which does the same + something more.

Links https://docs.docker.com/compose/compose-file/#links
Links also express dependency between services in the same way as depends_on, so they determine the order of service startup.

What didn't work? I noticed that there are problems with java xdock, UDP sender sometimes fails to resolve agent's name.

@yurishkuro

This comment has been minimized.

Show comment
Hide comment
@yurishkuro

yurishkuro Nov 2, 2017

Member

What I mean is that, for example, crossdock driver pings the health endpoint of all services participating in the test until they respond with 200, and it keeps logging the attempts until it succeeds on all of them or times out. Using dependencies in the compose achieves a similar result, to a certain degree, but if something gets stuck do we have any logs about who's waiting on whom?

Also, starting the container is not the same as having that container being ready to service requests. E.g. the cassandra-schema container depends on cassandra, but internally loops until cassandra is actually available. While it loops, it itself is already "running" so the other containers that depend on it in compose will be started (like the collector), even though the underlying storage may still be unavailable.

I am not saying what you have here is not an improvement, I just wonder if we can use compose even more to actually manage dependencies with readiness, not just "process started".

Member

yurishkuro commented Nov 2, 2017

What I mean is that, for example, crossdock driver pings the health endpoint of all services participating in the test until they respond with 200, and it keeps logging the attempts until it succeeds on all of them or times out. Using dependencies in the compose achieves a similar result, to a certain degree, but if something gets stuck do we have any logs about who's waiting on whom?

Also, starting the container is not the same as having that container being ready to service requests. E.g. the cassandra-schema container depends on cassandra, but internally loops until cassandra is actually available. While it loops, it itself is already "running" so the other containers that depend on it in compose will be started (like the collector), even though the underlying storage may still be unavailable.

I am not saying what you have here is not an improvement, I just wonder if we can use compose even more to actually manage dependencies with readiness, not just "process started".

@pavolloffay

This comment has been minimized.

Show comment
Hide comment
@pavolloffay

pavolloffay Nov 2, 2017

Member

What I mean is that, for example, crossdock driver pings the health endpoint of all services participating in the test until they respond with 200, and it keeps logging the attempts until it succeeds on all of them or times out. Using dependencies in the compose achieves a similar result, to a certain degree, but if something gets stuck do we have any logs about who's waiting on whom?

Orchestrator does not wait it just executes or fails if the dependency does not exist.

I see what you mean. Right now we let jaeger-services fail and wait for the restart (also in k8s). It's not the most efficient solution but it works and we don't have to pollute the code with retries and checks. Here are some tips how we could deal with it https://docs.docker.com/compose/startup-order/.

@jpkrohling do you have some recommendations? My first impression is that it's better to keep things clean and just use restart.

Member

pavolloffay commented Nov 2, 2017

What I mean is that, for example, crossdock driver pings the health endpoint of all services participating in the test until they respond with 200, and it keeps logging the attempts until it succeeds on all of them or times out. Using dependencies in the compose achieves a similar result, to a certain degree, but if something gets stuck do we have any logs about who's waiting on whom?

Orchestrator does not wait it just executes or fails if the dependency does not exist.

I see what you mean. Right now we let jaeger-services fail and wait for the restart (also in k8s). It's not the most efficient solution but it works and we don't have to pollute the code with retries and checks. Here are some tips how we could deal with it https://docs.docker.com/compose/startup-order/.

@jpkrohling do you have some recommendations? My first impression is that it's better to keep things clean and just use restart.

@jpkrohling

This comment has been minimized.

Show comment
Hide comment
@jpkrohling

jpkrohling Nov 2, 2017

Member

I'm not familiar with Docker Compose, but in general, having the test infra assess the readiness of the components under test would make sense to me. Letting things fail/restart is how we do for Kubernetes, but the ideal is to have services not to fail unless they really need to. For instance, the collector could not mark itself ready while a Cassandra connection isn't available, with a hard failure after a timeout of 1 minute (like the create-schema image).

Member

jpkrohling commented Nov 2, 2017

I'm not familiar with Docker Compose, but in general, having the test infra assess the readiness of the components under test would make sense to me. Letting things fail/restart is how we do for Kubernetes, but the ideal is to have services not to fail unless they really need to. For instance, the collector could not mark itself ready while a Cassandra connection isn't available, with a hard failure after a timeout of 1 minute (like the create-schema image).

@pavolloffay

This comment has been minimized.

Show comment
Hide comment
@pavolloffay

pavolloffay Nov 2, 2017

Member

Should we create an issue for it? It will require a similar check for all external components/storages.

Member

pavolloffay commented Nov 2, 2017

Should we create an issue for it? It will require a similar check for all external components/storages.

@jpkrohling

This comment has been minimized.

Show comment
Hide comment
@jpkrohling

jpkrohling Nov 2, 2017

Member

Should we create an issue for it? It will require a similar check for all external components/storages.

+1 , that would be awesome to have!

Member

jpkrohling commented Nov 2, 2017

Should we create an issue for it? It will require a similar check for all external components/storages.

+1 , that would be awesome to have!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment