Support building a debuggable image for development #926

philipgough · 2018-05-03T14:59:39Z

Describe what this PR does and why we need it:

Change to help with the development process from a contributor point of view. Allows building of a remotely debuggable image so a debugger can be attached when needed.

Feel free to close this if it is seen to have no value but I found it useful when working on recent issues so thought I would share.

On a side note the docs to run a local broker did not work for me on OSx on either minishift or oc cluster up but perhaps I'm missing something (broker started but errors out).

eriknelson

Looks cool!

jmrodri · 2018-05-03T15:15:58Z

AWESOME! Thanks for the contribution.

rthallisey · 2018-05-03T15:20:46Z

@philipgough can you add some docs around this?

philipgough · 2018-05-03T15:28:30Z

@rthallisey Yep, will do just wanted to see if it was valid first

djzager

I think having a debuggable broker image is great. I have a few comments and one concern (entrypoint.sh).

Adding the steps necessary to debug the broker to the broker-apb would be awesome to me. Thank you for the contribution.

djzager · 2018-05-03T15:19:47Z

build/entrypoint.sh

@@ -16,4 +16,8 @@ if [ ! -f "$BROKER_CONFIG" ] ; then
 fi
 echo "Using config file mounted to $BROKER_CONFIG"

-exec asbd -c $BROKER_CONFIG $FLAGS
+if [ ${DEBUG_ENABLED:-False} == "True" ]; then
+    dlv  --listen=0.0.0.0:${DEBUG_PORT} --headless=true --api-version=2 --log=true exec asbd -- -c $BROKER_CONFIG $FLAGS


Could you skip modifying the entrypoint and simply add this to the debug Dockerfile?

Adding this logic to the entrypoint production broker's use (including downstream) scares me.

Yes can do I wasn't sure if this entrypoint.sh was used in production rpm

djzager · 2018-05-03T15:22:33Z

build/Dockerfile-debug

@@ -0,0 +1,49 @@
+FROM philipgough/go-centos:1.10.1 AS build-env


Is there a library image we can use here?

My initial thought with this is that it should mirror the existing build images as best as possible. Even better if you could modify the Dockerfile-canary with build arguments to optionally build it for debugging.

This is the first time I've seen a Dockerfile like this 👍

Is there a library image we can use here?

Couldn't find one. There is community golang images but this needs to be built on centos

Even better if you could modify the Dockerfile-canary with build arguments to optionally build it for debugging.

That could be done but not without the modified entrypoint script right?

The main problem here is what we would need to have go installed in the broker base, which is currently just based off centos to achieve this, where as at the moment we build the binaries and copy them in. I think anything other than a build container is problematic if that makes sense?

My primary reason for starting this conversation is, if we can avoid having another Dockerfile, that would best. Both the Dockerfile-latest and Dockerfile-nightly use rpms so we can't do anything there. Dockerfile-localdev does the copying but Dockerfile-canary, as you can see here builds the broker from source.

I would be curious:

how many contributors are using the localdev variant, if that's even well supported anymore I don't know.

if we could simply build dlv in the Dockerfile-canary and update the broker-apb to deploy the broker with the appropriate command and args in the deployment(config) for debugging.

how many contributors are using the localdev variant

That has been my workflow up to now, not sure about others. cc @maleck13 do you also use the build-image target?

@djzager @philipgough I use the Dockerfile-localdev ALL THE TIME :) my workflow recently has been to do a make build-image ORG=jmrodri TAG=demo and pushing that to my org, then redeploying with catasb. I would be okay moving the debug stuff to the localdev dockerfile

djzager · 2018-05-03T15:28:24Z

Makefile

@@ -104,6 +118,9 @@ clean: ## Clean up your working environment
 really-clean: clean cleanup-ci ## Really clean up the working environment
 	@rm -f $(KUBERNETES_FILES)

+clean-debug: ## Revert changes made to support debug image


This is a big IMO, so other reviewers can downvote me if I'm wrong. I don't believe that we need this make target (or the associated script), with the move to a broker-apb. My reason for this is that, with our move to a broker-apb, you could simply deprovision the broker and start fresh. I also normally view clean make targets to be cleaning up stuff on the host...I'm not super concerned about the cluster.

I'm fine with that, really just wanted this to get state back to where it was in case there were any concerns in that regard.

djzager · 2018-05-03T15:30:58Z

scripts/prep_debug_env.sh

+DEBUG_PORT=$1
+PROJECT=${ASB_PROJECT:="ansible-service-broker"}
+
+oc set probe dc/asb --remove --readiness --liveness -n ${PROJECT}


We have the https://github.com/automationbroker/automation-broker-apb maybe this is something that could be added there (ie. you provision a debuggable broker).

Sure I can take a look there also

jmrodri

A few suggestions to make it a little more friendly.

jmrodri · 2018-05-03T15:17:13Z

Makefile

@@ -13,6 +13,7 @@ SVC_ACCT_DIR     := /var/run/secrets/kubernetes.io/serviceaccount
 KUBERNETES_FILES := $(addprefix $(SVC_ACCT_DIR)/,ca.crt token tls.crt tls.key)
 COVERAGE_SVC     := travis-ci
 .DEFAULT_GOAL    := build
+ASB_DEBUG_PORT   := 9000


Is this configurable? If so, then I'd make it ASB_DEBUG_PORT ?= 9000 which will allow you to pass in a parameter to make. That is make ASB_DEBUG_PORT=9999. If it isn't configurable then leave it as is.

I was already able to override this when running make debug-image ASB_DEBUG_PORT=xyz. Im no make expert though, what advantage does the ? provide?

jmrodri · 2018-05-03T15:21:38Z

build/Dockerfile-debug

@@ -0,0 +1,49 @@
+FROM philipgough/go-centos:1.10.1 AS build-env


This doesn't seem to be valid. It fails when I run the target:

[jesusr@speed3 ansible-service-broker{PhilipGough-debug-image}]$ make debug-image env GOOS=linux go build -i -gcflags='-N -l' -o "/home/jesusr/dev/src/github.com/openshift/ansible-service-broker/build"/broker ./cmd/broker env GOOS=linux go build -i -ldflags="-s -s" -o "/home/jesusr/dev/src/github.com/openshift/ansible-service-broker/build"/migration ./cmd/migration env GOOS=linux go build -i -ldflags="-s -s" -o "/home/jesusr/dev/src/github.com/openshift/ansible-service-broker/build"/dashboard-redirector ./cmd/dashboard-redirector docker build -f "/home/jesusr/dev/src/github.com/openshift/ansible-service-broker/build"/Dockerfile-debug -t docker.io/ansibleplaybookbundle/origin-ansible-service-broker:latest "/home/jesusr/dev/src/github.com/openshift/ansible-service-broker/build" --build-arg DEBUG_PORT=9000 Sending build context to Docker daemon 115.8 MB Step 1/20 : FROM philipgough/go-centos:1.10.1 AS build-env Error parsing reference: "philipgough/go-centos:1.10.1 AS build-env" is not a valid repository/tag: invalid reference format make: *** [Makefile:96: debug-image] Error 1

As you stated in IRC, this is using multistage builds which isn't in the default fedora install:

$ docker version Client: Version: 1.13.1 API version: 1.26 Package version: docker-1.13.1-51.git4032bd5.fc27.x86_64 Go version: go1.9.4 Git commit: 7f1fa5c-unsupported Built: Wed Mar 28 13:58:10 2018 OS/Arch: linux/amd64 Server: Version: 1.13.1 API version: 1.26 (minimum version 1.12) Package version: docker-1.13.1-51.git4032bd5.fc27.x86_64 Go version: go1.9.4 Git commit: 7f1fa5c-unsupported Built: Wed Mar 28 13:58:10 2018 OS/Arch: linux/amd64 Experimental: false

I'll attempt a different solution here then because it should work for those on fedora by default. Using osx here so it never showed up.

jmrodri · 2018-05-03T15:36:52Z

Makefile

