Added Dockerfile to individual components #157

jpkrohling · 2017-05-12T13:16:02Z

No description provided.

jpkrohling · 2017-05-12T13:20:09Z

This PR adds the following make targets:

docker -- to build the Docker images locally, based on the newly created Dockerfiles. It does not build docker images for the existing images, as those are handled by other tasks (all-in-one and crossdock)
docker-push -- to push the images built by the docker target to a repository. This requires a previous docker login.

The Travis changes were made kind of in a blind mode, as I'm not sure how I can test it. The bash parts were tested as much as I could.

I'm also not quite sure how I can remove the submodule updates to this commit, as I never really dealt with submodules. Any hints there is highly appreciated :)

coveralls · 2017-05-12T13:26:47Z

Coverage remained the same at 100.0% when pulling fc807c5 on jpkrohling:JPK-Dockerfiles into baa60f6 on uber:master.

jpkrohling · 2017-05-12T13:20:51Z

plugin/storage/cassandra/schema.sh

@@ -0,0 +1,31 @@
+#!/bin/bash


Not quite sure if this is the most appropriate for this script. It has to be either on the same directory as the Dockerfile or in a sub directory, but not on a parent directory.

looks ok here, given the other scripts. I would just rename it to create-schema.sh

jpkrohling · 2017-05-12T13:21:59Z

cmd/agent/Dockerfile

@@ -0,0 +1,6 @@
+FROM centos:7


The images are inheriting the CentOS 7 images, as those provide some tools which are useful for debugging. Once the images mature a bit on the community, we can change the base image to Alpine or similar.

The smallest base image that still allows basic debug abilities would be best.

The CentOS image isn't that big (192.5 MB), which is shared by all images. But if you have a specific image to suggest, I could try that out and see if it works.

jpkrohling · 2017-05-12T13:22:55Z

.travis.yml

@@ -16,6 +16,9 @@ matrix:
  - go: 1.7
    env:
    - CROSSDOCK=true
+  - go: 1.7
+    env:
+    - DOCKER=true


I have to admit that I don't know for sure what this does. I guess this will trigger different executors on Travis, but I just did a copy/paste of the sections above.

this is a travis test matrix; for each one of the entries, travis will kick off a new build with the environment variables set in the entry. We did this to speed up the travis build process by parallelizing the subcomponents. The copy pasta is good

jpkrohling · 2017-05-12T13:23:24Z

Makefile

@@ -46,7 +47,7 @@ md-to-godoc-gen:

 .PHONY: clean
 clean:
-	rm -rf cover.out cover.html lint.log fmt.log
+	rm -rf cover.out cover.html lint.log fmt.log jaeger-ui-build


This doesn't really belong to this commit. I hope it's OK to leave this change here.

jpkrohling · 2017-05-12T13:24:05Z

Makefile

+	rm -rf cmd/query/jaeger-ui-build
+
+.PHONY: docker-push
+docker-push:


This target is only useful for development purposes and it's not being used for the actual builds. Should this be removed?

I feel like we should never be manually pushing and always have travis push the repos on a successful build.

Don't think we'd ever have need for manually pushing anything; should always wait for travis to push on a successful build.

I did quite a few times under my own namespace, to test the images on OpenShift/Kubernetes. If I add a comment to this target, saying that this is intended only for local development purposes, would that be OK to leave it there?

jpkrohling · 2017-05-12T13:26:35Z

plugin/storage/cassandra/Dockerfile

@@ -0,0 +1,7 @@
+FROM jpkroehling/cassandra


This was discussed on the mailing list. I'll either change this to use the "official" Dockerhub image for Cassandra or change to use an image we are building for OpenShift. I don't expect to change this before this PR is merged, though.

add TODO here and a task once this lands

black-adder · 2017-05-12T14:27:55Z

.travis.yml

@@ -16,6 +16,9 @@ matrix:
  - go: 1.7
    env:
    - CROSSDOCK=true
