feat: support metadata to start debugger in wait mode for g… #9105

pkoutsovasilis · 2023-09-30T16:07:31Z

Implements #4870:

support for starting the respective runtime debugger in "wait" mode for go, python, nodejs, jvm if the container image has the label skaffold.dev/debug-suspend [docker deploy]
support for starting the respective runtime debugger in "wait" mode for go, python, nodejs, jvm if the pod template has the annotation the skaffold.dev/debug-suspend [k8s deploy]

google-cla · 2023-09-30T16:07:35Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

anthonyalayo · 2023-11-10T18:47:50Z

This is my top desired feature for skaffold. I hope the maintainers can take a look at this soon.

…r debugger to be attached or not

pkoutsovasilis · 2024-04-24T21:25:14Z

I am not sure why this PR doesn't get any attention from the maintainers, but I rebased it to resolve the conflicts with main. In my workflow, and I think I am not alone, I need a breakpoint as soon as the first line in main() so such a feature is crucial.

mepi262 · 2024-12-05T03:27:26Z

@idsulik @tejal29 @nathanperkins @menahyouyeah @briandealwis @louisjimenez @plumpy @ChrisGe4 @alphanota @johannespfrang @katiexzhang @mattsanta @gsquared94
Would someone review this pull request?

menahyouyeah · 2024-12-06T21:12:20Z

pkg/skaffold/debug/transform_go.go

+
+		if dmd != nil {
+			spec.wait = dmd.ShouldWait
+		} else {


by default bools are false - is this set somewhere else? Otherwise, I think you could just delete the else clause.

in go sure bools are instantiated as false, but here not this one, again if you prefer this boolean to start with a false value I need to rename the wait to continue

menahyouyeah · 2024-12-06T21:13:08Z

pkg/skaffold/debug/transform_go.go

@@ -155,7 +163,7 @@ func extractDlvSpec(args []string) *dlvSpec {
 		return nil
 	}
 	// delve's defaults
-	spec := dlvSpec{apiVersion: 2, log: false, headless: false}
+	spec := dlvSpec{apiVersion: 2, log: false, headless: false, wait: true}


This is changing the default behavior. Are you sure about that? This could instead by false by default and if someone wants to have wait as true, then they'd need to have the label or annotation

This has to start from true as the default behaviour of dlv is also to wait, this changes with the --continue arg which makes the wait to false (here) and the default behaviour of dlv. I can rename the wait to continue and have it start at false?!

menahyouyeah · 2024-12-06T21:22:44Z

pkg/skaffold/debug/transform_go.go

@@ -207,7 +217,12 @@ func (spec dlvSpec) asArguments() []string {
 	if spec.headless {
 		args = append(args, "--headless")
 	}
-	args = append(args, "--continue", "--accept-multiclient")
+
+	if !spec.wait {


on L178, if the arg is "continue" you set spec.wait to false. Then here, if the value is false, you append the "--continue" arg. I'm trying to figure out why you need to do both. In other words, if spec.Wait is false, why does the --continue arg need to be added? Isn't that the default behavior?

I am not getting entirely what you mean. In the previous code you start from an image entrypoint/cmd and you instantiate accordingly a dlvSpec (we don't hold the args inside the dlvSpec). Where here you generate from a dlvSpec the appropriate cli command with args. This logic was already in place and not introduced by this PR 🙂

PS: the default of dlv is to wait and not continue thus adding the --continue is required

menahyouyeah · 2024-12-06T21:31:49Z

pkg/skaffold/debug/transform_nodejs.go

 	container := adapter.GetContainer()
 	log.Entry(context.TODO()).Infof("Configuring %q for node.js debugging", container.Name)

 	// try to find existing `--inspect` command
 	spec := retrieveNodeInspectSpec(config)
 	if spec == nil {
 		spec = &inspectSpec{host: "0.0.0.0", port: portAlloc(defaultDevtoolsPort)}
+		if dmd != nil {
+			spec.brk = dmd.ShouldWait
+		} else {


same comment - could you leave the else off since bool are by default false?

I replied to the same comment above 🙂

menahyouyeah · 2024-12-06T21:33:40Z

pkg/skaffold/kubernetes/debugging/transform_go_test.go

@@ -58,6 +59,20 @@ func TestDlvTransformerApply(t *testing.T) {
 			debugConfig: types.ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
 			image:       "go",
 		},
+		{
+			description:   "basic with wait debugger",


nit: "with wait enabled in the debugger metadata"

menahyouyeah · 2024-12-06T21:34:48Z

pkg/skaffold/kubernetes/debugging/transform_jvm_test.go

@@ -62,6 +63,21 @@ func TestJdwpTransformerApply(t *testing.T) {
 			},
 			debugConfig: types.ContainerDebugConfiguration{Runtime: "jvm", Ports: map[string]uint32{"jdwp": 5005}},
 		},
+		{
+			description: "existing port with suspend debugger",


nit: "with wait enabled in metadata"

menahyouyeah · 2024-12-06T21:35:39Z

pkg/skaffold/kubernetes/debugging/transform_nodejs_test.go

@@ -72,6 +73,20 @@ func TestNodeTransformer_Apply(t *testing.T) {
 			},
 			debugConfig: types.ContainerDebugConfiguration{Runtime: "nodejs", Ports: map[string]uint32{"devtools": 9229}},
 		},
+		{
+			description:   "entrypoint with PATH and brk",


nit: not sure what brk means I would say something like "entrypoint with PATH and wait enabled in debugger metadata"

the inspiration came from here, but sure I can change it 🙂

menahyouyeah · 2024-12-06T21:35:56Z

pkg/skaffold/kubernetes/debugging/transform_python_test.go

@@ -60,6 +61,20 @@ func TestPythonTransformer_Apply(t *testing.T) {
 			debugConfig: types.ContainerDebugConfiguration{Runtime: "python", Ports: map[string]uint32{"dap": 5678}},
 			image:       "python",
 		},
+		{
+			description:   "basic with wait debugger",


nit: same comment on test description

pkoutsovasilis requested a review from a team as a code owner September 30, 2023 16:07

pkoutsovasilis requested a review from gsquared94 September 30, 2023 16:07

pull-request-size bot added the size/M label Sep 30, 2023

pkoutsovasilis force-pushed the pkoutsovasilis/4870 branch from 8e4f6c0 to 91bdebc Compare October 3, 2023 07:45

pull-request-size bot added size/L and removed size/M labels Oct 3, 2023

pkoutsovasilis changed the title ~~feat: support docker image label to start debugger in wait mode for g…~~ feat: support metadata to start debugger in wait mode for g… Oct 3, 2023

pkoutsovasilis mentioned this pull request Nov 10, 2023

debug should make suspending on start to be configurable #4870

Open

feat: introduce DebuggerMetaData to control if process should wait fo…

82e162d

…r debugger to be attached or not

pkoutsovasilis force-pushed the pkoutsovasilis/4870 branch from 2931f5a to 82e162d Compare April 24, 2024 21:21

Merge remote-tracking branch 'skaffold/main' into pkoutsovasilis/4870

a1bd236

menahyouyeah reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support metadata to start debugger in wait mode for g… #9105

feat: support metadata to start debugger in wait mode for g… #9105

pkoutsovasilis commented Sep 30, 2023 •

edited

Loading

google-cla bot commented Sep 30, 2023

anthonyalayo commented Nov 10, 2023

pkoutsovasilis commented Apr 24, 2024

mepi262 commented Dec 5, 2024

menahyouyeah Dec 6, 2024

pkoutsovasilis Dec 6, 2024

menahyouyeah Dec 6, 2024

pkoutsovasilis Dec 6, 2024

menahyouyeah Dec 6, 2024

pkoutsovasilis Dec 6, 2024 •

edited

Loading

menahyouyeah Dec 6, 2024

pkoutsovasilis Dec 6, 2024

menahyouyeah Dec 6, 2024

menahyouyeah Dec 6, 2024

menahyouyeah Dec 6, 2024

pkoutsovasilis Dec 6, 2024

menahyouyeah Dec 6, 2024

feat: support metadata to start debugger in wait mode for g… #9105

Are you sure you want to change the base?

feat: support metadata to start debugger in wait mode for g… #9105

Conversation

pkoutsovasilis commented Sep 30, 2023 • edited Loading

google-cla bot commented Sep 30, 2023

anthonyalayo commented Nov 10, 2023

pkoutsovasilis commented Apr 24, 2024

mepi262 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkoutsovasilis Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkoutsovasilis commented Sep 30, 2023 •

edited

Loading

pkoutsovasilis Dec 6, 2024 •

edited

Loading