Add go remote debugging support in TILT#4216
Conversation
❌ 2 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
🔄 Flaky Test DetectedAnalysis: The test failed due to a PostgreSQL SQLSTATE 57P01 (admin_shutdown) error — the catalog DB connection was killed by an external administrator command mid-test, a transient CI infrastructure issue unrelated to any code change. ✅ Automatically retrying the workflow |
pfcoperez
left a comment
pfcoperez
left a comment
There was a problem hiding this comment.
This is fantastic!
Tbh, I think this opens the doors for debugging beyond "just" vs code. For example, we can add the instructions to attach the debugger at https://github.com/PeerDB-io/peerdb/blob/main/.claude/skills/skills/local-debug/SKILL.md . This way we'd close the loop on automatic debugging which so far was limited to test code
| ENV GOCACHE=/root/.cache/go-build | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go build -o /root/peer-flow | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go build ${DEBUG_BUILD:+-gcflags} ${DEBUG_BUILD:+"all=-N -l"} -o /root/peer-flow | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go install github.com/go-delve/delve/cmd/dlv@latest |
There was a problem hiding this comment.
Shouldn't executing this command be conditioned on DEBUG_BUILD too?
There was a problem hiding this comment.
@masterashu you added a 👍 to this comment but the command still runs unconditionally, did you forget to add that or you think we can skip?
There was a problem hiding this comment.
Yeah, i was thinking of moving this in the flow-base-debug stage, but its better to have it in build stage itself, added a condition now.
| COPY --from=builder /go/bin/dlv /usr/local/bin/dlv | ||
| USER peerdb | ||
| EXPOSE 40000 | ||
| ENTRYPOINT ["dlv", "--headless", "--continue", "--accept-multiclient", "--listen=:40000", "--api-version=2", "exec", "./peer-flow", "--", "worker"] |
There was a problem hiding this comment.
Nit: Would it make sense to make the hard-coded port number '40000' configurable through an env var?
| dockerfile: stacks/flow.Dockerfile | ||
| target: flow-worker | ||
| ports: | ||
| - 40000:40000 |
There was a problem hiding this comment.
Same comment about the port being hard coded.
| ENTRYPOINT ["./peer-flow", "snapshot-worker"] | ||
|
|
||
|
|
||
| FROM flow-base AS flow-worker-debug |
There was a problem hiding this comment.
I am wondering if the CI steps building and pushing images can inadvertently get added a new image to build. I tend to think that's not the case because CI steps building images such us customer-docker.yml use https://github.com/PeerDB-io/peerdb/blob/main/docker-bake.hcl .
flow-worker-debug is not in that build definition file so, by default, we are not going to build it.
If you can confirm, this is a no-comment. Otherwise, we might want to move flow-worker-debug and other future debug images to a separate Dockerfile.
There was a problem hiding this comment.
it won't happen unless we want to build & publish debug images
|
|
||
| flow_ignore = ['flow/e2e/', 'flow/**/*_test.go'] | ||
|
|
||
| docker_build('flow-api', '.', |
There was a problem hiding this comment.
Is there any reason to stop at flow-worker-debug? Why not including flow-api and others too? Is it because you want to start with one?
There was a problem hiding this comment.
Added for flow-snapshot worker and flow-api as well,
🔄 Flaky Test DetectedAnalysis: Both failing runs hit a transient PostgreSQL catalog connection termination (SQLSTATE 57P01 — "terminating connection due to administrator command") mid-test in ✅ Automatically retrying the workflow |
🔄 Flaky Test DetectedAnalysis: TestApiPg/TestQRep failed with PostgreSQL SQLSTATE 57P01 (admin_shutdown) — a transient catalog DB connection termination during the CI run, not a code regression. ✅ Automatically retrying the workflow |
|
@pfcoperez For now, changed the ports in docker-compose for now. seeing how to make it dynamic based on env, also refactored dockerfile and added the debug for flow-api and flow-snapshot worker |
…docker compose Co-authored-by: Copilot <copilot@github.com>
masterashu
force-pushed
the
add-vscode-remote-go-debugger-in-tilt
branch
from
8d722c6 to
0feb42f
Compare
🔄 Flaky Test DetectedAnalysis: TestCancelTableAdditionRemoveAddRemove failed due to a transient PostgreSQL connection drop (SQLSTATE 57P01 - admin_shutdown), where the catalog connection was terminated by an administrator command mid-test rather than any code regression. ✅ Automatically retrying the workflow |
🔄 Flaky Test DetectedAnalysis: The e2e test ✅ Automatically retrying the workflow |
| docker_build('flow-worker', '.', | ||
| dockerfile='stacks/flow.Dockerfile', | ||
| target='flow-worker', | ||
| target='flow-worker-debug' if resolve_env('DOCKER_GO_DEBUG_FLOW_WORKER') == '1' else 'flow-worker', |
There was a problem hiding this comment.
Surely, someone will try to use value true just from pure inertia. I think we can protect a bit by allowing 1 or true .
| ENV GOCACHE=/root/.cache/go-build | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go build -o /root/peer-flow | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go build ${DEBUG_BUILD:+-gcflags} ${DEBUG_BUILD:+"all=-N -l"} -o /root/peer-flow | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" go install github.com/go-delve/delve/cmd/dlv@latest |
There was a problem hiding this comment.
@masterashu you added a 👍 to this comment but the command still runs unconditionally, did you forget to add that or you think we can skip?
🔄 Flaky Test DetectedAnalysis: Both failing tests ( ✅ Automatically retrying the workflow |
…le instances of tilt for peerdb Co-authored-by: Copilot <copilot@github.com>
|
hey @pfcoperez made some improvements. |
|
test |
Code ReviewTwo issues found. Checked for bugs and CLAUDE.md compliance. Issue 1: Tiltfile/Dockerfile mismatch causes build failure when DEBUG_BUILD=true File: The Tiltfile accepts both if [[ "$DEBUG_BUILD" = "1" ]]; thenWhen a user sets Note that the parameter expansion on line 28 already handles any non-empty value correctly. Suggested fix: change to Issue 2: Unrelated volume mount path change may break existing dev environments File: The catalog volume mount was changed from Was this change intentional? |
…4306) In #4216, we exposed Go debug port for services in local dev-env. The sample `.env.example` ports for these were carefully chosen so they wouldn't collide with ClickHouse internal testing tooling: https://github.com/PeerDB-io/peerdb/blob/1bce75b767bfd293e18f3a4c671c75090fce7651/.env.example#L69-L71 But the fallback ports in Tiltfile didn't fall into the same range. This PR fixes the fallback ports.
2 participants
Summary of Changes: