Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Python debugging support #2205

Merged
merged 20 commits into from
Jun 19, 2019

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented May 30, 2019

This patch adds support to skaffold debug for Python 2.7 and 3.x debugging. Python requires additional debugging runtime support files, which are fetched and installed using an initContainer base.

These debugging runtime support files are installed via initContainers from images defined in https://github.com/GoogleContainerTools/container-debug-support/. These images are currently manually pushed to gcr.io/gcp-dev-tools/duct-tape/XXX where XXX is a runtime-specific identifier like python (supports both Python 2.7 and 3.7).

The Python image installs the ptvsd module, a wrapper that uses the IDE/editor-agnostic debug adapter protocol (DAP). This protocol has become a de facto standard for interacting with remote runtimes.

Manual Testing

For manual testing, you'll need to use VS Code or Eclipse with LSP4e, or some other editor that supports the DAP. I had to launch my debug session with the following launch parameters, to map the local file system to the remote file system in the container:

{"pathMappings": [ { "localRoot": "_/your/local/path_", "remoteRoot": "/app" }]}

The debugger is on port 5678.

Fixes #2173

@codecov-io
Copy link

codecov-io commented May 31, 2019

Codecov Report

Merging #2205 into master will increase coverage by 0.45%.
The diff coverage is 87.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2205      +/-   ##
==========================================
+ Coverage    58.6%   59.06%   +0.45%     
==========================================
  Files         188      189       +1     
  Lines        7805     7924     +119     
==========================================
+ Hits         4574     4680     +106     
- Misses       2859     2866       +7     
- Partials      372      378       +6
Impacted Files Coverage Δ
pkg/skaffold/util/util.go 76.59% <100%> (+1.04%) ⬆️
pkg/skaffold/debug/transform_jvm.go 94.89% <100%> (+0.1%) ⬆️
pkg/skaffold/debug/debug.go 44.82% <50%> (ø) ⬆️
pkg/skaffold/debug/transform_nodejs.go 79.24% <66.66%> (+0.39%) ⬆️
pkg/skaffold/debug/transform_python.go 85.55% <85.55%> (ø)
pkg/skaffold/debug/transform.go 85.03% <96%> (+2.63%) ⬆️

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 342b210...5d27b3d. Read the comment docs.

pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
}
}
if len(supportFilesRequired) > 0 {
logrus.Infof("Configuring installation of debugging support files")
supportVolume := v1.Volume{Name: "debugging-support-files", VolumeSource: v1.VolumeSource{EmptyDir: &v1.EmptyDirVolumeSource{}}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"debugging-support-files" is used on multiple lines. Extract to constant?

pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some stuff to clarify things for me?

pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform_python.go Show resolved Hide resolved
@briandealwis
Copy link
Member Author

PTAL @loosebazooka @ILMTitan

@briandealwis
Copy link
Member Author

@quoctruong PTAL

pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform.go Show resolved Hide resolved
pkg/skaffold/debug/transform.go Show resolved Hide resolved
pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform_python.go Outdated Show resolved Hide resolved
@briandealwis briandealwis added the kokoro:run runs the kokoro jobs on a PR label Jun 17, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 17, 2019
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some language stuff I'm not sure about, feel free to ignore or whatever.

docs/content/en/docs/how-tos/debug/_index.md Outdated Show resolved Hide resolved
pkg/skaffold/debug/transform.go Outdated Show resolved Hide resolved
docs/content/en/docs/how-tos/debug/_index.md Show resolved Hide resolved
@loosebazooka
Copy link
Member

Also might be worth doing a ux study with the team to see if we can all follow the instructions 🛩️

@briandealwis
Copy link
Member Author

PTAL @dgageot @balopat — need your approvals due to doc changes

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Re Approvals: you had changes in a main util file hence our review was required. Also more "debug" related files were changed for integration testing that weren't owned by you, so I added those to the CODEOWNERS in #2292

@balopat balopat merged commit e59fc2f into GoogleContainerTools:master Jun 19, 2019
@briandealwis briandealwis deleted the dbg-python branch November 11, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Python for skaffold debug
8 participants