+  - go: 1.7
+    env:
+    - DOCKER=true


this is a travis test matrix; for each one of the entries, travis will kick off a new build with the environment variables set in the entry. We did this to speed up the travis build process by parallelizing the subcomponents. The copy pasta is good

black-adder · 2017-05-12T14:43:32Z

Makefile

+	rm -rf cmd/query/jaeger-ui-build
+
+.PHONY: docker-push
+docker-push:


I feel like we should never be manually pushing and always have travis push the repos on a successful build.

black-adder · 2017-05-12T14:46:14Z

Makefile

+	rm -rf cmd/query/jaeger-ui-build
+
+.PHONY: docker-push
+docker-push:


Don't think we'd ever have need for manually pushing anything; should always wait for travis to push on a successful build.

black-adder · 2017-05-12T14:47:17Z

cmd/agent/Dockerfile

@@ -0,0 +1,6 @@
+FROM centos:7


The smallest base image that still allows basic debug abilities would be best.

black-adder · 2017-05-12T14:48:17Z

plugin/storage/cassandra/Dockerfile

@@ -0,0 +1,7 @@
+FROM jpkroehling/cassandra


add TODO here and a task once this lands

black-adder · 2017-05-12T14:59:14Z

.travis.yml

@@ -43,3 +46,4 @@ script:
  - if [ "$COVERAGE" == true ]; then travis_retry goveralls -coverprofile=cover.out -service=travis-ci || true ; else echo 'skipping coverage'; fi


on L41 above, you probably have to change it to:

- if [[ "$ALL_IN_ONE" == true || "$DOCKER" == true ]]; then bash ./travis/install-ui-deps.sh ; fi

because we need yarn to build the ui which is needed in build-docker-images.sh

or we can add a new environment variable called INSTALL_YARN and add it to the matrix for ALL_IN_ONE and DOCKER

black-adder · 2017-05-12T15:00:55Z

travis/build-docker-images.sh

+
+source ~/.nvm/nvm.sh
+nvm use 6
+DOCKER_NAMESPACE=jaegertracing make docker


stupid question incoming: shouldn't there be a newline before make docker?

Not really. In bash, this has the effect of setting this var for this command, so, make will see DOCKER_NAMESPACE as jaegertracing without the parent script needing to export it first.

black-adder · 2017-05-12T15:08:52Z

Makefile

+.PHONY: docker
+docker: build_ui build-agent-linux build-collector-linux build-query-linux
+	cp -r jaeger-ui-build/build/ cmd/query/jaeger-ui-build
+	docker build -t $(DOCKER_NAMESPACE)/jaeger-cassandra-schema plugin/storage/cassandra/ ; \


Can we call this jaeger-cassandra instead? The schema gets applied on startup and after that it's essentially just a cassandra box. The name might throw off some people.

There's no "after" for this image :) It's used as a Kubernetes "Job": it will just run the shell script that generates the keyspace and will gracefully die afterwards. It's derived from the Cassandra image because it needs the cqlsh tools, so, I thought it would be easier (and save space) to just consume a image that I know existed there, but Cassandra itself will never start on this image.

ahhh gotcha. Maybe jaeger-cassandra-bootstrap? fine with jaeger-cassandra-schema too.

I would prefer to use the same mechanism across all artifacts.

@black-adder It sounds like this is doing something different from what e2e test is doing to init Cassandra.

What we do for e2e is pretty ridiculous though, we keep bringing up collector and wait until it doesn't fatal out due to cassandra not being ready. I prefer this new approach.

^ above answer is an answer to a different c* question below

For this question, we have the e2e handler actively probe the cassandra cluster to see if it's ready.

I talked to @jsanda last week about something related to this and he made me realize that this might also not be appropriate.

In any case: I think we could add a separate task for having a more reliable bootstrap method, perhaps consistent among all deployment architectures. In the meantime, I'd keep it like this, so that we can move forward.

yurishkuro · 2017-05-12T16:23:05Z

Makefile

@@ -111,6 +112,21 @@ build-query-linux:
 build-collector-linux:
 	CGO_ENABLED=0 GOOS=linux installsuffix=cgo go build -o ./cmd/collector/collector-linux ./cmd/collector/main.go

+.PHONY: docker
+docker: build_ui build-agent-linux build-collector-linux build-query-linux
+	cp -r jaeger-ui-build/build/ cmd/query/jaeger-ui-build


I am not clear why this is needed. We already build all-in-one without copying this dir into query source, instead we just mount it to the Docker image from ./jaeger-ui-build

That's because the Dockerfile is within the cmd/query , and Docker can only build stuff that is on its own directory or any sub directory, but not in any parent directory (for security reasons I suppose). So, we need to have a copy there.

yurishkuro · 2017-05-12T16:27:01Z

Makefile

+.PHONY: docker
+docker: build_ui build-agent-linux build-collector-linux build-query-linux
+	cp -r jaeger-ui-build/build/ cmd/query/jaeger-ui-build
+	docker build -t $(DOCKER_NAMESPACE)/jaeger-cassandra-schema plugin/storage/cassandra/ ; \


I would prefer to use the same mechanism across all artifacts.

@black-adder It sounds like this is doing something different from what e2e test is doing to init Cassandra.

yurishkuro · 2017-05-12T16:28:51Z

cmd/collector/Dockerfile

@@ -0,0 +1,6 @@
+FROM centos:7


I thought we'd put all Docker files in one dir. But no strong feeling either way.

We could, as long as the contents for the Docker images are in sub directories from where the Dockerfiles are located. I'd rather keep it alongside with the image's contents.

yurishkuro · 2017-05-12T16:34:29Z

cmd/collector/Dockerfile

+
+COPY collector-linux /go/bin/
+
+CMD ["/go/bin/collector-linux"]


This comment applies to all images - don't we need to be translating env vars into command line switches, to make the images follow https://12factor.net/ ?

Ideally our binaries could be reading env vars directly, there are frameworks like spf13/Cobra that support that, but we haven't implemented it yet. So the only way currently to configure each binary is to translate env vars into command line switches.

Not necessarily: I'm not quite sure how it would work for Docker Compose, but for Docker at the command line, Kubernetes and OpenShift, you can override the command line and set extra env vars.

So, the best approach is to specify only what's really required to get the binary running, so that you don't need to maintain two places with default values.

This is an example of how to set an extra env var:

$ docker run -e MY_ENV_VAR=test -it jpkroehling/jaeger-agent bash [root@86fc09d20bfc /]# echo $MY_ENV_VAR test

And here is an example of how to override the command to execute when running an image: https://github.com/jpkrohling/jaeger-openshift/blob/JPK-OpenShift-IndividualComponents/template.yml#L147-L149

yurishkuro · 2017-05-12T16:35:30Z

plugin/storage/cassandra/schema.sh

+echo "Generating the schema for the keyspace ${KEYSPACE} and datacenter ${DATACENTER}"
+export KEYSPACE
+
+# the `test` parameter is to force the script to use a SimpleStrategy instead of


why? simple strategy only makes sense in test env with a single Cassandra host.

I was actually using the prod, but got into a Cassandra issue. Something like Error reading service_names from storage: Cannot achieve consistency level LOCAL_ONE.

prod configuration defines replication factor 2, so you need at least 2 Cassandra nodes.

Ok, that's it then. I'm not that familiar with Cassandra and noticed that I could change the script to test and it would work :-) My deployment script creates 2 Cassandra nodes, but it seems it was still not enough to avoid this error.

Should we move all these Cassandra-related tasks to a new task?

yurishkuro · 2017-05-12T16:45:37Z

plugin/storage/cassandra/schema.sh

@@ -0,0 +1,31 @@
+#!/bin/bash


looks ok here, given the other scripts. I would just rename it to create-schema.sh

yurishkuro · 2017-05-12T16:46:31Z

plugin/storage/cassandra/schema.sh

+
+CQLSH_HOST=${CQLSH_HOST:-"cassandra"}
+CASSANDRA_WAIT_TIMEOUT=${CASSANDRA_WAIT_TIMEOUT:-"60"}
+DATACENTER=${DATACENTER:-"openshift"}


this doesn't look like a good default. I am actually not sure what datacenter the official Cassandra image mentions.

The datacenter is actually required by the existing schema generator: https://github.com/uber/jaeger/blob/master/plugin/storage/cassandra/cassandra3v001-schema.sh#L23

So, it's either prod or test, and when passing prod, a datacenter has to be provided.

Yes, it's required for prod because it uses the name when defining replication strategy. That datacenter must match what Cassandra uses in its cassandra-rackdc.properties.

yurishkuro · 2017-05-12T16:48:34Z

plugin/storage/cassandra/schema.sh

+total_wait=0
+while true
+do
+  ping -c 1 ${CQLSH_HOST} > /dev/null 2>&1


is this the typical way of checking that the container app is running? Seems line it might succeed as soon as the container / IP is available, but the Cassandra server may still be starting.

@black-adder what did we use for e2e test? Didn't we wait for the actual response from Cassandra?

Kubernetes/OpenShift will establish the connection only once the target service has a container passing the "readiness" check. We don't have such a check right now, but this is more like a hack and might eventually fail on the situation you just mentioned.

I think @jsanda can help us here in coming up with a reliable solution, but I'd prefer to have it as part of another task.

coveralls · 2017-05-16T22:23:58Z

Coverage remained the same at 100.0% when pulling a55c7bf on jpkrohling:JPK-Dockerfiles into 49dbd3c on uber:master.

jpkrohling · 2017-05-21T11:45:29Z

@yurishkuro , @black-adder , I just updated this PR, rebasing it on the latest master and adding a couple of fixes based on the comment reviews. If this looks good, I'll squash it and it would be ready for merging.

There are still a couple of items open for Cassandra, mostly related to tuning it to be more resilient and reliable (replication factor, schema creation, ...) , but I'd like to have that as another follow up task, instead of blocking this PR. I'll then ask @jsanda for help on that task, as he's far more experienced with Cassandra than I am :-)

coveralls · 2017-05-21T11:50:08Z

Coverage remained the same at 100.0% when pulling 972b25b on jpkrohling:JPK-Dockerfiles into 437ec41 on uber:master.

yurishkuro · 2017-05-21T22:50:12Z

I am not clear why the build failed, no output in the Travis logs.
By default the new images are published to jaegertracing/jaeger-{component}, I assume we need to create repos on Docker hub?
The images are missing any sort of tags, usually we at least add -t $REPO:$COMMIT
Can we exclude changes to jaeger-ui from this PR?

jsanda · 2017-05-22T13:42:43Z

You will certainly run into problems by just pinging the the pod. Cassandra start up times will vary, particularly when you take commit log replay into consideration. That can slow down start up significantly. I suggest using nodetool statusbinary. You can grep the output for running.

To avoid overly long start up times and to avoid commit log corruption, I strongly recommend running nodetool drain on shutdown.

coveralls · 2017-05-24T12:20:34Z

Coverage remained the same at 100.0% when pulling 99238f7 on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

coveralls · 2017-05-24T12:22:37Z

Coverage remained the same at 100.0% when pulling 99238f7 on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

coveralls · 2017-05-24T12:30:37Z

Coverage remained the same at 100.0% when pulling b122fce on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

coveralls · 2017-05-24T12:38:41Z

Coverage remained the same at 100.0% when pulling 56eb6f5 on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

coveralls · 2017-05-24T12:55:33Z

Coverage remained the same at 100.0% when pulling fee8371 on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

jpkrohling · 2017-05-24T14:11:22Z

I updated the PR to use another command for checking Cassandra's state. I decided not to use nodetool, as it would require exposing the JMX port just for this purpose. If we do need JMX in the future, that's fine, but it sounded too much for this specific purpose.

I am not clear why the build failed, no output in the Travis logs.

It works now. I had to add a dedicated conditional on .travis.yml/install for $DOCKER, instead of using a || on the $ALL_IN_ONE part.

By default the new images are published to jaegertracing/jaeger-{component}, I assume we need to create repos on Docker hub?

They are created on demand on the first build, provided the account has push permissions on the organization.

The images are missing any sort of tags, usually we at least add -t $REPO:$COMMIT

It's actually done on the upload-to-docker.sh script, called at the end of the script. It's the same script the other ones execute.

Can we exclude changes to jaeger-ui from this PR?

I tried every git trick I know, but the only thing that helped was to do a diff against master, create a new branch off of master and apply the new diff onto this new branch :/

I also opened #175 to track the points to improve in our usage of Cassandra.

coveralls · 2017-05-24T14:16:49Z

Coverage remained the same at 100.0% when pulling 2abe6ef on jpkrohling:JPK-Dockerfiles into 7a965e9 on uber:master.

jpkrohling · 2017-05-25T07:31:42Z

Rebased with the latest master. This is ready for a re-review or eventual merge.

coveralls · 2017-05-25T07:36:52Z

Coverage remained the same at 100.0% when pulling 585a670 on jpkrohling:JPK-Dockerfiles into 894cf6e on uber:master.

jpkrohling commented May 12, 2017

View reviewed changes

black-adder reviewed May 12, 2017

View reviewed changes

yurishkuro reviewed May 12, 2017

View reviewed changes

yurishkuro mentioned this pull request May 16, 2017

Consider adding tracing support for OpenTracing (or similar) kubernetes/kubernetes#26507

Closed

Added Dockerfile to individual components

585a670

yurishkuro approved these changes May 26, 2017

View reviewed changes

yurishkuro merged commit 9bbe029 into jaegertracing:master May 26, 2017

yurishkuro mentioned this pull request Jun 15, 2017

Provide docker image for agent, collector, and query service #152

Closed

4 tasks

pavolloffay mentioned this pull request Jul 14, 2017

Cassandra Replication factor jaegertracing/jaeger-kubernetes#15

Closed

ideepika pushed a commit to ideepika/jaeger that referenced this pull request Oct 22, 2017

Build Docker images for individual components (jaegertracing#157)

15dd8b1

jpkrohling deleted the JPK-Dockerfiles branch July 28, 2021 19:23

		@@ -43,3 +46,4 @@ script:
		- if [ "$COVERAGE" == true ]; then travis_retry goveralls -coverprofile=cover.out -service=travis-ci \|\| true ; else echo 'skipping coverage'; fi


		COPY collector-linux /go/bin/

		CMD ["/go/bin/collector-linux"]

Added Dockerfile to individual components #157

Added Dockerfile to individual components #157

Conversation

jpkrohling commented May 12, 2017

jpkrohling commented May 12, 2017

coveralls commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

black-adder May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling May 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 16, 2017

jpkrohling commented May 21, 2017

coveralls commented May 21, 2017

yurishkuro commented May 21, 2017

jsanda commented May 22, 2017

coveralls commented May 24, 2017

coveralls commented May 24, 2017

coveralls commented May 24, 2017

coveralls commented May 24, 2017

coveralls commented May 24, 2017

jpkrohling commented May 24, 2017

coveralls commented May 24, 2017

jpkrohling commented May 25, 2017

coveralls commented May 25, 2017

black-adder May 12, 2017 •

edited

jpkrohling May 13, 2017 •

edited