SANDBOX-1561 | feature: debug builds for the host operator #1227
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-enabled multi-stage debug Dockerfile, a DEBUG_MODE path in the entrypoint to run the operator under dlv, a debug Go build target and podman image/push targets, and a templated/overridable registration-service command in the deployment template. Changes
sequenceDiagram
autonumber
participant Dev as Developer/CI
participant Builder as Delve Builder
participant Runtime as Debug Runtime Image
participant Entrypoint as Container Entrypoint
participant Delve as Delve (dlv)
participant Operator as Operator Binary
Dev->>Builder: build stage (build/Dockerfile.debug) → produce `dlv`
Dev->>Runtime: assemble runtime image (copy operator, dlv, entrypoint)
Runtime->>Entrypoint: start container (ENV: DEBUG_MODE, OPERATOR, USER_UID)
Entrypoint->>Entrypoint: check DEBUG_MODE
alt DEBUG_MODE set
Entrypoint->>Delve: exec dlv --headless --listen=:50000 --api-version=2 --continue --accept-multiclient -- /path/to/operator
Delve->>Operator: run operator (debug build)
else DEBUG_MODE unset
Entrypoint->>Operator: exec /path/to/operator
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)controllers/toolchainconfig/toolchainconfig_controller.go (1)
⏰ 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)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
build/bin/entrypoint (1)
12-17: Consider usingexecfor the Delve command to ensure proper signal handling.Line 14 runs Delve without the
execprefix, while line 16 uses it. Withoutexec, the shell process remains as PID 1 in the container, which can lead to signal handling issues and zombie processes. Consider addingexecbefore/usr/local/bin/dlvunless there's a specific reason to keep the shell alive during debugging.🔎 Proposed fix
if [ -n "${DEBUG_MODE}" ] then - /usr/local/bin/dlv --listen=:50000 --headless --continue --api-version=2 --accept-multiclient exec "${OPERATOR}" "$@" + exec /usr/local/bin/dlv --listen=:50000 --headless --continue --api-version=2 --accept-multiclient exec "${OPERATOR}" "$@" else exec ${OPERATOR} "$@" ficontrollers/toolchainconfig/toolchainconfig_controller.go (1)
154-162: Consider documenting the expected JSON array format for the override.The environment variable
REGISTRATION_SERVICE_COMMANDis used directly without validation. Users must provide it in JSON array format (e.g.,["dlv", "exec", ...]) to match the template's expectation. Consider adding a comment or documentation to clarify the expected format, or optionally add basic validation to catch common mistakes.💡 Optional: Add validation for JSON array format
// Allow overriding the registration service's command via an environment // variable. + // Note: The value must be a valid JSON array, e.g., `["registration-service"]` or `["dlv", "exec", "..."]` command := os.Getenv(RegistrationServiceCommandEnvKey) if command != "" { vars["COMMAND"] = command } else { vars["COMMAND"] = `["registration-service"]` }Alternatively, you could add JSON validation:
if command != "" { // Basic check that it looks like a JSON array if !strings.HasPrefix(strings.TrimSpace(command), "[") { // Log warning or return error } vars["COMMAND"] = command } else { vars["COMMAND"] = `["registration-service"]` }build/Dockerfile.debug (2)
1-23: Consider pinning base image versions for reproducibility.Lines 2 and 23 use
:latesttags, which can lead to non-reproducible builds. For debug images, this may be acceptable, but consider using specific versions (e.g.,ubi8:8.9and a specific Delve version) to ensure consistent behavior across builds.💡 Example with pinned versions
-FROM registry.access.redhat.com/ubi8/ubi:latest AS delve-builder +FROM registry.access.redhat.com/ubi8/ubi:8.9 AS delve-builder # ... rest of the file ... -RUN GOBIN=/tmp/bin go install github.com/go-delve/delve/cmd/dlv@latest +RUN GOBIN=/tmp/bin go install github.com/go-delve/delve/cmd/[email protected]
25-52: The debug runtime image is correctly configured.The runtime stage properly:
- Enables DEBUG_MODE for the entrypoint script
- Copies both the operator binary and Delve
- Exposes port 50000 for remote debugging
- Runs as non-root user for security
Line 26 also uses
:latesttag (same suggestion as the builder stage regarding version pinning).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build/Dockerfile.debugbuild/bin/entrypointcontrollers/toolchainconfig/env.gocontrollers/toolchainconfig/toolchainconfig_controller.godeploy/templates/registration-service/registration-service.yamlmake/go.mkmake/podman.mk
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/toolchainconfig/toolchainconfig_controller.go (1)
controllers/toolchainconfig/env.go (1)
RegistrationServiceCommandEnvKey(5-5)
⏰ 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). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Test with Coverage
🔇 Additional comments (5)
make/podman.mk (2)
13-16: The Go version extraction is stable for standard Go distributions.The command
go version | awk '{print $$3}'extracts the Go version (e.g., "go1.21.5") and passes it to the Dockerfile. This approach is appropriate for standard Go installations.
23-26: LGTM!The debug push target correctly mirrors the standard push workflow and depends on the appropriate debug image build.
controllers/toolchainconfig/env.go (1)
3-6: LGTM!The new constant follows the existing naming convention and provides a clear environment variable key for the registration service command override.
deploy/templates/registration-service/registration-service.yaml (2)
104-104: LGTM!The templated command field correctly integrates with the controller's environment-based override logic, enabling dynamic command injection for debugging scenarios.
273-274: LGTM!The COMMAND parameter is correctly defined with an appropriate default value that maintains backward compatibility.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
2 times, most recently
from
5f56f77 to
a3e01d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build/Dockerfile.debugbuild/bin/entrypointcontrollers/toolchainconfig/env.gocontrollers/toolchainconfig/toolchainconfig_controller.godeploy/templates/registration-service/registration-service.yamlmake/go.mkmake/podman.mk
🚧 Files skipped from review as they are similar to previous changes (2)
- build/Dockerfile.debug
- make/go.mk
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/toolchainconfig/toolchainconfig_controller.go (1)
controllers/toolchainconfig/env.go (1)
RegistrationServiceCommandEnvKey(5-5)
🪛 GitHub Actions: ci-build
controllers/toolchainconfig/env.go
[error] 4-4: File is not properly formatted (gofmt). Run 'gofmt -w' or 'go fmt' to fix formatting.
🪛 GitHub Check: GolangCI Lint
controllers/toolchainconfig/env.go
[failure] 4-4:
File is not properly formatted (gofmt)
🔇 Additional comments (4)
controllers/toolchainconfig/toolchainconfig_controller.go (1)
154-162: LGTM! Clean environment-driven override pattern.The implementation correctly allows debugging workflows to override the registration service command while preserving the default production behavior.
make/podman.mk (1)
13-16: LGTM! Debug targets follow existing patterns.The new debug-specific targets correctly mirror the structure of existing podman targets and maintain consistent dependency chains.
Also applies to: 23-26
deploy/templates/registration-service/registration-service.yaml (1)
104-104: LGTM! Templating correctly preserves the default behavior.The command is now templated with an appropriate default value that matches the original hardcoded command. The
${{COMMAND}}syntax ensures proper JSON array parsing.Also applies to: 273-274
build/bin/entrypoint (1)
12-17: DEBUG_MODE is properly isolated to debug builds and not enabled in production.The production
build/Dockerfiledoes not set DEBUG_MODE, while onlybuild/Dockerfile.debugenables it for local debugging. No deployment configurations reference DEBUG_MODE. The code is secure as deployed.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
a3e01d5 to
9a5e69f
Compare
In order to be able to debug the operator 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 purpose, a new Dockerfile is added which can build the image that way, and the corresponding Make targets make it easy to kick off the whole process with a single command. The Makefile targets also enable the "toolchain-e2e" repository to easily build the "debug" image. Also, when Developer Sandbox is deployed locally, usually the registration service gets managed by the "ToolChain config controller", which launches it with a specific command. In order to be able to launch the registration service with Delve, a few minor modifications are made so that that launch command can be overridden. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
9a5e69f to
29d520b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/templates/registration-service/registration-service.yaml (1)
273-274: LGTM! Default value preserves original behavior.The new
COMMANDparameter is correctly defined with a default value that preserves the original behavior. The value'["registration-service"]'will be parsed as a JSON array when substituted via${{COMMAND}}.💡 Optional: Add description field for clarity
Consider adding a
descriptionfield to document the parameter's purpose:- name: COMMAND value: '["registration-service"]' + description: 'Command to execute in the registration-service container. Override for debugging (e.g., with Delve).'This helps operators understand when and why to override this parameter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build/Dockerfile.debugbuild/bin/entrypointcontrollers/toolchainconfig/env.gocontrollers/toolchainconfig/toolchainconfig_controller.godeploy/templates/registration-service/registration-service.yamlmake/go.mkmake/podman.mk
🚧 Files skipped from review as they are similar to previous changes (5)
- build/bin/entrypoint
- build/Dockerfile.debug
- make/go.mk
- controllers/toolchainconfig/toolchainconfig_controller.go
- make/podman.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). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Test with Coverage
🔇 Additional comments (2)
controllers/toolchainconfig/env.go (1)
3-6: LGTM! Clean constant addition.The new
RegistrationServiceCommandEnvKeyconstant follows the same naming pattern as the existing constant and enables the command override functionality for debug builds. The grouped const block is idiomatic Go.deploy/templates/registration-service/registration-service.yaml (1)
104-104: LGTM! Command override enables debug functionality.The templated command field correctly uses
${{COMMAND}}syntax to enable runtime command override, which is essential for launching the registration service under Delve for debugging. The OpenShift template syntax is correct.
|
|
@MikelAlejoBR: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Closing in favor of #1228. |
2 participants
In order to be able to debug the operator 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 purpose, a new Dockerfile is added which can build the image that way, and the corresponding Make targets make it easy to kick off the whole process with a single command.
The Makefile targets also enable the "toolchain-e2e" repository to easily build the "debug" image.
Also, when Developer Sandbox is deployed locally, usually the registration service gets managed by the "ToolChain config controller", which launches it with a specific command. In order to be able to launch the registration service with Delve, a few minor modifications are made so that that launch command can be overridden.
Related PRs
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.