Skip to content
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

Add standalone Dockerfile that runs agent, collector, and query #69

Merged
merged 24 commits into from
Apr 8, 2017

Conversation

badiib
Copy link
Contributor

@badiib badiib commented Mar 29, 2017

  • UI is not part of this docker instance

EXPOSE 5775 6831 6832 14267 16686

#example build command from github.com/uber/jaeger directory: docker build -f cmd/standalone/Dockerfile -t standalone .
#example docker run command: docker run --publish 5755:5775 --publish 6831:6831 --publish 6832:6832 --publish 14267:14267 --publish 16686:16686 --name standalone --rm standalone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain what you mean by "these", please?


ENTRYPOINT go run /go/src/github.com/uber/jaeger/cmd/standalone/main.go --span-storage.type=memory

EXPOSE 5775 6831 6832 14267 16686
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a comment on earlier PRs that we need to define a single ports.go file and consolidate all ports used by the backend in one place. The docker image should be able to expose all ports with N-M range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find this comment. Our port definitions are all over the place, including work that was done in the agent. Is this something we want to tackle in this PR or is this a request that should be encapsulated in a separate ticket?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a pre-requisite

@yurishkuro
Copy link
Member

let's keep this open and define a proper task (#70) with all requirements.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.365% when pulling f783410 on rooz/dockerize-single-application into 00435d3 on master.

RUN node -v
RUN npm -v

RUN git clone https://github.com/uber/jaeger-ui
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worthwhile to publish to npmjs and depend on that instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saminzadeh ^ what do these words mean? Is it possible? we're trying to build a docker container that contains the UI, query service and hotrod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to use a git submodule. Publishing to NPM is unnecessary overhead & release friction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, not worth it publish to NPM.

FROM golang:1.7
EXPOSE 5775 6831 6832 14267 3000 3001 8080 8081 8082 8083

ADD . /go/src/github.com/uber/jaeger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the crossdock tests do the build outside of container and only copy the binary. Why would we do this differently?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like what I for crossdock, I first wanted to build everything inside the container but couldn't figure out how. I prefer building everything inside the container instead of doing it in travis and passing the binaries. I see no benefit of having travis do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. building outside of the container is faster than inside it, which is especially useful for npm install which takes a long time.
  2. this is a docker image we are going to publish and people are going to download. It makes no sense to have all the source files in that image. You could do the build and remove the sources, but then see (1). Also, the base image can be a lot smaller if we only need to put a binary in there, as opposed to having both Go and Node toolchains installed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okey..... ill do it that way


RUN git clone https://github.com/uber/jaeger-ui

RUN cd jaeger-ui && npm install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to Go binary, the npm build should happen outside of container, we only need to copy the results of the build to container

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@black-adder black-adder Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why though? when we can encapsulate everything in one place.

@yurishkuro
Copy link
Member

I assume this also needs a travis change

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.31% when pulling 0f4ac46 on rooz/dockerize-single-application into 2b99d9d on master.

@black-adder black-adder force-pushed the rooz/dockerize-single-application branch from 0f4ac46 to f82d370 Compare April 6, 2017 18:52
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.31% when pulling b5141eb on rooz/dockerize-single-application into 2b99d9d on master.


ENTRYPOINT /bin/hotrod-demo-linux all & /bin/standalone-linux --span-storage.type=memory --query.port=3001

#example build command from github.com/uber/jaeger directory: docker build -f cmd/standalone/Dockerfile -t standalone .
Copy link
Contributor

@vprithvi vprithvi Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that the examples go into a separate Makefile or some such, so that we can simply ask users to make docker_build or make docker_run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now, docker_build command depends on the user having jaeger-ui and npm, etc. For now I'm not going to expose it in the makefile. Will do in the future when we make jaeger-ui a submodule.

