Create debug docker images for jaeger backends #2545

Ashmita152 · 2020-10-09T00:45:45Z

Signed-off-by: Ashmita Bohara [email protected]

Which problem is this PR solving?

Tackles #2493

Short description of the changes

As part of this PR, we are creating separate images for jaeger backend components.

codecov · 2020-10-09T01:43:24Z

Codecov Report

Merging #2545 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2545      +/-   ##
==========================================
- Coverage   95.33%   95.32%   -0.02%     
==========================================
  Files         208      208              
  Lines        9281     9281              
==========================================
- Hits         8848     8847       -1     
  Misses        355      355              
- Partials       78       79       +1

Impacted Files	Coverage Δ
cmd/query/app/server.go	`88.52% <0.00%> (-1.64%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d99871...3d95017. Read the comment docs.

yurishkuro

Thanks for the PR.

I would strongly prefer having a single Dockerfile for each binary, not copy-pasting content across two files. For example, you are already missing ARG USER_UID=10001 directive that's in master.

For example, would the following work?

# default build
FROM alpine:latest as certs
RUN apk add --update --no-cache ca-certificates
COPY ./scripts/fake-dlv /go/bin/dlv

# debug build
FROM golang:1.15.2-alpine as debug
...

# for debug builds, these vars should be replaced with BASEIMAGE=BUILDIMAGE=debug
ARG BASEIMAGE=scratch
ARG BUILDIMAGE=certs

FROM $BASEIMAGE
COPY --from=$BUILDIMAGE /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
COPY --from=$BUILDIMAGE /go/bin/dlv /go/bin/dlv
...

Ashmita152 · 2020-10-09T04:37:39Z

Hi @yurishkuro

Thank you for the review. It definitely makes sense to generate two images out of a single Dockerfile. Let me work on it and update the PR.

yurishkuro · 2020-10-09T04:37:59Z

Makefile

@@ -328,13 +328,19 @@ docker-images-jaeger-backend:
 	docker build -t $(DOCKER_NAMESPACE)/jaeger-opentelemetry-agent:${DOCKER_TAG} -f ${OTEL_COLLECTOR_DIR}/cmd/agent/Dockerfile cmd/opentelemetry/cmd/agent --build-arg TARGETARCH=$(GOARCH)
 	docker build -t $(DOCKER_NAMESPACE)/jaeger-opentelemetry-ingester:${DOCKER_TAG} -f ${OTEL_COLLECTOR_DIR}/cmd/ingester/Dockerfile cmd/opentelemetry/cmd/ingester --build-arg TARGETARCH=$(GOARCH)

+.PHONY: docker-images-jaeger-backend-debug


I think this target should just delegate to docker-images-jaeger-backend with some extra variables

Ashmita152 · 2020-10-09T04:40:23Z

I have another question related to this debug image:

Do we need any other debugging tool as part of this image other than delve ?

yurishkuro · 2020-10-09T04:40:40Z

Makefile

 .PHONY: docker-images-tracegen
 docker-images-tracegen:
 	docker build -t $(DOCKER_NAMESPACE)/jaeger-tracegen:${DOCKER_TAG} cmd/tracegen/ --build-arg TARGETARCH=$(GOARCH)
 	@echo "Finished building jaeger-tracegen =============="

 .PHONY: docker-images-only
-docker-images-only: docker-images-cassandra docker-images-elastic docker-images-jaeger-backend docker-images-tracegen
+docker-images-only: docker-images-cassandra docker-images-elastic docker-images-jaeger-backend docker-images-jaeger-backend-debug docker-images-tracegen


The long line is getting difficult to grok. Is there a way to break the dependency targets into multiple lines, something like:

a: \ b \ c \ d

?

yurishkuro · 2020-10-09T04:48:30Z

Do we need any other debugging tool as part of this image other than delve ?

I think the biggest ask was not so much the debug tools, but just any tools, because when we build from scratch we don't even get a shell for minimal troubleshooting.

jpkrohling · 2020-10-09T07:36:24Z

I think the biggest ask was not so much the debug tools, but just any tools, because when we build from scratch we don't even get a shell for minimal troubleshooting.

True. For the debug images, I would even base it on something else. Note that the CMD instruction should also be custom for the debug image.

Ashmita152 · 2020-10-09T08:05:22Z

Few more questions I have related to this:

Let's say a user face an issue in production. Having separate image for debugging means asking user to deploy that debug image in production separately. In case of systems like k8s, we can't have just one pod of different image, that means we need to deploy this debug image in every pod. Do you think this is the right approach. Somehow I am not convinced with this approach. I would better provide a mechanism to attach a sidecar debug container.
Is there any particular reason why we are using scratch as the base image instead of let's say alpine (which has minimal shell) ?

jpkrohling · 2020-10-09T08:17:02Z

We talked before about changing the base image (#2116), and to be honest, I forgot to add it to my queue. I guess it's on yours now :-)

That said, for this specific issue here, users would need to deploy a different image anyway, as the process has to be started with dlv as the CMD.

Still about your first point: while I agree that people should be able to enter the container when they face an issue, I don't think it's unreasonable to ask them to deploy a debug image if problems can't be easily detected (#2452 comes to my mind). They can create a second deployment with this special image, and change the fronting service to direct part of the traffic to it: if there are 10 matching pods, one of them being from the debug deployment, then only 10% of the traffic will reach the debug image.

Ashmita152 · 2020-10-10T17:33:19Z

Thank you for answering my questions earlier. This PR is ready for review again. I have updated the PR with:

Merging two dockerfiles into one and using docker build command's --target option (https://docs.docker.com/develop/develop-images/multistage-build/#stop-at-a-specific-build-stage) to build two images.
Switching to alpine (from scratch) as the base image for docker images.

I have one followup question:

While trying out delve I found that delve suggest to compile with optimisations disabled for golang binaries (https://github.com/go-delve/delve/blob/master/Documentation/usage/dlv_exec.md) Would you suggest for jaeger components binaries used in debug images ?

Ashmita152 · 2020-10-12T07:53:41Z

I noticed @jpkrohling comment in the github issue about disabling optimizations, hence updated the PR accordingly.

jpkrohling

It's looking good! I need to try it out myself, but I have a couple of comments:

looks like it's missing the all-in-one image?
can we somehow have a base image for our components? Or a script that generates the Dockerfile? Most of it seems to be the same plumbing code, and maintaining is probably becoming harder.
perhaps as a follow-up PR, but we need to publish the images as well. Take a look at the Travis configuration and look for DOCKER=true: that's how/where we are building the images and pushing to the Docker Hub.
documentation, which would belong to jaegertracing/documentation

Makefile

Ashmita152 · 2020-10-12T15:30:07Z

Hi @jpkrohling

Thank you for giving detailed feedback earlier. I have updated the branch with the changes for second point in the review.

can we somehow have a base image for our components? Or a script that generates the Dockerfile? Most of it seems to be the same plumbing code, and maintaining is probably becoming harder.

Can I ask for a review again please. I will work on rest three feedbacks once we're convinced with the dockerfiles layout changes since those three will depend on it.

jpkrohling

I'm really excited about this change, and I think we are almost at a working state.

Makefile

cmd/agent/Dockerfile

jpkrohling · 2020-10-13T15:35:45Z

To recap, this PR is still missing:

apply the same to the all-in-one binary/container image
documentation

cmd/query/Dockerfile

scripts/travis/build-all-in-one-image.sh

Ashmita152 · 2020-10-14T14:00:45Z

I am still working on the documentation part.

Ashmita152 · 2020-10-15T13:41:13Z

Hello @jpkrohling

I have raised a PR in documentation repo. I wasn't sure about what to document so most likely I am missing something. Thank you.

jpkrohling · 2020-10-15T15:14:31Z

I'll do a final check next Monday, but looks good to me! If anyone else wants to try this out before that, it would be awesome.

jpkrohling

LGTM. I tested the collector image before and just tested the all-in-one and it looks good!

- Use alpine as the base image for jaeger backends Signed-off-by: Ashmita Bohara <[email protected]>

Signed-off-by: Ashmita Bohara <[email protected]>

Pull request has been modified.

Ashmita152 · 2020-10-20T09:29:04Z

Resolved the merge conflict due to another PR merge in Makefile.

jpkrohling · 2020-10-20T10:46:15Z

Thanks for your contribution!

yurishkuro · 2020-10-21T00:30:50Z

.travis.yml

@@ -67,7 +67,7 @@ script:
  - export BRANCH=$(if [ "$TRAVIS_PULL_REQUEST" == "false" ]; then echo $TRAVIS_BRANCH; else echo $TRAVIS_PULL_REQUEST_BRANCH; fi)
  - if [ "$TESTS" == true ]; then make test-ci ; else echo 'skipping tests'; fi
  - if [ "$PROTO_GEN_TEST" == true ]; then make proto && git diff --name-status --exit-code ; else echo 'skipping protoc validation'; fi
-  - if [ "$ALL_IN_ONE" == true ]; then bash ./scripts/travis/build-all-in-one-image.sh ; else echo 'skipping all_in_one'; fi
+  - if [ "$ALL_IN_ONE" == true ]; then make create-baseimg-debugimg && bash ./scripts/travis/build-all-in-one-image.sh ; else echo 'skipping all_in_one'; fi


Why is the extra step make create-baseimg-debugimg needed here but not in DOCKER step in L73?
Also, why not call it from ./scripts/travis/build-all-in-one-image.sh?

Why is the extra step make create-baseimg-debugimg needed here but not in DOCKER step in L73?

My understanding is that script calls: make docker -> make docker-images-only -> make docker-images-jaeger-backend-debug which has dependencies on create-baseimg and create-debugimg.

Also, why not call it from ./scripts/travis/build-all-in-one-image.sh?

We can do it if you think that is the better place.

Should probably be done after #2325, as it touches this code as well.

if need to support multi arch for other components.

base images need to published with multi arch too.
or just upgrade Dockerfile like https://github.com/morlay/jaeger/blob/master/cmd/all-in-one/Dockerfile

yurishkuro · 2020-11-02T17:52:08Z

cmd/agent/Dockerfile


-FROM scratch
+FROM $base_image AS release
 ARG TARGETARCH=amd64
 ARG USER_UID=10001


@Ashmita152 was there a reason for TARGETARCH and USER_UID args to be repeated in each target, instead of being defined at the top of the file?

If not, probably another opportunity to clean up / DRY the docker files.

Sure, I will clean that today.

Ashmita152 requested a review from a team as a code owner October 9, 2020 00:45

Ashmita152 requested a review from pavolloffay October 9, 2020 00:45

yurishkuro reviewed Oct 9, 2020

View reviewed changes

jpkrohling reviewed Oct 12, 2020

View reviewed changes

Makefile Show resolved Hide resolved

jpkrohling reviewed Oct 13, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

cmd/agent/Dockerfile Outdated Show resolved Hide resolved

jpkrohling reviewed Oct 13, 2020

View reviewed changes

cmd/query/Dockerfile Outdated Show resolved Hide resolved

mergify bot assigned jpkrohling Oct 14, 2020

morlay mentioned this pull request Oct 14, 2020

Setup docker buildx for all-in-one #2325

Closed

jpkrohling reviewed Oct 14, 2020

View reviewed changes

scripts/travis/build-all-in-one-image.sh Show resolved Hide resolved

Ashmita152 mentioned this pull request Oct 15, 2020

Update troubleshooting page with debug images information jaegertracing/documentation#452

Merged

jpkrohling added the hacktoberfest-accepted label Oct 15, 2020

jpkrohling previously approved these changes Oct 19, 2020

View reviewed changes

Ashmita152 added 2 commits October 20, 2020 17:18

- Create debug docker images for jaeger backends

9ea34fe

- Use alpine as the base image for jaeger backends Signed-off-by: Ashmita Bohara <[email protected]>

Add builds for jaeger backend binaries with go optimizations disabled

3175215

Signed-off-by: Ashmita Bohara <[email protected]>

Ashmita152 added 6 commits October 20, 2020 17:23

Moved base images build as separate

cdd9cc2

Signed-off-by: Ashmita Bohara <[email protected]>

Docker image naming change

6717613

Signed-off-by: Ashmita Bohara <[email protected]>

Replaced CMD with ENTRYPOINT with some extra flags

37bce77

Signed-off-by: Ashmita Bohara <[email protected]>

Added changes for all-in-one binary and docker image

e19338c

Signed-off-by: Ashmita Bohara <[email protected]>

Add a makefile directive to build base and debug images

2f352fc

Signed-off-by: Ashmita Bohara <[email protected]>

Updated script to upload debug images to dockerhub

3d95017

Signed-off-by: Ashmita Bohara <[email protected]>

jpkrohling approved these changes Oct 20, 2020

View reviewed changes

jpkrohling merged commit 9f954fe into jaegertracing:master Oct 20, 2020

Ashmita152 deleted the debug-images branch October 20, 2020 12:17

yurishkuro reviewed Oct 21, 2020

View reviewed changes

yurishkuro mentioned this pull request Oct 22, 2020

Change base image #2116

Closed

yurishkuro reviewed Nov 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create debug docker images for jaeger backends #2545

Create debug docker images for jaeger backends #2545

Ashmita152 commented Oct 9, 2020

codecov bot commented Oct 9, 2020 •

edited

Loading

yurishkuro left a comment

Ashmita152 commented Oct 9, 2020

yurishkuro Oct 9, 2020

Ashmita152 commented Oct 9, 2020

yurishkuro Oct 9, 2020

yurishkuro commented Oct 9, 2020

jpkrohling commented Oct 9, 2020

Ashmita152 commented Oct 9, 2020 •

edited

Loading

jpkrohling commented Oct 9, 2020

Ashmita152 commented Oct 10, 2020 •

edited

Loading

Ashmita152 commented Oct 12, 2020

jpkrohling left a comment

Ashmita152 commented Oct 12, 2020

jpkrohling left a comment

jpkrohling commented Oct 13, 2020 •

edited

Loading

Ashmita152 commented Oct 14, 2020

Ashmita152 commented Oct 15, 2020

jpkrohling commented Oct 15, 2020

jpkrohling left a comment

Ashmita152 commented Oct 20, 2020

jpkrohling commented Oct 20, 2020

yurishkuro Oct 21, 2020

Ashmita152 Oct 21, 2020 •

edited

Loading

jpkrohling Oct 21, 2020

morlay Oct 21, 2020

yurishkuro Nov 2, 2020

Ashmita152 Nov 3, 2020

Create debug docker images for jaeger backends #2545

Create debug docker images for jaeger backends #2545

Conversation

Ashmita152 commented Oct 9, 2020

Which problem is this PR solving?

Short description of the changes

codecov bot commented Oct 9, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Ashmita152 commented Oct 9, 2020

yurishkuro Oct 9, 2020

Choose a reason for hiding this comment

Ashmita152 commented Oct 9, 2020

yurishkuro Oct 9, 2020

Choose a reason for hiding this comment

yurishkuro commented Oct 9, 2020

jpkrohling commented Oct 9, 2020

Ashmita152 commented Oct 9, 2020 • edited Loading

jpkrohling commented Oct 9, 2020

Ashmita152 commented Oct 10, 2020 • edited Loading

Ashmita152 commented Oct 12, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

Ashmita152 commented Oct 12, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Oct 13, 2020 • edited Loading

Ashmita152 commented Oct 14, 2020

Ashmita152 commented Oct 15, 2020

jpkrohling commented Oct 15, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

Ashmita152 commented Oct 20, 2020

jpkrohling commented Oct 20, 2020

yurishkuro Oct 21, 2020

Choose a reason for hiding this comment

Ashmita152 Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

jpkrohling Oct 21, 2020

Choose a reason for hiding this comment

morlay Oct 21, 2020

Choose a reason for hiding this comment

yurishkuro Nov 2, 2020

Choose a reason for hiding this comment

Ashmita152 Nov 3, 2020

Choose a reason for hiding this comment

codecov bot commented Oct 9, 2020 •

edited

Loading

Ashmita152 commented Oct 9, 2020 •

edited

Loading

Ashmita152 commented Oct 10, 2020 •

edited

Loading

jpkrohling commented Oct 13, 2020 •

edited

Loading

Ashmita152 Oct 21, 2020 •

edited

Loading