SANDBOX-1561 | feature: enable debug builds for the service#566
SANDBOX-1561 | feature: enable debug builds for the service#566MikelAlejoBR wants to merge 2 commits intocodeready-toolchain:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikelAlejoBR The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds a Delve-based debug Dockerfile, enables Delve-friendly Go build flags, introduces Make targets to build/push a debug image, and extends the deploy script with a debug mode that builds, pushes, and patches the deployment to run the service under Delve. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Script as deploy-dev.sh
participant Make as Make (build/podman)
participant Docker as Container build
participant Registry as Container registry
participant K8s as Kubernetes
Dev->>Script: ./deploy-dev.sh debug
Script->>Script: set DEBUG=true
alt DEBUG mode
Script->>Make: build-dev
Make->>Docker: compile binary with -gcflags "-N -l"
Docker-->>Make: binary
Script->>Make: podman-image-debug
Make->>Docker: build image using Dockerfile.debug (delve builder stage)
Docker->>Docker: compile dlv, assemble final image
Docker-->>Make: debug image
Script->>Registry: push debug image
Registry-->>Script: image available
Script->>K8s: patch deployment to run dlv headless + service
K8s-->>Dev: service running under Delve (port 50000)
else Standard mode
Script->>Make: build
Make->>Docker: standard compile
Docker-->>Make: binary
Script->>Make: podman-image
Make->>Docker: build standard image
Docker-->>Make: image
Script->>Registry: push standard image
Registry-->>Script: image available
Script->>K8s: deploy standard service
K8s-->>Dev: service running normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #566 +/- ##
=======================================
Coverage 81.54% 81.54%
=======================================
Files 46 46
Lines 2802 2802
=======================================
Hits 2285 2285
Misses 431 431
Partials 86 86
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/Dockerfile.debugmake/go.mkmake/podman.mkscripts/deploy-dev.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (10)
make/go.mk (2)
14-17: LGTM!The enhanced comment clearly documents the purpose of the
build-devtarget for debugging workflows.
20-20: LGTM!The
-gcflags "all=-N -l"flags correctly disable optimizations and inlining, which is essential for Delve debugging support.build/Dockerfile.debug (2)
25-37: LGTM!The base image setup and user configuration follow best practices for containerized services.
39-43: LGTM!The service binary and Delve debugger are correctly installed from the appropriate source locations.
scripts/deploy-dev.sh (5)
35-41: LGTM!The DEBUG flag logic is clear and correctly determines whether to enter debug mode based on the first argument.
43-51: LGTM!The conditional build logic correctly switches between
build-devandbuildtargets based on the DEBUG flag.
53-65: LGTM!The conditional image build logic correctly uses
podman-image-debugorpodman-imagebased on the DEBUG flag.
101-103: LGTM with consistency note.The help text correctly documents the new debug mode. Note that line 103 mentions "port 50000", which is correct per the Dockerfile but inconsistent with the current implementation on line 80 (which uses port 40000). Once the port mismatch is fixed, this documentation will be accurate.
88-90: LGTM!The improved HOST_NS check with better error handling is a good enhancement.
make/podman.mk (1)
13-16: LGTM.The
podman-image-debugtarget correctly depends onbuild-devand passes the Go version to the Dockerfile. The use of$(go version | awk '{print $$3}')correctly extracts the version in the formatgo1.x.y(e.g.,go1.25.1), which matches whatbuild/Dockerfile.debugexpects for downloading the Go release tarball fromgo.dev/dl.
In order to be able to debug the service live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work. For that purose, a new Dockerfile has been added which can build the image that way, and the corresponding Make target make it easy to kick off the whole process with a single command. The Makefile target also enable the "toolchain-e2e" repository to easily build the "debug" image. The "scripts/deploy-dev.sh" script is also modified to be able to refresh the registration service with the debug image if so desired. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
a4dd435 to
edc063d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/deploy-dev.sh (1)
77-81: Critical: Delve port 50000 must be exposed in the deployment.The deployment command is patched to run Delve listening on port 50000, but the container's port configuration is not updated to expose this port. Without exposing the port, external debuggers cannot connect to the service, which defeats the purpose of this debug feature.
🔎 Proposed fix to expose the Delve port
Add a patch after line 81 to expose the debugger port:
oc patch deployment/registration-service --namespace "${HOST_NS}" --type='json' --patch='[{"op": "replace", "path": "/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/registration-service"]}]' + echo "✏️ exposing the Delve debugger port 50000 in the deployment" + oc patch deployment/registration-service --namespace "${HOST_NS}" --type='json' --patch='[{"op": "add", "path": "/spec/template/spec/containers/0/ports/-", "value": {"containerPort": 50000, "name": "delve", "protocol": "TCP"}}]' fiAdditionally, consider patching or creating a Service to expose this port externally:
echo "✏️ patching the service to expose the Delve debugger port" oc patch service/registration-service --namespace "${HOST_NS}" --type='json' --patch='[{"op": "add", "path": "/spec/ports/-", "value": {"name": "delve", "port": 50000, "protocol": "TCP", "targetPort": 50000}}]'Users can then connect via
oc port-forwardor expose the service externally as needed.
🧹 Nitpick comments (2)
scripts/deploy-dev.sh (2)
57-57: Optional: Remove unnecessary blank line.This empty line appears to be unnecessary and could be removed for cleaner code.
101-103: LGTM: Clear help text for debug mode.The help text correctly documents the debug mode and specifies port 50000. Consider adding a note about using
oc port-forwardto connect debuggers after the deployment is running.💡 Optional enhancement
Consider adding a note about port forwarding:
echo "setup Configure the deployment with a single pod" echo "refresh Rebuild the registration-service and update the pod" -echo "refresh debug Rebuild the registration service with Delve on it listening on port 50000" +echo "refresh debug Rebuild the registration service with Delve on it listening on port 50000" +echo " (use 'oc port-forward deployment/registration-service 50000:50000 -n \$HOST_NS' to connect your debugger)" echo ""
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/Dockerfile.debugmake/go.mkmake/podman.mkscripts/deploy-dev.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- build/Dockerfile.debug
- make/podman.mk
- make/go.mk
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (4)
scripts/deploy-dev.sh (4)
35-41: LGTM: Clean debug flag initialization.The conditional logic to set the DEBUG flag is correct and uses proper bash syntax.
45-50: LGTM: Correct conditional build logic.The conditional build command properly switches between
build-dev(for debug) andbuild(for production), maintaining consistency with the DEBUG flag.
58-64: LGTM: Proper conditional image building.The logic correctly switches between debug and standard image builds using the appropriate make targets.
88-90: LGTM: Improved HOST_NS validation.The tightened HOST_NS check with explicit exit improves clarity and error handling.
|
|
Closing in favor of #568. |
2 participants
In order to be able to debug the service live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work.
For that purose, a new Dockerfile has been added which can build the image that way, and the corresponding Make target make it easy to kick off the whole process with a single command.
The Makefile target also enable the "toolchain-e2e" repository to easily build the "debug" image.
The "scripts/deploy-dev.sh" script is also modified to be able to refresh the registration service with the debug image if so desired.
Related PRs
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.