.travis.yml Outdated
- secure: cnVh5hOEa/UIBP1dJlZGpY7yyQlV+X3dXDuC7atePUVUTRNiRXtG96BGkz3l1mOb6R2s1We1e4SDoADP8NQfsn0TABXrAUu2ZuDW7NwMT5xoJ53aehpmXfRqO55Wau2m8X/kHl3V7eECsdekCXWua8ijmMfqYySUXwIZvuyT1tObqEpv4aItDIUFuv8app30dIZF1cE9hM91gl+0gu5N+FuwO7AAVt+Q0a3B1Ntv8mTb/rmz/lYkBBSaClgrBMQ2sjAuDDPOWfZla0SVMrymCI/F4JtR2eo82Enn6Z+JBwHRFZgMyc05EDoNh9m7hvQCqDVsvMExuf/qSVXTAlL1Tqzk6uOCyLoB1XM6SydEUIMukiNFN3h+v0WVczK9MyRaFuhN3CS6BAn1jwW9BAQKAXC4DZ8bQTCKzWT1Jd8+9x32HJrhB8ylBXA/ER8xghiIMv/Wk8bFW6tfnTYU/uiCtoFfSk5PI9cLLVhuSUxgyX/9KWBPGnA1/PhC2W3H4vs+uVHTisE5+LPBTWoSII7Z82u+UZTLH/wmAgOroNEHCULdSd82Efn8El9e4ebl6zR5MWOtgsK1zKd4wdq00oc19x0vLgAUeznTc+XwhLfSN7WwpR0mYEiZnOubvscc/rTo01FnPlIbn1vKdwUKw2GfXafqq2Vsa0p/q1Eo4pZfOzE=
- secure: BMpuCCm+z/sOAMZ5BDX7BDzI5y/H32KpwfJaVNL7womV5Mzxz9yPe1muQG+Z9cL6aCBQwToRTbvuFLdSzjPT9A+EBQuDuWUfVC3vkpbkXk1vdN73D1LeWkAsjI7OZ32fim3+xksHnGMs6VsWsObZrmq6QXeHl50MmXeeYqMnc+4g3TkhIyFnYeVCHUuiidMmVqikJm7ujLvoN89tXP3QZ4khw74nHd3IiT45h06GPkoYRs8euT9x8wNBkhm2RVNBkTeClbiqlhfywDbU2/O6otQ5+6A21QNBAk0rKH4OCc+lJ39hRV2bmPnqbw0vFhGk9YdIN3JmydpmOQW3vWr7eiBOrI8eMBXbsNzddYCISOzEtgApJnaK8tQIeEY8QvoqdgVwk2mEXUKn9EJii0BhkAwv3KF5Obi3x5jZcEo8YP5dn05gXzpjk2cFJB8npBlSSlkKcKYQKX605UYwEbs3XxOU90Mg03nwxNKwk26aFx0T1mUIzYcx0h/a99OLMFr/mK5BKRLQaFh03kp6HCS0q5j/fotN4ZW41QYPKIjvv+FyuMcGNtx5C5qTlEWZcECVJoM4dKxMa/z3REeKVqtQYq0azCSjG5Tje8cslUbLNF4N7NvNOJux6GHVGtmE29atzrw3Wsra4hARFm+kOAE9b/1Qfjnz3C3XDmFe07Sxntg=
- secure: tlWX/wnfuyvTTEBG6UwIrAugCLKKYuEZvcwrOYgaO27Q0SQmHOevGhsCOVDDJ2e01no60IR6Cu67O+JlRrZqOuZIkl9uvnf60SZWDbqCMhZraVh5b0iZD1li0XHXo56zj+ptDXc3fOmnUTT2PYnZ+4+sR76jBw2PnZKE/u6NNSm3KkKvEQGfFkFdId/17z6Lob++BwXqmIw1n2DG7Ts+VxyCVf5HBfe8RfDPWDlXfqQUE5x9hbl+VbwCR+4LDAAJmh9h7JTItX7nvNG91EzEGedKea8IWn7fkLcQSZWuMalpGADqMx0nsPyJk8fzc0X17w595P54ktatwcWWgVv+ZhzozoO3FPWOom7c35gx+WAlMeIBtpxYQtixP2xPpK1i4HJXjW4KS+zjTEkoqeGQrFXcIKHUM0YXQvRIp5rfRky/INDzm2AYTReGoLX+ZQ7O/tSVDaBfdvTd96hRP6M1t4zCGULaglQGdE7/RLQrz7S6j2ys0fhF5gFBmOSodGt5gG+YFjzKiu+5FYMICwpUaOO+h4mLTsP0t63vFKGfXt79eWFJ+pRaMyWb68geZqB5u+P64sTnEIa8LjP/Lhrgnh0vab0O0m2KgnRMKBVlc4modKLiTXbYn1KZUyk0dfKYR2lCQHxAZW94nBOXzynDu8qZE0nXemVokFkEETm1FB0=
- secure: aR7jrdGh3cXncHlIJ6FgVf5jqMiUHSWiGON3tocCUIAB3GyRoT4lFsHg4y86c4qDkpiTrFZT2mY7Iyqs/Y+Hi64V2pGP2FouN9cZK8b5IuBDboa//9nebAvTVuzsuUBLmRwtaQ2qvpS+Jg7ApXyLjF+Cdvfh4WRm4IcwxbvmMr/+zVxkPbEBNnnALx7h1kNiAtQVrCjiHJ1Q+p2RzER+OSTtdvJRqMzkwI741ttj7pvxElbexdPwiU+9lLC9IoW/lSOSwx29Ph8WAK2aF2VrxxrUbNiTd8o2U11Zc2ic44FhOadTrGk6bJBLHOerFnDt9ieoGpbocinviZJ2OGvSoXWaI9XNPaxCNSuA3woRRX4+j9SK/AFunSxUtC2gyUhCvMKLcz1WbVtto9kAXZKq+N1zBrA5zeFCArbKCLOidTdrv1iY5eC/mV6RPhvy2knpDRzFtk6UP/7oiTiUu1yI1CGDRrWRQaaguRcLdhSKFKfgjfRNksfO1QQ6rokZWZdc5kUchLEcb2QtCRWOAShfuvyr+j38l8Rgx5DwBAnd7zov0H4cXUZ1oBOzbHmejHQF7KMs6cnGgCvDT2mQYEwvMc9cueD8S3qWrkaLIm78pQesKLuKarYHeAF8pWivQPr9hwSSPnY944E9YT0odMbNmNrWruwF2YuyFKTMdIVhy3Q=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add comments showing which env var is defined by each line

