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

Implement Debugger for Docker Deployer #6528

Merged
merged 9 commits into from
Sep 14, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Aug 26, 2021

Fixes #6107

NOTE: This change also unhides the Docker deployer, making it publicly available for use.

Description
This change implements debug support for the Docker deployer, the final piece of the initial implementation for the Deployer.

Any containers created by the Docker deployer will go through the existing entrypoint rewriting logic in Skaffold, which will generate a set of modified applications containers along with a set of init containers. These init containers will be created before the application containers, and will create volumes in the local docker daemon that will be mounted into the application containers. These volumes function in a similar way to the ones created in the existing Kubernetes debug implementation, which contain the shared debugging files necessary to get the debugging binaries into the application containers.

A DebugManager object is created in the Deployer, which is primarily responsible for tracking the modified resources during a debug run. The DebugManager also tracks the support resources created to ensure smooth creation of init containers, as well as emitting events during the skaffold lifecycle.

The port allocation logic from the existing debugging implementation has been slightly modified to consume a Deployer-specific implementation of the isPortAvailable() function, such that it can be reused across multiple packages.

@nkubala nkubala requested a review from a team as a code owner August 26, 2021 18:11
@nkubala nkubala requested a review from MarlonGamez August 26, 2021 18:11
@google-cla google-cla bot added the cla: yes label Aug 26, 2021
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #6528 (f75ccfa) into main (290280e) will decrease coverage by 0.61%.
The diff coverage is 36.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6528      +/-   ##
==========================================
- Coverage   70.48%   69.86%   -0.62%     
==========================================
  Files         515      520       +5     
  Lines       23150    23528     +378     
==========================================
+ Hits        16317    16438     +121     
- Misses       5776     6030     +254     
- Partials     1057     1060       +3     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 89.00% <0.00%> (-1.82%) ⬇️
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
pkg/skaffold/build/buildpacks/logger.go 0.00% <ø> (ø)
pkg/skaffold/build/cluster/logs.go 0.00% <ø> (-16.67%) ⬇️
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/debug/apply_transforms.go 22.64% <0.00%> (-0.44%) ⬇️
pkg/skaffold/debug/transform.go 95.91% <ø> (ø)
pkg/skaffold/deploy/docker/deploy.go 0.00% <0.00%> (ø)
pkg/skaffold/docker/client.go 90.36% <ø> (ø)
pkg/skaffold/docker/debugger/adapter.go 0.00% <0.00%> (ø)
... and 57 more

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 d64637c...f75ccfa. Read the comment docs.

@nkubala nkubala force-pushed the docker-debug branch 3 times, most recently from 2197615 to 46f40ff Compare August 27, 2021 20:31
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Some comments from a first pass

pkg/skaffold/debug/apply_transforms.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/debug.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Outdated Show resolved Hide resolved
@nkubala nkubala force-pushed the docker-debug branch 2 times, most recently from 1d80aa8 to 888c700 Compare September 7, 2021 17:51
@gsquared94
Copy link
Contributor

Can you please add instructions on how to test the Docker Deployer debugger against VSCode on any sample project?

pkg/skaffold/docker/debugger/transform.go Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Show resolved Hide resolved
@@ -67,18 +74,24 @@ func NewDeployer(ctx context.Context, cfg dockerutil.Config, labeller *label.Def
return nil, err
}

debugHelpersRegistry, err := config.GetDebugHelpersRegistry(cfg.GlobalConfig())
Copy link
Member

Choose a reason for hiding this comment

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

We can do this later, but IMHO this should be config.GetDebugHelpersRegistry() with no argument, and the implementation should be looking into the global config. This deployer code should not need to know how the registry is being looked up.

pkg/skaffold/deploy/docker/deploy.go Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Show resolved Hide resolved
@@ -53,12 +53,22 @@ var (

type Config interface {
Prune() bool
GlobalConfig() string
Copy link
Member

Choose a reason for hiding this comment

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

See other comment about how GlobalConfig should be hidden as an implementation detail.

pkg/skaffold/docker/debugger/transform.go Outdated Show resolved Hide resolved
@nkubala nkubala force-pushed the docker-debug branch 3 times, most recently from 76e2421 to 4924335 Compare September 14, 2021 16:37
@nkubala
Copy link
Contributor Author

nkubala commented Sep 14, 2021

@gsquared94

Can you please add instructions on how to test the Docker Deployer debugger against VSCode on any sample project?

All you'll need to do is add the following setting to your VSCode settings.json:

    "cloudcode.autoDependencies": "off",

VSCode will then use the skaffold binary on your PATH, and you can run the normal debug workflow against it. configure your sample project to use a docker deployer and you're all set.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Some minor nits

integration/debug_test.go Outdated Show resolved Hide resolved
integration/debug_test.go Show resolved Hide resolved
pkg/skaffold/config/options.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/docker/deploy.go Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/debugger/transform.go Outdated Show resolved Hide resolved
pkg/skaffold/util/port.go Show resolved Hide resolved
@nkubala nkubala merged commit 5808146 into GoogleContainerTools:main Sep 14, 2021
@nkubala nkubala deleted the docker-debug branch September 14, 2021 20:00
@nkubala
Copy link
Contributor Author

nkubala commented Sep 14, 2021

kokoro seems to be hanging but all other tests are passing, so self merging

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.

[FR] Deploy to Docker
3 participants