+	env GOOS=linux go build -i -ldflags="-s -s" -o ${BUILD_DIR}/migration ./cmd/migration
+	env GOOS=linux go build -i -ldflags="-s -s" -o ${BUILD_DIR}/dashboard-redirector ./cmd/dashboard-redirector
+	docker build -f ${BUILD_DIR}/Dockerfile-debug -t ${BROKER_IMAGE}:${TAG} ${BUILD_DIR} --build-arg DEBUG_PORT=${ASB_DEBUG_PORT}
+	@./scripts/prep_debug_env.sh ${ASB_DEBUG_PORT}


This script requires the cluster to be running, if it isn't you get an error:

Successfully built 8d891fe58f2e error: the server doesn't have a resource type "dc" error: the server doesn't have a resource type "svc" make: *** [Makefile:97: debug-image] Error 1

I don't think we have anything that requires the cluster to be up from the Makefile. Might need to either separate this or warn at the beginning. Not completely sure yet the best approach for this.

For now, I'd probably change this to echo the command they should run:

@echo "Please run scripts/prep_debug_env.sh ${ASB_DEBUG_PORT}"

Or something along those lines.

jmrodri · 2018-05-03T16:53:06Z

build/Dockerfile-debug

+
+RUN git clone https://github.com/derekparker/delve.git $GOPATH/src/github.com/derekparker/delve && \
+    cd $GOPATH/src/github.com/derekparker/delve && \
+    make install


I would do a go get -u github.com/derekparker/delve/cmd/dlv

jmrodri · 2018-05-03T16:54:42Z

build/entrypoint.sh

+    dlv  --listen=0.0.0.0:${DEBUG_PORT} --headless=true --api-version=2 --log=true exec asbd -- -c $BROKER_CONFIG $FLAGS
+else
+    exec asbd -c $BROKER_CONFIG $FLAGS
+fi


entrypoint looks good.

Are you ok with this change going into the productised version?

jmrodri · 2018-05-03T16:54:53Z

scripts/clean_debug_env.sh

+    {"op": "remove",
+     "path": "/spec/ports/0"
+    }
+]'


clean_debug_env script looks good.

jmrodri · 2018-05-03T16:55:04Z

scripts/prep_debug_env.sh

+PROJECT=${ASB_PROJECT:="ansible-service-broker"}
+
+oc set probe dc/asb --remove --readiness --liveness -n ${PROJECT}
+oc patch svc asb --patch='{"spec":{"ports":[{"name":"debug", "port":'${DEBUG_PORT}',"targetPort":'${DEBUG_PORT}}']}}'


prep_debug_env looks good as well.

jmrodri · 2018-05-03T16:56:00Z

Makefile

+	@./scripts/prep_debug_env.sh ${ASB_DEBUG_PORT}
+	@echo
+	@echo "Remember you need to push your image before updating deployment config"
+	@echo "    docker push ${BROKER_IMAGE}:${TAG}"


add an @echo "" above the docker push line

jmrodri · 2018-05-03T16:58:15Z

Makefile

+	@echo "    docker push ${BROKER_IMAGE}:${TAG}"
+	@echo
+	@echo "Run the following before connecting the debugger once the container is running"
+	@echo "oc port-forward <oc get po -o jsonpath='{.items[?(@.metadata.labels.service=="asb")].metadata.name}')> ${ASB_DEBUG_PORT}"


add an @echo "" above the oc port-forward line also add 4 spaces in front of the oc command so that the output lines up with the docker push from above.

The current output looks like this:

Remember you need to push your image before updating deployment config docker push docker.io/ansibleplaybookbundle/origin-ansible-service-broker:latest Run the following before connecting the debugger once the container is running oc port-forward <oc get po -o jsonpath='{.items[?(@.metadata.labels.service==asb)].metadata.name}')> 9000

I believe this would look better, as it makes the commands more prominent:

Remember you need to push your image before updating deployment config docker push docker.io/ansibleplaybookbundle/origin-ansible-service-broker:latest Run the following before connecting the debugger once the container is running oc port-forward <oc get po -o jsonpath='{.items[?(@.metadata.labels.service==asb)].metadata.name}')> 9000

philipgough · 2018-05-10T16:16:11Z

Makefile

@@ -88,6 +89,19 @@ release-image:
 	@echo "Remember you need to push your image before calling make deploy"
 	@echo "    make push"

+debug-image: ## Build a docker image with the broker binary started in debug mode
+	docker run --rm -v ${BUILD_DIR}:/tmp/artifact docker.io/philipgough/dlv:centos cp /go/bin/dlv /tmp/artifact


I'm happy to maintain this image. This is a workaround to support those who cannot do muti stage builds

Is there a reason you can't go get it?

philipgough · 2018-05-10T16:18:03Z

@jmrodri I've made some changes here which fix the issues discussed. If you are happy with these changes I would like to move the debug work into the the make build-image target and debugging would be enabled in a single image by updating an env var.

This would mean we maintain a single Dockerfile and single make target for local dev with an additional entrypoint file for local usage

eriknelson · 2018-05-15T16:11:43Z

I feel this is ready to merge, but I'm not going to offer any support if it breaks.

philipgough · 2018-05-15T16:16:38Z

@eriknelson

I feel this is ready to merge, but I'm not going to offer any support if it breaks.

I'd like to get @jmrodri approval on this one because I want to put this stuff into the local image and reduce the overall maintenance. Debugging will be switched on with a simple env var setting at that point. If not we can merge as is.

djzager

1 nitpicky comment about delve but other than that, this looks good to me. I believe the dynamics of how to use the debuggable broker will change with the broker-apb PR and our ability to add debug to the canary image. However, I think this is a good start.

djzager · 2018-05-15T16:32:40Z

Makefile

@@ -88,6 +89,19 @@ release-image:
 	@echo "Remember you need to push your image before calling make deploy"
 	@echo "    make push"

+debug-image: ## Build a docker image with the broker binary started in debug mode
+	docker run --rm -v ${BUILD_DIR}:/tmp/artifact docker.io/philipgough/dlv:centos cp /go/bin/dlv /tmp/artifact


Is there a reason you can't go get it?

rthallisey · 2018-05-15T19:13:48Z

@philipgough nice work! Can you create an issue for adding docs around this?

@jmrodri do you have any more comments?

philipgough · 2018-05-16T08:05:45Z

@rthallisey Sure, I'll land docs with this as soon as its finalised

Is there a reason you can't go get it?

@djzager The reason I have taken this approach is because it is faster. A go get would need to be called on each image build otherwise, slowing down the development cycle.

djzager · 2018-05-16T12:19:36Z

@philipgough wouldn't the go get really only the first time make debug-image was called and just be there on subsequent calls?

This isn't a breaker for me. In the end I would rather this simply be built (go get and all) inside the Dockerfile where it can be properly cached in one of the layers.

philipgough · 2018-05-16T19:01:16Z

@djzager Hey, not sure I'm totally following. I have baked the delve binary into the image here and am copying from the container to local disk via a docker run command. If the docker run command does a go get instead on a centos image that has go installed, I think that will run each time and don't see anything being cached? Maybe I am missing something or misunderstanding.

The only alternative I see is to maintain an additional Dockerfile on this repo and have a build done from that which would then be cached but I don't like that approach as much. I have done a go get to build this image referenced in the make file.

I see the approach I've taken as a workaround essentially for those that cannot do multi stage Docker builds.

djzager · 2018-05-16T20:01:51Z

scripts/prep_debug_env.sh

+DEBUG_PORT=$1
+PROJECT=${ASB_PROJECT:="ansible-service-broker"}
+
+oc rollout pause dc/asb -n ${PROJECT}


The broker's deployment config, when deployed via the broker-apb, will not be named asb. In #939 every script that I could find that will deploy the broker will use the apb and fusor/catasb#242 changes catasb to enable the broker (which also uses an apb).

djzager · 2018-05-16T20:09:55Z

Hey, not sure I'm totally following. I have baked the delve binary into the image here and am copying from the container to local disk via a docker run command. If the docker run command does a go get instead on a centos image that has go installed, I think that will run each time and don't see anything being cached? Maybe I am missing something or misunderstanding.

Here is how I changed the Dockerfile-canary:

--- a/build/Dockerfile-canary
+++ b/build/Dockerfile-canary
@@ -41,6 +41,7 @@ RUN mkdir -p ${BROKER_PATH}
 RUN git clone -b $VERSION https://github.com/openshift/ansible-service-broker ${BROKER_PATH}
 WORKDIR ${BROKER_PATH}

+RUN go get -u github.com/derekparker/delve/cmd/dlv
 RUN go build -i -ldflags="-s -w" ./cmd/broker && mv broker /usr/bin/asbd
 RUN go build -i -ldflags="-s -w" ./cmd/migration && mv migration /usr/bin/migration

On the second run of docker build -t broker:canary -f build/Dockerfile-canary build:

Step 18/26 : RUN go get -u github.com/derekparker/delve/cmd/dlv
 ---> Using cache
 ---> a25234b25eca

philipgough · 2018-05-16T20:18:38Z

@djzager Ah ok, so we were talking about different things :) I intended to merge this work into the local dev image only which does not have golang env available, so the command in the make file makes sense in that respect.

But yeah, if you guys are happy to get this stuff into the canary image also then I'll make the required changes there tomorrow, update the docs and hopefully wrap this up at that point :)

djzager · 2018-05-16T20:33:54Z

But yeah, if you guys are happy to get this stuff into the canary image also then I'll make the required changes there tomorrow, update the docs and hopefully wrap this up at that point :)

I would like to get this wrapped up too. Sorry to be slowing it down.

coveralls · 2018-06-14T16:08:48Z

Coverage remained the same at 36.603% when pulling b319f1c on PhilipGough:debug-image into dc522d1 on openshift:master.

Updates to debug image Add debug capabilities to localdev image and canary image and update docs

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 3, 2018

eriknelson requested review from jmrodri and shawn-hurley May 3, 2018 15:03

eriknelson approved these changes May 3, 2018

View reviewed changes

djzager requested changes May 3, 2018

View reviewed changes

jmrodri suggested changes May 3, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2018

philipgough commented May 10, 2018

View reviewed changes

shawn-hurley approved these changes May 15, 2018

View reviewed changes

djzager approved these changes May 15, 2018

View reviewed changes

rthallisey added the 3.11 | release-1.3 Kubernetes 1.11 | Openshift 3.11 | Broker release-1.3 label May 15, 2018

djzager reviewed May 16, 2018

View reviewed changes

djzager approved these changes May 17, 2018

View reviewed changes

jmrodri approved these changes Jun 1, 2018

View reviewed changes

Support building a debuggable image for development

b319f1c

Updates to debug image Add debug capabilities to localdev image and canary image and update docs

djzager approved these changes Jun 19, 2018

View reviewed changes

djzager merged commit 5cf450b into openshift:master Jun 26, 2018

		@@ -0,0 +1,49 @@
		FROM philipgough/go-centos:1.10.1 AS build-env

Support building a debuggable image for development #926

Support building a debuggable image for development #926

Conversation

philipgough commented May 3, 2018

Describe what this PR does and why we need it:

eriknelson left a comment

Choose a reason for hiding this comment

jmrodri commented May 3, 2018

rthallisey commented May 3, 2018

philipgough commented May 3, 2018

djzager left a 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

philipgough May 3, 2018 • 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

jmrodri May 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipgough May 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a 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

philipgough commented May 10, 2018 • edited

eriknelson commented May 15, 2018

philipgough commented May 15, 2018

djzager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented May 15, 2018

philipgough commented May 16, 2018 • edited

djzager commented May 16, 2018 • edited

philipgough commented May 16, 2018

Choose a reason for hiding this comment

djzager commented May 16, 2018

philipgough commented May 16, 2018

djzager commented May 16, 2018

coveralls commented Jun 14, 2018 • edited

philipgough May 3, 2018 •

edited

jmrodri May 3, 2018 •

edited

philipgough May 3, 2018 •

edited

philipgough commented May 10, 2018 •

edited

philipgough commented May 16, 2018 •

edited

djzager commented May 16, 2018 •

edited

coveralls commented Jun 14, 2018 •

edited