COPY ./examples/hotrod/hotrod-demo-linux /bin/
COPY ./cmd/standalone/standalone-linux /bin/

ENTRYPOINT /bin/hotrod-demo-linux all & /bin/standalone-linux --span-storage.type=memory --query.port=3001
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: what's the reason we need to change query port?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ui looks for 3001 specifically

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're running it incorrectly. There's no 3001 in our internal build of query + ui.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well yeah, internally we build the static files and move them into the query service. We haven't exposed that in oss. Do you want me to do that too for this?

FROM golang:1.7
EXPOSE 5775 6831 6832 14267 3000 3001 8080 8081 8082 8083

COPY ./examples/hotrod/hotrod-demo-linux /bin/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to do that. This image is meant to run jaeger-backend only. The demo can be run from source, especially since for the purpose of actual demos it requires changing the source.

@@ -0,0 +1,10 @@
FROM golang:1.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there may be a lighter base image than the one with full go tool chain (add TODO)

Copy link
Contributor

@vprithvi vprithvi Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alpine image should work because we don't depend on git, etc
We can simply use golang:1.7-alpine

@black-adder black-adder changed the title Add standalone Dockerfile that runs agent, collector, and query [WIP] Add standalone Dockerfile that runs agent, collector, and query Apr 6, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.378% when pulling 6df44b7 on rooz/dockerize-single-application into 2b99d9d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 98.378% when pulling 1740b79 on rooz/dockerize-single-application into 2b99d9d on master.

@black-adder black-adder changed the title [WIP] Add standalone Dockerfile that runs agent, collector, and query Add standalone Dockerfile that runs agent, collector, and query Apr 7, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.227% when pulling 2bb5afc on rooz/dockerize-single-application into 527cde4 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.227% when pulling b4bfb0e on rooz/dockerize-single-application into 527cde4 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0007%) to 98.339% when pulling b4bfb0e on rooz/dockerize-single-application into 527cde4 on master.

.travis.yml Outdated
- make install_ci

script:
- make test_ci
- travis_retry goveralls -coverprofile=cover.out -service=travis-ci || true

after_success:
- git clone https://github.com/uber/jaeger-ui
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like this to be a submodule, not an explicit clone. Submodule allows precise version control of the UI project when we're building query service, but clone always pulls latest that may be incompatible.

.travis.yml Outdated
- make install_ci

script:
- make test_ci
- travis_retry goveralls -coverprofile=cover.out -service=travis-ci || true

after_success:
- git clone https://github.com/uber/jaeger-ui
- cd jaeger-ui && npm install && npm run build && cd ..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to do take this in parentheses, then you don't need cd .. (which incidentally may fail)

.travis.yml Outdated
- make install_ci

script:
- make test_ci
- travis_retry goveralls -coverprofile=cover.out -service=travis-ci || true

after_success:
- git clone https://github.com/uber/jaeger-ui
- cd jaeger-ui && npm install && npm run build && cd ..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure if yarn is installed by default on travis, if it is, the yarn install is a lot faster than npm install

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, why don't we have a make target for this? it will be needed when building / testing query service locally anyway, less noise in travis file.

install:
- nvm version
- node -v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a minimum version, I would suggest nvm use 4 somewhere in this file - @saminzadeh thoughts?

.travis.yml Outdated
- docker build -f cmd/standalone/Dockerfile -t $REPO:$COMMIT .
# - docker tag $REPO:$COMMIT $REPO:$TAG TODO docker doesn't like tags with \ need to omit them
- docker tag $REPO:$COMMIT $REPO:travis-$TRAVIS_BUILD_NUMBER
- docker push $REPO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if some of these commands fails? I think the may execution still continues, i.e. we may fail to build the UI yet we'd continue creating the invalid docker image. We don't have to address it in this diff, but need a better strategy. Perhaps take a look at how zipkin-docker travis files are setup.

.travis.yml Outdated
- echo "TRAVIS_BRANCH=$TRAVIS_BRANCH, REPO=$REPO, PR=$PR, BRANCH=$BRANCH, TAG=$TAG"
- docker login -u $DOCKER_USER -p $DOCKER_PASS
- docker build -f cmd/standalone/Dockerfile -t $REPO:$COMMIT .
# - docker tag $REPO:$COMMIT $REPO:$TAG TODO docker doesn't like tags with \ need to omit them
Copy link
Member

@yurishkuro yurishkuro Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you just need to include it in quotes? "$REPO:$TAG"
without this line we are never going to mark something latest

Makefile Outdated
@@ -82,6 +82,12 @@ install_examples: install
build_examples:
go build -o ./examples/hotrod/hotrod-demo ./examples/hotrod/main.go

build_standalone:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really need this, let's not pollute makefile

.travis.yml Outdated
- git clone https://github.com/uber/jaeger-ui
- cd jaeger-ui && npm install && npm run build && cd ..
- mkdir jaeger-ui-build && cp -r jaeger-ui/build jaeger-ui-build/
- make build_standalone_linux
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call it build-all-in-one-linux (with dash, not underscore - we should fix all targets with underscore, bad practice)

ENTRYPOINT /go/bin/standalone-linux --span-storage.type=memory --query.static-files=/go/src/jaeger-ui-build/build/

#example build command from github.com/uber/jaeger directory: docker build -f cmd/standalone/Dockerfile -t standalone .
#example docker run command: docker run --publish 5755:5775 --publish 6831:6832 --publish 14267:14267 --publish 16686:16686 --name standalone --rm standalone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these?

Makefile Outdated
build-all-in-one-linux:
cd jaeger-ui && npm install && npm run build
rm -rf jaeger-ui-build && mkdir jaeger-ui-build
cp -r jaeger-ui/build jaeger-ui-build/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move the above 3 lines to build-ui target and make this one depend on it (or even not depend)

@yurishkuro
Copy link
Member

don't forget to move back to after_script:

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 98.272% when pulling c01a660 on rooz/dockerize-single-application into 527cde4 on master.

@black-adder black-adder merged commit 0ab671e into master Apr 8, 2017
@black-adder black-adder deleted the rooz/dockerize-single-application branch April 8, 2017 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants